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-preseed: support for preseeding of snapd and core18 #8170
snap-preseed: support for preseeding of snapd and core18 #8170
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.
A couple of minor things, but other than those LGTM
ess := seed.EssentialSnaps() | ||
if len(ess) > 0 { | ||
if ess[0].SnapName() == "core" { | ||
coreSnapPath = ess[0].Path | ||
// core / snapd snap is the first essential snap. |
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.
👍
echo "Checking that there were no other 'Done' tasks when preseeding" | ||
[ "$(grep -c ' Done ' tasks.log)" = "18" ] |
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.
👍
…ing in the spread test (thanks Ian).
command works, up to the point where the image is ready to be booted. | ||
The test assumes cloud image with a core and lxd snaps in its seeds/. | ||
|
||
systems: [ubuntu-20.04-*] |
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.
Forgive my ignorance, why is this limited to Ubuntu 20.04?
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.
Because no ubuntu cloud image seeds core18+snapd yet, and 20.04 is the only possible candidate (or maybe this is even too optimistic assumption).
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.
Could you add this as a comment please.
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 I think they might consider that switch for bionic too, because otherwise they will end up with core + core18 on those at some point
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.
Okay, I can enable it for more systems; if they start seeding snapd on 20.04 but not on others I can make setup_preseeding_core18 conditional and modify the image only where it is still needed.
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.
Enabled for 19.10 as well, but not for 18.04 since it doesn't do seeding at all at the moment. I will enable once it actually introduces seeds.
if seed.UsesSnapdSnap() { | ||
return "", fmt.Errorf("preseeding with snapd snap is not supported yet") | ||
required = "snapd" |
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 should probably still fail here if the model is not classic ? that needs its own tests and need to deal with wrappers/core18.go, no ?
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.
Hmm you're right, and I missed wrappers for core18, definately needs a close look, probably needs extra checks in the spread test too. Thanks
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.
to be clear the code in wrappers/core18.go is a nop for classic afair
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.
Indeed, it's a noop.
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 should probably still fail here if the model is not classic ? that needs its own tests and need to deal with wrappers/core18.go, no ?
Added error on non-classic + test.
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.
LGTM but please check my comment on the spread test.
snap debug state "$IMAGE_MOUNTPOINT"/var/lib/snapd/state.json --change=1 > tasks.log | ||
|
||
echo "Check that the tasks of preseeded snapd have expected statuses" | ||
# Note, these checks match statuses, but not the order |
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.
That's an interesting way to handle a blob of text that has variable elements.
MATCH "Done .+ setup-aliases +Setup snap \"core18\" aliases" < tasks.log | ||
|
||
echo "Checking that there were no other 'Done' tasks when preseeding" | ||
[ "$(grep -c ' Done ' tasks.log)" = "18" ] |
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.
That's really cool, good thinking.
if err != nil { | ||
return "", err | ||
} | ||
if !model.Classic() { |
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 should probably leave a TODO note to enable this on core as well.
Co-Authored-By: Zygmunt Krynicki <me@zygoon.pl>
Support for preseeding of snapd+core18 snaps on classic.
Also added a couple more checks to the TestPreseedOnClassicHappy test.