o/snapstate: handle refreshing a snap that only has component revision changes #14486

andrewphelpsj · 2024-09-11T13:17:37Z

This handles the case where we go to refresh a snap and see that only components have been refreshed, rather than just the snap.

…that are used with store actions

…d component

…e code

…-component-during-refresh

andrewphelpsj · 2024-09-11T13:45:48Z

overlord/snapstate/handlers_components.go

@@ -561,12 +565,6 @@ func (m *SnapManager) doUnlinkComponent(t *state.Task, _ *tomb.Tomb) (err error)
 		return err
 	}

-	// TODO:COMPS: test taking this branch when unlinking components during a
-	// refresh where we lose components
-	if err := saveCurrentKernelModuleComponents(t, snapSup, snapSt); err != nil {


This was in the wrong place. We now do this in these 3 places:

link-component unlink-current-component link-snap

Previously we had it in unlink-component, rather than unlink-current-component. unlink-component isn't run during a snap refresh, that is only run when we are explicitly removing a snap (with its components)

are we sure we do not need to save the kernel-modules components here too? For instance, if we are removing one of these components, to get the state restored on rollback. I'm not sure if in that case we do an unlink current component.

In that case we use "unlink-current-component", since we don't support removing components for anything but the current snap revision.

See here: https://github.com/canonical/snapd/blob/master/overlord/snapstate/component.go?plain=1#L560

…n changes

pedronis

thanks, some comments also I'm not sure I fully understand the change in handlers.go

pedronis · 2024-09-11T18:31:28Z

overlord/snapstate/component.go

+	compSetupTaskID     string
+	beforeLink          []*state.Task
+	linkTask            *state.Task
+	postOpHookToDiscard []*state.Task


what does ToDiscard means here?

The range of the tasks, [post-refresh/install hook -> discard). I'm happy to take suggestions on naming there.

maybe all *state.Task field should have a name ending in "Task", and all the []*state.Task ones, one ending in "Tasks" ?

overlord/snapstate/snapstate.go

pedronis · 2024-09-11T18:38:50Z

overlord/snapstate/snapstate.go

@@ -1881,12 +1885,32 @@ type update struct {
 // revision of the snap.


it seems the doc comment for this needs changes, also I wonder if turning this around and calling it revisionDiverged wouldn't be clear?

I personally think that "satisfied" works better here as a method on update, since it reads as the update has already been satisfied. Using diverged implies something different to me, almost as if something has unexpectedly changed.

pedronis · 2024-09-11T18:45:40Z

overlord/snapstate/handlers.go

@@ -2714,7 +2714,7 @@ func (m *SnapManager) undoLinkSnap(t *state.Task, _ *tomb.Tomb) error {
 		return err
 	}

-	if len(snapst.Sequence.Revisions) == 1 {
+	if oldCurrent.Unset() {


this mean doing this only on firstInstall ? maybe is a good idea to call the name the predicate like that and use the var? I'm not entirely sure I understand the need for this change

During a refresh where we only refresh components, we treat it a lot like snap refresh --revision=n snapname, where n is the current revision of the snap. In that case, this undo handler wouldn't do the right thing. We don't want to treat it as if the snap is completely removed, since it isn't.

Naming the predicate to firstInstall is a good idea, I'll do that.

Added tests for this case here: 541066d

…esh-only-components

pedronis

thanks, I answered about the naming question

alfonsosanchezbeato

Thanks, I have a couple of questions

alfonsosanchezbeato · 2024-09-12T15:42:20Z

overlord/snapstate/component.go

+	compSetupTaskID          string
+	beforeLinkTasks          []*state.Task
+	linkTask                 *state.Task
+	postOpHookToDiscardTasks []*state.Task


nitpick²: maybe postLinkHookToDiscardTasks now that you are changing names? The "Op" is not saying too much I think.

alfonsosanchezbeato · 2024-09-12T15:54:57Z

overlord/snapstate/handlers_components.go

@@ -561,12 +565,6 @@ func (m *SnapManager) doUnlinkComponent(t *state.Task, _ *tomb.Tomb) (err error)
 		return err
 	}

-	// TODO:COMPS: test taking this branch when unlinking components during a
-	// refresh where we lose components
-	if err := saveCurrentKernelModuleComponents(t, snapSup, snapSt); err != nil {


are we sure we do not need to save the kernel-modules components here too? For instance, if we are removing one of these components, to get the state restored on rollback. I'm not sure if in that case we do an unlink current component.

overlord/snapstate/snapstate.go

…licit

alfonsosanchezbeato

LGTM, thanks!

…esh-only-components

codecov · 2024-09-12T21:58:25Z

Codecov Report

Attention: Patch coverage is 73.68421% with 15 lines in your changes missing coverage. Please review.

Project coverage is 78.83%. Comparing base (f60b3de) to head (d838bc8).
Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
overlord/snapstate/snapstate.go	74.28%	6 Missing and 3 partials ⚠️
overlord/snapstate/handlers_components.go	25.00%	2 Missing and 1 partial ⚠️
overlord/snapstate/target.go	40.00%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #14486   +/-   ##
=======================================
  Coverage   78.83%   78.83%           
=======================================
  Files        1078     1078           
  Lines      145031   145096   +65     
=======================================
+ Hits       114332   114393   +61     
- Misses      23540    23543    +3     
- Partials     7159     7160    +1

Flag	Coverage Δ
unittests	`78.83% <73.68%> (+<0.01%)`	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

andrewphelpsj added 6 commits August 19, 2024 16:02

o/snapstate: add currently installed components to the current snaps …

f129277

…that are used with store actions

o/snapstate: put options for component refresh test into struct

95f7eb9

o/snapstate: handle refreshing a snap that loses a currently installe…

cea160f

…d component

o/snapstate: always initialize resources map that is sent to the stor…

eeb3824

…e code

o/snapstate: build updatedCompRevisions with opts.postRefreshComponents

14165a1

Merge branch 'master' of https://github.com/canonical/snapd into lose…

e21ad34

…-component-during-refresh

andrewphelpsj commented Sep 11, 2024

View reviewed changes

andrewphelpsj force-pushed the refresh-only-components branch from 5befa9a to 0c1194a Compare September 11, 2024 13:51

andrewphelpsj requested review from pedronis and alfonsosanchezbeato September 11, 2024 13:51

o/snapstate: handle refreshing a snap that only has component revisio…

188128b

…n changes

andrewphelpsj force-pushed the refresh-only-components branch from 0c1194a to 188128b Compare September 11, 2024 14:53

pedronis reviewed Sep 11, 2024

View reviewed changes

andrewphelpsj added 4 commits September 11, 2024 15:04

o/snapstate: update doc comment on revisionSatisfied

bc036e4

o/snapstate: use firstInstall variable in conditional for readability

3712562

o/snapstate: add tests for doing/undoing refresh to current revision

541066d

Merge branch 'master' of https://github.com/canonical/snapd into refr…

6d38bec

…esh-only-components

andrewphelpsj requested a review from pedronis September 12, 2024 13:53

pedronis approved these changes Sep 12, 2024

View reviewed changes

alfonsosanchezbeato reviewed Sep 12, 2024

View reviewed changes

o/snapstate: make names of fields in componentInstallTaskSet more exp…

9254023

…licit

andrewphelpsj force-pushed the refresh-only-components branch from 06d7322 to 9254023 Compare September 12, 2024 17:25

andrewphelpsj requested a review from alfonsosanchezbeato September 12, 2024 17:33

alfonsosanchezbeato approved these changes Sep 12, 2024

View reviewed changes

Merge branch 'master' of https://github.com/canonical/snapd into refr…

d838bc8

…esh-only-components

andrewphelpsj merged commit 3b2350f into canonical:master Sep 13, 2024
55 of 56 checks passed

andrewphelpsj deleted the refresh-only-components branch September 13, 2024 13:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

o/snapstate: handle refreshing a snap that only has component revision changes #14486

o/snapstate: handle refreshing a snap that only has component revision changes #14486

andrewphelpsj commented Sep 11, 2024 •

edited

Loading

andrewphelpsj Sep 11, 2024 •

edited

Loading

alfonsosanchezbeato Sep 12, 2024

andrewphelpsj Sep 12, 2024

andrewphelpsj Sep 12, 2024

pedronis left a comment

pedronis Sep 11, 2024

andrewphelpsj Sep 11, 2024

pedronis Sep 12, 2024 •

edited

Loading

pedronis Sep 11, 2024

andrewphelpsj Sep 11, 2024

pedronis Sep 11, 2024

andrewphelpsj Sep 11, 2024

andrewphelpsj Sep 12, 2024

pedronis left a comment

alfonsosanchezbeato left a comment

alfonsosanchezbeato Sep 12, 2024

alfonsosanchezbeato Sep 12, 2024

alfonsosanchezbeato left a comment

codecov bot commented Sep 12, 2024 •

edited

Loading

		@@ -1881,12 +1885,32 @@ type update struct {
		// revision of the snap.

o/snapstate: handle refreshing a snap that only has component revision changes #14486

o/snapstate: handle refreshing a snap that only has component revision changes #14486

Conversation

andrewphelpsj commented Sep 11, 2024 • edited Loading

andrewphelpsj Sep 11, 2024 • 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

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

pedronis Sep 12, 2024 • 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

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

alfonsosanchezbeato left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 12, 2024 • edited Loading

Codecov Report

andrewphelpsj commented Sep 11, 2024 •

edited

Loading

andrewphelpsj Sep 11, 2024 •

edited

Loading

pedronis Sep 12, 2024 •

edited

Loading

codecov bot commented Sep 12, 2024 •

edited

Loading