cmd/snap-bootstrap: mount base directly on /sysroot #14537

alfonsosanchezbeato · 2024-09-25T19:53:23Z

Mount the base directly on /sysroot in all UC modes, while for hybrid
we mount /run/mnt/data instead of the /run/mnt/sysroot symlink that
was pointing to the former. With this change we do not need to have
the /run/mnt/base mount anymore.

alfonsosanchezbeato · 2024-09-25T19:54:46Z

Opening as draft as we will need canonical/core-initrd#265 landed first. As a side note, we should discuss how to handle this initramfs/snap-bootstrap interlocks in an easier way.

codecov · 2024-09-25T20:10:44Z

Codecov Report

Attention: Patch coverage is 45.87156% with 59 lines in your changes missing coverage. Please review.

Please upload report for BASE (master@6fa2df6). Learn more about missing BASE report.
Report is 59 commits behind head on master.

Files with missing lines	Patch %	Lines
cmd/snap-bootstrap/cmd_initramfs_mounts.go	45.00%	37 Missing and 7 partials ⚠️
cmd/snap-bootstrap/initramfs_systemd_mount.go	48.27%	10 Missing and 5 partials ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##             master   #14537   +/-   ##
=========================================
  Coverage          ?   78.20%           
=========================================
  Files             ?     1162           
  Lines             ?   154334           
  Branches          ?        0           
=========================================
  Hits              ?   120692           
  Misses            ?    26199           
  Partials          ?     7443

Flag	Coverage Δ
unittests	`78.20% <45.87%> (?)`

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 · 2024-09-30T16:25:42Z

cmd/snap-bootstrap/cmd_initramfs_mounts.go

 		}
 	}

+	// Do a daemon reload so systemd knows about the new mount units


will it also perform the mount then?

Not really, as the new mount unit has an After=snap-initramfs-mounts.service dependency

valentindavid · 2024-11-01T13:12:28Z

cmd/snap-bootstrap/cmd_initramfs_mounts.go

 		}
 	}

+	// Do a daemon reload so systemd knows about the new mount units
+	sysd := systemd.New(systemd.SystemMode, nil)
+	if err := sysd.DaemonReload(); err != nil {


Will will always have units that need loading? daemon-reload is costly. If we could try to not do it if we did not call writeSysrootMountUnit, then it would be very good.

Right, but note that we are always creating the unit in this case (the base is always in the essSnaps slice).

alfonsosanchezbeato

I have rebased this PR and added the changes to the initramfs that were previously in a PR in the core-initrd repo.

The changes in snap-bootstrap could need to check for the UC release. It depends on whether we want the initramfs changes to be backported to UC20/22.

alfonsosanchezbeato · 2025-01-30T20:04:34Z

cmd/snap-bootstrap/cmd_initramfs_mounts.go

 		}
 	}

