o/devicestate: consider snapd snap when remodeling #13242

alfonsosanchezbeato · 2023-10-02T12:48:10Z

Changes to snapd in a new model assertion (which can be only changes
in tracked channel at the moment) where not being applied, make sure
this does not happen anymore. Fixes LP#2035186.

codecov-commenter · 2023-10-02T13:11:33Z

Codecov Report

Merging #13242 (76e6425) into master (73c90a1) will decrease coverage by 0.01%.
Report is 54 commits behind head on master.
The diff coverage is 91.66%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##           master   #13242      +/-   ##
==========================================
- Coverage   78.83%   78.83%   -0.01%     
==========================================
  Files        1016     1013       -3     
  Lines      126663   126392     -271     
==========================================
- Hits        99857    99636     -221     
+ Misses      20557    20527      -30     
+ Partials     6249     6229      -20

Flag	Coverage Δ
unittests	`78.83% <91.66%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
aspects/schema.go	`91.26% <100.00%> (+1.67%)`	⬆️
bootloader/assets/grub_recovery_cfg_asset.go	`100.00% <100.00%> (ø)`
cmd/snap-failure/cmd_snapd.go	`84.67% <ø> (ø)`
cmd/snap-repair/runner.go	`82.91% <100.00%> (+0.75%)`	⬆️
cmd/snapd/main.go	`34.69% <ø> (-1.31%)`	⬇️
dirs/dirs.go	`93.46% <ø> (-0.03%)`	⬇️
interfaces/builtin/common.go	`71.76% <ø> (ø)`
interfaces/builtin/gpio_control.go	`100.00% <ø> (ø)`
interfaces/core.go	`88.57% <ø> (ø)`
interfaces/policy/basedeclaration.go	`38.88% <ø> (ø)`
... and 11 more

... and 17 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

andrewphelpsj

Looks good! One quick question.

andrewphelpsj · 2023-10-02T14:42:21Z

overlord/devicestate/devicestate.go

+	// new required snaps or a track for existing ones needs to be updated
+	for _, modelSnap := range new.Snaps() {
+		switch modelSnap.SnapType {
+		case "gadget", "kernel", "base", "core":


Would

if snapsAccountedFor[modelSnap.Name] { continue }

work instead of specifically calling out out the already handled ones? It looks like new.Base(), which is used as a key in snapsAccountedFor earlier, returns something that may not be the name, so it might not. If it does, though, I think it'd be a bit better.

Actually this is an excellent suggestion, as now I notice that this could overlook non-rootfs bases (we can have UC22 but also a core20 snap installed).

pedronis

not sure it makes sense to treat snapd as we treat app snaps

pedronis · 2023-10-03T16:12:37Z

overlord/devicestate/devicestate.go

+	for _, modelSnap := range new.Snaps() {
+		if snapsAccountedFor[modelSnap.Name] {
+			continue
+		}


I don't think we want to switch snapd track in the middle of other things, it needs to be treated as one of the essential snaps I think and processed first as we do during regular refreshes

It'd be really cool if we could do the essential snaps in a loop, rather than having them all listed like they are now. Although some of the special cases make that difficult. Specifically, new.BaseSnap() can return nil on UC16, and only the gadget snap's task set gets passed to updateNeededSnapsFromTs.

I want ahead with this, hopefully it looks a bit better now.

alfonsosanchezbeato · 2023-10-11T15:48:17Z

Thanks for the reviews, I've repushed now so snapd is considered jointly with the other essential snaps. To be clear, by looking at how are things implemented at the moment I don't think it will make a difference with the previous approach.

andrewphelpsj

Looks good! Just one comment

andrewphelpsj · 2023-10-11T16:25:47Z

asserts/model.go

@@ -523,6 +523,14 @@ func (mod *Model) StorageSafety() StorageSafety {
 	return mod.storageSafety
 }

+// SnapdSnap returns the details of the snapd snap for this model.
+func (mod *Model) SnapdSnap() *ModelSnap {
+	if mod.Base() == "" || mod.Base() == "core" {


A comment explaining why these checks are made might be useful. Additionally, rather than directly accessing the slice of snaps, maybe store a pointer to the snap on *Model like the rest of the essential snaps. Although that really doesn't matter too much.

I think core18 style classic models can have no base but a snapd snap?

But then how you would know whether to seed core or core18? I don't see examples of 18.04 classic models in https://github.com/snapcore/models/tree/master , do you know where could I find one?

I think the hybrid installer might have one, maybe some cloud images. We should have some example in our own unit test somewhere, though they test might be signing it on the fly.

also on classic afair if there's a snapd it will be used in preference to core

I've changed the strategy and now I simply check the name of the first snap in the slice, to avoid this issues. The only example that I've seen of classic 18 model is snapd/tests/lib/assertions/developer1-my-classic-w-gadget-18.model, which has:

required-snaps: - core18

pedronis

question

pedronis · 2023-10-17T15:31:18Z

asserts/model.go

@@ -523,6 +523,15 @@ func (mod *Model) StorageSafety() StorageSafety {
 	return mod.storageSafety
 }

+// SnapdSnap returns the details of the snapd snap for this model.
+func (mod *Model) SnapdSnap() *ModelSnap {


it's unclear to me if this is enough, snapd can be explicit or implicit in UC18 and UC20+ models, but seed code will pick what to use even if it's implicit:

https://github.com/snapcore/snapd/blob/2.60.4/seed/seed20.go#L699
https://github.com/snapcore/snapd/blob/2.60.4/seed/seed16.go#L321

and at least with UC20+ potentially moving from an implicit to and explict or the other way around snapd might involve a channel change but is not clear to me that this simple method plus the code in devicestate will cover those scenarios correctly?

I have rewritten the PR so this method is not needed anymore, accounting for cases when snapd information is implicit. In the end, modelSnaps type and other types reflect only the explicit information in the model assertion, and we are not changing that now, although I think it would be good to reflect also implicit information there in the future.

andrewphelpsj · 2023-10-18T13:38:37Z

overlord/devicestate/devicestate.go

@@ -541,6 +536,10 @@ func remodelEssentialSnapTasks(ctx context.Context, st *state.State, pathSI *pat
 	}

 	if !changed {
+		addExistingSnapTasks := snapstate.LinkNewBaseOrKernel


I like this change, but I think it'd be even more clear if addExistingSnapTasks was eliminated and the functions were called directly.

andrewphelpsj · 2023-10-18T13:42:07Z

overlord/devicestate/devicestate.go

+	var pathSi *pathSideInfo
+	// Implicit new channel if snapd is not explicitly in the model
+	newSnapdChannel := "latest/stable"
+	essentialSnaps := newModel.EssentialSnaps()
+	if essentialSnaps[0].SnapType == "snapd" {
+		// snapd can be specified explicitly in the model (UC20+)
+		newSnapdChannel = essentialSnaps[0].DefaultChannel
+	}
+	pathSi = sideInfoAndPathFromID(localSnaps, paths, naming.WellKnownSnapID("snapd"))


Suggested change

var pathSi *pathSideInfo

// Implicit new channel if snapd is not explicitly in the model

newSnapdChannel := "latest/stable"

essentialSnaps := newModel.EssentialSnaps()

if essentialSnaps[0].SnapType == "snapd" {

// snapd can be specified explicitly in the model (UC20+)

newSnapdChannel = essentialSnaps[0].DefaultChannel

}

pathSi = sideInfoAndPathFromID(localSnaps, paths, naming.WellKnownSnapID("snapd"))

// Implicit new channel if snapd is not explicitly in the model

newSnapdChannel := "latest/stable"

essentialSnaps := newModel.EssentialSnaps()

if essentialSnaps[0].SnapType == "snapd" {

// snapd can be specified explicitly in the model (UC20+)

newSnapdChannel = essentialSnaps[0].DefaultChannel

}

pathSi := sideInfoAndPathFromID(localSnaps, paths, naming.WellKnownSnapID("snapd"))

No need to declare pathSi early.

Right, done now

andrewphelpsj · 2023-10-18T13:43:53Z

overlord/devicestate/devicestate.go

 			return nil, err
 		}
+		if ts != nil {
+			tss = append(tss, ts)
+			if modelSnap.SnapType == "gadget" {


Do you know why we have to have this special case? Is it because gadgets can have prerequisites, and other types cannot?

Yes, that is the case. Bases cannot depend on other base, and kernels do not depend on a base, at least for the moment. And neither of them can have content providers I think.

andrewphelpsj

Updates look good!

pedronis

thanks, left a couple of comments on the new version

overlord/devicestate/devicestate.go

pedronis · 2023-10-18T15:59:31Z

overlord/managers_test.go

+	snapdPath, _ := s.makeStoreTestSnap(c, "name: snapd\ntype: snapd\nversion: 123", "1")
+	s.serveSnap(snapdPath, "2")
+


why do we need this change? shouldn't we make it so that there's no channel change instead? the test doesn't seem to be about this anyway

Without adding this the test does not progress enough and the error:
unexpectedly empty response from the server (try again later)
is returned. snapd snap is set in the state for the test, so at some point the code is trying to contact the fakestore for information about it.

sorry, maybe I wasn't clear but the issue why you need this is that the new code thinks there's a channel change for snapd: this works too and is a bit cleaner I think:

diff --git a/overlord/managers_test.go b/overlord/managers_test.go index f216eccaf5..b51b17af30 100644 --- a/overlord/managers_test.go +++ b/overlord/managers_test.go @@ -8460,8 +8460,6 @@ func (s *mgrsSuiteCore) TestRemodelUC20SnapWithPrereqsMissingDeps(c *C) { snapPath, _ := s.makeStoreTestSnap(c, prereqSnapYaml, "1") s.serveSnap(snapPath, "1") - snapdPath, _ := s.makeStoreTestSnap(c, "name: snapd\ntype: snapd\nversion: 123", "1") - s.serveSnap(snapdPath, "2") st := s.o.State() st.Lock() @@ -8475,6 +8473,7 @@ func (s *mgrsSuiteCore) TestRemodelUC20SnapWithPrereqsMissingDeps(c *C) { }, Current: snap.R(1), SnapType: "snapd", + TrackingChannel: "latest/stable", Flags: snapstate.Flags{ Required: true, },

Oh, right, I've changed this now. Sorry, I should have read more carefully!

alfonsosanchezbeato

Thanks for the reviews, I've addressed the comments now.

alfonsosanchezbeato · 2023-10-19T07:49:39Z

overlord/devicestate/devicestate.go

@@ -541,6 +536,10 @@ func remodelEssentialSnapTasks(ctx context.Context, st *state.State, pathSI *pat
 	}

 	if !changed {
+		addExistingSnapTasks := snapstate.LinkNewBaseOrKernel


alfonsosanchezbeato · 2023-10-19T07:50:45Z

overlord/devicestate/devicestate.go

+	var pathSi *pathSideInfo
+	// Implicit new channel if snapd is not explicitly in the model
+	newSnapdChannel := "latest/stable"
+	essentialSnaps := newModel.EssentialSnaps()
+	if essentialSnaps[0].SnapType == "snapd" {
+		// snapd can be specified explicitly in the model (UC20+)
+		newSnapdChannel = essentialSnaps[0].DefaultChannel
+	}
+	pathSi = sideInfoAndPathFromID(localSnaps, paths, naming.WellKnownSnapID("snapd"))


Right, done now

alfonsosanchezbeato · 2023-10-19T07:56:22Z

overlord/devicestate/devicestate.go

 			return nil, err
 		}
+		if ts != nil {
+			tss = append(tss, ts)
+			if modelSnap.SnapType == "gadget" {


Yes, that is the case. Bases cannot depend on other base, and kernels do not depend on a base, at least for the moment. And neither of them can have content providers I think.

overlord/devicestate/devicestate.go

alfonsosanchezbeato · 2023-10-19T08:09:35Z

overlord/managers_test.go

+	snapdPath, _ := s.makeStoreTestSnap(c, "name: snapd\ntype: snapd\nversion: 123", "1")
+	s.serveSnap(snapdPath, "2")
+


Without adding this the test does not progress enough and the error:
unexpectedly empty response from the server (try again later)
is returned. snapd snap is set in the state for the test, so at some point the code is trying to contact the fakestore for information about it.

pedronis

thank you, further comment about the managers_test.go change

pedronis · 2023-10-19T09:49:07Z

overlord/managers_test.go

+	snapdPath, _ := s.makeStoreTestSnap(c, "name: snapd\ntype: snapd\nversion: 123", "1")
+	s.serveSnap(snapdPath, "2")
+


sorry, maybe I wasn't clear but the issue why you need this is that the new code thinks there's a channel change for snapd: this works too and is a bit cleaner I think:

diff --git a/overlord/managers_test.go b/overlord/managers_test.go index f216eccaf5..b51b17af30 100644 --- a/overlord/managers_test.go +++ b/overlord/managers_test.go @@ -8460,8 +8460,6 @@ func (s *mgrsSuiteCore) TestRemodelUC20SnapWithPrereqsMissingDeps(c *C) { snapPath, _ := s.makeStoreTestSnap(c, prereqSnapYaml, "1") s.serveSnap(snapPath, "1") - snapdPath, _ := s.makeStoreTestSnap(c, "name: snapd\ntype: snapd\nversion: 123", "1") - s.serveSnap(snapdPath, "2") st := s.o.State() st.Lock() @@ -8475,6 +8473,7 @@ func (s *mgrsSuiteCore) TestRemodelUC20SnapWithPrereqsMissingDeps(c *C) { }, Current: snap.R(1), SnapType: "snapd", + TrackingChannel: "latest/stable", Flags: snapstate.Flags{ Required: true, },

Changes to snapd in a new model assertion (which can be only changes in tracked channel at the moment) where not being applied, make sure this does not happen anymore. Fixes LP#2035186.

alfonsosanchezbeato · 2023-10-19T14:14:30Z

History rewritten and rebased to master.

alfonsosanchezbeato added the Run nested The PR also runs tests inluded in nested suite label Oct 2, 2023

alfonsosanchezbeato added this to the 2.61 milestone Oct 2, 2023

alfonsosanchezbeato requested review from pedronis and andrewphelpsj October 2, 2023 12:48

andrewphelpsj approved these changes Oct 2, 2023

View reviewed changes

pedronis requested changes Oct 3, 2023

View reviewed changes

alfonsosanchezbeato force-pushed the fix-snapd-remodel branch from 08da44d to c0c43ea Compare October 11, 2023 15:46

alfonsosanchezbeato requested review from pedronis and andrewphelpsj October 11, 2023 15:46

andrewphelpsj approved these changes Oct 11, 2023

View reviewed changes

pedronis reviewed Oct 11, 2023

View reviewed changes

alfonsosanchezbeato requested a review from pedronis October 12, 2023 08:32

pedronis reviewed Oct 17, 2023

View reviewed changes

alfonsosanchezbeato force-pushed the fix-snapd-remodel branch from 360fe17 to 76e6425 Compare October 18, 2023 12:32

alfonsosanchezbeato requested a review from pedronis October 18, 2023 12:37

andrewphelpsj reviewed Oct 18, 2023

View reviewed changes

andrewphelpsj approved these changes Oct 18, 2023

View reviewed changes

pedronis reviewed Oct 18, 2023

View reviewed changes

alfonsosanchezbeato commented Oct 19, 2023

View reviewed changes

alfonsosanchezbeato requested a review from pedronis October 19, 2023 08:13

pedronis approved these changes Oct 19, 2023

View reviewed changes

o/devicestate: consider snapd snap when remodeling

3ef4823

Changes to snapd in a new model assertion (which can be only changes in tracked channel at the moment) where not being applied, make sure this does not happen anymore. Fixes LP#2035186.

alfonsosanchezbeato force-pushed the fix-snapd-remodel branch from 760192a to 3ef4823 Compare October 19, 2023 14:13

alfonsosanchezbeato merged commit 5393602 into snapcore:master Oct 20, 2023
45 of 51 checks passed

ernestl mentioned this pull request Nov 16, 2023

release: 2.61.1 #13366

Merged

ernestl added the cherry-picked label Nov 17, 2023

		snapdPath, _ := s.makeStoreTestSnap(c, "name: snapd\ntype: snapd\nversion: 123", "1")
		s.serveSnap(snapdPath, "2")

o/devicestate: consider snapd snap when remodeling #13242

o/devicestate: consider snapd snap when remodeling #13242

Conversation

alfonsosanchezbeato commented Oct 2, 2023

codecov-commenter commented Oct 2, 2023 • edited

Codecov Report

andrewphelpsj left a 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

alfonsosanchezbeato commented Oct 11, 2023

andrewphelpsj 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

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

andrewphelpsj left a 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

alfonsosanchezbeato 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

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alfonsosanchezbeato commented Oct 19, 2023

codecov-commenter commented Oct 2, 2023 •

edited