OEM: ensure only one kernel gets installed #1703

ogayot · 2023-06-29T12:25:35Z

Some more testing is needed but it looks mostly ok.

Depends: #1702

dbungert

LGTM, minus the bit already raised on the other PR.

dbungert · 2023-06-29T20:53:47Z

subiquity/server/controllers/install.py

+            # If we already have the needed kernel installed, don't bother
+            # requesting curthooks to install it or we might end up with two
+            # kernels.
+            for pkg in await list_installed_kernels(Path(self.tpath())):


In what scenario could len(list_installed_kernels) > 1 be true? (not rejecting the for loop, just curious.) Is that indicative of a image construction bug?

I think that would only happen on broken or half broken scenarios, like:

two suggested OEM meta-packages pulling two different kernel flavors (for now there should be only one meta-package per hardware model - in theory)

one kernel in the squashfs (don't think we'll ever do that) and the OEM metapkg pulling another flavor

mwhudson · 2023-06-29T22:07:15Z

subiquity/server/controllers/install.py

+            # kernels.
+            for pkg in await list_installed_kernels(Path(self.tpath())):
+                if pkg == self.model.kernel.needed_kernel:
+                    self.model.kernel.curthooks_no_install


Um I think this line is incomplete :-)

Ouch! Thanks for noticing!

mwhudson · 2023-06-29T22:10:15Z

subiquity/server/kernel.py

+
+async def list_installed_kernels(
+        rootfs: pathlib.Path,
+        *, grep_status: str = "grep-status") -> List[str]:


This argument is unused.

I admit I expected you to fix this by removing the argument :-) What is the use case for passing a different value here?

It felt like it could be a way to substitute a command by another when doing dry-runs - as in
list_installed_kernels(..., grep_status='scripts/replay-grep-status') or something.

But it's not really usable as is - so I guess it should go.

mwhudson · 2023-06-29T22:12:37Z

snapcraft.yaml

@@ -110,6 +110,7 @@ parts:
      # This list includes the dependencies for curtin and probert as well,
      # there doesn't seem to be any real benefit to listing them separately.
      - cloud-init
+      - dctrl-tools


I guess we could just stage /usr/bin/grep-dctrl but the whole package is fairly tiny.

Yes - and no Depends so I guess I'll keep the whole package.

mwhudson · 2023-06-29T22:24:16Z

subiquity/server/controllers/install.py

+            # requesting curthooks to install it or we might end up with two
+            # kernels.
+            for pkg in await list_installed_kernels(Path(self.tpath())):
+                if pkg == self.model.kernel.needed_kernel:


This is all a bit confusing but I don't think we should check this, for two reasons. One is that pkg here will be something like "linux-image-6.2.0-23-generic" or "linux-image-6.1.0-1012-oem" and I think needed_kernel is going to be a metapackage name like "linux-oem-22.04" or "linux" and the other is that even if we think we want "linux-oem-22.04" installing the metapackage might actually install "linux-oem-22.04d" or something.

Basically IMO we should instruct curtin to not install a kernel iff there is any kernel installed already. Luckily this makes the code simpler :-)

I'm not sure how things should interact if the autoinstall explicitly requests a different flavour though.

Yeah, I think you're right. Dan was suggesting a runtime error if there's an explicit kernel flavor + an OEM meta-package. I think it is a sensible approach.

Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>

By default, OEM meta-packages get installed on ubuntu-desktop and don't get installed on ubuntu-server. Using autoinstall, we can now give more control. The autoinstall section supports the following: Install on server and desktop: oem: install: true Do not install even on desktop: oem: install: false Install only on desktop (the default): oem: install: auto Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>

Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>

mwhudson

Thanks, this is good stuff. I still have a couple of comments but they are very much at the nitpicking level now.

mwhudson · 2023-07-02T21:38:59Z

subiquity/server/controllers/install.py

