Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core-initrd,snap-bootstrap: look at env to mount directly to /sysroot #15076

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

alfonsosanchezbeato
Copy link
Member

Previously we looked at the model and mounted base/rootfs directly if it was Ubuntu 24+. Now, we check instead an environment variable that is set if the initramfs is 24+.

This is done so now the kernels that contain the 24+ initramfs can be booted in older Ubuntu releases with older bases. This situation is not something we really support, as the systemd of the initramfs will not match the one in the system, but it is something that could happen while remodeling and it seems safer to allow this at least temporarily.

@alfonsosanchezbeato
Copy link
Member Author

@ZeyadYasser this is probably the situation that you hit (24 kernel used in a 22 model).

Copy link

codecov bot commented Feb 13, 2025

Codecov Report

Attention: Patch coverage is 85.71429% with 2 lines in your changes missing coverage. Please review.

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

Files with missing lines Patch % Lines
cmd/snap-bootstrap/cmd_initramfs_mounts.go 85.71% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##             master   #15076   +/-   ##
=========================================
  Coverage          ?   78.14%           
=========================================
  Files             ?     1173           
  Lines             ?   157782           
  Branches          ?        0           
=========================================
  Hits              ?   123296           
  Misses            ?    26838           
  Partials          ?     7648           
Flag Coverage Δ
unittests 78.14% <85.71%> (?)

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.

Copy link

github-actions bot commented Feb 13, 2025

Wed Feb 19 15:45:59 UTC 2025
The following results are from: https://github.com/canonical/snapd/actions/runs/13413223462

Failures:

Preparing:

  • google-nested:ubuntu-24.04-64:tests/nested/core/
  • google-nested:ubuntu-24.04-64:tests/nested/manual/kernel-modules-components:encrypted
  • google-nested:ubuntu-24.04-64:tests/nested/manual/recovery-system-reboot:recover
  • google-nested:ubuntu-24.04-64:tests/nested/manual/uc-update-assets-secure:both
  • google-nested:ubuntu-24.04-64:tests/nested/manual/uc-grub-boot-chains
  • google-nested:ubuntu-24.04-64:tests/nested/manual/core22-basic:tokens
  • google-nested:ubuntu-24.04-64:tests/nested/manual/core20-fault-inject-on-remodel:kernel_reboot_remodel_boot_assets
  • google-nested:ubuntu-24.04-64:tests/nested/manual/hybrid-remodel
  • google-nested:ubuntu-24.04-64:tests/nested/manual/component-recovery-system

Executing:

  • openstack:fedora-41-64:tests/main/default-tracks
  • google-nested:ubuntu-22.04-64:tests/nested/manual/muinstaller-real:encrypted
  • google-nested:ubuntu-22.04-64:tests/nested/manual/muinstaller-real:partial
  • google-nested:ubuntu-22.04-64:tests/nested/manual/muinstaller-real:plain
  • google-nested:ubuntu-22.04-64:tests/nested/manual/muinstaller-real:seeded
  • google-nested:ubuntu-24.04-64:tests/nested/manual/core20-early-config
  • google-nested:ubuntu-24.04-64:tests/nested/manual/muinstaller-real:partial
  • google-nested:ubuntu-24.04-64:tests/nested/manual/muinstaller-core:install_optional_snap_and_comp
  • google-nested:ubuntu-24.04-64:tests/nested/manual/muinstaller-real:encrypted
  • google-core:ubuntu-core-18-64:tests/core/kernel-base-gadget-single-reboot
  • google:ubuntu-20.04-64:tests/main/preseed-core20
  • google:ubuntu-24.04-64:tests/unit/go:gcc

Restoring:

  • google-nested:ubuntu-24.04-64:tests/nested/core/
  • google-nested:ubuntu-24.04-64:tests/nested/core/
  • google-nested:ubuntu-24.04-64
  • google-core:ubuntu-core-18-64:tests/core/kernel-base-gadget-single-reboot
  • google-core:ubuntu-core-18-64:tests/core/
  • google-core:ubuntu-core-18-64

@alfonsosanchezbeato alfonsosanchezbeato added the Run nested The PR also runs tests inluded in nested suite label Feb 14, 2025
Copy link
Collaborator

@pedronis pedronis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, I think we want to keep a comment in the code why we are doing it this way, because is not super obvious

return false
default:
func createSysrootMount() bool {
isCore24plus := osGetenv("CORE24_PLUS_INITRAMFS")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we want a comment in code here, why we want to this based on initramfs and not the UC/hybrid version

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I've added a comment

Copy link
Collaborator

@pedronis pedronis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 from me

Copy link
Contributor

@ZeyadYasser ZeyadYasser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Thank you!

I tested it against the exact scenario (core22 base with a 24 kernel) and it passed installation and first boot. Test used for testing is from #14867.

case "core20", "core22", "core22-desktop":
return false
default:
func createSysrootMount() bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
func createSysrootMount() bool {
func shouldCreateSysrootMount() bool {

Comment on lines 1880 to 1883
if isCore24plus == "1" || isCore24plus == "true" {
return true
}
return false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if isCore24plus == "1" || isCore24plus == "true" {
return true
}
return false
return isCore24plus == "1" || isCore24plus == "true"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed

Previously we looked at the model and mounted base/rootfs directly if
it was Ubuntu 24+. Now, we check instead an environment variable that
is set if the initramfs is 24+.

This is done so now the kernels that contain the 24+ initramfs can be
booted in older Ubuntu releases with older bases. This situation is
not something we really support, as the systemd of the initramfs will
not match the one in the system, but it is something that could happen
while remodeling and it seems safer to allow this at least
temporarily.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Run nested The PR also runs tests inluded in nested suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants