-
Notifications
You must be signed in to change notification settings - Fork 594
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
snap-bootstrap: experimental: handling install/factory reset steps within snap-bootstrap #12382
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks promising, we need to look into:
- when do we do this? always? can we? not always but based on what?
- does that mean that install handlers go away?
- what to do about hooks, calling fde-reveal-key instead of fde-setup is a change of contract that doesn't make sense, also it means we cannot really do this always
- we need to properly split the parts of devicestate we are reusing to a package (to avoid bringing in most of overlord in snap-bootstrap), and also try to have even more higher-level helpers, there is still quite a bit of stuff that feels repeated from install code and could go out of sync
- we need to see what do about places that look at the kernel command line directly, I could spot:
- https://github.com/snapcore/core-base/search?q=ConditionKernelCommandLine
-
./overlord/configstate/configcore/services.go: mode, _, _ := boot.ModeAndRecoverySystemFromKernelCommandLine() ./cmd/snap/cmd_auto_import.go: mode, _, err := boot.ModeAndRecoverySystemFromKernelCommandLine() ./cmd/snap-bootstrap/cmd_initramfs_mounts.go: mode, recoverySystem, err := boot.ModeAndRecoverySystemFromKernelCommandLine()
- what to do about install-device hook
@@ -149,7 +151,7 @@ func generateInitramfsMounts() (err error) { | |||
case "recover": | |||
err = generateMountsModeRecover(mst) | |||
case "install": | |||
err = generateMountsModeInstall(mst) | |||
err = installNewSystemGenerateMounts(mst) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the idea to always do this? should we do this only if preseed is setup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems we can't do this in general because of the different hook contract ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a rather radical opinion here :)
I thin we should completely eliminate "install boot" for all the scenarios unless proven otherwise that this is needed.
initramfs is exactly what we want as an ephemeral system to run install in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as we discussed we have to:
- start smaller and decide how to trigger this (which probably relates to the question about the hook contract)
- we still have use cases for the full install mode (dedicated installers for example)
// 1.1 load seed | ||
deviceSeed, err := loadDeviceSeed(systemLabel) | ||
if err != nil { | ||
return err | ||
} | ||
// 1.2 load seed assertions | ||
assertDb, err := loadSeedAssertions(deviceSeed) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
// 1.3 load sead essential meta | ||
if err = loadSeedEssentialMeta(deviceSeed, systemLabel); err != nil { | ||
return err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this probably can be one helper?
} | ||
|
||
// 2.2 ensure next boot is to run mode | ||
if err := boot.EnsureNextBootToRunMode(systemLabel); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is a bit more than this happening in doRestartSystemToRunMode that we might want/need to take care of
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, this should be removed from here and system should be marked as bootable later once it actually boots.
If we mark system runable here and it fails later, the install would not be re-run. Perhaps more fitting place for this would be:
https://github.com/snapcore/snapd/blob/master//boot/boot.go#L355
|
||
func generateSnapMountsModeRun(mst *initramfsMountsState, isClassic, hasSeedPart bool, rootfsDir string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: this has a bit too many bool args
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally I'd extend initramfsMountsState
struct to do more, so we do not pass this many things.
But fair point
|
||
// to set sysconfig.ApplyFilesystemOnlyDefaultsImpl | ||
_ "github.com/snapcore/snapd/overlord/configstate/configcore" | ||
"github.com/snapcore/snapd/overlord/devicestate" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this brings in most overlord which we don't want, we will have to split out the bits we need of devicestate to their own package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had this very concern when I was prototyping with a separate go binary.
In case of snap-bootstrap
we are unfortunately already pulling it in:
https://github.com/snapcore/snapd/blob/master/cmd/snap-bootstrap/cmd_initramfs_mounts.go#L46
So defo something to improve. And yes I can confirm it does increate size noticeably.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
configcore is different, is designed not to pull in most of overlord if things are compiled with -tags nomanagers
which we do for snap-boostrap or should
// encryptionType, err := m.checkEncryption(st, deviceCtx) | ||
// if err != nil { | ||
// return err | ||
// } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, we cannot assume we will do encryption
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's potentially next TODO to fix this
return nil | ||
} | ||
|
||
func doRunInstall(deviceSeed seed.Seed, recoverySystemLabel string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to find a way to share even more of this with install handler code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ultimately I think we should move the install handler to bootstrap.
preseedAs, err := devicestate.InstallHandlerDoReadPreseedAssertion(tmpDb, model, boot.InitramfsUbuntuSeedDir, systemLabel) | ||
if err != nil { | ||
return fmt.Errorf("failed to read preseed assertion: %v", err) | ||
} | ||
writableDir := boot.InstallHostWritableDir(model) | ||
if err != nil { | ||
return err | ||
} | ||
// TODO: improve handling of the preseed, we do double hashing for already loaded snaps | ||
// at the same time, we need to process them to generarate dir structures e.g. /sna/<name>/<rev> | ||
if err = devicestate.InstallHandlerDoMaybeApplyPreseededData(deviceSeed.Model(), | ||
preseedAs, preseedArtifact, boot.InitramfsUbuntuSeedDir, systemLabel, writableDir); err != nil { | ||
return fmt.Errorf("failed to apply preseed data: %v", err) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again it seems this should be made into one helper
mountInfos, err := osutil.LoadMountInfo() | ||
if err != nil { | ||
return fmt.Errorf("failed to load mount info: %v", err) | ||
} | ||
ubuntuDataDevice := "" | ||
for _, mnt := range mountInfos { | ||
if mnt.MountDir == InstallUbuntuDataDir { | ||
ubuntuDataDevice = mnt.MountSource | ||
break | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks a bit odd, there should be a more direct to know this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd love a simpler way here, but I could not find a go helper to get single-mount info.
Ultimately I do not think we will need this.
We divided /run/mnt/data
and /run/mnt/ubuntu-data
because we boot into install mode, where /run/mnt/data
is tmpfs
but this is no more concern. So my preference would be to run the install directly to /run/mnt/data
and this extra dance is no more needed.
|
||
// helper to clean up as much as we can at fail | ||
// TODO: do we even need this? | ||
func cleanupAtFail() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not called anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was calling it in my early PoC, but then I notice snap-bootstrap is not doing any cleanup so I'm not calling it now. but at least keep the code in case we decide to call it.
For debugging it was also helpful to see the state of the system where it bailed out.
It's a bit philosophical if we need to clean, or if we even should:
- in secure mode, there is non-interactive shell anyway, and we anyway wiped the whole thing.
- in debug mode, it's probably good to give a change to see what is where. If we unmount enc partitions and lock fde-hook, extend TPM, there is no way to mount them again....
c753bc4
to
48f9089
Compare
53aefed
to
111225b
Compare
111225b
to
26cb27c
Compare
8378a13
to
b9b10f0
Compare
bed97dc
to
de00bdc
Compare
e969c1f
to
c5362d4
Compare
7133e7e
to
cd61bb2
Compare
cd61bb2
to
abf5c69
Compare
abf5c69
to
9580148
Compare
9580148
to
21f49f7
Compare
972ee39
to
420dd05
Compare
420dd05
to
b3b58aa
Compare
8bd7d7c
to
54c9436
Compare
54c9436
to
dbd8062
Compare
@kubiko is this still useful/relevant? |
Discussed with @kubiko , this is still relevant and should not be removed. |
c000138
to
9d98b26
Compare
9d98b26
to
a5efc58
Compare
a5efc58
to
0672b9b
Compare
da6bb73
to
1803413
Compare
Signed-off-by: Ondrej Kubik <[email protected]>
Signed-off-by: Ondrej Kubik <[email protected]>
Signed-off-by: Ondrej Kubik <[email protected]>
Test show that we can use more cores before we hit I/O limit Do not artificially limit to 2 jobs, modern systems have 4+ cores and high disk I/O Signed-off-by: Ondrej Kubik <[email protected]>
During normal runtime fdekeymanager executable is snap-fde-keymgr (part of the snapd snap) When running within initramfs, effort is made to avoid multiple go binaries, so fdekeymanager functionality is provided by snap-bootstrap Provide option to set fdekeymanager executable name to support snap-bootstrap run environment. Signed-off-by: Ondrej Kubik <[email protected]>
…all package Signed-off-by: Ondrej Kubik <[email protected]>
Signed-off-by: Ondrej Kubik <[email protected]>
Signed-off-by: Ondrej Kubik <[email protected]>
Signed-off-by: Ondrej Kubik <[email protected]>
Signed-off-by: Ondrej Kubik <[email protected]>
Signed-off-by: Ondrej Kubik <[email protected]>
1803413
to
f66e03c
Compare
Fri Jan 31 16:56:11 UTC 2025 No spread failures reported |
This is a reference PR implementing
install
andfactory-reset
modes withinsnap-bootstrap
It should server also as tracking for the work done so far.
This activity is split across multiple PRs and not all might be mentioned here, especially if they have already landed.
As such, this PR can be regularly rebased and force-pushed to update tracking.
Current state:
install
mode support had landed on the master branchRelated PRs:
Open:
#12641
Landed:
#12846
Keeping original descriptions around for reference:
Original description: