daemon, snapstate: move ensureCore from daemon/api.go into snapstate.go #3727

mvo5 · 2017-08-14T14:32:15Z

This is prereq work for auto-installing missing base snapd and
missing default-provider for content snaps.

It moves the ensureCore() from daemon/api.go to overlord/snapstate/snapstate.go as part of doInstall().

There is a bit of TOCTOU in the check (below in line 437) if we need to download the core, if something else creates a change in parallel that also ensureCore() and both are not yet added to a change then the check if we have something else that already installs core will not see this (I think

We probably also want to pass the userID to "snapstate.InstallPath()" and "snapstate.Try()" as an additional parameter so that the download code can set this correctly. This is not done yet but I plan to do it here (with some more tests). Then this can be expanded to cover bases and content snaps.

This is prereq work for auto-installing missing base snapd and missing default-provider for content snaps.

pedronis · 2017-08-14T15:31:10Z

overlord/managers_test.go

+		"timestamp":    time.Now().Format(time.RFC3339),
+	}
+	headers["snap-id"] = fakeSnapID(headers["snap-name"].(string))
+	a, err := ms.storeSigning.RootSigning.Sign(asserts.SnapDeclarationType, headers, nil, "")


ms.storeSigning is enough . RootSigning should be rarely needed (mostly to sign main keys)

Maybe I'm missing something, but when I just use the "storeSigning" I get an error like this:

FAIL: managers_test.go:105: mgrsSuite.SetUpTest managers_test.go:174: c.Assert(err, IsNil) ... value *errors.errorString = &errors.errorString{s:"no matching public key \"XcSmSI47H8EdldbP1BMKBUXs_5Jr0Ck2wSecjcExc5KMEAzsfZocbpaoSvQAW7_9\" for signature by \"can0nical\""} ("no matching public key \"XcSmSI47H8EdldbP1BMKBUXs_5Jr0Ck2wSecjcExc5KMEAzsfZocbpaoSvQAW7_9\" for signature by \"can0nical\"")

usually that means the test is missing to add the store key in the local database, I would need to look more carefully. But SnapDeclarations are not signed by the root key so it's better to keep things realistic and use the store key.

something like this:

err := assertstate.Add(st, ms.storeSigning.StoreAccountKey("")) c.Assert(err, IsNil)

pedronis · 2017-08-14T15:37:27Z

overlord/snapstate/snapstate.go

+					return nil, err
+				}
+				// some other change aleady installs core
+				if snapsup.Name() == defaultCoreSnapName {


this is problematic because we could end up with this snap but without core if that other core install failed, or try to run things too early in this install that might need core to be there. I fear we need to give up instead for now or construct a cross change dependency which we never tried

pedronis · 2017-08-14T15:37:57Z

overlord/snapstate/snapstate.go

+	}
+
+	// check that core is not installed already
+	_, err := CoreInfo(st)


this could be done before the check over all Changes

pedronis · 2017-08-14T15:43:13Z

overlord/snapstate/snapstate.go

