Merge lp:~jtv/maas/bootresources-rewrite-marker into lp:~maas-maintainers/maas/new-import-script-integration

[Jeroen T. Vermeulen]

First step towards writing a bootresources.yaml during cluster upgrade, based on locally available old-style boot images.

Pre-imp was with Julian. I'm keeping this to small steps, because there's going to be quite some diff and I'm messing with production filesystems here.

In this first step, I add a config item to bootresources.yaml: "configure_me" — meaning "please either edit me manually or overwrite me automatically."

The rewrite_boot_resources_config stub will look for old-style boot images on the cluster's filesystem (reviving the old code for list_boot_images as a special-purpose migration helper) and rewrite bootresources.yaml with sources/selections to get similar downloads from the new script.

Our cluster upgrade mechanism requires the upgrade code to be able to tell whether it's supposed to do anything or not. And so, when the configuration is the default (which will be just fine for most users), it needs to figure out whether it is that way because that's how we believe the user wants it, or whether it's simply because nothing has been done about it yet. To that end, I added a configure_me item. After a rewrite, that item will be gone forever.

Testing this led me down some dead ends. The ConfigFixture is not suitable for patching configurations other than pserv.yaml, and attempts to fix it led right down a rabbit hole. And so I'm solving this problem ad hoc, which turns out not to be all that bad.

Jeroen

[Raphaël Badin (rvb)]

Looks good. The usage of the 'configure_me' config item is a bit messy, but I don't see a better way, and at least it's explicit.

[0]

'configure_me' is a bit vague? How about 'auto_configure_legacy' to convey the idea that this is going to be done 'automatically' by MAAS and that this is to cope with legacy installations?

[1]

16 + # If the setting is True, the upgrade procedure will look for downloaded
17 + # boot images that predate the current, Simplestreams-based import
18 + # mechanism, and rewrite the configuration based on what it finds. The new
19 + # configuration will import the new-style equivalents of the images that
20 + # were imported in the old setup.

Maybe mention that, if someone installed fresh on Trusty or up, there is no need to worry about any of this? Ideally, I'd have the packaging code remove that bit for fresh Trusty installations. I think this whole thing can be pretty confusing for users.

[Raphaël Badin (rvb)]

review: Approve

[Jeroen T. Vermeulen (jtv)]

> [0]
>
> 'configure_me' is a bit vague? How about 'auto_configure_legacy' to convey
> the idea that this is going to be done 'automatically' by MAAS and that this
> is to cope with legacy installations?

I had a long and descriptive name originally, but it was too specific: users who know what they're doing may still want to edit the config manually instead. The name says that nobody has touched the file yet, and somebody needs to.

The meaning of configure_me is basically "if True, your config will be rewritten on upgrade." The rewrite removes the configure_me flag, even on a fresh install where there is no migration to be done. It's aggressive, but it's also very simple.

Under normal circumstances, the user never sees any of this. The upgrade hook runs during installation, and it rewrites the config. The rewrite defines sources and selections, but also removes the configure_me flag. So even if the user does need to edit this file, by that time the flag and any mention of it are already gone. It's documented for development, and for debugging of broken installs, not for configuration.

> [1]
>
> 16 + # If the setting is True, the upgrade procedure will look for
> downloaded
> 17 + # boot images that predate the current, Simplestreams-based import
> 18 + # mechanism, and rewrite the configuration based on what it finds.
> The new
> 19 + # configuration will import the new-style equivalents of the images
> that
> 20 + # were imported in the old setup.
>
> Maybe mention that, if someone installed fresh on Trusty or up, there is no
> need to worry about any of this? Ideally, I'd have the packaging code remove
> that bit for fresh Trusty installations. I think this whole thing can be
> pretty confusing for users.

We don't want to put more of this stuff in the packaging, especially if we're also going to be editing the same file from the cluster upgrade hooks. That's just doubling the risks. But yes, the setting is designed to be completely removed from the file during upgrade, not just to have its value changed.

=== modified file 'etc/maas/bootresources.yaml'
--- etc/maas/bootresources.yaml	2014-03-25 03:03:48 +0000
+++ etc/maas/bootresources.yaml	2014-03-25 11:14:18 +0000
@@ -3,6 +3,21 @@
 ##
 
 boot:
+  ## Marker: this configuration has not been initialised yet.
+  #
+  # If this is set to True during installation or a later upgrade, it means
+  # that this configuration file has not been edited by a human or by MAAS
+  # itself.  When you edit this configuration to suit your needs, be sure to
+  # remove this setting, or change it to False, so that a future upgrade will
+  # know not to overwrite the configuration file.
+  #
+  # If the setting is True, the upgrade procedure will look for downloaded
+  # boot images that predate the current, Simplestreams-based import
+  # mechanism, and rewrite the configuration based on what it finds.  The new
+  # configuration will import the new-style equivalents of the images that
+  # were imported in the old setup.
+  configure_me = True
+
   ## Storage location for boot images.
   #
   # These images can get quite large: some files are hundreds of megabytes,

