many: make kernel-modules components available on initramfs installation by alfonsosanchezbeato · Pull Request #14810 · canonical/snapd

alfonsosanchezbeato · 2024-12-05T16:05:12Z

Make sure that modules shipped in kernel-modules components are available on early boot when the system is installed from the initramfs. This is a follow-up from #14744.

This requires some changes in core-initrd and in snapd-generator, so I am opening as draft for the moment. In fact the pre-requisites are needed for having a working drivers tree at all on first boot after initramfs driven installation.

Depends on

codecov · 2024-12-05T16:23:42Z

Codecov Report

Attention: Patch coverage is 65.80311% with 66 lines in your changes missing coverage. Please review.

Please upload report for BASE (master@953a5d8). Learn more about missing BASE report.
Report is 37 commits behind head on master.

Files with missing lines	Patch %	Lines
cmd/snap-bootstrap/cmd_initramfs_mounts.go	66.32%	22 Missing and 11 partials ⚠️
overlord/install/install.go	0.00%	31 Missing ⚠️
overlord/devicestate/handlers_install.go	95.00%	0 Missing and 1 partial ⚠️
overlord/snapstate/snapstate.go	96.42%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##             master   #14810   +/-   ##
=========================================
  Coverage          ?   78.24%           
=========================================
  Files             ?     1159           
  Lines             ?   153925           
  Branches          ?        0           
=========================================
  Hits              ?   120443           
  Misses            ?    26068           
  Partials          ?     7414

