boot: helper for setting up a try recover system #9921

bboozzoo · 2021-02-10T10:02:07Z

Add a new helper for setting up a new recovery system to be tried. A request to
reboot into given recovery system must be issued separately.

Based on top of #9918, plus some trivial enhancements in seal and bootloader test.

Improve some error messages in the sealing code. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

…BootVars errors This gives slightly more flexibility in mocking different failure modes. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

Add a new helper for setting up a new recovery system to be tried. A request to reboot into given recovery system must be issued separately. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

…overy-mgmt-boot-set-try

pedronis

looks good, some minor comments and a question

boot/systems.go

pedronis · 2021-02-10T17:39:11Z

boot/systems.go

+)
+
+func clearTryRecoverySystem(dev Device, systemLabel string) error {
+	if !dev.HasModeenv() {


do we expect to call this in other contexts more standalone to have to do this check again?

Yes, this eventually becomes ClearTryRecoverySystem and gets called from a devicestate handler.

if indeed this function will be used in more places than just to specifically cleanup in failure modes of SetTryRecoverySystem we need to think carefully about the order of operations here to make sure we don't accidentally brick things with an unexpected reboot in between operations

pedronis · 2021-02-10T17:43:58Z

boot/systems.go

+	}
+	vars := map[string]string{
+		"try_recovery_system":    systemLabel,
+		"recovery_system_status": "try",


these will not appear on the kernel command line, right?

Only bootenv. Well, at least our boot script is not passing that on the command line.

pedronis · 2021-02-10T17:45:30Z

boot/seal.go

@@ -592,6 +592,10 @@ func runModeBootChains(rbl, bl bootloader.Bootloader, model *asserts.Model, mode
 // hashes from modeenv plus it returns separately the last BootFile
 // which is for the kernel.
 func buildBootAssets(bootFiles []bootloader.BootFile, modeenv *Modeenv) (assets []bootAsset, kernel bootloader.BootFile, err error) {
+	if len(bootFiles) == 0 {
+		// useful in testing, when mocking is insufficient
+		return nil, bootloader.BootFile{}, fmt.Errorf("internal error: no boot files")


maybe it doesn't matter because it will show up only in tests, so there's context. I'm just slightly worried that as a message in doesn't make too clear where it is coming from. Though at least atm we don't have any ohter "no boot files" error

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

anonymouse64

looks good, some questions about the ordering of operations though as it pertains to getting rebooted in the middle of things

anonymouse64 · 2021-02-11T22:19:17Z

boot/systems.go

+	if found {
+		// we may be repeating the cleanup, in which case the system may
+		// not be present in modeenv already
+		if err := m.Write(); err != nil {


Suggested change

if found {

// we may be repeating the cleanup, in which case the system may

// not be present in modeenv already

if err := m.Write(); err != nil {

// we may be repeating the cleanup, in which case the system was

// already removed from the modeenv and we don't need to rewrite

// the modeenv

if found {

if err := m.Write(); err != nil {

anonymouse64 · 2021-02-11T22:21:17Z

boot/systems.go

+	resealErr := resealKeyToModeenv(dirs.GlobalRootDir, dev.Model(), m, expectReseal)
+
+	// clear both variables, no matter the values they hold
+	vars := map[string]string{
+		"try_recovery_system":    "",
+		"recovery_system_status": "",
+	}
+	// try to clear regardless of reseal failing
+	blErr := bl.SetBootVars(vars)


is the ordering here safe? if we end up resealing before we manage to write the boot vars and got rebooted in between, would the system try to try booting back into that recovery system which is no longer trusted since we resealed to remove it? I would think that we would want the opposite order, writing bootloader variables first, then resealing

speaking of this kind of ordering, what is your opinion on how hard it would be to make something in resealKeyToModeenv panic() thus simulating a reboot in the middle of the operation to ensure that operations are in a consistent state? too much work / too little reward since it's a fairly simple set of operations right now?

I think Ian is right here about the order unless we are confused

Booting into the system is controlled by snapd_recovery_mode and snapd_recovery_system, which are set separately. This cleanup is invoked if SetTryRecoverySystem fails, which is before we try to reboot to a new system, or after we're back to run mode, in which case the bootloader variables selecting the mode/system would have been modified and would no longer boot into the tried system. Need to think about it a bit, maybe I missed something.

anonymouse64 · 2021-02-11T22:23:37Z

boot/systems.go

+		}
+	}()
+	const expectReseal = true
+	if err := resealKeyToModeenv(dirs.GlobalRootDir, dev.Model(), m, expectReseal); err != nil {


fwiw I think the ordering of operations here is safe, we are resealing first, but only indicate to the bootloader that it is safe to boot into those by setting the vars after resealing is successful

anonymouse64 · 2021-02-12T03:50:18Z

boot/systems_test.go

+	err := boot.SetTryRecoverySystem(s.uc20dev, "1234")
+	c.Assert(err, IsNil)
+
+	c.Check(mtbl.BootVars, DeepEquals, map[string]string{


let's try and use GetBootVars as much as possible so we can stop exporting BootVars at some point and make the mock bootloaders act more like real bootloaders

We may still need to have something line GetAllBootVars() to make sure nothing unexpected was set.

yes agreed on that

anonymouse64 · 2021-02-12T03:51:11Z

boot/systems_test.go

+
+	modeenvRead, err := boot.ReadModeenv("")
+	c.Assert(err, IsNil)
+	c.Check(modeenvRead.CurrentRecoverySystems, DeepEquals, []string{


nitpick, but I also have slight preference for doing a full DeepEquals comparison to ensure that nothing else was modified accidentally on the object

DeepEquals pick up the read flag that gets set when it's read from a file.

ah right, since we are in the boot pkg though you could use modeenvRead.DeepEqual(exp) I think

Thanks for the hint, I've updated the tests.

anonymouse64 · 2021-02-12T03:52:32Z

boot/systems_test.go

+	})
+}
+
+func (s *systemsSuite) TestSetTryRecoverySystemSetBootVarsErr(c *C) {


anonymouse64 · 2021-02-12T03:56:15Z

boot/systems_test.go

+	})
+}
+
+func (s *systemsSuite) TestSetTryRecoverySystemCleanupOnErrorAfterReseal(c *C) {


also a great test, thanks for this

anonymouse64 · 2021-02-12T03:59:00Z

boot/systems.go

+)
+
+func clearTryRecoverySystem(dev Device, systemLabel string) error {
+	if !dev.HasModeenv() {


if indeed this function will be used in more places than just to specifically cleanup in failure modes of SetTryRecoverySystem we need to think carefully about the order of operations here to make sure we don't accidentally brick things with an unexpected reboot in between operations

…overy-mgmt-boot-set-try

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

pedronis · 2021-02-12T13:18:13Z

boot/systems.go

+}
+
+// SetTryRecoverySystem sets up the boot environment for trying out a recovery
+// system with given label. Once done, the caller should request switching to a


"switching to the..." ?

Missed that when I pushed the first few fixed, but added it now.

…t' into bboozzoo/uc20-recovery-mgmt-boot-set-try

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

…overy-mgmt-boot-set-try

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

bboozzoo · 2021-03-02T13:38:12Z

boot/systems.go

+	}
+
+	const expectReseal = true
+	return resealKeyToModeenv(dirs.GlobalRootDir, dev.Model(), m, expectReseal)


I have altered the order of operations here, to match what we have in the document, that is:

set try_recovery_system, recovery_system_status boot variables

reseal

If for any reason we reboot, and unexpectedly boot into the tried recovery system before resealing, with the changes in #9940 accessing data will be impossible and we will reboot back to run mode.

it is fairly strange to do it differently than everything else though

this is fine I think but it merits a comment explaining what you said but also that is up to the caller to also switch the mode touching the env var that actually affect where we boto, I also wonder if it means that we should have a helper actually doing both? but maybe that's clearer when one sees the full code

I agree that these things should be called out in comments, but the ordering and logic here makes sense

added comments

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

pedronis

this is ok but still a bit confusing so if nothing else it needs more comment, I also wonder if we need a helper that actually does more, basically an EnsureNextBootToTryRecoverySystem ? maybe is clear in the full code

pedronis · 2021-03-03T13:21:40Z

boot/systems.go

+
+// SetTryRecoverySystem sets up the boot environment for trying out a recovery
+// system with given label. Once done, the caller should request switching to
+// the given recovery system.


This should describe that this adds to the current_recovery_systems and also reseals to it.

pedronis · 2021-03-03T13:23:00Z

boot/systems.go

+	}
+
+	const expectReseal = true
+	return resealKeyToModeenv(dirs.GlobalRootDir, dev.Model(), m, expectReseal)


this is fine I think but it merits a comment explaining what you said but also that is up to the caller to also switch the mode touching the env var that actually affect where we boto, I also wonder if it means that we should have a helper actually doing both? but maybe that's clearer when one sees the full code

anonymouse64

I agree with @pedronis about adding a comment, but otherwise lgtm, thanks for the work here. One nitpicky thing about tests that you can take in a followup or disregard as you prefer.

anonymouse64 · 2021-03-04T02:21:28Z

boot/systems.go

+	}
+
+	const expectReseal = true
+	return resealKeyToModeenv(dirs.GlobalRootDir, dev.Model(), m, expectReseal)


I agree that these things should be called out in comments, but the ordering and logic here makes sense

anonymouse64 · 2021-03-04T02:26:20Z

boot/systems_test.go

+
+	modeenvRead, err := boot.ReadModeenv("")
+	c.Assert(err, IsNil)
+	c.Check(modeenvRead.CurrentRecoverySystems, DeepEquals, []string{


ah right, since we are in the boot pkg though you could use modeenvRead.DeepEqual(exp) I think

@anonymouse64

Thanks to @anonymouse64 for the suggestion. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

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

bboozzoo requested a review from pedronis February 10, 2021 10:02

bboozzoo force-pushed the bboozzoo/uc20-recovery-mgmt-boot-set-try branch from 1c1d4b0 to 63794c5 Compare February 10, 2021 10:49

anonymouse64 self-requested a review February 10, 2021 14:20

anonymouse64 added the UC20 label Feb 10, 2021

bboozzoo added 3 commits February 10, 2021 15:47

boot: improve errors in sealing code

f0d1b30

Improve some error messages in the sealing code. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

bootloader/bootloadertest: allow setting up a function for mocked Set…

80968be

…BootVars errors This gives slightly more flexibility in mocking different failure modes. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

boot: helper for setting up a try recovery system

879e3c8

Add a new helper for setting up a new recovery system to be tried. A request to reboot into given recovery system must be issued separately. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

bboozzoo force-pushed the bboozzoo/uc20-recovery-mgmt-boot-set-try branch from 63794c5 to 879e3c8 Compare February 10, 2021 14:50

Merge remote-tracking branch 'upstream/master' into bboozzoo/uc20-rec…

312c7d7

…overy-mgmt-boot-set-try

pedronis reviewed Feb 10, 2021

View reviewed changes

bboozzoo added 2 commits February 11, 2021 14:26

boot: add comments

5303c60

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

boot: tweak internal error message in sealing helpers

940b71f

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

bboozzoo mentioned this pull request Feb 11, 2021

boot: cmd/snap-bootstrap: handle a candidate recovery system #9928

Closed

pedronis self-requested a review February 11, 2021 16:15

anonymouse64 reviewed Feb 12, 2021

View reviewed changes

bboozzoo added 2 commits February 12, 2021 14:10

Merge remote-tracking branch 'upstream/master' into bboozzoo/uc20-rec…

e2df577

…overy-mgmt-boot-set-try

boot: tweak comments, access boot variables though GetBootVars even w…

111b295

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

pedronis reviewed Feb 12, 2021

View reviewed changes

bboozzoo added 6 commits March 2, 2021 13:35

Merge branch 'bboozzoo/uc20-recovery-mgmt-sb-try-handling-v2-just-boo…

d447539

…t' into bboozzoo/uc20-recovery-mgmt-boot-set-try

boot: drop unnecessary bits in test set up

5ec5569

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

boot: tweak comments

2a23330

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

boot: only rewrite modeenv if it has been modified

21a140a

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

Merge remote-tracking branch 'upstream/master' into bboozzoo/uc20-rec…

9cd1786

…overy-mgmt-boot-set-try

boot: tweak the order of operations setting up a try recovery system

a1b3dee

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

bboozzoo commented Mar 2, 2021

View reviewed changes

boot: tweak the order of operations during try recovery system cleanup

d12eb62

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

bboozzoo requested review from anonymouse64 and pedronis March 2, 2021 15:06

pedronis approved these changes Mar 3, 2021

View reviewed changes

anonymouse64 approved these changes Mar 4, 2021

View reviewed changes

bboozzoo added 2 commits March 4, 2021 11:12

boot: tweak how modeenv is checked in the unit tests

d344d86

Thanks to @anonymouse64 for the suggestion. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

boot: tweak comments about setting up a tried recovery system

0b8231a

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

bboozzoo merged commit 5721e5e into snapcore:master Mar 8, 2021

boot: helper for setting up a try recover system #9921

boot: helper for setting up a try recover system #9921

Conversation

bboozzoo commented Feb 10, 2021

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anonymouse64 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anonymouse64 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment