From fdd1879d5010112730d137e2f7d0b06a52c41ae5 Mon Sep 17 00:00:00 2001 From: Michael Hudson-Doyle Date: Thu, 5 Aug 2021 13:21:15 +1200 Subject: [PATCH 1/5] adapt SubiquityModel api to allow a bit more flexibility --- subiquity/models/subiquity.py | 17 +++++++++++++---- subiquity/server/controllers/install.py | 9 +++++---- 2 files changed, 18 insertions(+), 8 deletions(-) diff --git a/subiquity/models/subiquity.py b/subiquity/models/subiquity.py index 2537ec66..ccc76a43 100644 --- a/subiquity/models/subiquity.py +++ b/subiquity/models/subiquity.py @@ -129,15 +129,15 @@ class SubiquityModel: self.updates = UpdatesModel() self.userdata = {} - self.confirmation = asyncio.Event() + self._confirmation = asyncio.Event() self._events = { name: asyncio.Event() for name in ALL_MODEL_NAMES } - self.install_events = { + self._install_events = { self._events[name] for name in INSTALL_MODEL_NAMES } - self.postinstall_events = { + self._postinstall_events = { self._events[name] for name in POSTINSTALL_MODEL_NAMES } @@ -161,13 +161,22 @@ class SubiquityModel: "model %s for %s is configured, to go %s", model_name, stage, unconfigured) + async def wait_install(self): + await asyncio.wait({e.wait() for e in self._install_events}) + + async def wait_postinstall(self): + await asyncio.wait({e.wait() for e in self._postinstall_events}) + + async def wait_confirmation(self): + await self._confirmation.wait() + def needs_configuration(self, model_name): if model_name is None: return False return not self._events[model_name].is_set() def confirm(self): - self.confirmation.set() + self._confirmation.set() def get_target_groups(self): command = ['chroot', self.target, 'getent', 'group'] diff --git a/subiquity/server/controllers/install.py b/subiquity/server/controllers/install.py index 8095b24f..747ba2e5 100644 --- a/subiquity/server/controllers/install.py +++ b/subiquity/server/controllers/install.py @@ -199,7 +199,9 @@ 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}) + 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 +209,7 @@ class InstallController(SubiquityController): self.app.update_state(ApplicationState.NEEDS_CONFIRMATION) - await self.model.confirmation.wait() + await self.model.wait_confirmation() self.app.update_state(ApplicationState.RUNNING) @@ -224,8 +226,7 @@ class InstallController(SubiquityController): self.app.update_state(ApplicationState.POST_WAIT) - await asyncio.wait( - {e.wait() for e in self.model.postinstall_events}) + await self.model.wait_postinstall() self.app.update_state(ApplicationState.POST_RUNNING) From e7d2959625e12fb37f0bb769a4ae340fcb188549 Mon Sep 17 00:00:00 2001 From: Michael Hudson-Doyle Date: Thu, 5 Aug 2021 14:01:32 +1200 Subject: [PATCH 2/5] have SubiquityModel know which models are required for which variants --- subiquity/models/subiquity.py | 95 +++++++++++------------- subiquity/models/tests/test_subiquity.py | 52 ++++++++++--- subiquity/server/controller.py | 1 - subiquity/server/controllers/timezone.py | 1 - subiquity/server/server.py | 33 ++++++-- 5 files changed, 115 insertions(+), 67 deletions(-) diff --git a/subiquity/models/subiquity.py b/subiquity/models/subiquity.py index ccc76a43..1caea314 100644 --- a/subiquity/models/subiquity.py +++ b/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) -POSTINSTALL_MODEL_NAMES = [ - "identity", - "locale", - "packages", - "snaplist", - "ssh", - "timezone", - "userdata", - ] +class ModelNames: + def __init__(self, default_names, **per_variant_names): + self.default_names = default_names + self.per_variant_names = per_variant_names -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 @@ -131,41 +122,45 @@ class SubiquityModel: self._confirmation = asyncio.Event() - self._events = { - name: asyncio.Event() for name in ALL_MODEL_NAMES - } - self._install_events = { - self._events[name] for name in INSTALL_MODEL_NAMES - } - self._postinstall_events = { - self._events[name] for name in POSTINSTALL_MODEL_NAMES - } + self._configured_names = set() + self._install_model_names = install_model_names + self._postinstall_model_names = postinstall_model_names + self._cur_install_model_names = install_model_names.default_names + self._cur_postinstall_model_names = \ + postinstall_model_names.default_names + self._install_event = asyncio.Event() + 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) def configured(self, model_name): - if model_name not in ALL_MODEL_NAMES: - return - self._events[model_name].set() - if model_name in INSTALL_MODEL_NAMES: + self._configured_names.add(model_name) + if model_name in self._cur_install_model_names: stage = 'install' - unconfigured = { - mn for mn in INSTALL_MODEL_NAMES - if not self._events[mn].is_set() - } - elif model_name in POSTINSTALL_MODEL_NAMES: + names = self._cur_install_model_names + event = self._install_event + elif model_name in self._cur_postinstall_model_names: stage = 'postinstall' - unconfigured = { - mn for mn in POSTINSTALL_MODEL_NAMES - if not self._events[mn].is_set() - } + names = self._cur_postinstall_model_names + event = self._postinstall_event + 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() async def wait_install(self): - await asyncio.wait({e.wait() for e in self._install_events}) + await self._install_event.wait() async def wait_postinstall(self): - await asyncio.wait({e.wait() for e in self._postinstall_events}) + await self._postinstall_event.wait() async def wait_confirmation(self): await self._confirmation.wait() @@ -173,7 +168,7 @@ class SubiquityModel: def needs_configuration(self, model_name): if model_name is None: return False - return not self._events[model_name].is_set() + return model_name not in self._configured_names def confirm(self): self._confirmation.set() @@ -223,7 +218,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()) @@ -350,7 +345,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()) diff --git a/subiquity/models/tests/test_subiquity.py b/subiquity/models/tests/test_subiquity.py index 96839882..f0c79a65 100644 --- a/subiquity/models/tests/test_subiquity.py +++ b/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_proxy = SubiquityModel('test') + model_no_proxy = self.make_model() + 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') diff --git a/subiquity/server/controller.py b/subiquity/server/controller.py index 085c2e75..90e9eabf 100644 --- a/subiquity/server/controller.py +++ b/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) diff --git a/subiquity/server/controllers/timezone.py b/subiquity/server/controllers/timezone.py index 64a0a611..b3403003 100644 --- a/subiquity/server/controllers/timezone.py +++ b/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) diff --git a/subiquity/server/server.py b/subiquity/server/server.py index b433ff10..f0b1a28e 100644 --- a/subiquity/server/server.py +++ b/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: - if variant not in controller.relevant_variants: - controller.configured() + self.app.base_model.set_source_variant(variant) 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) From 9080a568048ef6ef0d64a5bbb96c1046e1b33650 Mon Sep 17 00:00:00 2001 From: Michael Hudson-Doyle Date: Thu, 5 Aug 2021 16:18:18 +1200 Subject: [PATCH 3/5] tweak when x-status: confirm gets sent --- subiquity/models/subiquity.py | 7 +++---- subiquity/server/server.py | 2 +- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/subiquity/models/subiquity.py b/subiquity/models/subiquity.py index 1caea314..98bd1ef4 100644 --- a/subiquity/models/subiquity.py +++ b/subiquity/models/subiquity.py @@ -165,10 +165,9 @@ class SubiquityModel: async def wait_confirmation(self): await self._confirmation.wait() - def needs_configuration(self, model_name): - if model_name is None: - return False - return model_name not in self._configured_names + 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() diff --git a/subiquity/server/server.py b/subiquity/server/server.py index f0b1a28e..a46ca3a9 100644 --- a/subiquity/server/server.py +++ b/subiquity/server/server.py @@ -376,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}) From a5ae411c556dd7412cc14b3e65fad76707b4d5e1 Mon Sep 17 00:00:00 2001 From: Michael Hudson-Doyle Date: Thu, 5 Aug 2021 14:20:14 +1200 Subject: [PATCH 4/5] allow app state transition from NEEDS_CONFIRMATION to WAITING consider the following scenario, admittedly one that is not possible today: * There is an "install model" that is required for server and not desktop (let's say "mirror") * The user initially selects a desktop install and moves through the screens until they are asked for confirmation. * At this point the user moves back through the screens and selects a server variant. Now the application state of NEEDS_CONFIRMATION is misleading; the state needs to move back to WAITING until the mirror model is configured. This is all probably excessively general but I feel like the core control flow of the installer needs to be able to handle this sort of thing... --- subiquity/models/subiquity.py | 22 +++++++++++++++++++++- subiquity/server/controllers/install.py | 16 +++++++++------- 2 files changed, 30 insertions(+), 8 deletions(-) diff --git a/subiquity/models/subiquity.py b/subiquity/models/subiquity.py index 98bd1ef4..93c93470 100644 --- a/subiquity/models/subiquity.py +++ b/subiquity/models/subiquity.py @@ -121,6 +121,7 @@ class SubiquityModel: self.userdata = {} self._confirmation = asyncio.Event() + self._confirmation_task = None self._configured_names = set() self._install_model_names = install_model_names @@ -136,6 +137,15 @@ class SubiquityModel: 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): self._configured_names.add(model_name) @@ -163,7 +173,17 @@ class SubiquityModel: await self._postinstall_event.wait() async def wait_confirmation(self): - await self._confirmation.wait() + 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 + 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 \ diff --git a/subiquity/server/controllers/install.py b/subiquity/server/controllers/install.py index 747ba2e5..42a0f197 100644 --- a/subiquity/server/controllers/install.py +++ b/subiquity/server/controllers/install.py @@ -199,17 +199,19 @@ class InstallController(SubiquityController): async def install(self, *, context): context.set('is-install-context', True) try: - self.app.update_state(ApplicationState.WAITING) + while True: + self.app.update_state(ApplicationState.WAITING) - await self.model.wait_install() + await self.model.wait_install() - if not self.app.interactive: - if 'autoinstall' in self.app.kernel_cmdline: - self.model.confirm() + if not self.app.interactive: + if 'autoinstall' in self.app.kernel_cmdline: + self.model.confirm() - self.app.update_state(ApplicationState.NEEDS_CONFIRMATION) + self.app.update_state(ApplicationState.NEEDS_CONFIRMATION) - await self.model.wait_confirmation() + if await self.model.wait_confirmation(): + break self.app.update_state(ApplicationState.RUNNING) From 545d5cc4c10f8a957ecf32c42a526d4ff74b0688 Mon Sep 17 00:00:00 2001 From: Michael Hudson-Doyle Date: Thu, 5 Aug 2021 15:55:19 +1200 Subject: [PATCH 5/5] expand DESIGN.md a bit --- DESIGN.md | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/DESIGN.md b/DESIGN.md index 0c1324f2..232af8c9 100644 --- a/DESIGN.md +++ b/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