Flag	Coverage Δ
unittests	`78.24% <65.80%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2025-01-15T23:21:18Z

Tue Jan 28 13:39:30 UTC 2025
The following results are from: https://github.com/canonical/snapd/actions/runs/13000525487

Failures:

Preparing:

openstack:debian-12-64:tests/completion/

Executing:

google-nested:ubuntu-24.04-64:tests/nested/manual/kernel-modules-components:encrypted
google-nested:ubuntu-24.04-64:tests/nested/manual/remodel-min-size
openstack:opensuse-tumbleweed-64:tests/main/auto-refresh-pre-download:restart
openstack:opensuse-tumbleweed-64:tests/main/auto-refresh-pre-download:ignore
openstack:opensuse-tumbleweed-64:tests/main/auto-refresh-pre-download:close_mid_restart
openstack:opensuse-tumbleweed-64:tests/main/auto-refresh:regular
openstack:opensuse-tumbleweed-64:tests/main/auto-refresh:parallel
openstack:opensuse-tumbleweed-64:tests/main/auto-refresh-gating
openstack:opensuse-tumbleweed-64:tests/main/auto-refresh-pre-download:close
openstack:opensuse-tumbleweed-64:tests/main/auto-refresh-backoff
openstack:opensuse-tumbleweed-64:tests/main/snap-refresh-hold
openstack:opensuse-tumbleweed-64:tests/main/auto-refresh-gating-from-snap
openstack:opensuse-tumbleweed-64:tests/main/auto-refresh-retry
openstack:opensuse-tumbleweed-64:tests/main/refresh-app-awareness-notify

Restoring:

openstack:debian-12-64:tests/completion/
openstack:debian-12-64:tests/main/refresh:strict_fake
openstack:debian-12-64:tests/main/
openstack:opensuse-tumbleweed-64:tests/main/refresh-app-awareness-notify

alfonsosanchezbeato · 2025-01-17T17:27:11Z

This is ready for review now, the core24 dependency is merged and the others are unnecessary as what they did is done now by snap-bootstrap.

pedronis

couple of code org question/comments

asserts/model.go

gadget/install/kernel.go

alfonsosanchezbeato

@pedronis thanks, comments addressed now

gadget/install/kernel.go

asserts/model.go

pedronis

thanks, one small thing

pedronis · 2025-01-22T08:56:37Z

overlord/install/install.go

 )

+// BuildKernelBootInfo constructs a KernelBootInfo.
+func BuildKernelBootInfo(kernInfo *snap.Info, compSeedInfos []CompSeedInfo, kernMntPoint string, mntPtForComps map[string]string, isCore, needsDriversTree bool) KernelBootInfo {


nitpick: should the two bools become an Options struct ?

I'm +1 on that

Meulengracht

Overall nice, some comments

Meulengracht · 2025-01-23T08:18:03Z

cmd/snap-bootstrap/cmd_initramfs_mounts.go

+	if err != nil {
+		return nil, err
+	}
+	if ci.Revision.Unset() {


In what cases are the revision unset? And why does it need to be set to x1?

When reading from the seed, if the component is unasserted. Note that this is the same that was being done for snaps in readSnapInfo. I've added comments to clarify.

Meulengracht · 2025-01-23T08:21:47Z

cmd/snap-bootstrap/cmd_initramfs_mounts.go

+			continue
+		}
+
+		// Mount ephemerally the kernel-modules components


This comment explains what we are doing here, which is farily obvious from the code, maybe a comment about why they are mounted ephemerally would make more sense

Good point, added more information

Meulengracht · 2025-01-23T08:26:20Z

cmd/snap-bootstrap/cmd_initramfs_mounts.go

 	}

-	installedSystem, err := gadgetInstallRun(model, gadgetMountDir, kernelSnapInfo, bootDevice, options, installObserver, timings.New(nil))
+	preseed := false


i think this should be defined before preseedSeed, ok := currentSeed.(seed.PreseedCapable) instead, it doesn't need to be defined here

Meulengracht · 2025-01-23T08:28:44Z

cmd/snap-bootstrap/cmd_initramfs_mounts.go

 	}

-	currentSeed, err := mst.LoadSeed(mst.recoverySystem)
+	if preseed {


A comment about why different behavior is required for this case would be nice

Meulengracht · 2025-01-23T08:31:08Z

cmd/snap-bootstrap/cmd_initramfs_mounts.go

+		}
+	}
+
+	// Create drivers tree mount units


This comment is not very helpful either - i think a comment about the expected behaviour in this case and how it's consumed would be much more helpful

tweaked a bit

Meulengracht · 2025-01-23T08:33:48Z

kernel/kernel_drivers.go

 	return nil
 }
+
+// NeedsKernelDriversTree tells if we need a kernel drivers tree for this model.


Suggested change

// NeedsKernelDriversTree tells if we need a kernel drivers tree for this model.

// NeedsKernelDriversTree returns true if we need a kernel drivers tree for this model.

Meulengracht · 2025-01-23T08:34:41Z

cmd/snap-bootstrap/cmd_initramfs_mounts.go

+			ReadOnly:  true,
+			Private:   true,
+			Ephemeral: true}); err != nil {
+			return err


Do we need a test in this error case to ensure the unmounts are called correctly?

I've added a test now

Meulengracht · 2025-01-23T08:36:23Z

overlord/install/install.go


+// CompSeedInfo contains information for a component from the seed and
+// from its metadata.
+type CompSeedInfo struct {


This is a nitpick but for me, when it comes to exported structures that are part of an API it feels more appropriate to spell it out ComponentSeedInfo instead of shorting it, which is what you might do for the variable name.

Meulengracht · 2025-01-23T08:39:43Z

overlord/install/install.go

+	CompInfo *snap.ComponentInfo
+	CompSeed *seed.Component


Would read even nicer when used? Anyway, nitpicks here, feel free to ignore

Suggested change

CompInfo *snap.ComponentInfo

CompSeed *seed.Component

Info *snap.ComponentInfo

Seed *seed.Component

Meulengracht · 2025-01-23T08:40:25Z

overlord/install/install.go

 )

+// BuildKernelBootInfo constructs a KernelBootInfo.
+func BuildKernelBootInfo(kernInfo *snap.Info, compSeedInfos []CompSeedInfo, kernMntPoint string, mntPtForComps map[string]string, isCore, needsDriversTree bool) KernelBootInfo {


I'm +1 on that

alfonsosanchezbeato

@Meulengracht thanks, I've addressed your comments now

alfonsosanchezbeato · 2025-01-23T14:43:17Z

cmd/snap-bootstrap/cmd_initramfs_mounts.go

+	if err != nil {
+		return nil, err
+	}
+	if ci.Revision.Unset() {


When reading from the seed, if the component is unasserted. Note that this is the same that was being done for snaps in readSnapInfo. I've added comments to clarify.

alfonsosanchezbeato · 2025-01-23T14:46:30Z

cmd/snap-bootstrap/cmd_initramfs_mounts.go

+			continue
+		}
+
+		// Mount ephemerally the kernel-modules components


Good point, added more information

alfonsosanchezbeato · 2025-01-23T15:18:19Z

cmd/snap-bootstrap/cmd_initramfs_mounts.go

+			ReadOnly:  true,
+			Private:   true,
+			Ephemeral: true}); err != nil {
+			return err


I've added a test now

alfonsosanchezbeato · 2025-01-23T15:19:09Z

cmd/snap-bootstrap/cmd_initramfs_mounts.go

 	}

-	installedSystem, err := gadgetInstallRun(model, gadgetMountDir, kernelSnapInfo, bootDevice, options, installObserver, timings.New(nil))
+	preseed := false


alfonsosanchezbeato · 2025-01-23T15:21:18Z

cmd/snap-bootstrap/cmd_initramfs_mounts.go

 	}

-	currentSeed, err := mst.LoadSeed(mst.recoverySystem)
+	if preseed {


alfonsosanchezbeato · 2025-01-23T15:23:14Z

cmd/snap-bootstrap/cmd_initramfs_mounts.go

+		}
+	}
+
+	// Create drivers tree mount units


tweaked a bit

alfonsosanchezbeato · 2025-01-23T15:28:25Z

overlord/install/install.go


+// CompSeedInfo contains information for a component from the seed and
+// from its metadata.
+type CompSeedInfo struct {


alfonsosanchezbeato · 2025-01-23T15:30:00Z

overlord/install/install.go

+	CompInfo *snap.ComponentInfo
+	CompSeed *seed.Component


alfonsosanchezbeato · 2025-01-23T15:37:17Z

overlord/install/install.go

 )

+// BuildKernelBootInfo constructs a KernelBootInfo.
+func BuildKernelBootInfo(kernInfo *snap.Info, compSeedInfos []CompSeedInfo, kernMntPoint string, mntPtForComps map[string]string, isCore, needsDriversTree bool) KernelBootInfo {


to install package so it can be used by snap-bootstrap.

on first boot, when installing from the initramfs.

Introduce NESTED_COMP_KERNEL_MODULE_NAME and NESTED_KERNEL_MODULES_COMP env vars. If present, a kernel-modules component containing the specified kernel module will be created and will be added when building the nested test image.

Create the drivers tree mounts when installing UC from the initramfs.

to build the drivers tree.

Run the prepare-kernel-modules-components task when creating the pre-seed tarball. This was not happening as pre-seeding stops the run of snap install tasks right before running hooks, and this task is run after them. To workaround this, add a prepare-kernel-modules-components task that runs before hooks, only for preseeding.

It will be part of the preseed tarball.

…nstallation (canonical#14810)" This reverts commit b803966.

alfonsosanchezbeato requested review from bboozzoo, pedronis and valentindavid December 5, 2024 16:05

alfonsosanchezbeato added the Run nested The PR also runs tests inluded in nested suite label Dec 5, 2024

alfonsosanchezbeato force-pushed the include-kern-mod-comps-on-initramfs-install branch from 2b388eb to 66cc66b Compare January 15, 2025 20:51

alfonsosanchezbeato force-pushed the include-kern-mod-comps-on-initramfs-install branch 2 times, most recently from 6bcd667 to 652d0f3 Compare January 17, 2025 17:24

alfonsosanchezbeato marked this pull request as ready for review January 17, 2025 17:24

alfonsosanchezbeato mentioned this pull request Jan 17, 2025

factory: mount drivers tree when installing from initramfs canonical/core-initrd#279

Closed

alfonsosanchezbeato closed this Jan 17, 2025

alfonsosanchezbeato reopened this Jan 17, 2025

alfonsosanchezbeato force-pushed the include-kern-mod-comps-on-initramfs-install branch from 652d0f3 to 77f8567 Compare January 17, 2025 18:40

pedronis reviewed Jan 21, 2025

View reviewed changes

asserts/model.go Outdated Show resolved Hide resolved

gadget/install/kernel.go Outdated Show resolved Hide resolved

alfonsosanchezbeato commented Jan 21, 2025

View reviewed changes

gadget/install/kernel.go Outdated Show resolved Hide resolved

asserts/model.go Outdated Show resolved Hide resolved

alfonsosanchezbeato requested a review from pedronis January 21, 2025 20:12

pedronis approved these changes Jan 22, 2025

View reviewed changes

Meulengracht reviewed Jan 23, 2025

View reviewed changes

alfonsosanchezbeato commented Jan 23, 2025

View reviewed changes

alfonsosanchezbeato requested a review from Meulengracht January 23, 2025 15:47

alfonsosanchezbeato added 4 commits January 24, 2025 10:35

asserts,overlord: move NeedsKernelSetup to a model method

32e8a2e

gadget,o/devicestate: move build of kernel info for boot

4e85be2

to install package so it can be used by snap-bootstrap.

seed,overlord: add new component to snap for unit testing

d1c93a5

cmd/snap-bootstrap: make kernel-modules components available

1736cec

on first boot, when installing from the initramfs.

alfonsosanchezbeato force-pushed the include-kern-mod-comps-on-initramfs-install branch from b24de36 to 73ccbf2 Compare January 24, 2025 15:46

alfonsosanchezbeato added 3 commits January 24, 2025 15:06

tests: introduce vars to create kernel comps for nested

eb5b50c

Introduce NESTED_COMP_KERNEL_MODULE_NAME and NESTED_KERNEL_MODULES_COMP env vars. If present, a kernel-modules component containing the specified kernel module will be created and will be added when building the nested test image.

many: create KernelBootInfo struct to be returned by BuildKernelBootInfo

d39de47

cmd/snap-bootstrap: create drivers tree mounts when installing

caf563f

Create the drivers tree mounts when installing UC from the initramfs.

alfonsosanchezbeato added 7 commits January 24, 2025 15:06

cmd/snap-bootstrap: unmount properly component mounts used just

a167da8

to build the drivers tree.

tests/uc20-install-in-initrd: add checks for mount files

f6ab58a

cmd/snap-bootstrap: do not create drivers tree if preseeding

9dcf60b

It will be part of the preseed tarball.

many: move NeedsKernelSetup to kernel package

5a8cded

many: move BuildKernelBootInfo to overlord/install package

0969574

many: some refactoring and name changes

ac13f3c

alfonsosanchezbeato force-pushed the include-kern-mod-comps-on-initramfs-install branch from 73ccbf2 to ac13f3c Compare January 24, 2025 20:06

Meulengracht approved these changes Jan 27, 2025

View reviewed changes

tests: do not use kernel-modules components when UC < UC24

ff3bc0f

alfonsosanchezbeato merged commit b803966 into canonical:master Jan 28, 2025
64 of 67 checks passed

kubiko added a commit to kubiko/snapd that referenced this pull request Feb 3, 2025

Revert "many: make kernel-modules components available on initramfs i…

cafa75d

…nstallation (canonical#14810)" This reverts commit b803966.

	// NeedsKernelDriversTree tells if we need a kernel drivers tree for this model.
	// NeedsKernelDriversTree returns true if we need a kernel drivers tree for this model.

Conversation

alfonsosanchezbeato commented Dec 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Dec 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Jan 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Failures:

Preparing:

Executing:

Restoring:

Uh oh!

alfonsosanchezbeato commented Jan 17, 2025

Uh oh!

pedronis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alfonsosanchezbeato left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pedronis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Meulengracht left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alfonsosanchezbeato left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alfonsosanchezbeato commented Dec 5, 2024 •

edited

Loading

codecov bot commented Dec 5, 2024 •

edited

Loading

github-actions bot commented Jan 15, 2025 •

edited

Loading