From b2cd25b299bae492a49b156e222d1b2762174e82 Mon Sep 17 00:00:00 2001 From: Michael Hudson-Doyle Date: Wed, 24 Nov 2021 13:31:30 +1300 Subject: [PATCH 01/19] skeleton of a ubuntu-drivers controller and api [Olivier Gayot] * update copyright year 2018 -> 2022 * drop explicit inheritance from object Signed-off-by: Olivier Gayot --- autoinstall-schema.json | 3 ++ subiquity/common/apidef.py | 4 ++ subiquity/models/drivers.py | 22 +++++++++++ subiquity/models/subiquity.py | 2 + subiquity/server/controllers/__init__.py | 2 + subiquity/server/controllers/drivers.py | 50 ++++++++++++++++++++++++ subiquity/server/server.py | 2 + 7 files changed, 85 insertions(+) create mode 100644 subiquity/models/drivers.py create mode 100644 subiquity/server/controllers/drivers.py diff --git a/autoinstall-schema.json b/autoinstall-schema.json index a331f065..69a402a9 100644 --- a/autoinstall-schema.json +++ b/autoinstall-schema.json @@ -395,6 +395,9 @@ "additionalProperties": false } }, + "drivers": { + "type": "boolean" + }, "timezone": { "type": "string" }, diff --git a/subiquity/common/apidef.py b/subiquity/common/apidef.py index 8ad2fabe..8d850805 100644 --- a/subiquity/common/apidef.py +++ b/subiquity/common/apidef.py @@ -293,6 +293,10 @@ class API: def POST(data: Payload[ModifyPartitionV2]) \ -> StorageResponseV2: ... + class drivers: + def GET(wait: bool = False) -> bool: ... + def POST(install: bool): ... + class snaplist: def GET(wait: bool = False) -> SnapListResponse: ... def POST(data: Payload[List[SnapSelection]]): ... diff --git a/subiquity/models/drivers.py b/subiquity/models/drivers.py new file mode 100644 index 00000000..30167d9f --- /dev/null +++ b/subiquity/models/drivers.py @@ -0,0 +1,22 @@ +# Copyright 2021 Canonical, Ltd. +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU Affero General Public License as +# published by the Free Software Foundation, either version 3 of the +# License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU Affero General Public License for more details. +# +# You should have received a copy of the GNU Affero General Public License +# along with this program. If not, see . + +import logging + +log = logging.getLogger('subiquity.models.drivers') + + +class DriversModel: + do_install = False diff --git a/subiquity/models/subiquity.py b/subiquity/models/subiquity.py index fffc4bad..1302df9c 100644 --- a/subiquity/models/subiquity.py +++ b/subiquity/models/subiquity.py @@ -30,6 +30,7 @@ from subiquitycore.file_util import write_file from subiquity.common.resources import get_users_and_groups from subiquity.server.types import InstallerChannels +from .drivers import DriversModel from .filesystem import FilesystemModel from .identity import IdentityModel from .kernel import KernelModel @@ -132,6 +133,7 @@ class SubiquityModel: self.chroot_prefix = [] self.debconf_selections = DebconfSelectionsModel() + self.drivers = DriversModel() self.filesystem = FilesystemModel() self.identity = IdentityModel() self.kernel = KernelModel() diff --git a/subiquity/server/controllers/__init__.py b/subiquity/server/controllers/__init__.py index 3e0e65d3..323b920a 100644 --- a/subiquity/server/controllers/__init__.py +++ b/subiquity/server/controllers/__init__.py @@ -15,6 +15,7 @@ from .cmdlist import EarlyController, LateController, ErrorController from .debconf import DebconfController +from .drivers import DriversController from .filesystem import FilesystemController from .identity import IdentityController from .install import InstallController @@ -39,6 +40,7 @@ from .zdev import ZdevController __all__ = [ 'DebconfController', + 'DriversController', 'EarlyController', 'ErrorController', 'FilesystemController', diff --git a/subiquity/server/controllers/drivers.py b/subiquity/server/controllers/drivers.py new file mode 100644 index 00000000..5615b6c4 --- /dev/null +++ b/subiquity/server/controllers/drivers.py @@ -0,0 +1,50 @@ +# Copyright 2022 Canonical, Ltd. +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU Affero General Public License as +# published by the Free Software Foundation, either version 3 of the +# License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU Affero General Public License for more details. +# +# You should have received a copy of the GNU Affero General Public License +# along with this program. If not, see . + +import asyncio +import logging + +from subiquity.common.apidef import API +from subiquity.server.controller import SubiquityController + + +log = logging.getLogger('subiquity.server.controllers.drivers') + + +class DriversController(SubiquityController): + + endpoint = API.drivers + + autoinstall_key = model_name = "drivers" + autoinstall_schema = { + 'type': 'boolean', + } + autoinstall_default = False + + def make_autoinstall(self): + return self.model.do_install + + def load_autoinstall_data(self, data): + self.model.do_install = data + + def start(self): + asyncio.create_task(self.configured()) + + async def GET(self, wait: bool = False) -> bool: + return False + + async def POST(self, install: bool): + self.model.do_install = install + await self.configured() diff --git a/subiquity/server/server.py b/subiquity/server/server.py index 2b3639a0..1e9df82e 100644 --- a/subiquity/server/server.py +++ b/subiquity/server/server.py @@ -200,6 +200,7 @@ INSTALL_MODEL_NAMES = ModelNames({ }) POSTINSTALL_MODEL_NAMES = ModelNames({ + "drivers", "identity", "locale", "network", @@ -252,6 +253,7 @@ class SubiquityServer(Application): "Identity", "SSH", "SnapList", + "Drivers", "TimeZone", "Install", "Updates", From 5a4141050a221c1bf968b6440ec79c59ae7f4d3c Mon Sep 17 00:00:00 2001 From: Michael Hudson-Doyle Date: Wed, 24 Nov 2021 15:11:54 +1300 Subject: [PATCH 02/19] actually run ubuntu-drivers --- subiquity/common/apidef.py | 2 +- subiquity/server/controllers/drivers.py | 34 ++++++++++++++++++++++--- subiquity/server/controllers/install.py | 10 ++++++-- subiquity/server/types.py | 1 + 4 files changed, 40 insertions(+), 7 deletions(-) diff --git a/subiquity/common/apidef.py b/subiquity/common/apidef.py index 8d850805..5ec2ffb7 100644 --- a/subiquity/common/apidef.py +++ b/subiquity/common/apidef.py @@ -294,7 +294,7 @@ class API: -> StorageResponseV2: ... class drivers: - def GET(wait: bool = False) -> bool: ... + def GET(wait: bool = False) -> Optional[bool]: ... def POST(install: bool): ... class snaplist: diff --git a/subiquity/server/controllers/drivers.py b/subiquity/server/controllers/drivers.py index 5615b6c4..4c4d631f 100644 --- a/subiquity/server/controllers/drivers.py +++ b/subiquity/server/controllers/drivers.py @@ -15,10 +15,14 @@ import asyncio import logging +from typing import Optional + +from subiquitycore.context import with_context +from subiquitycore.utils import arun_command from subiquity.common.apidef import API from subiquity.server.controller import SubiquityController - +from subiquity.server.types import InstallerChannels log = logging.getLogger('subiquity.server.controllers.drivers') @@ -33,6 +37,8 @@ class DriversController(SubiquityController): } autoinstall_default = False + has_drivers = None + def make_autoinstall(self): return self.model.do_install @@ -40,10 +46,30 @@ class DriversController(SubiquityController): self.model.do_install = data def start(self): - asyncio.create_task(self.configured()) + self.app.hub.subscribe( + InstallerChannels.APT_CONFIGURED, + self._wait_apt_configured) - async def GET(self, wait: bool = False) -> bool: - return False + def _wait_apt_configured(self): + self._drivers_task = asyncio.create_task(self._list_drivers()) + + @with_context() + async def _list_drivers(self, context): + path = self.app.controllers.Install.for_install_path + cmd = ['chroot', path, 'ubuntu-drivers', 'list'] + if self.app.base_model.source.current.variant == 'server': + cmd.append('--gpgpu') + if self.app.opts.dry_run: + del cmd[:2] + result = await arun_command(cmd) + self.has_drivers = bool(result.stdout.strip()) + if not self.has_drivers: + await self.configured() + + async def GET(self, wait: bool = False) -> Optional[bool]: + if wait: + await self._drivers_task + return self.has_drivers async def POST(self, install: bool): self.model.do_install = install diff --git a/subiquity/server/controllers/install.py b/subiquity/server/controllers/install.py index 48efc7f0..d853b31f 100644 --- a/subiquity/server/controllers/install.py +++ b/subiquity/server/controllers/install.py @@ -45,6 +45,10 @@ from subiquity.server.curtin import ( run_curtin_command, start_curtin_command, ) +from subiquity.server.types import ( + InstallerChannels, + ) + log = logging.getLogger("subiquity.server.controllers.install") @@ -151,14 +155,16 @@ class InstallController(SubiquityController): self.app.update_state(ApplicationState.RUNNING) - for_install_path = await self.configure_apt(context=context) + self.for_install_path = await self.configure_apt(context=context) + + await self.app.hub.abroadcast(InstallerChannels.APT_CONFIGURED) if os.path.exists(self.model.target): await self.unmount_target( context=context, target=self.model.target) await self.curtin_install( - context=context, source='cp://' + for_install_path) + context=context, source='cp://' + self.for_install_path) self.app.update_state(ApplicationState.POST_WAIT) diff --git a/subiquity/server/types.py b/subiquity/server/types.py index 0078f66f..baf92e18 100644 --- a/subiquity/server/types.py +++ b/subiquity/server/types.py @@ -21,3 +21,4 @@ class InstallerChannels(CoreChannels): SNAPD_NETWORK_CHANGE = 'snapd-network-change' GEOIP = 'geoip' CONFIGURED = 'configured' + APT_CONFIGURED = 'apt-configured' From 9936fdb77f5b6e54045c7cc45b764b8dd213abde Mon Sep 17 00:00:00 2001 From: Michael Hudson-Doyle Date: Wed, 24 Nov 2021 15:54:30 +1300 Subject: [PATCH 03/19] actually run ubuntu-drivers install --- subiquity/server/controllers/install.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/subiquity/server/controllers/install.py b/subiquity/server/controllers/install.py index d853b31f..fd421693 100644 --- a/subiquity/server/controllers/install.py +++ b/subiquity/server/controllers/install.py @@ -196,6 +196,16 @@ class InstallController(SubiquityController): packages = await self.get_target_packages(context=context) for package in packages: await self.install_package(context=context, package=package) + if self.model.drivers.do_install: + with context.child( + "ubuntu-drivers-install", + "installing third-party drivers") as child: + cmd = ["ubuntu-drivers", "install"] + if self.model.source.current.variant == 'server': + cmd.append('--gpgpu') + await start_curtin_command( + self.app, child, "in-target", "-t", self.tpath(), + "--", *cmd) if self.model.network.has_network: self.app.update_state(ApplicationState.UU_RUNNING) From 6e65c801d4759f591e2775535f41da0216c5a186 Mon Sep 17 00:00:00 2001 From: Michael Hudson-Doyle Date: Wed, 24 Nov 2021 15:46:16 +1300 Subject: [PATCH 04/19] only run ubuntu-drivers in dry-run mode if asked for --- subiquity/server/controllers/drivers.py | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/subiquity/server/controllers/drivers.py b/subiquity/server/controllers/drivers.py index 4c4d631f..246bfeda 100644 --- a/subiquity/server/controllers/drivers.py +++ b/subiquity/server/controllers/drivers.py @@ -46,21 +46,30 @@ class DriversController(SubiquityController): self.model.do_install = data def start(self): + self._wait_apt = asyncio.Event() self.app.hub.subscribe( InstallerChannels.APT_CONFIGURED, - self._wait_apt_configured) - - def _wait_apt_configured(self): + self._wait_apt.set) self._drivers_task = asyncio.create_task(self._list_drivers()) @with_context() async def _list_drivers(self, context): + with context.child("wait_apt"): + await self._wait_apt.wait() path = self.app.controllers.Install.for_install_path cmd = ['chroot', path, 'ubuntu-drivers', 'list'] if self.app.base_model.source.current.variant == 'server': cmd.append('--gpgpu') if self.app.opts.dry_run: - del cmd[:2] + if 'has-drivers' in self.app.debug_flags: + self.has_drivers = True + return + elif 'run-drivers' in self.app.debug_flags: + del cmd[:2] + else: + self.has_drivers = False + await self.configured() + return result = await arun_command(cmd) self.has_drivers = bool(result.stdout.strip()) if not self.has_drivers: From ad26914a607e424d713ef675a2efed5a29f2b8d6 Mon Sep 17 00:00:00 2001 From: Michael Hudson-Doyle Date: Thu, 25 Nov 2021 16:11:40 +1300 Subject: [PATCH 05/19] slightly insane gyrations to invoke ubuntu-drivers in better environment --- subiquity/server/apt.py | 33 +++++++++++++++++++------ subiquity/server/controllers/drivers.py | 13 ++++++---- subiquity/server/controllers/install.py | 6 ++--- subiquity/server/curtin.py | 21 +++++++++------- subiquity/server/runner.py | 31 +++++++++++++++-------- 5 files changed, 69 insertions(+), 35 deletions(-) diff --git a/subiquity/server/apt.py b/subiquity/server/apt.py index 9944e4bb..6dc27583 100644 --- a/subiquity/server/apt.py +++ b/subiquity/server/apt.py @@ -13,6 +13,7 @@ # You should have received a copy of the GNU Affero General Public License # along with this program. If not, see . +import contextlib import functools import logging import os @@ -153,7 +154,9 @@ class AptConfigurer: self._mounts.append(m) return m - async def unmount(self, mountpoint: str): + async def unmount(self, mountpoint: str, remove=True): + if remove: + self._mounts.remove(mountpoint) await self.app.command_runner.run(['umount', mountpoint]) async def setup_overlay(self, lowers: List[Lower]) -> OverlayMountpoint: @@ -224,11 +227,27 @@ class AptConfigurer: return self.install_tree.p() + @contextlib.asynccontextmanager + async def overlay(self): + overlay = await self.setup_overlay([ + self.install_tree.upperdir, + self.configured_tree.upperdir, + self.source + ]) + try: + yield overlay + finally: + await self.unmount(overlay.mountpoint) + async def cleanup(self): for m in reversed(self._mounts): - await self.unmount(m.mountpoint) + await self.unmount(m.mountpoint, remove=False) for d in self._tdirs: shutil.rmtree(d) + if self.app.base_model.network.has_network: + await run_curtin_command( + self.app, context, "in-target", "-t", target, + "--", "apt-get", "update") async def deconfigure(self, context, target: str) -> None: target_mnt = Mountpoint(mountpoint=target) @@ -239,7 +258,7 @@ class AptConfigurer: 'cp', '-aT', self.configured_tree.p(dir), target_mnt.p(dir), ]) - await self.unmount(target_mnt.p('cdrom')) + await self.unmount(target_mnt.p('cdrom'), remove=False) os.rmdir(target_mnt.p('cdrom')) await _restore_dir('etc/apt') @@ -253,14 +272,12 @@ class AptConfigurer: await self.cleanup() - if self.app.base_model.network.has_network: - await run_curtin_command( - self.app, context, "in-target", "-t", target_mnt.p(), - "--", "apt-get", "update") - class DryRunAptConfigurer(AptConfigurer): + async def unmount(self, mountpoint: str, remove=True): + pass + async def setup_overlay(self, lowers: List[Lower]) -> OverlayMountpoint: # XXX This implementation expects that: # - on first invocation, the lowers list contains a single string diff --git a/subiquity/server/controllers/drivers.py b/subiquity/server/controllers/drivers.py index 246bfeda..5932d826 100644 --- a/subiquity/server/controllers/drivers.py +++ b/subiquity/server/controllers/drivers.py @@ -18,10 +18,10 @@ import logging from typing import Optional from subiquitycore.context import with_context -from subiquitycore.utils import arun_command from subiquity.common.apidef import API from subiquity.server.controller import SubiquityController +from subiquity.server.curtin import run_curtin_command from subiquity.server.types import InstallerChannels log = logging.getLogger('subiquity.server.controllers.drivers') @@ -56,8 +56,8 @@ class DriversController(SubiquityController): async def _list_drivers(self, context): with context.child("wait_apt"): await self._wait_apt.wait() - path = self.app.controllers.Install.for_install_path - cmd = ['chroot', path, 'ubuntu-drivers', 'list'] + apt = self.app.controllers.Mirror.apt_configurer + cmd = ['ubuntu-drivers', 'list'] if self.app.base_model.source.current.variant == 'server': cmd.append('--gpgpu') if self.app.opts.dry_run: @@ -65,12 +65,15 @@ class DriversController(SubiquityController): self.has_drivers = True return elif 'run-drivers' in self.app.debug_flags: - del cmd[:2] + pass else: self.has_drivers = False await self.configured() return - result = await arun_command(cmd) + async with apt.overlay() as d: + result = await run_curtin_command( + self.app, context, "in-target", "-t", d.mountpoint, + "--", *cmd, capture=True) self.has_drivers = bool(result.stdout.strip()) if not self.has_drivers: await self.configured() diff --git a/subiquity/server/controllers/install.py b/subiquity/server/controllers/install.py index fd421693..387a5c10 100644 --- a/subiquity/server/controllers/install.py +++ b/subiquity/server/controllers/install.py @@ -155,7 +155,7 @@ class InstallController(SubiquityController): self.app.update_state(ApplicationState.RUNNING) - self.for_install_path = await self.configure_apt(context=context) + for_install_path = await self.configure_apt(context=context) await self.app.hub.abroadcast(InstallerChannels.APT_CONFIGURED) @@ -164,7 +164,7 @@ class InstallController(SubiquityController): context=context, target=self.model.target) await self.curtin_install( - context=context, source='cp://' + self.for_install_path) + context=context, source='cp://' + for_install_path) self.app.update_state(ApplicationState.POST_WAIT) @@ -203,7 +203,7 @@ class InstallController(SubiquityController): cmd = ["ubuntu-drivers", "install"] if self.model.source.current.variant == 'server': cmd.append('--gpgpu') - await start_curtin_command( + await run_curtin_command( self.app, child, "in-target", "-t", self.tpath(), "--", *cmd) diff --git a/subiquity/server/curtin.py b/subiquity/server/curtin.py index 512bc308..d704f93a 100644 --- a/subiquity/server/curtin.py +++ b/subiquity/server/curtin.py @@ -100,17 +100,17 @@ class _CurtinCommand: cmd.extend(args) return cmd - async def start(self, context): + async def start(self, context, **opts): self._fd = journald_listen( asyncio.get_event_loop(), [self._event_syslog_id], self._event) # Yield to the event loop before starting curtin to avoid missing the # first couple of events. await asyncio.sleep(0) self._event_contexts[''] = context - self.proc = await self.runner.start(self._cmd) + self.proc = await self.runner.start(self._cmd, **opts) async def wait(self): - await self.runner.wait(self.proc) + result = await self.runner.wait(self.proc) waited = 0.0 while len(self._event_contexts) > 1 and waited < 5.0: await asyncio.sleep(0.1) @@ -118,10 +118,11 @@ class _CurtinCommand: log.debug("waited %s seconds for events to drain", waited) self._event_contexts.pop('', None) asyncio.get_event_loop().remove_reader(self._fd) + return result async def run(self, context): await self.start(context) - await self.wait() + return await self.wait() class _DryRunCurtinCommand(_CurtinCommand): @@ -146,7 +147,8 @@ class _FailingDryRunCurtinCommand(_DryRunCurtinCommand): event_file = 'examples/curtin-events-fail.json' -async def start_curtin_command(app, context, command, *args, config=None): +async def start_curtin_command( + app, context, command, *args, config=None, **opts): if app.opts.dry_run: if 'install-fail' in app.debug_flags: cls = _FailingDryRunCurtinCommand @@ -156,11 +158,12 @@ async def start_curtin_command(app, context, command, *args, config=None): cls = _CurtinCommand curtin_cmd = cls(app.opts, app.command_runner, command, *args, config=config) - await curtin_cmd.start(context) + await curtin_cmd.start(context, **opts) return curtin_cmd -async def run_curtin_command(app, context, command, *args, config=None): +async def run_curtin_command( + app, context, command, *args, config=None, **opts): cmd = await start_curtin_command( - app, context, command, *args, config=config) - await cmd.wait() + app, context, command, *args, config=config, **opts) + return await cmd.wait() diff --git a/subiquity/server/runner.py b/subiquity/server/runner.py index bc391eff..db103fe7 100644 --- a/subiquity/server/runner.py +++ b/subiquity/server/runner.py @@ -24,22 +24,27 @@ class LoggedCommandRunner: def __init__(self, ident): self.ident = ident - async def start(self, cmd): - proc = await astart_command([ - 'systemd-cat', '--level-prefix=false', '--identifier='+self.ident, - ] + cmd) + async def start(self, cmd, *, capture=False): + if not capture: + cmd = [ + 'systemd-cat', + '--level-prefix=false', + '--identifier='+self.ident, + ] + cmd + proc = await astart_command(cmd) proc.args = cmd return proc async def wait(self, proc): - await proc.communicate() + stdout, stderr = await proc.communicate() if proc.returncode != 0: raise subprocess.CalledProcessError(proc.returncode, proc.args) else: - return subprocess.CompletedProcess(proc.args, proc.returncode) + return subprocess.CompletedProcess( + proc.args, proc.returncode, stdout=stdout, stderr=stderr) - async def run(self, cmd): - proc = await self.start(cmd) + async def run(self, cmd, **opts): + proc = await self.start(cmd, **opts) return await self.wait(proc) @@ -49,16 +54,22 @@ class DryRunCommandRunner(LoggedCommandRunner): super().__init__(ident) self.delay = delay - async def start(self, cmd): + async def start(self, cmd, *, capture=False): if 'scripts/replay-curtin-log.py' in cmd: delay = 0 + elif cmd[-3:] == ['ubuntu-drivers', 'list', '--gpgpu']: + cmd = cmd[-3:] + delay = 0 + elif cmd[-2:] == ['ubuntu-drivers', 'list']: + cmd = cmd[-2:] + delay = 0 else: cmd = ['echo', 'not running:'] + cmd if 'unattended-upgrades' in cmd: delay = 3*self.delay else: delay = self.delay - proc = await super().start(cmd) + proc = await super().start(cmd, capture=capture) await asyncio.sleep(delay) return proc From e26f35ad109030a9ce241cd884d07cb292afe5a6 Mon Sep 17 00:00:00 2001 From: Michael Hudson-Doyle Date: Fri, 26 Nov 2021 15:41:15 +1300 Subject: [PATCH 06/19] remove duplicate lines --- subiquity/server/apt.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/subiquity/server/apt.py b/subiquity/server/apt.py index 6dc27583..48fa64ec 100644 --- a/subiquity/server/apt.py +++ b/subiquity/server/apt.py @@ -244,10 +244,6 @@ class AptConfigurer: await self.unmount(m.mountpoint, remove=False) for d in self._tdirs: shutil.rmtree(d) - if self.app.base_model.network.has_network: - await run_curtin_command( - self.app, context, "in-target", "-t", target, - "--", "apt-get", "update") async def deconfigure(self, context, target: str) -> None: target_mnt = Mountpoint(mountpoint=target) From 43f71a02be700a9599be6353ac9bab710a1a08e0 Mon Sep 17 00:00:00 2001 From: Olivier Gayot Date: Wed, 2 Feb 2022 15:54:54 +0100 Subject: [PATCH 07/19] Improve autoinstall schema for third-party drivers The autoinstall data for the drivers was only constituted of a boolean, e.g.: drivers: true or drivers: false It was not immediately clear that the boolean value meant (install/don't install the drivers). We now store the boolean in the "install" sub-element instead, e.g.: drivers: - install: true drivers: - install: false Signed-off-by: Olivier Gayot --- autoinstall-schema.json | 7 ++++++- subiquity/server/controllers/drivers.py | 16 ++++++++++++---- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/autoinstall-schema.json b/autoinstall-schema.json index 69a402a9..cd950b54 100644 --- a/autoinstall-schema.json +++ b/autoinstall-schema.json @@ -396,7 +396,12 @@ } }, "drivers": { - "type": "boolean" + "type": "object", + "properties": { + "install": { + "type": "boolean" + } + } }, "timezone": { "type": "string" diff --git a/subiquity/server/controllers/drivers.py b/subiquity/server/controllers/drivers.py index 5932d826..7b64e37d 100644 --- a/subiquity/server/controllers/drivers.py +++ b/subiquity/server/controllers/drivers.py @@ -33,17 +33,25 @@ class DriversController(SubiquityController): autoinstall_key = model_name = "drivers" autoinstall_schema = { - 'type': 'boolean', + 'type': 'object', + 'properties': { + 'install': { + 'type': 'boolean', + }, + }, } - autoinstall_default = False + autoinstall_default = {"install": False} has_drivers = None def make_autoinstall(self): - return self.model.do_install + return { + "install": self.model.do_install, + } def load_autoinstall_data(self, data): - self.model.do_install = data + if data is not None and "install" in data: + self.model.do_install = data["install"] def start(self): self._wait_apt = asyncio.Event() From 6f183a39ca5252e009830403ca4140d0cd66bbeb Mon Sep 17 00:00:00 2001 From: Olivier Gayot Date: Tue, 25 Jan 2022 14:34:36 +0100 Subject: [PATCH 08/19] Add do_install state in API response for drivers The drivers GET call returned only the value of "has_drivers" (when known). In order to have the client be able to know the current value of install/do_install, we now embed the value into the response as well: Before, the call would return either: true -> meaning that there are drivers available to install false -> meaning that there are no drivers available to install null -> retry later - we don't know yet Now we have: { "has_drivers": boolean or null if we don't know yet "install": boolean } Signed-off-by: Olivier Gayot --- subiquity/common/apidef.py | 5 +++-- subiquity/common/types.py | 11 +++++++++++ subiquity/server/controllers/drivers.py | 10 ++++++---- 3 files changed, 20 insertions(+), 6 deletions(-) diff --git a/subiquity/common/apidef.py b/subiquity/common/apidef.py index 5ec2ffb7..bb476174 100644 --- a/subiquity/common/apidef.py +++ b/subiquity/common/apidef.py @@ -39,6 +39,7 @@ from subiquity.common.types import ( ModifyPartitionV2, RefreshStatus, ShutdownMode, + DriversResponse, SnapInfo, SnapListResponse, SnapSelection, @@ -294,8 +295,8 @@ class API: -> StorageResponseV2: ... class drivers: - def GET(wait: bool = False) -> Optional[bool]: ... - def POST(install: bool): ... + def GET(wait: bool = False) -> DriversResponse: ... + def POST(install: bool) -> None: ... class snaplist: def GET(wait: bool = False) -> SnapListResponse: ... diff --git a/subiquity/common/types.py b/subiquity/common/types.py index ffc5ceed..9f2a870a 100644 --- a/subiquity/common/types.py +++ b/subiquity/common/types.py @@ -369,6 +369,17 @@ class SnapInfo: channels: List[ChannelSnapInfo] = attr.Factory(list) +@attr.s(auto_attribs=True) +class DriversResponse: + """ Response to GET request to drivers. + :install: tells whether third-party drivers will be installed (if any is + available). + :has_drivers: tells if any third-party driver is available. It will bet set + to None until we figure out what drivers are available. """ + install: bool + has_drivers: Optional[bool] + + @attr.s(auto_attribs=True) class SnapSelection: name: str diff --git a/subiquity/server/controllers/drivers.py b/subiquity/server/controllers/drivers.py index 7b64e37d..b8d439de 100644 --- a/subiquity/server/controllers/drivers.py +++ b/subiquity/server/controllers/drivers.py @@ -20,6 +20,7 @@ from typing import Optional from subiquitycore.context import with_context from subiquity.common.apidef import API +from subiquity.common.types import DriversResponse from subiquity.server.controller import SubiquityController from subiquity.server.curtin import run_curtin_command from subiquity.server.types import InstallerChannels @@ -42,7 +43,7 @@ class DriversController(SubiquityController): } autoinstall_default = {"install": False} - has_drivers = None + has_drivers: Optional[bool] = None def make_autoinstall(self): return { @@ -86,11 +87,12 @@ class DriversController(SubiquityController): if not self.has_drivers: await self.configured() - async def GET(self, wait: bool = False) -> Optional[bool]: + async def GET(self, wait: bool = False) -> DriversResponse: if wait: await self._drivers_task - return self.has_drivers + return DriversResponse(install=self.model.do_install, + has_drivers=self.has_drivers) - async def POST(self, install: bool): + async def POST(self, install: bool) -> None: self.model.do_install = install await self.configured() From 6241144681bb4a03b7329fb5cacb472cd8131809 Mon Sep 17 00:00:00 2001 From: Michael Hudson-Doyle Date: Mon, 29 Nov 2021 15:33:06 +1300 Subject: [PATCH 09/19] check for ubuntu-drivers before trying to run it [Olivier Gayot] * Drop TODO that is now implemented --- subiquity/server/controllers/drivers.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/subiquity/server/controllers/drivers.py b/subiquity/server/controllers/drivers.py index b8d439de..fb6ca838 100644 --- a/subiquity/server/controllers/drivers.py +++ b/subiquity/server/controllers/drivers.py @@ -15,6 +15,7 @@ import asyncio import logging +import subprocess from typing import Optional from subiquitycore.context import with_context @@ -80,6 +81,15 @@ class DriversController(SubiquityController): await self.configured() return async with apt.overlay() as d: + try: + await self.app.command_runner.run( + ['chroot', d.mountpoint, + 'sh', '-c', + "command -v ubuntu-drivers"]) + except subprocess.CalledProcessError: + self.has_drivers = False + await self.configured() + return result = await run_curtin_command( self.app, context, "in-target", "-t", d.mountpoint, "--", *cmd, capture=True) From abf05cf31e14a2920fde65843123ed47a8055935 Mon Sep 17 00:00:00 2001 From: Olivier Gayot Date: Wed, 26 Jan 2022 15:06:16 +0100 Subject: [PATCH 10/19] Pass the list of drivers instead of a boolean In the reponse to the /a/drivers GET call, we now include a list of packages / drivers instead of just storing a boolean telling if any driver has been found. Each element from the list corresponds to the name of a package to install. Signed-off-by: Olivier Gayot --- subiquity/common/types.py | 8 +++++--- subiquity/server/controllers/drivers.py | 27 ++++++++++++++++--------- subiquity/server/runner.py | 8 ++++---- 3 files changed, 27 insertions(+), 16 deletions(-) diff --git a/subiquity/common/types.py b/subiquity/common/types.py index 9f2a870a..713e7c17 100644 --- a/subiquity/common/types.py +++ b/subiquity/common/types.py @@ -374,10 +374,12 @@ class DriversResponse: """ Response to GET request to drivers. :install: tells whether third-party drivers will be installed (if any is available). - :has_drivers: tells if any third-party driver is available. It will bet set - to None until we figure out what drivers are available. """ + :drivers: tells what third-party drivers will be installed should we decide + to do it. It will bet set to None until we figure out what drivers are + available. + """ install: bool - has_drivers: Optional[bool] + drivers: Optional[List[str]] @attr.s(auto_attribs=True) diff --git a/subiquity/server/controllers/drivers.py b/subiquity/server/controllers/drivers.py index fb6ca838..f82c90ed 100644 --- a/subiquity/server/controllers/drivers.py +++ b/subiquity/server/controllers/drivers.py @@ -16,7 +16,7 @@ import asyncio import logging import subprocess -from typing import Optional +from typing import List, Optional from subiquitycore.context import with_context @@ -44,7 +44,7 @@ class DriversController(SubiquityController): } autoinstall_default = {"install": False} - has_drivers: Optional[bool] = None + drivers: Optional[List[str]] = None def make_autoinstall(self): return { @@ -67,17 +67,18 @@ class DriversController(SubiquityController): with context.child("wait_apt"): await self._wait_apt.wait() apt = self.app.controllers.Mirror.apt_configurer - cmd = ['ubuntu-drivers', 'list'] + # TODO make sure --recommended is a supported option + cmd = ['ubuntu-drivers', 'list', '--recommended'] if self.app.base_model.source.current.variant == 'server': cmd.append('--gpgpu') if self.app.opts.dry_run: if 'has-drivers' in self.app.debug_flags: - self.has_drivers = True + self.drivers = ["nvidia-driver-470"] return elif 'run-drivers' in self.app.debug_flags: pass else: - self.has_drivers = False + self.drivers = [] await self.configured() return async with apt.overlay() as d: @@ -87,21 +88,29 @@ class DriversController(SubiquityController): 'sh', '-c', "command -v ubuntu-drivers"]) except subprocess.CalledProcessError: - self.has_drivers = False + self.drivers = [] await self.configured() return result = await run_curtin_command( self.app, context, "in-target", "-t", d.mountpoint, "--", *cmd, capture=True) - self.has_drivers = bool(result.stdout.strip()) - if not self.has_drivers: + # Drivers are listed one per line, but each is followed by a + # linux-modules-* package (which we are not interested in) ; e.g.: + # $ ubuntu-drivers list --recommended + # nvidia-driver-470 linux-modules-nvidia-470-generic-hwe-20.04 + self.drivers = [] + for line in [x.strip() for x in result.stdout.split("\n")]: + if not line: + continue + self.drivers.append(line.split(" ", maxsplit=1)[0]) + if not self.drivers: await self.configured() async def GET(self, wait: bool = False) -> DriversResponse: if wait: await self._drivers_task return DriversResponse(install=self.model.do_install, - has_drivers=self.has_drivers) + drivers=self.drivers) async def POST(self, install: bool) -> None: self.model.do_install = install diff --git a/subiquity/server/runner.py b/subiquity/server/runner.py index db103fe7..e75b7c0a 100644 --- a/subiquity/server/runner.py +++ b/subiquity/server/runner.py @@ -57,11 +57,11 @@ class DryRunCommandRunner(LoggedCommandRunner): async def start(self, cmd, *, capture=False): if 'scripts/replay-curtin-log.py' in cmd: delay = 0 - elif cmd[-3:] == ['ubuntu-drivers', 'list', '--gpgpu']: - cmd = cmd[-3:] + elif cmd[-4:] == ['ubuntu-drivers', 'list', '--recommended', '--gpgpu']: + cmd = cmd[-4:] delay = 0 - elif cmd[-2:] == ['ubuntu-drivers', 'list']: - cmd = cmd[-2:] + elif cmd[-3:] == ['ubuntu-drivers', 'list', '--recommended']: + cmd = cmd[-3:] delay = 0 else: cmd = ['echo', 'not running:'] + cmd From d72fae0c8f2d7495ad3a71c02b6edc6139c7726b Mon Sep 17 00:00:00 2001 From: Olivier Gayot Date: Mon, 21 Feb 2022 18:10:54 +0100 Subject: [PATCH 11/19] Fix dry-run with overlay Signed-off-by: Olivier Gayot --- subiquity/server/apt.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/subiquity/server/apt.py b/subiquity/server/apt.py index 48fa64ec..3c6f22c0 100644 --- a/subiquity/server/apt.py +++ b/subiquity/server/apt.py @@ -298,6 +298,10 @@ class DryRunAptConfigurer(AptConfigurer): mountpoint=target, upperdir=None) + @contextlib.asynccontextmanager + async def overlay(self): + yield await self.setup_overlay(self.install_tree.mountpoint) + async def deconfigure(self, context, target): return From d6f48f8d9c45d09eef0f3ef225be0cb2b459f314 Mon Sep 17 00:00:00 2001 From: Olivier Gayot Date: Mon, 21 Feb 2022 18:28:14 +0100 Subject: [PATCH 12/19] Make sure we decode the output of ubuntu-drivers Using run_curtin_command, we have no way to specify the encoding or something like universal_newlines=True. This is actually not supported by asyncio.create_subprocess_exec(...). We have partial support for this in Subiquity when using arun_command() but run_curtin_command() actually relies on astart_command() + wait(). Therefore, we now decode the output at the very end, just before reading the outout of ubuntu-drivers. Signed-off-by: Olivier Gayot --- subiquity/server/controllers/drivers.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/subiquity/server/controllers/drivers.py b/subiquity/server/controllers/drivers.py index f82c90ed..9b55a1ab 100644 --- a/subiquity/server/controllers/drivers.py +++ b/subiquity/server/controllers/drivers.py @@ -99,7 +99,10 @@ class DriversController(SubiquityController): # $ ubuntu-drivers list --recommended # nvidia-driver-470 linux-modules-nvidia-470-generic-hwe-20.04 self.drivers = [] - for line in [x.strip() for x in result.stdout.split("\n")]: + # Currently we have no way to specify universal_newlines=True or + # encoding="utf-8" to run_curtin_command. + stdout = result.stdout.decode("utf-8") + for line in [x.strip() for x in stdout.split("\n")]: if not line: continue self.drivers.append(line.split(" ", maxsplit=1)[0]) From b0eb06328893c7880d569e659abd7ac61211ade4 Mon Sep 17 00:00:00 2001 From: Olivier Gayot Date: Tue, 22 Feb 2022 11:04:48 +0100 Subject: [PATCH 13/19] Return a different flavor of driver when running on server vs desktop Signed-off-by: Olivier Gayot --- subiquity/server/controllers/drivers.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/subiquity/server/controllers/drivers.py b/subiquity/server/controllers/drivers.py index 9b55a1ab..d3ff956d 100644 --- a/subiquity/server/controllers/drivers.py +++ b/subiquity/server/controllers/drivers.py @@ -69,11 +69,15 @@ class DriversController(SubiquityController): apt = self.app.controllers.Mirror.apt_configurer # TODO make sure --recommended is a supported option cmd = ['ubuntu-drivers', 'list', '--recommended'] - if self.app.base_model.source.current.variant == 'server': + server: bool = self.app.base_model.source.current.variant == "server" + if server: cmd.append('--gpgpu') if self.app.opts.dry_run: if 'has-drivers' in self.app.debug_flags: - self.drivers = ["nvidia-driver-470"] + if server: + self.drivers = ["nvidia-driver-470-server"] + else: + self.drivers = ["nvidia-driver-510"] return elif 'run-drivers' in self.app.debug_flags: pass From de148289aad959856e4511dca799e3584f485b88 Mon Sep 17 00:00:00 2001 From: Olivier Gayot Date: Tue, 22 Feb 2022 18:07:12 +0100 Subject: [PATCH 14/19] Move the calls to ubuntu-drivers out of the drivers controller We now provide implementation for different interfaces to list and install drivers using ubuntu-drivers. The implementations are outside the drivers controller. We have: * the normal interface that calls ubuntu-drivers in the root directory specified (i.e., in /target or an overlay). * a dry-run interface that calls the system's ubuntu-drivers for listing the drivers available (but does not install anything). The listing of drivers will fail if --recommended is not an available option in the system's ubuntu-drivers implementation. * a dry-run interface that returns an hard-coded list of drivers and does nothing on installation. * a dry-run interface that returns an empty list of drivers. Signed-off-by: Olivier Gayot --- subiquity/server/controllers/drivers.py | 60 +++------ subiquity/server/controllers/install.py | 9 +- subiquity/server/runner.py | 6 - subiquity/server/ubuntu_drivers.py | 162 ++++++++++++++++++++++++ 4 files changed, 183 insertions(+), 54 deletions(-) create mode 100644 subiquity/server/ubuntu_drivers.py diff --git a/subiquity/server/controllers/drivers.py b/subiquity/server/controllers/drivers.py index d3ff956d..4da69cee 100644 --- a/subiquity/server/controllers/drivers.py +++ b/subiquity/server/controllers/drivers.py @@ -15,7 +15,6 @@ import asyncio import logging -import subprocess from typing import List, Optional from subiquitycore.context import with_context @@ -23,8 +22,12 @@ from subiquitycore.context import with_context from subiquity.common.apidef import API from subiquity.common.types import DriversResponse from subiquity.server.controller import SubiquityController -from subiquity.server.curtin import run_curtin_command from subiquity.server.types import InstallerChannels +from subiquity.server.ubuntu_drivers import ( + CommandNotFoundError, + UbuntuDriversInterface, + get_ubuntu_drivers_interface, + ) log = logging.getLogger('subiquity.server.controllers.drivers') @@ -46,6 +49,11 @@ class DriversController(SubiquityController): drivers: Optional[List[str]] = None + def __init__(self, app) -> None: + super().__init__(app) + self.ubuntu_drivers: UbuntuDriversInterface = \ + get_ubuntu_drivers_interface(app) + def make_autoinstall(self): return { "install": self.model.do_install, @@ -67,49 +75,17 @@ class DriversController(SubiquityController): with context.child("wait_apt"): await self._wait_apt.wait() apt = self.app.controllers.Mirror.apt_configurer - # TODO make sure --recommended is a supported option - cmd = ['ubuntu-drivers', 'list', '--recommended'] - server: bool = self.app.base_model.source.current.variant == "server" - if server: - cmd.append('--gpgpu') - if self.app.opts.dry_run: - if 'has-drivers' in self.app.debug_flags: - if server: - self.drivers = ["nvidia-driver-470-server"] - else: - self.drivers = ["nvidia-driver-510"] - return - elif 'run-drivers' in self.app.debug_flags: - pass - else: - self.drivers = [] - await self.configured() - return async with apt.overlay() as d: try: - await self.app.command_runner.run( - ['chroot', d.mountpoint, - 'sh', '-c', - "command -v ubuntu-drivers"]) - except subprocess.CalledProcessError: + # Make sure ubuntu-drivers is available. + self.ubuntu_drivers.ensure_cmd_exists(d.mountpoint) + except CommandNotFoundError: self.drivers = [] - await self.configured() - return - result = await run_curtin_command( - self.app, context, "in-target", "-t", d.mountpoint, - "--", *cmd, capture=True) - # Drivers are listed one per line, but each is followed by a - # linux-modules-* package (which we are not interested in) ; e.g.: - # $ ubuntu-drivers list --recommended - # nvidia-driver-470 linux-modules-nvidia-470-generic-hwe-20.04 - self.drivers = [] - # Currently we have no way to specify universal_newlines=True or - # encoding="utf-8" to run_curtin_command. - stdout = result.stdout.decode("utf-8") - for line in [x.strip() for x in stdout.split("\n")]: - if not line: - continue - self.drivers.append(line.split(" ", maxsplit=1)[0]) + else: + self.drivers = await self.ubuntu_drivers.list_drivers( + root_dir=d.mountpoint, + context=context) + log.debug("Available drivers to install: %s", self.drivers) if not self.drivers: await self.configured() diff --git a/subiquity/server/controllers/install.py b/subiquity/server/controllers/install.py index 387a5c10..46ed4463 100644 --- a/subiquity/server/controllers/install.py +++ b/subiquity/server/controllers/install.py @@ -200,12 +200,9 @@ class InstallController(SubiquityController): with context.child( "ubuntu-drivers-install", "installing third-party drivers") as child: - cmd = ["ubuntu-drivers", "install"] - if self.model.source.current.variant == 'server': - cmd.append('--gpgpu') - await run_curtin_command( - self.app, child, "in-target", "-t", self.tpath(), - "--", *cmd) + ubuntu_drivers = self.app.controllers.Drivers.ubuntu_drivers + await ubuntu_drivers.install_drivers(root_dir=self.tpath(), + context=child) if self.model.network.has_network: self.app.update_state(ApplicationState.UU_RUNNING) diff --git a/subiquity/server/runner.py b/subiquity/server/runner.py index e75b7c0a..f8e9f821 100644 --- a/subiquity/server/runner.py +++ b/subiquity/server/runner.py @@ -57,12 +57,6 @@ class DryRunCommandRunner(LoggedCommandRunner): async def start(self, cmd, *, capture=False): if 'scripts/replay-curtin-log.py' in cmd: delay = 0 - elif cmd[-4:] == ['ubuntu-drivers', 'list', '--recommended', '--gpgpu']: - cmd = cmd[-4:] - delay = 0 - elif cmd[-3:] == ['ubuntu-drivers', 'list', '--recommended']: - cmd = cmd[-3:] - delay = 0 else: cmd = ['echo', 'not running:'] + cmd if 'unattended-upgrades' in cmd: diff --git a/subiquity/server/ubuntu_drivers.py b/subiquity/server/ubuntu_drivers.py new file mode 100644 index 00000000..aa5728bb --- /dev/null +++ b/subiquity/server/ubuntu_drivers.py @@ -0,0 +1,162 @@ +# Copyright 2022 Canonical, Ltd. +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU Affero General Public License as +# published by the Free Software Foundation, either version 3 of the +# License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU Affero General Public License for more details. +# +# You should have received a copy of the GNU Affero General Public License +# along with this program. If not, see . + +""" Module that defines helpers to use the ubuntu-drivers command. """ + +from abc import ABC, abstractmethod +import logging +import subprocess +from typing import List, Type + +from subiquity.server.curtin import run_curtin_command +from subiquitycore.utils import arun_command + + +log = logging.getLogger("subiquity.server.ubuntu_drivers") + + +class CommandNotFoundError(Exception): + """ Exception to be raised when the ubuntu-drivers command is not + available. + """ + + +class UbuntuDriversInterface(ABC): + def __init__(self, app, gpgpu: bool) -> None: + self.app = app + + self.list_drivers_cmd = [ + "ubuntu-drivers", "list", + "--recommended", + ] + self.install_drivers_cmd = [ + "ubuntu-drivers", "install", + ] + if gpgpu: + self.list_drivers_cmd.append("--gpgpu") + self.install_drivers_cmd.append("--gpgpu") + + @abstractmethod + async def ensure_cmd_exists(self, root_dir: str) -> None: + pass + + @abstractmethod + async def list_drivers(self, root_dir: str, context) -> List[str]: + pass + + async def install_drivers(self, root_dir: str, context) -> None: + await run_curtin_command( + self.app, context, + "in-target", "-t", root_dir, "--", *self.install_drivers_cmd) + + def _drivers_from_output(self, output: str) -> List[str]: + """ Parse the output of ubuntu-drivers list --recommended and return a + list of drivers. """ + drivers: List[str] = [] + # Drivers are listed one per line, but each driver is followed by a + # linux-modules-* package (which we are not interested in showing). + # e.g.,: + # $ ubuntu-drivers list --recommended + # nvidia-driver-470 linux-modules-nvidia-470-generic-hwe-20.04 + for line in [x.strip() for x in output.split("\n")]: + if not line: + continue + drivers.append(line.split(" ", maxsplit=1)[0]) + + return drivers + + +class UbuntuDriversClientInterface(UbuntuDriversInterface): + """ UbuntuDrivers interface that uses the ubuntu-drivers command from the + specified root directory. """ + + async def ensure_cmd_exists(self, root_dir: str) -> None: + # TODO This does not tell us if the "--recommended" option is + # available. + try: + await self.app.command_runner.run( + ['chroot', root_dir, + 'sh', '-c', + "command -v ubuntu-drivers"]) + except subprocess.CalledProcessError: + raise CommandNotFoundError( + f"Command ubuntu-drivers is not available in {root_dir}") + + async def list_drivers(self, root_dir: str, context) -> List[str]: + result = await run_curtin_command( + self.app, context, + "in-target", "-t", root_dir, "--", *self.list_drivers_cmd, + capture=True) + # Currently we have no way to specify universal_newlines=True or + # encoding="utf-8" to run_curtin_command so we need to decode the + # output. + return self._drivers_from_output(result.stdout.decode("utf-8")) + + +class UbuntuDriversHasDriversInterface(UbuntuDriversInterface): + """ A dry-run implementation of ubuntu-drivers that returns a hard-coded + list of drivers. """ + gpgpu_drivers: List[str] = ["nvidia-driver-470-server"] + not_gpgpu_drivers: List[str] = ["nvidia-driver-510"] + + def __init__(self, app, gpgpu: bool) -> None: + super().__init__(app, gpgpu) + self.drivers = self.gpgpu_drivers if gpgpu else self.not_gpgpu_drivers + + async def ensure_cmd_exists(self, root_dir: str) -> None: + pass + + async def list_drivers(self, root_dir: str, context) -> List[str]: + return self.drivers + + +class UbuntuDriversNoDriversInterface(UbuntuDriversHasDriversInterface): + """ A dry-run implementation of ubuntu-drivers that returns a hard-coded + empty list of drivers. """ + + gpgpu_drivers: List[str] = [] + not_gpgpu_drivers: List[str] = [] + + +class UbuntuDriversRunDriversInterface(UbuntuDriversInterface): + """ A dry-run implementation of ubuntu-drivers that actually runs the + ubuntu-drivers command but locally. """ + async def ensure_cmd_exists(self, root_dir: str) -> None: + # TODO This does not tell us if the "--recommended" option is + # available. + try: + await arun_command(["command", "-v", "ubuntu-drivers"]) + except subprocess.CalledProcessError: + raise CommandNotFoundError( + "Command ubuntu-drivers is not available in this system") + + async def list_drivers(self, root_dir: str, context) -> List[str]: + # We run the command locally - ignoring the root_dir. + result = await arun_command(self.list_drivers_cmd) + return self._drivers_from_output(result.stdout) + + +def get_ubuntu_drivers_interface(app) -> UbuntuDriversInterface: + is_server = app.base_model.source.current.variant == "server" + cls: Type[UbuntuDriversInterface] = UbuntuDriversClientInterface + if app.opts.dry_run: + if 'has-drivers' in app.debug_flags: + cls = UbuntuDriversHasDriversInterface + elif 'run-drivers' in app.debug_flags: + cls = UbuntuDriversRunDriversInterface + else: + cls = UbuntuDriversNoDriversInterface + + return cls(app, gpgpu=is_server) From f50b5c005770772320c012f20ccdd9acfef751b7 Mon Sep 17 00:00:00 2001 From: Olivier Gayot Date: Tue, 22 Feb 2022 18:46:47 +0100 Subject: [PATCH 15/19] Make sure the drivers ctrler calls .configured until we have client ctrler Signed-off-by: Olivier Gayot --- subiquity/server/controllers/drivers.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/subiquity/server/controllers/drivers.py b/subiquity/server/controllers/drivers.py index 4da69cee..b85e9aea 100644 --- a/subiquity/server/controllers/drivers.py +++ b/subiquity/server/controllers/drivers.py @@ -88,6 +88,9 @@ class DriversController(SubiquityController): log.debug("Available drivers to install: %s", self.drivers) if not self.drivers: await self.configured() + else: + # TODO Remove this once we have the GUI controller. + await self.POST(install=True) async def GET(self, wait: bool = False) -> DriversResponse: if wait: From 382f73150d7dc2ed98a63538a4c5611c4ffa369f Mon Sep 17 00:00:00 2001 From: Olivier Gayot Date: Wed, 23 Feb 2022 10:17:21 +0100 Subject: [PATCH 16/19] Add unit-tests for ubuntu-drivers Signed-off-by: Olivier Gayot --- subiquity/server/tests/test_ubuntu_drivers.py | 135 ++++++++++++++++++ 1 file changed, 135 insertions(+) create mode 100644 subiquity/server/tests/test_ubuntu_drivers.py diff --git a/subiquity/server/tests/test_ubuntu_drivers.py b/subiquity/server/tests/test_ubuntu_drivers.py new file mode 100644 index 00000000..e869985c --- /dev/null +++ b/subiquity/server/tests/test_ubuntu_drivers.py @@ -0,0 +1,135 @@ +# Copyright 2022 Canonical, Ltd. +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU Affero General Public License as +# published by the Free Software Foundation, either version 3 of the +# License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU Affero General Public License for more details. +# +# You should have received a copy of the GNU Affero General Public License +# along with this program. If not, see . + +from subprocess import CalledProcessError +import unittest +from unittest.mock import patch, AsyncMock, Mock + +from subiquitycore.tests.mocks import make_app +from subiquitycore.tests.util import run_coro + +from subiquity.server.ubuntu_drivers import ( + UbuntuDriversInterface, + UbuntuDriversClientInterface, + CommandNotFoundError, + ) + + +class TestUbuntuDriversInterface(unittest.TestCase): + def setUp(self): + self.app = make_app() + + @patch.multiple(UbuntuDriversInterface, __abstractmethods__=set()) + def test_init(self): + ubuntu_drivers = UbuntuDriversInterface(self.app, gpgpu=False) + + self.assertEqual(ubuntu_drivers.app, self.app) + self.assertEqual(ubuntu_drivers.list_drivers_cmd, [ + "ubuntu-drivers", "list", + "--recommended", + ]) + self.assertEqual(ubuntu_drivers.install_drivers_cmd, [ + "ubuntu-drivers", "install"]) + + ubuntu_drivers = UbuntuDriversInterface(self.app, gpgpu=True) + self.assertEqual(ubuntu_drivers.list_drivers_cmd, [ + "ubuntu-drivers", "list", + "--recommended", "--gpgpu", + ]) + self.assertEqual(ubuntu_drivers.install_drivers_cmd, [ + "ubuntu-drivers", "install", "--gpgpu"]) + + @patch.multiple(UbuntuDriversInterface, __abstractmethods__=set()) + @patch("subiquity.server.ubuntu_drivers.run_curtin_command") + def test_install_drivers(self, mock_run_curtin_command): + ubuntu_drivers = UbuntuDriversInterface(self.app, gpgpu=False) + run_coro(ubuntu_drivers.install_drivers( + root_dir="/target", + context="installing third-party drivers")) + mock_run_curtin_command.assert_called_once_with( + self.app, "installing third-party drivers", + "in-target", "-t", "/target", + "--", + "ubuntu-drivers", "install" + ) + + @patch.multiple(UbuntuDriversInterface, __abstractmethods__=set()) + def test_drivers_from_output(self): + ubuntu_drivers = UbuntuDriversInterface(self.app, gpgpu=False) + + output = """\ +nvidia-driver-470 linux-modules-nvidia-470-generic-hwe-20.04 +""" + self.assertEqual( + ubuntu_drivers._drivers_from_output(output=output), + ["nvidia-driver-470"]) + + # Make sure empty lines are discarded + output = """ +nvidia-driver-470 linux-modules-nvidia-470-generic-hwe-20.04 + +nvidia-driver-510 linux-modules-nvidia-510-generic-hwe-20.04 + +""" + + self.assertEqual( + ubuntu_drivers._drivers_from_output(output=output), + ["nvidia-driver-470", "nvidia-driver-510"]) + + +class TestUbuntuDriversClientInterface(unittest.TestCase): + def setUp(self): + self.app = make_app() + self.ubuntu_drivers = UbuntuDriversClientInterface( + self.app, gpgpu=False) + + def test_ensure_cmd_exists(self): + with patch.object( + self.app, "command_runner", + create=True, new_callable=AsyncMock) as mock_runner: + # On success + run_coro(self.ubuntu_drivers.ensure_cmd_exists("/target")) + mock_runner.run.assert_called_once_with( + [ + "chroot", "/target", + "sh", "-c", "command -v ubuntu-drivers", + ]) + + # On process failure + mock_runner.run.side_effect = CalledProcessError( + returncode=1, + cmd=["sh", "-c", "command -v ubuntu-drivers"]) + + with self.assertRaises(CommandNotFoundError): + run_coro(self.ubuntu_drivers.ensure_cmd_exists("/target")) + + @patch("subiquity.server.ubuntu_drivers.run_curtin_command") + def test_list_drivers(self, mock_run_curtin_command): + # Make sure this gets decoded as utf-8. + mock_run_curtin_command.return_value = Mock(stdout=b"""\ +nvidia-driver-510 linux-modules-nvidia-510-generic-hwe-20.04 +""") + drivers = run_coro(self.ubuntu_drivers.list_drivers( + root_dir="/target", + context="listing third-party drivers")) + + mock_run_curtin_command.assert_called_once_with( + self.app, "listing third-party drivers", + "in-target", "-t", "/target", + "--", + "ubuntu-drivers", "list", "--recommended", + capture=True) + + self.assertEqual(drivers, ["nvidia-driver-510"]) From 98860b10ca6344008c8a85c817f8cac2eae1fc3c Mon Sep 17 00:00:00 2001 From: Olivier Gayot Date: Wed, 23 Feb 2022 17:57:32 +0100 Subject: [PATCH 17/19] Fix unmount of overlay broken by merge Signed-off-by: Olivier Gayot --- subiquity/server/apt.py | 20 +++++++++++++------ subiquity/server/tests/test_apt.py | 31 +++++++++++++++++++++++++++++- 2 files changed, 44 insertions(+), 7 deletions(-) diff --git a/subiquity/server/apt.py b/subiquity/server/apt.py index 3c6f22c0..42f2f1eb 100644 --- a/subiquity/server/apt.py +++ b/subiquity/server/apt.py @@ -154,10 +154,10 @@ class AptConfigurer: self._mounts.append(m) return m - async def unmount(self, mountpoint: str, remove=True): + async def unmount(self, mountpoint: Mountpoint, remove=True): if remove: self._mounts.remove(mountpoint) - await self.app.command_runner.run(['umount', mountpoint]) + await self.app.command_runner.run(['umount', mountpoint.mountpoint]) async def setup_overlay(self, lowers: List[Lower]) -> OverlayMountpoint: tdir = self.tdir() @@ -237,11 +237,17 @@ class AptConfigurer: try: yield overlay finally: - await self.unmount(overlay.mountpoint) + # TODO self.unmount expects a Mountpoint object. Unfortunately, the + # one we created in setup_overlay was discarded and replaced by an + # OverlayMountPoint object instead. Here we re-create a new + # Mountpoint object and (thanks to attr.s) make sure that it + # compares equal to the one we discarded earlier. + # But really, there should be better ways to handle this. + await self.unmount(Mountpoint(mountpoint=overlay.mountpoint)) async def cleanup(self): for m in reversed(self._mounts): - await self.unmount(m.mountpoint, remove=False) + await self.unmount(m, remove=False) for d in self._tdirs: shutil.rmtree(d) @@ -254,7 +260,9 @@ class AptConfigurer: 'cp', '-aT', self.configured_tree.p(dir), target_mnt.p(dir), ]) - await self.unmount(target_mnt.p('cdrom'), remove=False) + await self.unmount( + Mountpoint(mountpoint=target_mnt.p('cdrom')), + remove=False) os.rmdir(target_mnt.p('cdrom')) await _restore_dir('etc/apt') @@ -271,7 +279,7 @@ class AptConfigurer: class DryRunAptConfigurer(AptConfigurer): - async def unmount(self, mountpoint: str, remove=True): + async def unmount(self, mountpoint: Mountpoint, remove=True): pass async def setup_overlay(self, lowers: List[Lower]) -> OverlayMountpoint: diff --git a/subiquity/server/tests/test_apt.py b/subiquity/server/tests/test_apt.py index 40111536..44a73d81 100644 --- a/subiquity/server/tests/test_apt.py +++ b/subiquity/server/tests/test_apt.py @@ -13,10 +13,11 @@ # You should have received a copy of the GNU Affero General Public License # along with this program. If not, see . -from unittest.mock import Mock +from unittest.mock import Mock, patch, AsyncMock from subiquitycore.tests import SubiTestCase from subiquitycore.tests.mocks import make_app +from subiquitycore.tests.util import run_coro from subiquity.server.apt import ( AptConfigurer, Mountpoint, @@ -50,6 +51,34 @@ class TestAptConfigurer(SubiTestCase): expected['apt']['https_proxy'] = proxy self.assertEqual(expected, self.configurer.apt_config()) + def test_mount_unmount(self): + # Make sure we can unmount something that we mounted before. + with patch.object(self.app, "command_runner", + create=True, new_callable=AsyncMock): + m = run_coro(self.configurer.mount("/dev/cdrom", "/target")) + run_coro(self.configurer.unmount(m)) + + def test_overlay(self): + self.configurer.install_tree = OverlayMountpoint( + upperdir="upperdir-install-tree", + lowers=["lowers1-install-tree"], + mountpoint="mountpoint-install-tree", + ) + self.configurer.configured_tree = OverlayMountpoint( + upperdir="upperdir-install-tree", + lowers=["lowers1-install-tree"], + mountpoint="mountpoint-install-tree", + ) + self.source = "source" + + async def coro(): + async with self.configurer.overlay(): + pass + + with patch.object(self.app, "command_runner", + create=True, new_callable=AsyncMock): + run_coro(coro()) + class TestLowerDirFor(SubiTestCase): def test_lowerdir_for_str(self): From 3c9e5ca98fee453c37660c961de26ebe8e7dcaa8 Mon Sep 17 00:00:00 2001 From: Olivier Gayot Date: Wed, 23 Feb 2022 20:06:38 +0100 Subject: [PATCH 18/19] Use body for parameters of POST /drivers Signed-off-by: Olivier Gayot --- subiquity/common/apidef.py | 3 ++- subiquity/common/types.py | 5 +++++ subiquity/server/controllers/drivers.py | 6 +++--- 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/subiquity/common/apidef.py b/subiquity/common/apidef.py index bb476174..0f783447 100644 --- a/subiquity/common/apidef.py +++ b/subiquity/common/apidef.py @@ -40,6 +40,7 @@ from subiquity.common.types import ( RefreshStatus, ShutdownMode, DriversResponse, + DriversPayload, SnapInfo, SnapListResponse, SnapSelection, @@ -296,7 +297,7 @@ class API: class drivers: def GET(wait: bool = False) -> DriversResponse: ... - def POST(install: bool) -> None: ... + def POST(data: Payload[DriversPayload]) -> None: ... class snaplist: def GET(wait: bool = False) -> SnapListResponse: ... diff --git a/subiquity/common/types.py b/subiquity/common/types.py index 713e7c17..ab543ee9 100644 --- a/subiquity/common/types.py +++ b/subiquity/common/types.py @@ -382,6 +382,11 @@ class DriversResponse: drivers: Optional[List[str]] +@attr.s(auto_attribs=True) +class DriversPayload: + install: bool + + @attr.s(auto_attribs=True) class SnapSelection: name: str diff --git a/subiquity/server/controllers/drivers.py b/subiquity/server/controllers/drivers.py index b85e9aea..71bf2962 100644 --- a/subiquity/server/controllers/drivers.py +++ b/subiquity/server/controllers/drivers.py @@ -20,7 +20,7 @@ from typing import List, Optional from subiquitycore.context import with_context from subiquity.common.apidef import API -from subiquity.common.types import DriversResponse +from subiquity.common.types import DriversPayload, DriversResponse from subiquity.server.controller import SubiquityController from subiquity.server.types import InstallerChannels from subiquity.server.ubuntu_drivers import ( @@ -98,6 +98,6 @@ class DriversController(SubiquityController): return DriversResponse(install=self.model.do_install, drivers=self.drivers) - async def POST(self, install: bool) -> None: - self.model.do_install = install + async def POST(self, data: DriversPayload) -> None: + self.model.do_install = data.install await self.configured() From fe6d0878fc70eab4371f4d27a9b603398ef5af6d Mon Sep 17 00:00:00 2001 From: Olivier Gayot Date: Wed, 23 Feb 2022 21:13:16 +0100 Subject: [PATCH 19/19] Make sure the FS unmounted were previously mounted in dry-run mode Signed-off-by: Olivier Gayot --- subiquity/server/apt.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/subiquity/server/apt.py b/subiquity/server/apt.py index 42f2f1eb..c03e9d55 100644 --- a/subiquity/server/apt.py +++ b/subiquity/server/apt.py @@ -280,7 +280,8 @@ class AptConfigurer: class DryRunAptConfigurer(AptConfigurer): async def unmount(self, mountpoint: Mountpoint, remove=True): - pass + if remove: + self._mounts.remove(mountpoint) async def setup_overlay(self, lowers: List[Lower]) -> OverlayMountpoint: # XXX This implementation expects that: @@ -311,7 +312,7 @@ class DryRunAptConfigurer(AptConfigurer): yield await self.setup_overlay(self.install_tree.mountpoint) async def deconfigure(self, context, target): - return + await self.cleanup() def get_apt_configurer(app, source: str):