tests: add uc20 kernel snap upgrade managers test, fix bootloadertest bugs #8185

anonymouse64 · 2020-02-24T19:32:48Z

This adds some high level managers test which ensure that everything working together is successful with a kernel snap update and a kernel snap update undo.

Also made a new helper for these tests to create a fake test snap with files and return the snap.Info for it to compare with other things.

I also fixed a bug with the bootloadertest that prevents these tests from fulling working end-to-end.

anonymouse64 · 2020-02-26T01:48:06Z

There does seem to be a real failure here in that we don't remove extracted kernel assets on the old kernel snap when we install a new one, unclear if this is desirable or not? We do move the symlink, but for example we have now this situation on ubuntu-boot after finishing an upgrade to pc-kernel revision x2:

* grub.cfg
* grubenv
* kernel.efi -> pc-kernel_x2.snap/kernel.efi
* pc-kernel_x1.snap/
    kernel.efi
* pc-kernel_x2.snap/
    kernel.efi

@pedronis WDYT? Should we be deleting old kernel.efi's on snap refresh? Or perhaps there is an issue with my test setup here that prevents snapd from properly calling removeInactiveRevision and thus RemoveSnapFiles and thus RemoveKernelAssets ?

Not doing this breaks managers tests because tests expect these methods to actually work. Signed-off-by: Ian Johnson <person.uwsome@gmail.com>

Signed-off-by: Ian Johnson <person.uwsome@gmail.com>

This is useful in tests where you want to create a snap file, as well as get the snap.Info for example in manager tests where you want to compare snap.PlaceInfo's between what was provided to the bootloader when installing a new kernel snap and what you built to manually install the snap. Signed-off-by: Ian Johnson <person.uwsome@gmail.com>

For UC20 kernel snaps, we only care about kernel_status, but UC18 and UC16, we need snap_mode. Signed-off-by: Ian Johnson <person.uwsome@gmail.com>

This test is the first of many to test kernel snap upgrades on UC20. Much of this logic should probably be put in some helper. Signed-off-by: Ian Johnson <person.uwsome@gmail.com>

This is the UC20 equivalent of the kernel snap upgrade and undo change, where we finish installing a new kernel snap, and immediately have to undo that. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

anonymouse64 · 2020-02-27T17:40:58Z

As per IRC conversation, I have removed the check about removing the old kernel assets, this only happens when the kernel snap is garbage collected, and that doesn't happen until we have 3+ snaps by default, so that cleanup won't be triggered under this test.

This should be ready to review now

pedronis

lgtm with some comments (and some whitespace nitpicks)

bootloader/bootloadertest/bootloadertest.go

pedronis · 2020-02-27T19:07:46Z

overlord/managers_test.go

+	defer bootloader.Force(nil)
+
+	// the reboot var we care about for UC20 is kernel_status
+	bloader.RebootStatusVar = "kernel_status"


this feels like the wrong level of detail to control here like this. I suggest either:

pass another param to Mock but that might be too annoying, too much churn

a new bootloader.MockUC20 (but this might give the impression that is a completely different thing ?)

a pattern that we don't use often but could work here, do

bloader := bootloader.Mock("mock", c.MkDir).UC20RunMode()

where UC20RunMode returns its target and tweaks all the bits to make it behave in the right way for a UC20 run mode bootloader?

Well we do already have boottest.MockDevice and boottest.MockUC20Device which do very similar things to what we do here, just return the same thing with a little internal tweak on it, so I would say let's go with that, but since the only real difference here is in the bootenv var used when emulating a reboot, I'd be afraid of folks thinking that there's more that needs to change, when really the only place you would need to use bootloader.MockUC20 instead of bootloader.Mock would be in these tests specifically.

For these reasons, I think I will go with the 3rd alternative, but rename the method to be bloader := bootloader.Mock("mock",c.Mkdir).UC20RebootMode() because that's what is really being changed here.

overlord/managers_test.go

snap/snaptest/snaptest.go

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

…otStatusVar Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

…ossReboot We will need to do this soon, but lest we forget add a comment about it now. Also delete unnecessary whitespace Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

…ap tests Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

pedronis · 2020-02-28T09:07:03Z

bootloader/bootloadertest/bootloadertest.go

 	}
 }

