overlord/devicestate, overlord/snapstate: add task for updating kernel command lines from gadget #10148

bboozzoo · 2021-04-14T12:05:41Z

~~Stacked on top of #10143.~~ The relevant commits are in the overlord package and subpackages, start with 1351e70 and then subsequent ones.

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

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

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

…le secure boot. fix races, enable debugs Also verify that boot chains exist Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

…ate-gadget-cmdline-tasks

anonymouse64

a couple nitpicks about the logging, but otherwise the main code and the spread test looks good to me. I didn't finish reviewing the unit tests yet however

anonymouse64 · 2021-04-15T01:25:45Z

overlord/devicestate/handlers_gadget.go

+	var seeded bool
+	err := st.Get("seeded", &seeded)
+	if err != nil && err != state.ErrNoState {
+		fmt.Printf("not needed\n")


Suggested change

fmt.Printf("not needed\n")

Hah, thanks for catching that.

anonymouse64 · 2021-04-15T01:26:40Z

overlord/devicestate/handlers_gadget.go

+		return err
+	}
+	if !updated {
+		t.Logf("no kernel command line update from gadget")


Suggested change

t.Logf("no kernel command line update from gadget")

t.Debugf("no kernel command line update from gadget")

I've updated the code here and at the place of the other comment to use logger.Debugf()

anonymouse64 · 2021-04-15T01:27:31Z

overlord/devicestate/handlers_gadget.go

+		return err
+	}
+	if !updated {
+		t.Logf("no kernel command line update to undo")


Suggested change

t.Logf("no kernel command line update to undo")

t.Debugf("no kernel command line update to undo")

And there should be a return nil here right after the log too.

pedronis

thanks for this, I did a first pass, mostly minor comments but a question about reboots

pedronis · 2021-04-15T11:59:50Z

boot/bootstate20.go

-		// not using trusted boot assets, bootloader config is not
-		// managed and command line can be manipulated externally
-		return update, nil
-	}


so we understand, what's the situation here, grub is used but FDE is not enabled?

Yes, the scenario is no FDE, in which case we do not track the trusted boot assets, but that unnecessarily gated the command line handling in bootstate.

Added a test case for this.

pedronis · 2021-04-15T12:01:26Z

boot/cmdline.go

+		// and system bootloader, indicating that the command line is
+		// not being tracked
+		return m, nil
+	}


I think these changes in boot needs new unit tests for the new scenarios, no?

Added a test for this too. The scenario is basically a uc20 device which does not have the trusted assets bootloader, so kernel command lines will not be populated, and we do not expect a command line for it anyway.

pedronis · 2021-04-15T12:11:46Z

overlord/devicestate/handlers_gadget.go