=== modified file 'src/provisioningserver/config.py'
--- src/provisioningserver/config.py	2014-03-24 09:51:27 +0000
+++ src/provisioningserver/config.py	2014-03-25 11:14:18 +0000
@@ -70,6 +70,7 @@
     )
 from formencode.declarative import DeclarativeMeta
 from formencode.validators import (
+    Bool,
     Int,
     RequireIfPresent,
     Set,
@@ -169,6 +170,11 @@
     sources = ForEach(
         ConfigBootSource, if_missing=[ConfigBootSource.to_python({})])
 
+    # Marker in the bootresources.yaml file: if True, the file has not been
+    # edited yet and needs to be either configured with initial choices, or
+    # rewritten based on previously downloaded boot images.
+    configure_me = Bool(if_missing=False)
+
 
 class ConfigMeta(DeclarativeMeta):
     """Metaclass for the root configuration schema."""

=== modified file 'src/provisioningserver/tests/test_config.py'
--- src/provisioningserver/tests/test_config.py	2014-03-24 09:34:44 +0000
+++ src/provisioningserver/tests/test_config.py	2014-03-25 11:14:18 +0000
@@ -152,6 +152,7 @@
                 },
             ],
             'storage': '/var/lib/maas/boot-resources/',
+            'configure_me': False,
         },
         'broker': {
             'host': 'localhost',

=== modified file 'src/provisioningserver/tests/test_upgrade_cluster.py'
--- src/provisioningserver/tests/test_upgrade_cluster.py	2014-03-18 04:25:00 +0000
+++ src/provisioningserver/tests/test_upgrade_cluster.py	2014-03-25 11:14:18 +0000
@@ -25,10 +25,17 @@
 import os.path
 
 from maastesting.factory import factory
-from maastesting.matchers import MockCalledOnceWith
+from maastesting.matchers import (
+    MockCalledOnceWith,
+    MockNotCalled,
+    )
 from maastesting.testcase import MAASTestCase
-from mock import Mock
+from mock import (
+    ANY,
+    Mock,
+    )
 from provisioningserver import upgrade_cluster
+from provisioningserver.config import Config
 from provisioningserver.pxe.install_image import install_image
 from provisioningserver.testing.config import ConfigFixture
 from testtools.matchers import (
@@ -580,3 +587,50 @@
         entries_before = listdir(tftproot)
         upgrade_cluster.add_label_directory_level_to_boot_images()
         self.assertItemsEqual(entries_before, listdir(tftproot))
+
+
+class TestGenerateBootResourcesConfig(MAASTestCase):
+    """Tests for the `generate_boot_resources_config` upgrade."""
+
+    def patch_rewrite_boot_resources_config(self):
+        """Patch `rewrite_boot_resources_config` with a mock."""
+        return self.patch(upgrade_cluster, 'rewrite_boot_resources_config')
+
+    def patch_config(self, config):
+        """Patch the `bootresources.yaml` config with a given dict."""
+        original_load = Config.load_from_cache
+
+        @classmethod
+        def fake_config_load(cls, filename=None):
+            """Fake `Config.load_from_cache`.
+
+            Returns a susbtitute for `bootresources.yaml`, but defers to the
+            original implementation for other files.  This means we can still
+            patch the original, and it means we'll probably get a tell-tale
+            error if any code underneath the tests accidentally tries to
+            load pserv.yaml.
+            """
+            if os.path.basename(filename) == 'bootresources.yaml':
+                return config
+            else:
+                return original_load(Config, filename=filename)
+
+        self.patch(Config, 'load_from_cache', fake_config_load)
+
+    def test_does_nothing_if_configure_me_is_False(self):
+        self.patch_config({'boot': {'configure_me': False}})
+        rewrite_config = self.patch_rewrite_boot_resources_config()
+        upgrade_cluster.generate_boot_resources_config()
+        self.assertThat(rewrite_config, MockNotCalled())
+
+    def test_does_nothing_if_configure_me_is_missing(self):
+        self.patch_config({'boot': {}})
+        rewrite_config = self.patch_rewrite_boot_resources_config()
+        upgrade_cluster.generate_boot_resources_config()
+        self.assertThat(rewrite_config, MockNotCalled())
+
+    def test_rewrites_if_configure_me_is_True(self):
+        self.patch_config({'boot': {'configure_me': True}})
+        rewrite_config = self.patch_rewrite_boot_resources_config()
+        upgrade_cluster.generate_boot_resources_config()
+        self.assertThat(rewrite_config, MockCalledOnceWith(ANY))

=== modified file 'src/provisioningserver/upgrade_cluster.py'
--- src/provisioningserver/upgrade_cluster.py	2014-03-18 04:10:37 +0000
+++ src/provisioningserver/upgrade_cluster.py	2014-03-25 11:14:18 +0000
@@ -44,7 +44,10 @@
 from shutil import rmtree
 
 from provisioningserver.config import Config
-from provisioningserver.utils import ensure_dir
+from provisioningserver.utils import (
+    ensure_dir,
+    locate_config,
+    )
 
 
 logger = getLogger(__name__)
@@ -246,6 +249,25 @@
             move_real_boot_image(tftproot, image)
 
 
+def rewrite_boot_resources_config(config_file):
+    """Rewrite the `bootresources.yaml` configuration."""
+
+
+def generate_boot_resources_config():
+    """Upgrade hook: rewrite `bootresources.yaml` based on boot images.
+
+    This finds boot images downloaded into the old, pre-Simplestreams tftp
+    root, and writes a boot-resources configuration to import a similar set of
+    images using Simplestreams.
+    """
+    config_file = locate_config('bootresources.yaml')
+    boot_resources = Config.load_from_cache(config_file)
+    if not boot_resources['boot'].get('configure_me', False):
+        # Already configured.
+        return
+    rewrite_boot_resources_config(config_file)
+
+
 # Upgrade hooks, from oldest to newest.  The hooks are callables, taking no
 # arguments.  They are called in order.
 #
@@ -253,6 +275,7 @@
 # no record of previous upgrades.
 UPGRADE_HOOKS = [
     add_label_directory_level_to_boot_images,
+    generate_boot_resources_config,
     ]