o/snapstate: properly handle components when refreshing to revision that has been on the system before #14498

andrewphelpsj · 2024-09-12T14:39:07Z

This change properly handles components when refreshing to a snap revision that has already been on the system.

When doing something like: snap refresh --revision=n snapname, where n has already been installed on the system, we were previously just switching back to the components that were installed with that revision. This is more of a revert than a refresh.

To properly handle this, we should consider which components are currently installed, and remove any that are not available for the revision n, and check with the store for the most up-to-date revisions of components.

andrewphelpsj · 2024-09-12T14:52:16Z

I forgot a commit here, will open again shortly.

…hat has been on the system before

…ady installed for a snap revision that we don't want anymore

…tore

pedronis

I'm a bit confused by the change in snapstate.go

pedronis · 2024-09-16T15:36:31Z

overlord/snapstate/snapstate_update_test.go

@@ -13796,10 +13796,10 @@ func (s *snapmgrTestSuite) TestUpdateStateConflictRemoved(c *C) {

 func (s *snapmgrTestSuite) TestUpdateWithComponentsBackToPrevRevision(c *C) {


shouldn't we have kept both a version with and without components of this?

I can add the old version back if you like, but this is really just additive, the functionality of the original test is still there.

pedronis · 2024-09-16T15:43:19Z

overlord/snapstate/snapstate.go

+//
+// This is mostly relevant when we're moving from one snap revision to another
+// snap revision that has already been installed on the system. The target snap
+// might have components that are installed that we don't want any more.


I'm confused by this comment, aren't the component linked to a given snap revision, why would we remove components when switch around by revision?

Yes, they are linked by snap revision. Consider this case:

snap revision 1, components a, b snap revision 2, components b, c

When we move from snap revision 2 -> 1, we need to unlink component a (and discard it, if nothing else references it) and install component c.

I am a bit confused about the comment too, should "target snap might have components that are installed that we don't want any more" be "target revision might have components that were installed in the past but that we don't want any more"? Or maybe state more clearly that this is about checking a revision that was active in the past and still around but not currently. I always find misleading when we talk about installed revisions for non-active but present revisions.

I see what you mean, I'll update it to be explicit when I'm talking about something that was installed in the past.

overlord/snapstate/snapstate.go

…vision we are moving to

andrewphelpsj · 2024-09-16T16:18:50Z

Here is the set of tasks generated by the new test:

task prerequisites (1)
task prepare-snap (2)
task unlink-component (3), unlink "snap-with-components+test-component-extra" for snap revision 7, the target snap revision
task unlink-component (5), unlink "snap-with-components+test-component-present-in-sequence" for snap revision 7, the target snap revision
task download-component (6)
task validate-component (7)
task mount-component (8)
task run-hook (9), pre-refresh hook of "snap-with-components_key+kernel-modules-component"
task download-component (12)
task validate-component (13)
task mount-component (14)
task run-hook (15), pre-refresh hook of "snap-with-components_key+test-component"
task run-hook (18), pre-refresh hook of "snap-with-components_key"
task stop-snap-services (19)
task remove-aliases (20)
task unlink-current-snap (21)
task copy-snap-data (22)
task setup-profiles (23)
task link-snap (24)
task link-component (10)
task link-component (16)
task auto-connect (25)
task set-auto-aliases (26)
task setup-aliases (27)
task run-hook (28), post-refresh hook of "snap-with-components_key"
task run-hook (11), post-refresh hook of "snap-with-components_key+kernel-modules-component"
task run-hook (17), post-refresh hook of "snap-with-components_key+test-component"
task prepare-kernel-modules-components (29)
task start-snap-services (30)
task discard-component (4), discard "snap-with-components+test-component-extra" since it is not referenced by sequence
task cleanup (31)
task run-hook (32), configure hook of "snap-with-components_key"
task run-hook (33), health check of "snap-with-components_key"
task check-rerefresh (34)

…ponents This test ensures that we don't hit the store when we have no components to check on.

pedronis

couple of worries/questions

pedronis · 2024-09-17T07:39:13Z

overlord/snapstate/component.go

+func installComponentAction(st *state.State, snapst SnapState, snapRev snap.Revision, channel string, opts Options) (*store.SnapAction, error) {
+	var si *snap.SideInfo
+	if snapRev.Unset() {
+		si = snapst.CurrentSideInfo()


mmh, test don't seem to hit this case?

Looking at the code, we can't reach that case. Removed.

pedronis · 2024-09-17T07:50:21Z

overlord/snapstate/snapstate.go

+			i18n.G("Unlink component %q for snap revision %s"), ci.Component, snapsup.Revision(),
+		))
+
+		unlink.Set("snap-setup", snapsup)


will this confuse findSnapSetupTask ? that's one example but I fear we might have a few bits of logic that assume there is mainly one task in a snap lane with snap-setup attached to it vs id?

I've change this in 6014886.

Don't know why I didn't do that in the first place.

…ng extra already-present components

pedronis

thank you

alfonsosanchezbeato

LGTM, just some nitpicks about comments

alfonsosanchezbeato · 2024-09-19T20:11:06Z

overlord/snapstate/snapstate.go

+//
+// This is mostly relevant when we're moving from one snap revision to another
+// snap revision that has already been installed on the system. The target snap
+// might have components that are installed that we don't want any more.


I am a bit confused about the comment too, should "target snap might have components that are installed that we don't want any more" be "target revision might have components that were installed in the past but that we don't want any more"? Or maybe state more clearly that this is about checking a revision that was active in the past and still around but not currently. I always find misleading when we talk about installed revisions for non-active but present revisions.

alfonsosanchezbeato · 2024-09-19T20:21:00Z

overlord/snapstate/snapstate.go

+
+		// note that we don't need to worry about kernel module components here,
+		// since the components that we are removing are not associated with the
+		// currently installed snap revision. unlink-component differs from


and here it says "currently installed", which contradicts how "install" is used in the function description

alfonsosanchezbeato · 2024-09-19T20:42:08Z

overlord/snapstate/snapstate_update_test.go

@@ -14127,8 +14475,8 @@ type updateWithComponentsOpts struct {
 }

 func componentNameToType(c *C, name string) snap.ComponentType {
-	typ := strings.TrimSuffix(name, "-component")
-	if typ == name {
+	typ, _, ok := strings.Cut(name, "-component")


nitpicking, but this is not doing the same as TrimSuffix (not that I expect a component to be called xx-componentblah-component, but still...)

I did it this way because I wanted to be able to add some components like "test-component-extra", the difference was intentional.

Ok, I see, sounds good

andrewphelpsj requested review from pedronis and alfonsosanchezbeato September 12, 2024 14:39

andrewphelpsj marked this pull request as draft September 12, 2024 14:51

andrewphelpsj force-pushed the recheck-comp-revision-when-refreshing branch from 3118cf4 to a655bf5 Compare September 12, 2024 17:18

andrewphelpsj marked this pull request as ready for review September 12, 2024 17:19

andrewphelpsj force-pushed the recheck-comp-revision-when-refreshing branch 4 times, most recently from 8e7d528 to 0bfd630 Compare September 13, 2024 16:05

o/snapstate: properly handle components when refreshing to revision t…

880d889

…hat has been on the system before

andrewphelpsj force-pushed the recheck-comp-revision-when-refreshing branch 3 times, most recently from e1bab5e to 5a3180e Compare September 16, 2024 12:50

andrewphelpsj added 2 commits September 16, 2024 09:19

o/snapstate: make sure to unlink and discard components that are alre…

10420aa

…ady installed for a snap revision that we don't want anymore

o/snapstate: make sure to set tracking channel after talking to the s…

93b98c2

…tore

andrewphelpsj force-pushed the recheck-comp-revision-when-refreshing branch from 5a3180e to 93b98c2 Compare September 16, 2024 13:19

pedronis reviewed Sep 16, 2024

View reviewed changes

overlord/snapstate/snapstate.go Outdated Show resolved Hide resolved

pedronis reviewed Sep 16, 2024

View reviewed changes

overlord/snapstate/snapstate.go Outdated Show resolved Hide resolved

o/snapstate: use targetRevision instead of newRevision to refer to re…

aa707be

…vision we are moving to

o/snapstate: add back test for refreshing to old revision without com…

2ac5298

…ponents This test ensures that we don't hit the store when we have no components to check on.

andrewphelpsj requested a review from pedronis September 16, 2024 16:53

pedronis reviewed Sep 17, 2024

View reviewed changes

andrewphelpsj added 2 commits September 17, 2024 09:04

o/snapstate: use snapsup from main chain of tasks in tasks for removi…

6014886

…ng extra already-present components

o/snapstate: remove branch that we cannot go into

6dc918b

andrewphelpsj requested a review from pedronis September 17, 2024 13:15

pedronis approved these changes Sep 17, 2024

View reviewed changes

pedronis added this to the 2.66 milestone Sep 18, 2024

alfonsosanchezbeato approved these changes Sep 19, 2024

View reviewed changes

o/snapstate: clear up some confusing language in comments

7ceabfe

andrewphelpsj merged commit 112e58d into canonical:master Sep 20, 2024
58 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

o/snapstate: properly handle components when refreshing to revision that has been on the system before #14498

o/snapstate: properly handle components when refreshing to revision that has been on the system before #14498

andrewphelpsj commented Sep 12, 2024

andrewphelpsj commented Sep 12, 2024

pedronis left a comment

pedronis Sep 16, 2024

andrewphelpsj Sep 16, 2024

pedronis Sep 16, 2024

andrewphelpsj Sep 16, 2024

alfonsosanchezbeato Sep 19, 2024

andrewphelpsj Sep 19, 2024

andrewphelpsj commented Sep 16, 2024

pedronis left a comment

pedronis Sep 17, 2024

andrewphelpsj Sep 17, 2024

pedronis Sep 17, 2024

andrewphelpsj Sep 17, 2024

andrewphelpsj Sep 17, 2024

pedronis left a comment

alfonsosanchezbeato left a comment

alfonsosanchezbeato Sep 19, 2024

alfonsosanchezbeato Sep 19, 2024

alfonsosanchezbeato Sep 19, 2024

andrewphelpsj Sep 19, 2024

alfonsosanchezbeato Sep 19, 2024

		@@ -13796,10 +13796,10 @@ func (s snapmgrTestSuite) TestUpdateStateConflictRemoved(c C) {

		func (s snapmgrTestSuite) TestUpdateWithComponentsBackToPrevRevision(c C) {

o/snapstate: properly handle components when refreshing to revision that has been on the system before #14498

o/snapstate: properly handle components when refreshing to revision that has been on the system before #14498

Conversation

andrewphelpsj commented Sep 12, 2024

andrewphelpsj commented Sep 12, 2024

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

andrewphelpsj commented Sep 16, 2024

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

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

Choose a reason for hiding this comment