+
+func (m *DeviceManager) doUpdateGadgetCommandLine(t *state.Task, _ *tomb.Tomb) error {
+	if release.OnClassic {
+		return fmt.Errorf("cannot run update gadget kernel command line task on a classic system")


this is an internal error

Updated this and the other place

pedronis · 2021-04-15T12:11:54Z

overlord/devicestate/handlers_gadget.go

+
+func (m *DeviceManager) undoUpdateGadgetCommandLine(t *state.Task, _ *tomb.Tomb) error {
+	if release.OnClassic {
+		return fmt.Errorf("cannot run update gadget kernel command line task on a classic system")


pedronis · 2021-04-15T12:12:13Z

overlord/devicestate/handlers_gadget.go

+		return err
+	}
+	if !updated {
+		t.Logf("no kernel command line update from gadget")


pedronis · 2021-04-15T12:15:26Z

overlord/managers_test.go

+        size: 500M
+`
+
+func (s *mgrsSuite) testGadgetKernelCommandLine(c *C, snapPath string, si *snap.SideInfo, bl bootloader.Bootloader, currentFiles [][]string, currentModeenvCmdline string, commandLineAfterReboot string, update bool) {


s/snapPath/gadgetPath/ and s/si/gadgetSideInfo/ for a bit more clarity?

pedronis · 2021-04-15T12:15:45Z

overlord/snapstate/snapstate.go

@@ -263,6 +263,12 @@ func doInstall(st *state.State, snapst *SnapState, snapsup *SnapSetup, flags int
 		addTask(gadgetUpdate)
 		prev = gadgetUpdate
 	}
+	if !release.OnClassic && snapsup.Type == snap.TypeGadget {
+		// XXX: kernel command line from gadget is for core systems only


why the XXX?

pedronis · 2021-04-15T12:20:47Z

overlord/devicestate/devicemgr.go

@@ -160,6 +160,8 @@ func Manager(s *state.State, hookManager *hookstate.HookManager, runner *state.T
 	// config assets are assumed to be always backwards compatible.
 	runner.AddHandler("update-managed-boot-config", m.doUpdateManagedBootConfig, nil)



I don't think we want this blank line, maybe just have a trivial comment if the issue with having nothing is the confusion with the previous comment

pedronis · 2021-04-15T12:22:20Z

overlord/devicestate/handlers_gadget.go

+	if !updated {
+		t.Logf("no kernel command line update to undo")
+	}
+	t.Logf("kernel command line update undone")


"reverted kernel command line", to be more similar to the Do case?

pedronis · 2021-04-15T12:24:16Z

overlord/devicestate/handlers_gadget.go

+	t.SetStatus(state.DoneStatus)
+
+	// kernel command line was updated, request a reboot to make it effective
+	st.RequestRestart(state.RestartSystem)


do we risk to reboot twice if there was an assets update from update-managed-boot-config as well? can we do something if that's the case?

Yes, there is a change to require 2 reboots. One the one hand it is an inconvenience, on the other happens only when both the assets and the command line is changed in the new revision.

can you leave a TODO about thinking further about this? I have some ideas but probably not worth pursuing immediately

Sure, I'll push it after the current spread run finishes

@pedronis

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

…ne was not changed Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

…ate-gadget-cmdline-tasks

Add test cases for compatibility scenarios: - a UC20 device with a bootloader that isn't the trusted assets one - a UC20 device with trusted assets bootloader, but no assets are tracked (as happens when FDE isn't used) Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

pedronis

thanks, small question/nitpick

pedronis · 2021-04-15T15:21:13Z

overlord/devicestate/handlers_gadget.go

 	}
-	t.Logf("kernel command line update undone")
+	t.Logf("Reverted kernel command line change")


isn't this checked in tests like the do case?

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

anonymouse64

lgtm, some super nitpicky comment suggestions

anonymouse64 · 2021-04-16T03:30:54Z

boot/boot_test.go

+		"snapd_recovery_mode=run panic=-1",
+		"snapd_recovery_mode=run candidate panic=-1",
+	})
+	// without encryption, the trusted assets are no tracked in the modeenv,


Suggested change

// without encryption, the trusted assets are no tracked in the modeenv,

// without encryption, the trusted assets are not tracked in the modeenv,

anonymouse64 · 2021-04-16T03:37:57Z

overlord/devicestate/devicestate_gadget_test.go

+}
+
+func (s *deviceMgrGadgetSuite) TestUpdateGadgetCommandlineWithNewArgs(c *C) {
+	// no command line arguments prior to the gadget up


Suggested change

// no command line arguments prior to the gadget up

// no command line arguments prior to the gadget update

anonymouse64 · 2021-04-16T03:39:29Z

overlord/devicestate/devicestate_gadget_test.go

+}
+
+func (s *deviceMgrGadgetSuite) TestUpdateGadgetCommandlineUnchanged(c *C) {
+	// no command line arguments prior to the gadget up


Suggested change

// no command line arguments prior to the gadget up

// no command line arguments prior to the gadget update

anonymouse64 · 2021-04-16T03:44:20Z

overlord/devicestate/handlers_gadget.go

+
+	t.SetStatus(state.DoneStatus)
+
+	// TODO: consider optimization to avoid double reboot when the gadget


thanks for this comment

anonymouse64 · 2021-04-16T03:47:32Z

overlord/managers_test.go

+func (s *mgrsSuite) testGadgetKernelCommandLine(c *C, gadgetPath string, gadgetSideInfo *snap.SideInfo,
+	bl bootloader.Bootloader, currentFiles [][]string, currentModeenvCmdline string,
+	commandLineAfterReboot string, update bool) {


oh boy that's quite the signature :-)

anonymouse64 · 2021-04-16T03:51:48Z

overlord/managers_test.go

+		{"meta/gadget.yaml", pcGadgetYaml},
+		{"cmdline.extra", "args from old gadget"},
+	}
+	// add new gadget snap kernel command line drop-in file


Suggested change

// add new gadget snap kernel command line drop-in file

// add new gadget snap kernel command line without the file

@anonymouse64

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

mvo5

Thanks, some nitpicks inside but looks good.

mvo5 · 2021-04-16T11:44:05Z

overlord/devicestate/handlers_gadget.go

+		// pre UC20 system, do nothing
+		return false, nil
+	}
+	// but when undoing, we use the current gadget which should have


Hm, this comment starts with // but ... - what does the "but" refers to? Should this maybe read just "when undoing, we use the current gadget which should have been restored"

Could this move into the "else" below? It seems we only need currentGadgetData inside this else condition?

mvo5 · 2021-04-16T11:46:24Z

overlord/devicestate/handlers_gadget.go

+		logger.Debugf("no kernel command line update from gadget")
+		return nil
+	}
+	t.Logf("Updated kernel command line")


Thanks for adding this

mvo5 · 2021-04-16T11:49:04Z

overlord/managers_test.go

+}
+
+func (s *mgrsSuite) TestGadgetKernelCommandLineAddCmdline(c *C) {
+	mabloader := bootloadertest.Mock("mock", c.MkDir()).WithTrustedAssets()


Would it make sense to have a "gadgetMgrsSuite" or something that does this bootloader setup in a single place? it seems like it's the same across the tests(?)

I can try to do that in a followup

mvo5 · 2021-04-16T11:50:22Z

overlord/snapstate/snapstate_update_test.go

@@ -5339,6 +5341,7 @@ func (s *snapmgrTestSuite) TestStopSnapServicesFirstSavesSnapSetupLastActiveDisa
 		},
 		Current: snap.R(11),
 		Active:  true,
+		// gofmt


Maybe expand this a little bit? I assuem you add it to avoid indent?

mvo5 · 2021-04-16T11:51:07Z

tests/nested/manual/core20-custom-kernel-commandline/task.yaml

+    # wait for previous change to finish before proceeding
+    nested_exec sudo snap watch "$REMOTE_CHG_ID"
+
+    # TODO: enable once full command line override is supported


This will be enabled once #10160 landed, yes?

@mvo5

Thanks to @mvo5 for suggestions Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

bboozzoo · 2021-04-16T14:47:46Z

@mvo5 can you land this PR? the failure in google:ubuntu-18.04-64:tests/main/interfaces-content-default-provider is unrelated.

bboozzoo added 4 commits April 14, 2021 13:56

overlord/devicestate: stubs for a task updating kernel command lines …

1351e70

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

overlord/devicestate: handle update of components that contribute to …

7c31a80

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

overlord/snapstate: add kernel command line update task for gadget snapd

2ae10f0

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

tests/nested/manual/core20-custom-kernel-commandline: extend the test

0546f0c

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

bboozzoo added the Run nested The PR also runs tests inluded in nested suite label Apr 14, 2021

bboozzoo requested review from mvo5, pedronis and anonymouse64 April 14, 2021 12:05

bboozzoo added 4 commits April 14, 2021 14:18

boot: enable boostate handling of kernel command lines always

687c0d3

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

overlord: make old gofmt happy again

9ac0e6a

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

tests/nested/manual/core20-custom-kernel-commandline: explicitly enab…

a1e5b28

…le secure boot. fix races, enable debugs Also verify that boot chains exist Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

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

033919a

…ate-gadget-cmdline-tasks

anonymouse64 reviewed Apr 15, 2021

View reviewed changes

pedronis reviewed Apr 15, 2021

View reviewed changes

bboozzoo added 4 commits April 15, 2021 15:56

overlord, overlord/devicestate: error tweaks, cleanups, message tweaks

fe0bac8

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

overlord/devicestate: do not trigger a reboot in undo when command li…

e9e8245

…ne was not changed Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

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

b41363f

…ate-gadget-cmdline-tasks

pedronis approved these changes Apr 15, 2021

View reviewed changes

overlord/devicestate: tweak unit tests, leave TODO about double reboot

73c5c9c

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

anonymouse64 approved these changes Apr 16, 2021

View reviewed changes

boot, overlord: comment tweaks

b58f58b

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

bboozzoo mentioned this pull request Apr 16, 2021

tests, overlord: extend unit tests, extend spread tests to cover full command line support #10160

Merged

mvo5 approved these changes Apr 16, 2021

View reviewed changes

overlord/devicestate: tweaks

bc11c2a

Thanks to @mvo5 for suggestions Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

mvo5 merged commit 148e4d1 into snapcore:master Apr 16, 2021

	t.Logf("no kernel command line update from gadget")
	t.Debugf("no kernel command line update from gadget")

	t.Logf("no kernel command line update to undo")
	t.Debugf("no kernel command line update to undo")

		@@ -160,6 +160,8 @@ func Manager(s state.State, hookManager hookstate.HookManager, runner *state.T
		// config assets are assumed to be always backwards compatible.
		runner.AddHandler("update-managed-boot-config", m.doUpdateManagedBootConfig, nil)

	// without encryption, the trusted assets are no tracked in the modeenv,
	// without encryption, the trusted assets are not tracked in the modeenv,

	// no command line arguments prior to the gadget up
	// no command line arguments prior to the gadget update


		t.SetStatus(state.DoneStatus)

		// TODO: consider optimization to avoid double reboot when the gadget

	// add new gadget snap kernel command line drop-in file
	// add new gadget snap kernel command line without the file

overlord/devicestate, overlord/snapstate: add task for updating kernel command lines from gadget #10148

overlord/devicestate, overlord/snapstate: add task for updating kernel command lines from gadget #10148

Conversation

bboozzoo commented Apr 14, 2021 • edited

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

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

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 Apr 15, 2021 • edited

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

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

mvo5 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

bboozzoo commented Apr 16, 2021

bboozzoo commented Apr 14, 2021 •

edited

pedronis Apr 15, 2021 •

edited