{device,snap}state: skip kernel extraction in seeding #10595
Conversation
When seeding the device we currently extract the kernel. This is unneeded because the kernel is already in place (without it the system would not have been booted). Either ubuntu-image or "install" mode extracted the kernel into the right place already. This commit changes the code to skip kernel extraction when seeding.
…f populateStateFromSeedImpl()
overlord/devicestate/firstboot.go
Outdated
| @@ -243,6 +243,15 @@ func populateStateFromSeedImpl(st *state.State, opts *populateStateFromSeedOptio | |||
| // wait for the previous configTss | |||
| configTss = chainTs(configTss, configTs) | |||
| } | |||
| // XXX: put this into "snapstate.Flags" instead? | |||
There was a problem hiding this comment.
yes, it should be a Flag like SkipConfigure
mvo5
force-pushed
the
no-kernel-double-extract-managers
branch
2 times, most recently
from
9bc3790
to
60c5324
Compare
mvo5
force-pushed
the
no-kernel-double-extract-managers
branch
from
60c5324
to
2cd5f73
Compare
|
I got a verbal okay from Samuele for the general idea and the implementation using snapstate.Flags so just a "normal" review is now required for the landing. I would like to include this in 2.51 but opinions are welcome. It should be safe but we should make sure this is tested on a freshly generated pi image with UC18. Spread will only test this on UC20 because on UC18 nothing in the kernel gets extracted for the architectures that spread uses. |
overlord/snapstate/backend/setup.go
Outdated
| @@ -39,8 +39,16 @@ type InstallRecord struct { | |||
| TargetSnapExisted bool `json:"target-snap-existed,omitempty"` | |||
| } | |||
|
|
|||
| type SetupSnapOpts struct { | |||
There was a problem hiding this comment.
super small nitpick: I think most options I've seen in the code end with "Options" instead of "Opts" so that would be a more consistent alternative
| return snapType, nil, fmt.Errorf("cannot install kernel: %s", err) | ||
| if !setupOpts.SkipKernelExtraction { | ||
| if err := boot.Kernel(s, t, dev).ExtractKernelAssets(snapf); err != nil { | ||
| return snapType, nil, fmt.Errorf("cannot install kernel: %s", err) |
There was a problem hiding this comment.
I didn't check if err can be of some type which we might want to distinguish but using the %w verb would make fmt.Errorf return an error that wraps err. Probably not be useful here at all, just thought to mention it
There was a problem hiding this comment.
Thanks for raising this. I won't change it here because the existing code is written in this way but it's a good point. We are underusing %w a bit - partly for historic reasons because until recently we were stuck in the go-1.9 stone age :) Fortunately this has changed now!
There was a problem hiding this comment.
yeah we should probably have a separate PR which goes through and changes everything over to use %w, we have numerous places around the codebase that could benefit from using %w but never could use it previously.
We also have other cruft too like ctrl17.go and ctrl16.go which should be updated/removed too now that we are on a more modern Go version
When seeding the device we currently extract the kernel. This is
unneeded because the kernel is already in place (without it the
system would not have been booted). Either ubuntu-image or "install"
mode extracted the kernel into the right place already.
This commit changes the code to skip kernel extraction when seeding.
This is an alternative and kind of complementary way to #10594. We
probably want to make the low-level bits also a bit more robust but this
high level approach is hopefully simpler.