+func (b *MockBootloader) UC20RebootMode() *MockBootloader {


I'm still not satisfied with this, not the name, nor the mechanics. Not your fault, I think the deeper issue is that we have hard-coded and given responsibilities to the MockBootloader, that actually pertain to boottest. As long as we had only one set of boot vars it was ok but now it's becoming confusing/not very clean.

I have some ideas how to fix it, which I think will make things slightly more verbose but also cleaner hopefully, I don't think we should block this PR on this though.

Please let's rename this to UC20RunModeRebootReady for now, and put an // FIXME: this clearly shows mixing of responsibilities here with boottest.

I'll try to work on cleaning this up as soon as possible.

To be clear this is not just a cleanliness concern, as we grow more of this kind of tests, there is a risk of testing the wrong thing, tests passing for the wrong reasons, or testing not realistic states.

…managers-reboot

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

bboozzoo

LGTM

bboozzoo · 2020-02-28T13:18:01Z

overlord/managers_test.go

+}
+
+func (s *mgrsSuite) TestInstallKernelSnap20UndoUpdatesBootloaderEnv(c *C) {
+	bloader := bootloadertest.Mock("mock", c.MkDir()).UC20RunModeRebootReady()


Bit annoying that there's only a handfull of lines changes in this test compared to the successful path one. However, this is a quite common in our codebase. Wonder if there's some sort of optimization, other than splitting this into a helper function, we could use here that would allow sharing the setup and provide enough flexibility to execute corresponding verification code that won't otherwise feel awkward to use.

I go back and forth on this a lot. OTOH, yes less duplicated code is good because it eliminates issues like copy-pasting unnecessary or even wrong bits for tests, but OTOOH, being less clever in tests and more explicit about what we're setting up I think makes the tests more readable and easier to understand - there's less magic to them which sometimes is what we want.

…managers-reboot

anonymouse64 added Test Robustness UC20 labels Feb 24, 2020

anonymouse64 requested a review from pedronis February 24, 2020 19:33

anonymouse64 force-pushed the feature/uc20-all-managers-reboot branch from c197ff3 to 400129c Compare February 24, 2020 23:00

anonymouse64 and others added 6 commits February 26, 2020 15:06

bootloadertest: actually save the EnableKernel and EnableTryKernel

5f128b7

Not doing this breaks managers tests because tests expect these methods to actually work. Signed-off-by: Ian Johnson <person.uwsome@gmail.com>

firstboot_test: wrap comment at 80 chars

9cc8f49

Signed-off-by: Ian Johnson <person.uwsome@gmail.com>

bootloadertest: allow configuring what boot var to use in mocking reboot

2679a46

For UC20 kernel snaps, we only care about kernel_status, but UC18 and UC16, we need snap_mode. Signed-off-by: Ian Johnson <person.uwsome@gmail.com>

managers_test: add UC20 kernel snap upgrade run through

d523579

This test is the first of many to test kernel snap upgrades on UC20. Much of this logic should probably be put in some helper. Signed-off-by: Ian Johnson <person.uwsome@gmail.com>

managers_test: add UC20 kernel snap upgrade + undo run through

6cf9a35

This is the UC20 equivalent of the kernel snap upgrade and undo change, where we finish installing a new kernel snap, and immediately have to undo that. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

anonymouse64 force-pushed the feature/uc20-all-managers-reboot branch from 400129c to 6cf9a35 Compare February 26, 2020 21:06

anonymouse64 mentioned this pull request Feb 27, 2020

boot_test: add many boot robustness tests for UC20 kernel MarkBootSuccessul and SetNextBoot #8208

Merged

pedronis reviewed Feb 27, 2020

View reviewed changes

anonymouse64 added 4 commits February 27, 2020 18:25

snap/snaptest: delete unnecessary whitespace

8eade3f

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

bootloadertest: expose UC20RebootMode instead of manual write to Rebo…

ae8c1d1

…otStatusVar Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

bootloadertest: add comment about implementing UC20 on SetRollbackAcr…

3bf3e03

…ossReboot We will need to do this soon, but lest we forget add a comment about it now. Also delete unnecessary whitespace Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

o/managers_test.go: check that snapst matches expected uc20 kernel sn…

79d32d2

…ap tests Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

pedronis reviewed Feb 28, 2020

View reviewed changes

bboozzoo added 2 commits February 28, 2020 13:53

Merge remote-tracking branch 'upstream/master' into feature/uc20-all-…

654e8e1

…managers-reboot

bootloader/bootloadertest: tweak name, leave FIXME

8a216cc

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

bboozzoo approved these changes Feb 28, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into feature/uc20-all-…

50d7c80

…managers-reboot

pedronis self-requested a review March 10, 2020 10:18

Merge remote-tracking branch 'upstream/master' into feature/uc20-all-…

b6c10de

…managers-reboot

pedronis merged commit 4c3526c into snapcore:master Mar 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests: add uc20 kernel snap upgrade managers test, fix bootloadertest bugs #8185

tests: add uc20 kernel snap upgrade managers test, fix bootloadertest bugs #8185

anonymouse64 commented Feb 24, 2020

anonymouse64 commented Feb 26, 2020

anonymouse64 commented Feb 27, 2020

pedronis left a comment

pedronis Feb 27, 2020

anonymouse64 Feb 28, 2020 •

edited

pedronis Feb 28, 2020

pedronis Feb 28, 2020

bboozzoo Feb 28, 2020

bboozzoo left a comment

bboozzoo Feb 28, 2020

anonymouse64 Feb 28, 2020

tests: add uc20 kernel snap upgrade managers test, fix bootloadertest bugs #8185

tests: add uc20 kernel snap upgrade managers test, fix bootloadertest bugs #8185

Conversation

anonymouse64 commented Feb 24, 2020

anonymouse64 commented Feb 26, 2020

anonymouse64 commented Feb 27, 2020

pedronis left a comment

Choose a reason for hiding this comment

pedronis Feb 27, 2020

Choose a reason for hiding this comment

anonymouse64 Feb 28, 2020 • edited

Choose a reason for hiding this comment

pedronis Feb 28, 2020

Choose a reason for hiding this comment

pedronis Feb 28, 2020

Choose a reason for hiding this comment

bboozzoo Feb 28, 2020

Choose a reason for hiding this comment

bboozzoo left a comment

Choose a reason for hiding this comment

bboozzoo Feb 28, 2020

Choose a reason for hiding this comment

anonymouse64 Feb 28, 2020

Choose a reason for hiding this comment

anonymouse64 Feb 28, 2020 •

edited