+	ts := state.NewTaskSet()
+	if flags&doNotAutoInstallMissingBases == 0 {
+		var err error
+		ts, err = ensureCore(st, snapsup.Name(), snapsup.UserID)


I think this is too simple, because during an InstallMany or UpdateMany we will get here many times but the change doesn't exist yet, so will create many tasks to install core unless I'm confused

mvo5 · 2017-08-15T07:38:32Z

Closing for now as this needs some re-thinking for {Install,Update}Many.

…apstate

This will automatically create Install tasks to fetch prerequisites.

mvo5 · 2017-08-18T10:11:25Z

Reopen for more early feedback from Samuele (and everyone else who is interessted)

pedronis

some initial comments

pedronis · 2017-08-18T12:03:58Z

overlord/snapstate/handlers.go

+	// the state lock is not enough to ensure this is not run in
+	// parallel as it is dropped in snapstate.Install() below
+	m.prerequisitesLock.Lock()
+	defer m.prerequisitesLock.Unlock()


we tend to use SetBlocked callbacks to serialize tasks, not locks, so they don't even start

pedronis · 2017-08-18T12:05:28Z

overlord/snapstate/handlers.go

+				if snapsup.Name() == defaultCoreSnapName {
+					// if something else installs core
+					// we need to wait for it
+					waitNextHelper(t.Change(), state.NewTaskSet(chg.Tasks()...))


unless I'm confused this will wait for the full change there, even the actual installed non-core snaps, shouldn't we just wait for things in the core lane/core itself ?

also I thought the original plan was not to cross wait, but return with Retry here and wait that way

Yeah, you are right, this is one step too far. I will do the Retry as originally planed (I was mostly curious about exploring the cross-change waits but this can wait :)

Updated this now.

pedronis · 2017-08-18T12:14:20Z

overlord/snapstate/handlers.go

+	}
+
+	// not installed, nor queued for install -> install it
+	ts, err := Install(st, defaultCoreSnapName, defaultBaseSnapsChannel, snap.R(0), snapsup.UserID, Flags{})


we need consider that people can still do "snap install core" and here we release the state lock, so this can return with a conflict I think and then we need to setup waiting instead

I added code for this now, I need to think a bit about a test.

pedronis · 2017-08-18T12:15:19Z

overlord/snapstate/handlers.go

+func (m *SnapManager) undoPrerequisites(t *state.Task, _ *tomb.Tomb) error {
+	// FIXME: undo any bases/content-providers once we start installing
+	//        those
+	return nil


we might use a gc mechanism instead, where once in a while we just remove unreferenced stuff?

Good point, I removed it for now.

pedronis · 2017-08-18T12:16:00Z

overlord/snapstate/handlers.go

+
+	waitNextHelper := func(chg *state.Change, ts *state.TaskSet) {
+		for _, t := range chg.Tasks() {
+			t.Kind()


was this a print at some point?

…waits

codecov-io · 2017-08-21T06:26:28Z

Codecov Report

Merging #3727 into master will increase coverage by 0.04%.
The diff coverage is 84.53%.

@@            Coverage Diff             @@
##           master    #3727      +/-   ##
==========================================
+ Coverage   75.78%   75.83%   +0.04%     
==========================================
  Files         414      414              
  Lines       35646    35687      +41     
==========================================
+ Hits        27014    27062      +48     
+ Misses       6729     6724       -5     
+ Partials     1903     1901       -2

Impacted Files	Coverage Δ
overlord/snapstate/snapstate.go	`80.86% <100%> (+0.45%)`	⬆️
overlord/snapstate/snapmgr.go	`80.57% <100%> (+0.91%)`	⬆️
daemon/api.go	`72.5% <100%> (+0.05%)`	⬆️
overlord/snapstate/handlers.go	`65.6% <75%> (+0.49%)`	⬆️
interfaces/sorting.go	`98.71% <0%> (-1.29%)`	⬇️
overlord/devicestate/devicemgr.go	`75.43% <0%> (+0.69%)`	⬆️
cmd/snap/cmd_aliases.go	`95% <0%> (+1.66%)`	⬆️
wrappers/binaries.go	`79.54% <0%> (+6.81%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c9e7ddf...9966425. Read the comment docs.

Add scenario so that some-snap gets installed and during the install (when unlocking for the store info call for core) an explicit "snap install core" happens. In this case the snapstate will return a change conflict. We handle this via a retry, ensure this is actually what happens.

pedronis · 2017-08-24T09:12:22Z

overlord/devicestate/firstboot_test.go

+	for !chg.IsReady() {
+		// nothing
+	}
+


this is strange, why would the changes affect first boot stuff?

I have not yet figured out why this affects the test, I mean, I understand why settle() never finishes, but I have not tracked down why this was working before because we call ensureSeedYaml a lot.

ok, I'll try to play with this locally myself tomorrow, it might be that the test was problematic already and some change here triggered something.

I played a bit, the "issue" seems to be that prerequisites not having an undo handler make it so that the change is ready earlier and so an ensure cycle triggers agains seed. I don't care too much either way, but I'm not a fan of the Overlord.Loop() in this approach. I think firstboot_test.go are the only tests that use a full overlord in *state/*_test.go. I have a plan since a while to have a overlord.Mock() that would give us a bit more control when we need to setup more than one manager, maybe it's time for me to look into that. It hopefully would help kill all the various settle helpers we have around as well but need to try.

pedronis · 2017-08-24T11:42:35Z

overlord/snapstate/handlers.go

+const defaultBaseSnapsChannel = "stable"
+
+// timeout for tasks to check if the prerequisites are ready
+var prerequisitesRetryTimeout = 1 * time.Minute


I think this could be shorter, like 30s.

pedronis · 2017-08-24T12:01:20Z

overlord/snapstate/handlers.go

+	for _, t := range chg.Tasks() {
+		t.WaitAll(ts)
+	}
+	chg.AddAll(ts)


we need to set the task state to done directly like we do for link-snap, otherwise there's non-zero chance that we crash in the middle of exiting the task and we will have the new tasks in the change but will rerun prerequisites again and it will conflict with itself, or we need to deal with that situation otherwise

…apstate

pedronis

I'm not sure why we check for only one kind of conflict error/case

pedronis · 2017-08-28T14:20:48Z

overlord/snapstate/handlers.go

+		t.WaitAll(ts)
+	}
+	chg.AddAll(ts)
+	// ensure this is not rerun again


maybe something more explicit // make sure that the new change is committed to the state together with marking this task done

pedronis · 2017-08-28T14:22:58Z

overlord/snapstate/handlers.go

+	ts, err := Install(st, defaultCoreSnapName, defaultBaseSnapsChannel, snap.R(0), snapsup.UserID, Flags{})
+	// something might have triggered an explicit install of core while
+	// the state was unlocked -> deal with that here
+	if _, ok := err.(changeDuringInstallError); ok {


shouldn't this check for changeConflictError too?

Thanks, I think it is sensible to test for both, I have not yet managed to write a test that triggered the changeConflictError condition, I will think about this tomorrow some more.

pedronis · 2017-08-28T14:24:35Z

overlord/snapstate/snapstate_test.go

+
+	// Two changes are created, the first will fails, the second will
+	// be fine. The order of what change runs first is random, the
+	// first change will also insall core in its own lane. This test


Nice catch, thank you!

…apstate

pedronis

thank you

…apstate

Move ensureCore from daemon/api.go into snapstate.go

446eb64

This is prereq work for auto-installing missing base snapd and missing default-provider for content snaps.

mvo5 requested a review from pedronis August 14, 2017 14:32

pedronis reviewed Aug 14, 2017

View reviewed changes

mvo5 closed this Aug 15, 2017

mvo5 added 6 commits August 17, 2017 10:12

Merge remote-tracking branch 'upstream/master' into ensure-core-in-sn…

ca4ede8

…apstate

Merge remote-tracking branch 'upstream/master' into ensure-core-in-sn…

c15c8d6

…apstate

Add new "prerequisites" task in downloads

462fa19

This will automatically create Install tasks to fetch prerequisites.

update tests

4c5991e

address review comments by Samuele (thanks!)

80245a9

Ensure parallel runs, add more unit tests

c933660

mvo5 reopened this Aug 18, 2017

pedronis reviewed Aug 18, 2017

View reviewed changes

mvo5 added 5 commits August 18, 2017 16:35

use blockedTask() to serialize the "prerequisites" tasks

ef4ce73

use state.Retry{} in doPrerequisites() instead of doing cross-change …

c550df1

…waits

Handle if "snap install core" happens during doPrerequisites()

017fbab

remove undoPrerequisites() in favour of a future GC

7e47181

fix TestPopulateFromSeedMissingBootloader hanging

e43b1f7

pedronis reviewed Aug 24, 2017

View reviewed changes

mvo5 added 2 commits August 24, 2017 18:09

Merge remote-tracking branch 'upstream/master' into ensure-core-in-sn…

40e1fc9

…apstate

address review feedback

70609e0

pedronis requested changes Aug 28, 2017

View reviewed changes

mvo5 added 2 commits August 28, 2017 17:06

address review feedback (thanks to Samuele)

e344be7

Merge remote-tracking branch 'upstream/master' into ensure-core-in-sn…

23aa1b4

…apstate

pedronis approved these changes Aug 31, 2017

View reviewed changes

mvo5 added 3 commits September 5, 2017 14:43

Merge remote-tracking branch 'upstream/master' into ensure-core-in-sn…

091af4c

…apstate

Merge remote-tracking branch 'upstream/master' into ensure-core-in-sn…

767a14c

…apstate

fix unittests

51e03f2

chipaca approved these changes Sep 6, 2017

View reviewed changes

Merge remote-tracking branch 'upstream/master' into ensure-core-in-sn…

9966425

…apstate

pedronis merged commit 3954d21 into canonical:master Sep 6, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

daemon, snapstate: move ensureCore from daemon/api.go into snapstate.go #3727

daemon, snapstate: move ensureCore from daemon/api.go into snapstate.go #3727

mvo5 commented Aug 14, 2017

pedronis Aug 14, 2017

mvo5 Aug 17, 2017

pedronis Aug 17, 2017

pedronis Aug 17, 2017

pedronis Aug 14, 2017

pedronis Aug 14, 2017

pedronis Aug 14, 2017

mvo5 commented Aug 15, 2017

mvo5 commented Aug 18, 2017

pedronis left a comment

pedronis Aug 18, 2017

pedronis Aug 18, 2017 •

edited

Loading

mvo5 Aug 18, 2017

mvo5 Aug 18, 2017

pedronis Aug 18, 2017

mvo5 Aug 18, 2017

pedronis Aug 18, 2017

mvo5 Aug 18, 2017

pedronis Aug 18, 2017

codecov-io commented Aug 21, 2017 •

edited

Loading

pedronis Aug 24, 2017

mvo5 Aug 24, 2017

pedronis Aug 24, 2017

pedronis Aug 25, 2017 •

edited

Loading

pedronis Aug 24, 2017

pedronis Aug 24, 2017

pedronis left a comment

pedronis Aug 28, 2017

pedronis Aug 28, 2017

mvo5 Aug 28, 2017

pedronis Aug 28, 2017

mvo5 Aug 28, 2017

pedronis left a comment

daemon, snapstate: move ensureCore from daemon/api.go into snapstate.go #3727

daemon, snapstate: move ensureCore from daemon/api.go into snapstate.go #3727

Conversation

mvo5 commented Aug 14, 2017

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 commented Aug 15, 2017

mvo5 commented Aug 18, 2017

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis Aug 18, 2017 • edited Loading

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

codecov-io commented Aug 21, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis Aug 25, 2017 • edited Loading

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

pedronis left a comment

Choose a reason for hiding this comment

pedronis Aug 18, 2017 •

edited

Loading

codecov-io commented Aug 21, 2017 •

edited

Loading

pedronis Aug 25, 2017 •

edited

Loading