Merge pull request #1021 from mwhudson/variant-flexibility

redo how install variant determines which models to wait for

DESIGN.md

@@ -345,7 +345,10 @@ The server code proceeds in stages:
     commands.
  2. Then it waits for all the model objects that feed into the curtin
     config to be configured.
- 3. It waits for confirmation.
+ 3. It waits for confirmation. This can in theory be interrupted by a
+    change in the model objects which are considered 'interesting',
+    see below, so it is possible to transition from here to the
+    previous state.
  4. It runs "curtin install" and waits for that to finish.
  5. It waits for the model objects that feed into the cloud-init config to be
     configured.
@@ -361,6 +364,14 @@ enum, so the client gets notified via long-polling `meta.status.GET()`
 of progress. In addition, `ApplicationState.ERROR` indicates something
 has gone wrong.
 
+Originally subiquity was just the server installer and so the set of
+models that have to be configured for each step was just static. But
+subiquity can also install desktop systems and maybe one day Ubuntu
+core, and different information is needed to fully configure each
+different variant. This information is handled by the
+`subiquity.models.subiquity.SubiquityModel.set_source_variant` method
+and surrounding code.
+
 ### Error handling
 
 As hard as it is to believe, things do go wrong during

subiquity/models/subiquity.py

@@ -69,29 +69,20 @@ ff02::1 ip6-allnodes
 ff02::2 ip6-allrouters
 """
 
-# Models that contribute to the curtin config
-INSTALL_MODEL_NAMES = [
-    "debconf_selections",
-    "filesystem",
-    "kernel",
-    "keyboard",
-    "mirror",
-    "network",
-    "proxy",
-    ]
 
-# Models that contribute to the cloud-init config (and other postinstall steps)
+class ModelNames:
-POSTINSTALL_MODEL_NAMES = [
+    def __init__(self, default_names, **per_variant_names):
-    "identity",
+        self.default_names = default_names
-    "locale",
+        self.per_variant_names = per_variant_names
-    "packages",
-    "snaplist",
-    "ssh",
-    "timezone",
-    "userdata",
-    ]
 
-ALL_MODEL_NAMES = INSTALL_MODEL_NAMES + POSTINSTALL_MODEL_NAMES
+    def for_variant(self, variant):
+        return self.default_names | self.per_variant_names.get(variant, set())
+
+    def all(self):
+        r = set(self.default_names)
+        for v in self.per_variant_names.values():
+            r |= v
+        return r
 
 
 class DebconfSelectionsModel:
@@ -108,7 +99,7 @@ class SubiquityModel:
 
     target = '/target'
 
-    def __init__(self, root):
+    def __init__(self, root, install_model_names, postinstall_model_names):
         self.root = root
         if root != '/':
             self.target = root
@@ -129,45 +120,77 @@ class SubiquityModel:
         self.updates = UpdatesModel()
         self.userdata = {}
 
-        self.confirmation = asyncio.Event()
+        self._confirmation = asyncio.Event()
+        self._confirmation_task = None
 
-        self._events = {
+        self._configured_names = set()
-            name: asyncio.Event() for name in ALL_MODEL_NAMES
+        self._install_model_names = install_model_names
-            }
+        self._postinstall_model_names = postinstall_model_names
-        self.install_events = {
+        self._cur_install_model_names = install_model_names.default_names
-            self._events[name] for name in INSTALL_MODEL_NAMES
+        self._cur_postinstall_model_names = \
-            }
+            postinstall_model_names.default_names
-        self.postinstall_events = {
+        self._install_event = asyncio.Event()
-            self._events[name] for name in POSTINSTALL_MODEL_NAMES
+        self._postinstall_event = asyncio.Event()
-            }
+
+    def set_source_variant(self, variant):
+        self._cur_install_model_names = \
+            self._install_model_names.for_variant(variant)
+        self._cur_postinstall_model_names = \
+            self._postinstall_model_names.for_variant(variant)
+        unconfigured_install_model_names = \
+            self._cur_install_model_names - self._configured_names
+        if unconfigured_install_model_names:
+            if self._install_event.is_set():
+                self._install_event = asyncio.Event()
+            if self._confirmation_task is not None:
+                self._confirmation_task.cancel()
+        else:
+            self._install_event.set()
 
     def configured(self, model_name):
-        if model_name not in ALL_MODEL_NAMES:
+        self._configured_names.add(model_name)
-            return
+        if model_name in self._cur_install_model_names:
-        self._events[model_name].set()
-        if model_name in INSTALL_MODEL_NAMES:
             stage = 'install'
-            unconfigured = {
+            names = self._cur_install_model_names
-                mn for mn in INSTALL_MODEL_NAMES
+            event = self._install_event
-                if not self._events[mn].is_set()
+        elif model_name in self._cur_postinstall_model_names:
-                }
-        elif model_name in POSTINSTALL_MODEL_NAMES:
             stage = 'postinstall'
-            unconfigured = {
+            names = self._cur_postinstall_model_names
-                mn for mn in POSTINSTALL_MODEL_NAMES
+            event = self._postinstall_event
-                if not self._events[mn].is_set()
+        else:
-                }
+            return
+        unconfigured = names - self._configured_names
         log.debug(
-            "model %s for %s is configured, to go %s",
+            "model %s for %s stage is configured, to go %s",
             model_name, stage, unconfigured)
+        if not unconfigured:
+            event.set()
 
-    def needs_configuration(self, model_name):
+    async def wait_install(self):
-        if model_name is None:
+        await self._install_event.wait()
+
+    async def wait_postinstall(self):
+        await self._postinstall_event.wait()
+
+    async def wait_confirmation(self):
+        if self._confirmation_task is None:
+            self._confirmation_task = asyncio.get_event_loop().create_task(
+                self._confirmation.wait())
+        try:
+            await self._confirmation_task
+        except asyncio.CancelledError:
             return False
-        return not self._events[model_name].is_set()
+        else:
+            return True
+        finally:
+            self._confirmation_task = None
+
+    def is_postinstall_only(self, model_name):
+        return model_name in self._cur_postinstall_model_names and \
+               model_name not in self._cur_install_model_names
 
     def confirm(self):
-        self.confirmation.set()
+        self._confirmation.set()
 
     def get_target_groups(self):
         command = ['chroot', self.target, 'getent', 'group']
@@ -214,7 +237,7 @@ class SubiquityModel:
                 config['ssh_authorized_keys'] = self.ssh.authorized_keys
         if self.ssh.install_server:
             config['ssh_pwauth'] = self.ssh.pwauth
-        for model_name in POSTINSTALL_MODEL_NAMES:
+        for model_name in self._postinstall_model_names.all():
             model = getattr(self, model_name)
             if getattr(model, 'make_cloudconfig', None):
                 merge_config(config, model.make_cloudconfig())
@@ -341,7 +364,7 @@ class SubiquityModel:
                     'permissions': 0o644,
                     }
 
-        for model_name in INSTALL_MODEL_NAMES:
+        for model_name in self._install_model_names.all():
             model = getattr(self, model_name)
             log.debug("merging config from %s", model)
             merge_config(config, model.render())

subiquity/models/tests/test_subiquity.py

@@ -17,7 +17,26 @@ import fnmatch
 import unittest
 import yaml
 
-from subiquity.models.subiquity import SubiquityModel
+from subiquity.models.subiquity import ModelNames, SubiquityModel
+from subiquity.server.server import (
+    INSTALL_MODEL_NAMES,
+    POSTINSTALL_MODEL_NAMES,
+    )
+
+
+class TestModelNames(unittest.TestCase):
+
+    def test_for_known_variant(self):
+        model_names = ModelNames({'a'}, var1={'b'}, var2={'c'})
+        self.assertEqual(model_names.for_variant('var1'), {'a', 'b'})
+
+    def test_for_unknown_variant(self):
+        model_names = ModelNames({'a'}, var1={'b'}, var2={'c'})
+        self.assertEqual(model_names.for_variant('var3'), {'a'})
+
+    def test_all(self):
+        model_names = ModelNames({'a'}, var1={'b'}, var2={'c'})
+        self.assertEqual(model_names.all(), {'a', 'b', 'c'})
 
 
 class TestSubiquityModel(unittest.TestCase):
@@ -71,8 +90,21 @@ class TestSubiquityModel(unittest.TestCase):
             cur = cur[component]
         self.fail("config has value {} for {}".format(cur, path))
 
+    def test_configure(self):
+        model = SubiquityModel(
+            'test', ModelNames({'a', 'b'}), ModelNames(set()))
+        model.set_source_variant('var')
+        model.configured('a')
+        self.assertFalse(model._install_event.is_set())
+        model.configured('b')
+        self.assertTrue(model._install_event.is_set())
+
+    def make_model(self):
+        return SubiquityModel(
+            'test', INSTALL_MODEL_NAMES, POSTINSTALL_MODEL_NAMES)
+
     def test_proxy_set(self):
-        model = SubiquityModel('test')
+        model = self.make_model()
         proxy_val = 'http://my-proxy'
         model.proxy.proxy = proxy_val
         config = model.render('ident')
@@ -87,7 +119,7 @@ class TestSubiquityModel(unittest.TestCase):
         self.assertTrue(len(confs) > 0)
 
     def test_proxy_notset(self):
-        model = SubiquityModel('test')
+        model = self.make_model()
         config = model.render('ident')
         self.assertConfigDoesNotHaveVal(config, 'proxy.http_proxy')
         self.assertConfigDoesNotHaveVal(config, 'proxy.https_proxy')
@@ -100,13 +132,13 @@ class TestSubiquityModel(unittest.TestCase):
         self.assertTrue(len(confs) == 0)
 
     def test_keyboard(self):
-        model = SubiquityModel('test')
+        model = self.make_model()
         config = model.render('ident')
         self.assertConfigWritesFile(config, 'etc/default/keyboard')
 
     def test_writes_machine_id_media_info(self):
-        model_no_proxy = SubiquityModel('test')
+        model_no_proxy = self.make_model()
-        model_proxy = SubiquityModel('test')
+        model_proxy = self.make_model()
         model_proxy.proxy.proxy = 'http://something'
         for model in model_no_proxy, model_proxy:
             config = model.render('ident')
@@ -114,12 +146,12 @@ class TestSubiquityModel(unittest.TestCase):
             self.assertConfigWritesFile(config, 'var/log/installer/media-info')
 
     def test_storage_version(self):
-        model = SubiquityModel('test')
+        model = self.make_model()
         config = model.render('ident')
         self.assertConfigHasVal(config, 'storage.version', 1)
 
     def test_write_netplan(self):
-        model = SubiquityModel('test')
+        model = self.make_model()
         config = model.render('ident')
         netplan_content = None
         for fspec in config['write_files'].values():
@@ -132,12 +164,12 @@ class TestSubiquityModel(unittest.TestCase):
         self.assertConfigHasVal(netplan, 'network.version', 2)
 
     def test_has_sources(self):
-        model = SubiquityModel('test')
+        model = self.make_model()
         config = model.render('ident')
         self.assertIn('sources', config)
 
     def test_mirror(self):
-        model = SubiquityModel('test')
+        model = self.make_model()
         mirror_val = 'http://my-mirror'
         model.mirror.set_mirror(mirror_val)
         config = model.render('ident')

subiquity/server/controller.py

@@ -35,7 +35,6 @@ class SubiquityController(BaseController):
     autoinstall_schema = None
     autoinstall_default = None
     endpoint = None
-    relevant_variants = ('desktop', 'server')
 
     def __init__(self, app):
         super().__init__(app)

subiquity/server/controllers/install.py

@@ -199,7 +199,10 @@ class InstallController(SubiquityController):
     async def install(self, *, context):
         context.set('is-install-context', True)
         try:
-            await asyncio.wait({e.wait() for e in self.model.install_events})
+            while True:
+                self.app.update_state(ApplicationState.WAITING)
+
+                await self.model.wait_install()
 
                 if not self.app.interactive:
                     if 'autoinstall' in self.app.kernel_cmdline:
@@ -207,7 +210,8 @@ class InstallController(SubiquityController):
 
                 self.app.update_state(ApplicationState.NEEDS_CONFIRMATION)
 
-            await self.model.confirmation.wait()
+                if await self.model.wait_confirmation():
+                    break
 
             self.app.update_state(ApplicationState.RUNNING)
 
@@ -224,8 +228,7 @@ class InstallController(SubiquityController):
 
             self.app.update_state(ApplicationState.POST_WAIT)
 
-            await asyncio.wait(
+            await self.model.wait_postinstall()
-                {e.wait() for e in self.model.postinstall_events})
 
             self.app.update_state(ApplicationState.POST_RUNNING)
 

subiquity/server/controllers/timezone.py

@@ -79,7 +79,6 @@ class TimeZoneController(SubiquityController):
         }
 
     autoinstall_default = ''
-    relevant_variants = ('desktop', )
 
     def load_autoinstall_data(self, data):
         self.deserialize(data)

subiquity/server/server.py

@@ -61,7 +61,10 @@ from subiquity.common.types import (
     LiveSessionSSHInfo,
     PasswordKind,
     )
-from subiquity.models.subiquity import SubiquityModel
+from subiquity.models.subiquity import (
+    ModelNames,
+    SubiquityModel,
+    )
 from subiquity.server.controller import SubiquityController
 from subiquity.server.geoip import GeoIP
 from subiquity.server.errors import ErrorController
@@ -111,9 +114,7 @@ class MetaController:
     async def client_variant_POST(self, variant: str) -> None:
         if variant not in ('desktop', 'server'):
             raise ValueError(f'unrecognized client variant {variant}')
-        for controller in self.app.controllers.instances:
+        self.app.base_model.set_source_variant(variant)
-            if variant not in controller.relevant_variants:
-                controller.configured()
 
     async def ssh_info_GET(self) -> Optional[LiveSessionSSHInfo]:
         ips = []
@@ -158,6 +159,27 @@ def get_installer_password_from_cloudinit_log():
     return None
 
 
+INSTALL_MODEL_NAMES = ModelNames({
+    "debconf_selections",
+    "filesystem",
+    "kernel",
+    "keyboard",
+    "mirror",
+    "network",
+    "proxy",
+    })
+
+POSTINSTALL_MODEL_NAMES = ModelNames({
+    "identity",
+    "locale",
+    "packages",
+    "snaplist",
+    "ssh",
+    "userdata",
+    },
+    desktop={"timezone"})
+
+
 class SubiquityServer(Application):
 
     snapd_socket_path = '/run/snapd.socket'
@@ -207,7 +229,8 @@ class SubiquityServer(Application):
         root = '/'
         if self.opts.dry_run:
             root = os.path.abspath('.subiquity')
-        return SubiquityModel(root)
+        return SubiquityModel(
+            root, INSTALL_MODEL_NAMES, POSTINSTALL_MODEL_NAMES)
 
     def __init__(self, opts, block_log_dir):
         super().__init__(opts)
@@ -353,7 +376,7 @@ class SubiquityServer(Application):
             if not controller.interactive():
                 override_status = 'skip'
             elif self.state == ApplicationState.NEEDS_CONFIRMATION:
-                if self.base_model.needs_configuration(controller.model_name):
+                if self.base_model.is_postinstall_only(controller.model_name):
                     override_status = 'confirm'
         if override_status is not None:
             resp = web.Response(headers={'x-status': override_status})