+	// Do a daemon reload so systemd knows about the new mount units
+	sysd := systemd.New(systemd.SystemMode, nil)
+	if err := sysd.DaemonReload(); err != nil {


Right, but note that we are always creating the unit in this case (the base is always in the essSnaps slice).

alfonsosanchezbeato · 2025-01-30T20:08:04Z

@valentindavid @pedronis this is ready now for review.

github-actions · 2025-01-30T22:32:57Z

Tue Feb 4 18:30:20 UTC 2025
The following results are from: https://github.com/canonical/snapd/actions/runs/13125002142

Failures:

Preparing:

google-nested:ubuntu-24.04-64:tests/nested/manual/remodel-min-size

Executing:

openstack:debian-sid-64:tests/main/microk8s-smoke:edge
google-nested:ubuntu-24.04-64:tests/nested/manual/recovery-system-reboot:factory_reset
openstack:opensuse-tumbleweed-64:tests/main/auto-refresh-pre-download:restart
openstack:opensuse-tumbleweed-64:tests/main/auto-refresh-pre-download:close_mid_restart
openstack:opensuse-tumbleweed-64:tests/main/snap-refresh-hold
openstack:opensuse-tumbleweed-64:tests/main/auto-refresh-pre-download:close
openstack:opensuse-tumbleweed-64:tests/main/auto-refresh-gating
openstack:opensuse-tumbleweed-64:tests/main/auto-refresh-pre-download:ignore
openstack:opensuse-tumbleweed-64:tests/main/auto-refresh-backoff
openstack:opensuse-tumbleweed-64:tests/main/auto-refresh:parallel
openstack:opensuse-tumbleweed-64:tests/main/auto-refresh:regular
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/microk8s-smoke:edge
openstack:opensuse-15.6-64:tests/main/microk8s-smoke:edge
openstack:opensuse-tumbleweed-64:tests/main/refresh-app-awareness-notify
google:ubuntu-25.04-64:tests/main/security-device-cgroups:kmsg
google:ubuntu-25.04-64:tests/main/security-device-cgroups-serial-port
google:ubuntu-25.04-64:tests/main/security-device-cgroups-required-or-optional
google:ubuntu-25.04-64:tests/main/cgroup-devices-v2
google:ubuntu-25.04-64:tests/main/security-device-cgroups-strict-enforced
google:ubuntu-25.04-64:tests/main/security-device-cgroups:uinput
google:ubuntu-25.04-64:tests/main/security-device-cgroups-self-manage
google:ubuntu-25.04-64:tests/main/security-device-cgroups-helper

Restoring:

openstack:opensuse-tumbleweed-64:tests/main/refresh-app-awareness-notify
google:ubuntu-25.04-64:tests/main/security-device-cgroups-strict-enforced

pedronis

thanks

pedronis · 2025-01-31T16:39:49Z

cmd/snap-bootstrap/initramfs_systemd_mount.go

+	}{what, mntType}); err != nil {
+		return err
+	}
+	unitContent.Bytes()


is this a spurious left over?

It is indeed, removed now, thanks

Mount the base directly on /sysroot in UC modes, while for hybrid we mount /run/mnt/data instead of the /run/mnt/sysroot symlink that was pointing to the former.

alfonsosanchezbeato · 2025-01-31T20:48:06Z