+                # needed manually. Ideally, we would not hardcode
+                # var/lib/dpkg/status because it is an implementation detail.
+                status = Path("var/lib/dpkg/status")
+                (Path(root) / status).parent.mkdir(parents=True, exist_ok=True)


This is nitpicky to the max but a) root is already a Path here b) Path / str works fine? so you can leave status as a string.

Good point, updated :)

mwhudson · 2023-07-02T21:40:52Z

subiquity/server/kernel.py

+
+async def list_installed_kernels(
+        rootfs: pathlib.Path,
+        *, grep_status: str = "grep-status") -> List[str]:


I admit I expected you to fix this by removing the argument :-) What is the use case for passing a different value here?

Before running curthooks, we now look in the target if there is an installed kernel. If there is, we instruct curtin _not_ to install another one. Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>

ogayot · 2023-07-03T08:03:18Z

I admit I expected you to fix this by removing the argument :-) What is the use case for passing a different value here?

It felt like it could be a way to substitute a command by another when doing dry-runs - as in

list_installed_kernels(..., grep_status='scripts/replay-grep-status')

or something.

But it's not really usable as is - so I guess it should go (and is now gone)

dbungert approved these changes Jun 29, 2023

View reviewed changes

mwhudson requested changes Jun 29, 2023

View reviewed changes

ogayot force-pushed the install-oem-ensure-one-kernel branch 2 times, most recently from 64a0c2a to 10b5714 Compare June 30, 2023 10:46

ogayot force-pushed the install-oem-kernel-selection branch from c222ea4 to 3088926 Compare June 30, 2023 16:24

ogayot force-pushed the install-oem-ensure-one-kernel branch 2 times, most recently from 364eb1c to 537e6a8 Compare June 30, 2023 18:37

ogayot force-pushed the install-oem-kernel-selection branch from 4352ec9 to c98afd4 Compare June 30, 2023 18:38

ogayot added 3 commits June 30, 2023 20:53

oem: raise error on certified hardware if a specific kernel is requested

7e8c1ee

Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>

oem: disable OEM meta-packages installation on core boot classic

b844119

Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>

ogayot force-pushed the install-oem-ensure-one-kernel branch from 537e6a8 to 817ad85 Compare June 30, 2023 18:53

ogayot changed the base branch from install-oem-kernel-selection to main June 30, 2023 18:54

ogayot requested a review from mwhudson June 30, 2023 18:55

mwhudson approved these changes Jul 2, 2023

View reviewed changes

oem: ensure a single kernel gets installed

6424c3c

Before running curthooks, we now look in the target if there is an installed kernel. If there is, we instruct curtin _not_ to install another one. Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>

ogayot force-pushed the install-oem-ensure-one-kernel branch from 817ad85 to 6424c3c Compare July 3, 2023 08:00

ogayot merged commit 54465d1 into canonical:main Jul 4, 2023
9 checks passed

ogayot deleted the install-oem-ensure-one-kernel branch July 4, 2023 10:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OEM: ensure only one kernel gets installed #1703

OEM: ensure only one kernel gets installed #1703

ogayot commented Jun 29, 2023 •

edited

dbungert left a comment

dbungert Jun 29, 2023

ogayot Jun 30, 2023

mwhudson Jun 29, 2023

ogayot Jun 30, 2023

mwhudson Jun 29, 2023

ogayot Jun 30, 2023

mwhudson Jul 2, 2023

ogayot Jul 3, 2023

mwhudson Jun 29, 2023

ogayot Jun 30, 2023

mwhudson Jun 29, 2023

mwhudson Jun 29, 2023

ogayot Jun 30, 2023

mwhudson left a comment

mwhudson Jul 2, 2023

ogayot Jul 3, 2023

mwhudson Jul 2, 2023

ogayot commented Jul 3, 2023

OEM: ensure only one kernel gets installed #1703

OEM: ensure only one kernel gets installed #1703

Conversation

ogayot commented Jun 29, 2023 • edited

dbungert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwhudson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogayot commented Jul 3, 2023

ogayot commented Jun 29, 2023 •

edited