many: handlers and backend for kernel-modules components #13512

alfonsosanchezbeato · 2024-01-23T14:11:41Z

Implementation of specific tasks and handlers for kernel-modules components, that need additional mount units and rebuilding modinfo files.

A spread test for this will depend on core24 changes.

The backend will create the necessary mount units for kernel-modules component type. These units need to be available early on boot so udevd and systemd-modules-load.service can access the contained modules and firmware. Additionally, depmod is invoked to recreate the modinfo files.

pedronis

I'm confused/worried about the consequences of the removing part of the setup. Maybe the description of the PR needs to explain a bit more what happens here?

pedronis · 2024-01-24T17:16:31Z

overlord/snapstate/backend_test.go

+func (f *fakeSnappyBackend) SetupKernelModulesComponent(cpi snap.ContainerPlaceInfo, cref naming.ComponentRef, kernelVersion string, meter progress.Meter) (err error) {
+	meter.Notify("setup-kernel-modules-component")
+	f.appendOp(&fakeOp{
+		op: "setup-kernel-modules-component",


don't we want to capture a bit more about the parameters? in retrospect we should have done so also for SetupComponent and UndoSetupComponent

Right, I've fixed that now.

overlord/snapstate/backend_test.go

pedronis · 2024-01-24T17:19:27Z

overlord/snapstate/handlers_components.go

@@ -396,3 +398,185 @@ func (m *SnapManager) undoUnlinkCurrentComponent(t *state.Task, _ *tomb.Tomb) (e

 	return nil
 }
+
+func kernelVersionFromPlaceInfo(spi snap.PlaceInfo) (string, error) {


should this go in the kernel package?

pedronis · 2024-01-24T17:26:17Z

overlord/snapstate/component.go

+			undoSetupKernMod := st.NewTask("cleanup-kernel-modules-component",
+				fmt.Sprintf(i18n.G("Clean-up previous kernel-modules component"),
+					compSi.Component, revisionStr))
+			addTask(undoSetupKernMod)


what happens if we boot in between here? shouldn't we do a unit replace/reload instead of removing/cleaning up stuff?

Or use symlinks if it's simpler

It a reboot happens between the clean-up of the old component and the creation of the new units for the new component, then the component won't be available after boot, but when the change tasks are restarted things should be fine. So whether this is critical or not depends on if we consider that having the component available early is necessary or not. And the answer is maybe yes, as in some cases we want the kernel modules to be loaded by udev rules, or maybe apps will want to load the modules when they are started.

I've been taking a deeper look at this and done some changes. The observations I have are:

There is not really a need to clean-up before installing here, as the mount units that are generated actually replace the files from the old component (mountpoints are the same). The switch happens when the new component is mounted to /run/mnt/kernel-modules/....

But, if there is an undo, old units need to be restored. To do this, now I restore them in the undo step of setup-kernel-modules-component in case the component was in the system previous to trying the installation, otherwise units are removed as before. So in the end no new task is needed.

As the unit files are atomically created I expect that some component will be available if a reboot in-between happens and that the change can progress as expected after the reboot. But there is still a possible issue with calls to depmod, as I don't think it is atomic. However it is doubtful that the modinfo cache will have a change when updating a component while the kernel revision is not changing (at least for Canonical maintained kernels).

To really do things as atomic as possible we should work in a separate kernel tree so depmod does not change the current cache and replace the current tree at the final step.

Still, I have kept for the moment the handlers for cleaning-up the component, we will need them for component removal anyway.

alfonsosanchezbeato · 2024-01-25T17:33:12Z

Related changes for the base in canonical/core-initrd#229

mounted kernel snap.

alfonsosanchezbeato

@pedronis thanks for the review, I've addressed your comments now

alfonsosanchezbeato · 2024-01-29T11:39:53Z

overlord/snapstate/component.go

+			undoSetupKernMod := st.NewTask("cleanup-kernel-modules-component",
+				fmt.Sprintf(i18n.G("Clean-up previous kernel-modules component"),
+					compSi.Component, revisionStr))
+			addTask(undoSetupKernMod)


I've been taking a deeper look at this and done some changes. The observations I have are:

There is not really a need to clean-up before installing here, as the mount units that are generated actually replace the files from the old component (mountpoints are the same). The switch happens when the new component is mounted to /run/mnt/kernel-modules/....

But, if there is an undo, old units need to be restored. To do this, now I restore them in the undo step of setup-kernel-modules-component in case the component was in the system previous to trying the installation, otherwise units are removed as before. So in the end no new task is needed.

As the unit files are atomically created I expect that some component will be available if a reboot in-between happens and that the change can progress as expected after the reboot. But there is still a possible issue with calls to depmod, as I don't think it is atomic. However it is doubtful that the modinfo cache will have a change when updating a component while the kernel revision is not changing (at least for Canonical maintained kernels).

To really do things as atomic as possible we should work in a separate kernel tree so depmod does not change the current cache and replace the current tree at the final step.

Still, I have kept for the moment the handlers for cleaning-up the component, we will need them for component removal anyway.

alfonsosanchezbeato · 2024-01-29T12:01:58Z

overlord/snapstate/backend_test.go

+func (f *fakeSnappyBackend) SetupKernelModulesComponent(cpi snap.ContainerPlaceInfo, cref naming.ComponentRef, kernelVersion string, meter progress.Meter) (err error) {
+	meter.Notify("setup-kernel-modules-component")
+	f.appendOp(&fakeOp{
+		op: "setup-kernel-modules-component",


Right, I've fixed that now.

overlord/snapstate/backend_test.go

alfonsosanchezbeato · 2024-01-29T14:24:37Z

overlord/snapstate/handlers_components.go

@@ -396,3 +398,185 @@ func (m *SnapManager) undoUnlinkCurrentComponent(t *state.Task, _ *tomb.Tomb) (e

 	return nil
 }
+
+func kernelVersionFromPlaceInfo(spi snap.PlaceInfo) (string, error) {


codecov-commenter · 2024-01-29T16:05:58Z

Codecov Report

Attention: 73 lines in your changes are missing coverage. Please review.

Comparison is base (9602137) 79.00% compared to head (99fdf26) 78.88%.
Report is 18 commits behind head on master.

Files	Patch %	Lines
...ord/snapstate/backend/kernel_modules_components.go	71.42%	26 Missing and 10 partials ⚠️
overlord/snapstate/handlers_components.go	79.87%	22 Missing and 11 partials ⚠️
kernel/kernel.go	75.00%	3 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #13512      +/-   ##
==========================================
- Coverage   79.00%   78.88%   -0.12%     
==========================================
  Files        1030     1032       +2     
  Lines      130360   131050     +690     
==========================================
+ Hits       102986   103375     +389     
- Misses      20968    21237     +269     
- Partials     6406     6438      +32

Flag	Coverage Δ
unittests	`78.88% <76.75%> (-0.12%)`	⬇️

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.

pedronis

thanks for the changes, did another pass

pedronis · 2024-01-30T09:31:13Z

kernel/kernel.go

+		return "", fmt.Errorf("internal error: %w", err)
+	}
+	if len(matches) != 1 {
+		return "", fmt.Errorf("number of matches for %s is %d", matchPath, len(matches))


maybe:

unexpected number of matches (%d) for kernel %s ?

pedronis · 2024-01-30T09:42:00Z

overlord/snapstate/backend/kernel_modules_components.go

+// UndoSetupKernelModulesComponent undoes the work of SetupKernelModulesComponent
+func (b Backend) UndoSetupKernelModulesComponent(cpi snap.ContainerPlaceInfo, cref naming.ComponentRef, kernelVersion string, meter progress.Meter) error {
+	hasModules, hasFirmware := checkKernelModulesCompContent(cpi.MountDir(), kernelVersion)
+	var partsToClean kernelModulesCleanupParts


probably good to have a comment about why rerunDepmod is not set

I've added a comment a bit below related to depmod - not 100% sure if that is what you meant though.

pedronis · 2024-01-30T10:52:18Z

overlord/snapstate/backend/kernel_modules_components.go

+	var cleanOnFailure kernelModulesCleanupParts
+	defer func() {
+		if err == nil {
+			return
+		}
+		if err := cleanupKernelModulesSetup(&cleanOnFailure, kernelVersion, meter); err != nil {
+			logger.Noticef("while cleaning up a failed kernel-modules set-up: %v", err)
+		}
+	}()


should we keep doing this here? or should we make it a problem of the caller, which might decide to do a switch back instead?

That's a good point as indeed in some cases we want to restore the old state and we might not want to do previously a clean-up.

pedronis · 2024-01-30T10:53:38Z

overlord/snapstate/backend/kernel_modules_components.go

+
+var runDepmod = runDepmodImpl
+
+func runDepmodImpl(baseDir, kernelVersion string) error {


should we have at least some tests using testutil.MockCmd to test this?

pedronis · 2024-01-30T10:55:09Z

overlord/snapstate/backend/kernel_modules_components.go

+}
+
+func componentMountPoint(componentName, kernelVersion string) string {
+	return filepath.Join("/run/mnt/kernel-modules/", kernelVersion, componentName)


I still wonder if it would be beneficial or not to include the revision in this one

pedronis · 2024-01-30T10:55:48Z

overlord/snapstate/backend/kernel_modules_components.go

+			What:          filepath.Join(componentMount, "modules", kernelVersion),
+			Where:         modulesDir,
+			Fstype:        "none",
+			Options:       []string{"bind"},


could we record somewhere a discussion of why using a bind mount and not a symlink for this?

MiguelPires

lgtm

MiguelPires · 2024-01-25T10:53:52Z

overlord/snapstate/backend/kernel_modules_components.go

+	if err := os.RemoveAll(mountDir); err != nil {
+		return err
+	}
+
+	return nil


nitpick: could be shortened if you wanted to

Suggested change

if err := os.RemoveAll(mountDir); err != nil {

return err

}

return nil

return os.RemoveAll(mountDir)

bboozzoo · 2024-01-31T13:54:46Z

kernel/kernel.go

+// snap (this would be the output if "uname -r" for a running kernel).
+func KernelVersionFromPlaceInfo(spi snap.PlaceInfo) (string, error) {
+	systemMapPathPrefix := filepath.Join(spi.MountDir(), "System.map-")
+	matchPath := systemMapPathPrefix + "*"


instead of guessig, could we read the snap.yaml and use the version declared there, assuming we can rely on the version being set and matching what would be found if we chopped the file names into pieces.

Unfortunately uname -r is not matching the version in snap.yaml and even if it did we might find non-Canonical kernels not following our conventions.

bboozzoo · 2024-01-31T13:56:20Z

overlord/snapstate/handlers_components.go

+
+	// Find kernel version from matching kernel
+	spi := snap.MinimalPlaceInfo(snapsup.InstanceName(), snapsup.Revision())
+	kernelVersion, err := kernel.KernelVersionFromPlaceInfo(spi)


we could do kernel.ReadInfo(spi.MountDir()) here

bboozzoo · 2024-01-31T13:58:38Z

overlord/snapstate/backend/kernel_modules_components.go

+	return nil
+}
+
+func checkKernelModulesCompContent(mountDir, kernelVersion string) (bool, bool) {


there isn't much in the way of checking, sounds more like discoverKernelModulesComContent?

bboozzoo · 2024-01-31T14:36:26Z

overlord/snapstate/backend/kernel_modules_components.go

+
+func runDepmodImpl(baseDir, kernelVersion string) error {
+	logger.Debugf("running depmod on %q for kernel %s", baseDir, kernelVersion)
+	stdout, stderr, err := osutil.RunSplitOutput("depmod", "-b", baseDir, kernelVersion)


btw. do you need to pass -e to get more useful error information maybe?

alfonsosanchezbeato · 2024-01-31T15:32:34Z

Thanks for the reviews - I am moving this to draft as we are rethinking how to do parts of this.

alfonsosanchezbeato added 4 commits January 23, 2024 10:57

snap: add kernel-modules component type

daa20d5

o/snapstate: add handlers to set-up kernel-modules components

26e8c17

o/snapstate: run specific kernel-modules tasks if needed

eab1d69

alfonsosanchezbeato requested review from pedronis and MiguelPires January 23, 2024 14:11

pedronis reviewed Jan 24, 2024

View reviewed changes

alfonsosanchezbeato added 6 commits January 26, 2024 14:01

fixup! o/snapstate: add handlers to set-up kernel-modules components

dbda1b6

fixup! o/snapstate: run specific kernel-modules tasks if needed

da36b51

fixup! o/snapstate: add handlers to set-up kernel-modules components

858a053

fixup! o/snapstate/backend: add support for kernel-modules component

74a4d2a

overlord,kernel: add function to extract kernel version from

68970fb

mounted kernel snap.

fixup! o/snapstate/backend: add support for kernel-modules component

99fdf26

alfonsosanchezbeato commented Jan 29, 2024

View reviewed changes

alfonsosanchezbeato requested a review from pedronis January 29, 2024 15:53

pedronis reviewed Jan 30, 2024

View reviewed changes

MiguelPires approved these changes Jan 30, 2024

View reviewed changes

alfonsosanchezbeato added 2 commits January 30, 2024 16:32

fixup! overlord,kernel: add function to extract kernel version from

3699010

fixup! o/snapstate: add handlers to set-up kernel-modules components

805cb4d

bboozzoo reviewed Jan 31, 2024

View reviewed changes

alfonsosanchezbeato marked this pull request as draft January 31, 2024 15:33

alfonsosanchezbeato closed this Mar 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

many: handlers and backend for kernel-modules components #13512

many: handlers and backend for kernel-modules components #13512

alfonsosanchezbeato commented Jan 23, 2024

pedronis left a comment

pedronis Jan 24, 2024

alfonsosanchezbeato Jan 29, 2024

pedronis Jan 24, 2024

alfonsosanchezbeato Jan 29, 2024

pedronis Jan 24, 2024 •

edited

Loading

alfonsosanchezbeato Jan 29, 2024

alfonsosanchezbeato Jan 29, 2024

alfonsosanchezbeato commented Jan 25, 2024

alfonsosanchezbeato left a comment

alfonsosanchezbeato Jan 29, 2024

alfonsosanchezbeato Jan 29, 2024

alfonsosanchezbeato Jan 29, 2024

codecov-commenter commented Jan 29, 2024

pedronis left a comment

pedronis Jan 30, 2024

alfonsosanchezbeato Jan 30, 2024

pedronis Jan 30, 2024

alfonsosanchezbeato Jan 30, 2024

pedronis Jan 30, 2024

alfonsosanchezbeato Jan 30, 2024

pedronis Jan 30, 2024

pedronis Jan 30, 2024

pedronis Jan 30, 2024

MiguelPires left a comment

MiguelPires Jan 25, 2024

bboozzoo Jan 31, 2024

alfonsosanchezbeato Jan 31, 2024

bboozzoo Jan 31, 2024

bboozzoo Jan 31, 2024

bboozzoo Jan 31, 2024

alfonsosanchezbeato commented Jan 31, 2024


		var runDepmod = runDepmodImpl

		func runDepmodImpl(baseDir, kernelVersion string) error {

many: handlers and backend for kernel-modules components #13512

many: handlers and backend for kernel-modules components #13512

Conversation

alfonsosanchezbeato commented Jan 23, 2024

pedronis 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

pedronis Jan 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alfonsosanchezbeato commented Jan 25, 2024

alfonsosanchezbeato 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

codecov-commenter commented Jan 29, 2024

Codecov Report

pedronis 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

MiguelPires 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

alfonsosanchezbeato commented Jan 31, 2024

pedronis Jan 24, 2024 •

edited

Loading