Some debugging has revealed that this was not working anymore because I had removed the static sysroot.mount unit, we have to keep it as daemon-reload does not trigger a full recalculation of dependencies (see systemd/systemd#23034).

With the snap-bootstrap changes, the base/rootfs is mounted directly to /sysroot. With this, we do not need the symlinks created by detect-*-sysroot.service units. However, we need to keep the static sysroot mount unit so dependencies are still honored. kernel-snap-generator and classic-mounts.service are not needed as for 24+ the kernel mount units are created by snap-bootstrap. The units that need to detect if we are on core/hybrid check now for the /run/mnt/data/system-data (if present, it is UC) instead of for a mountpoint in /run/mnt/base.

alfonsosanchezbeato · 2025-01-31T21:52:27Z

I have now also restricted the change to systems based on 24.04 or latter - otherwise we would need to wait for changes in the UC20/22 initramfs before releasing a new snapd and that can take some time.

pedronis

thanks

for UC24+.

We need to mount the base so we can build the password related files.

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

alfonsosanchezbeato requested review from valentindavid and pedronis September 25, 2024 19:53

pedronis reviewed Sep 30, 2024

View reviewed changes

alfonsosanchezbeato mentioned this pull request Oct 21, 2024

c/snap-bootstrap: import users from hybrid system into recovery system #14521

Merged

valentindavid reviewed Nov 1, 2024

View reviewed changes

alfonsosanchezbeato force-pushed the mount_base_to_sysroot branch from 87a834f to dcba9a9 Compare January 30, 2025 20:01

alfonsosanchezbeato commented Jan 30, 2025

View reviewed changes

alfonsosanchezbeato marked this pull request as ready for review January 30, 2025 20:07

alfonsosanchezbeato requested review from pedronis and valentindavid January 30, 2025 20:07

pedronis approved these changes Jan 31, 2025

View reviewed changes

cmd/snap-bootstrap: mount base directly on /sysroot

35cb6ee

Mount the base directly on /sysroot in UC modes, while for hybrid we mount /run/mnt/data instead of the /run/mnt/sysroot symlink that was pointing to the former.

alfonsosanchezbeato force-pushed the mount_base_to_sysroot branch from dcba9a9 to a1ae5af Compare January 31, 2025 20:45

alfonsosanchezbeato added 2 commits January 31, 2025 16:50

cmd/snap-bootstrap: restrict the change in base mount to UC24+

0a2606a

alfonsosanchezbeato force-pushed the mount_base_to_sysroot branch from a1ae5af to 0a2606a Compare January 31, 2025 21:50

alfonsosanchezbeato requested a review from pedronis January 31, 2025 21:52

pedronis approved these changes Feb 3, 2025

View reviewed changes

alfonsosanchezbeato added 3 commits February 3, 2025 10:47

tests/core/basic20plus: adapt to change in mount of sysroot

69b0f01

cmd/snap-bootstrap: do not try to read unmounted base on installation

bf49df8

for UC24+.

cmd/snap-bootstrap: mount base in recover mode

edb552f

We need to mount the base so we can build the password related files.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/snap-bootstrap: mount base directly on /sysroot #14537

cmd/snap-bootstrap: mount base directly on /sysroot #14537

alfonsosanchezbeato commented Sep 25, 2024

alfonsosanchezbeato commented Sep 25, 2024

codecov bot commented Sep 25, 2024 •

edited

Loading

pedronis Sep 30, 2024

alfonsosanchezbeato Sep 30, 2024

valentindavid Nov 1, 2024

alfonsosanchezbeato Jan 30, 2025

alfonsosanchezbeato left a comment

alfonsosanchezbeato Jan 30, 2025

alfonsosanchezbeato commented Jan 30, 2025

github-actions bot commented Jan 30, 2025 •

edited

Loading

pedronis left a comment

pedronis Jan 31, 2025

alfonsosanchezbeato Jan 31, 2025

alfonsosanchezbeato commented Jan 31, 2025

alfonsosanchezbeato commented Jan 31, 2025

pedronis left a comment

cmd/snap-bootstrap: mount base directly on /sysroot #14537

Are you sure you want to change the base?

cmd/snap-bootstrap: mount base directly on /sysroot #14537

Conversation

alfonsosanchezbeato commented Sep 25, 2024

alfonsosanchezbeato commented Sep 25, 2024

codecov bot commented Sep 25, 2024 • edited Loading

Codecov Report

pedronis Sep 30, 2024

Choose a reason for hiding this comment

alfonsosanchezbeato Sep 30, 2024

Choose a reason for hiding this comment

valentindavid Nov 1, 2024

Choose a reason for hiding this comment

alfonsosanchezbeato Jan 30, 2025

Choose a reason for hiding this comment

alfonsosanchezbeato left a comment

Choose a reason for hiding this comment

alfonsosanchezbeato Jan 30, 2025

Choose a reason for hiding this comment

alfonsosanchezbeato commented Jan 30, 2025

github-actions bot commented Jan 30, 2025 • edited Loading

Failures:

Preparing:

Executing:

Restoring:

pedronis left a comment

Choose a reason for hiding this comment

pedronis Jan 31, 2025

Choose a reason for hiding this comment

alfonsosanchezbeato Jan 31, 2025

Choose a reason for hiding this comment

alfonsosanchezbeato commented Jan 31, 2025

alfonsosanchezbeato commented Jan 31, 2025

pedronis left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 25, 2024 •

edited

Loading

github-actions bot commented Jan 30, 2025 •

edited

Loading