o/devicestate: refactor remodeling code to use new snapstate API #14898

andrewphelpsj · 2025-01-06T15:37:55Z

The new snapstate API supports components, so we need to swap over to that before anything else.

This also includes a refactor to move where we make some of the decisions during remodeling. Specifically, we were duplicating a lot of the logic for what snap to install/update for essential and non-essential snaps. This will get worse when introducing components, so I think it makes sense to move things around a bit so that we can share some more code for the essential and non-essential snaps.

pedronis

one question

pedronis · 2025-01-09T16:28:21Z

overlord/devicestate/devicestate.go

@@ -1597,11 +1561,29 @@ func createRecoverySystemTasks(st *state.State, label string, snapSetupTasks []s
 // that is represented by the snap.SideInfo.
 type LocalSnap struct {


can't we reuse types from snapstate like PathSnap etc?

pedronis

did a pass

pedronis · 2025-01-10T17:09:55Z

overlord/snapstate/target.go

+	Components []PathComponent
+}
+
+type PathComponent struct {


this needs a doc comment, also shouldn't this be defined before it is used?

overlord/devicestate/devicestate.go

pedronis · 2025-01-10T17:31:27Z

overlord/devicestate/devicestate.go

+	}), nil
+}
+
+func (r *remodeler) installedRevisionUpdateGoal(


this one probably merits a comment and its use of a StoreGoal which will not result in store usagge

On that note, about not reaching out to the store. This is problematic in case of components, since we always reach out to the store if the snap has components installed (since they might require updates, even if the snap doesn't).

I wonder, is this a place for a new *Goal, that only considers snaps and components that are already installed on the system?

overlord/devicestate/devicestate.go

pedronis · 2025-01-10T17:33:16Z

overlord/devicestate/devicestate.go

-	offline        bool
-	localSnaps     []*snap.SideInfo
-	localSnapPaths []string
+type remodeler struct {


it might be worth in a follow to move this and its dedicated helpers to a remodeler.go file?

bboozzoo · 2025-01-13T10:25:25Z

overlord/snapstate/target.go

@@ -813,7 +813,7 @@ type pathInstallGoal struct {

 // PathInstallGoal creates a new InstallGoal to install a snap from a given from
 // a path on disk. If instanceName is not provided, si.RealName will be used.
-func PathInstallGoal(instanceName, path string, si *snap.SideInfo, components map[*snap.ComponentSideInfo]string, opts RevisionOptions) InstallGoal {
+func PathInstallGoal(instanceName, path string, si *snap.SideInfo, components []PathComponent, opts RevisionOptions) InstallGoal {


since we're already touching all affected places, could this use PathSnap?

Done, thanks!

bboozzoo · 2025-01-13T10:28:29Z

overlord/devicestate/devicestate.go

-	path    string
+// remodelTarget represents a snap that is part of the model that we are
+// remodeling to.
+type remodelTarget struct {


Suggested change

type remodelTarget struct {

type remodelSnapTarget struct {

wdyt?

pedronis

thank you

codecov · 2025-01-13T17:46:50Z

Codecov Report

Attention: Patch coverage is 89.61938% with 30 lines in your changes missing coverage. Please review.

Project coverage is 78.30%. Comparing base (24a0034) to head (e159ff0).
Report is 107 commits behind head on master.

Files with missing lines	Patch %	Lines
overlord/devicestate/devicestate.go	87.65%	20 Missing and 10 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #14898      +/-   ##
==========================================
+ Coverage   78.20%   78.30%   +0.10%     
==========================================
  Files        1151     1154       +3     
  Lines      151396   153091    +1695     
==========================================
+ Hits       118402   119883    +1481     
- Misses      25662    25826     +164     
- Partials     7332     7382      +50

Flag	Coverage Δ
unittests	`78.30% <89.61%> (+0.10%)`	⬆️

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.

bboozzoo

LGTM

bboozzoo · 2025-01-14T07:18:06Z

A bunch of nested remodel tests fail with:

+ VERSION=24
+ remote.push /home/gopath/src/github.com/snapcore/snapd/tests/lib/assertions/classic-model-rev1-24.assert
Warning: Permanently added '[localhost]:8022' (ED25519) to the list of known hosts.
++ remote.exec 'sudo snap remodel --no-wait classic-model-rev1-24.assert'
sudo: unable to resolve host localhost.localdomain: Name or service not known
error: cannot remodel: cannot remodel device: cannot perform operation on local snap
+ change_id=
-----
.
2025-01-13 22:48:39 Debug output for google-nested:ubuntu-24.04-64:tests/nested/manual/hybrid-remodel (jan132223-957648) : saved to file spread-logs/google-nested_ubuntu-24.04-64_tests_nested_manual_hybrid-remodel.debug.log

github-actions · 2025-01-14T16:36:39Z

Tue Jan 14 22:19:17 UTC 2025

No spread failures

…take in a type that can wrap local snaps and components

… contains components

…r in preparation for components The refactor makes it so all of the decisions about what to do for a snap in the incoming model are all in the same place. I feel that this is pretty useful now, and it will simplify adding the logic that is needed for handling components as well.

… that is used in snapstate.PathSnap

…rom snapstate

…te.PathInstallGoal api

… current snap is not tracking a channel

andrewphelpsj · 2025-01-14T21:03:27Z

Required failures are:

google-nested:ubuntu-20.04-64:tests/nested/manual/core20-new-snapd-does-not-break-old-initrd:startwithnew
google-nested:ubuntu-24.04-64:tests/nested/manual/hybrid-remodel
google-nested:ubuntu-24.04-64:tests/nested/manual/remodel-min-size

These are known failures and are unrelated to this change.

andrewphelpsj requested a review from pedronis January 6, 2025 15:38

github-actions bot added the Needs Documentation -auto- Label automatically added which indicates the change needs documentation label Jan 6, 2025

andrewphelpsj requested a review from bboozzoo January 6, 2025 15:38

andrewphelpsj force-pushed the remodel-new-api-refactor branch from 92ea8ad to 3a2bbfd Compare January 7, 2025 14:14

pedronis reviewed Jan 9, 2025

View reviewed changes

andrewphelpsj requested a review from pedronis January 10, 2025 15:27

pedronis reviewed Jan 10, 2025

View reviewed changes

bboozzoo reviewed Jan 13, 2025

View reviewed changes

andrewphelpsj requested review from pedronis and bboozzoo January 13, 2025 14:29

pedronis approved these changes Jan 13, 2025

View reviewed changes

andrewphelpsj added the Run nested The PR also runs tests inluded in nested suite label Jan 13, 2025

andrewphelpsj closed this Jan 13, 2025

andrewphelpsj reopened this Jan 13, 2025

bboozzoo approved these changes Jan 14, 2025

View reviewed changes

andrewphelpsj force-pushed the remodel-new-api-refactor branch from 2288fe5 to 0036344 Compare January 14, 2025 16:26

andrewphelpsj added 11 commits January 14, 2025 12:25

o/devicestate, overlord, daemon: refactor devicestate.Remodel api to …

9f08b9b

…take in a type that can wrap local snaps and components

o/devicestate: remove usage of type that can be replaced by type that…

11e47e4

… contains components

o/snapstate, o/devicestate, daemon: introduce snapstate.PathComponent…

08c04ad

… that is used in snapstate.PathSnap

o/devicestate: eliminate some types that can be replaced with types f…

d65c0a8

…rom snapstate

o/devicestate: update some comments on the remodeler and methods

e1d946e

o/devicestate, o/snapstate: use snapstate.PathSnap as part of snapsta…

e9681f7

…te.PathInstallGoal api

o/devicestate: rename remodelTarget to remodelSnapTarget

7c7598c

o/snapstate: move PathComponent definition

41b1ea2

o/devicestate: do not change a snap's channel during a remodel if the…

1168f4a

… current snap is not tracking a channel

o/devicestate: remove unused function

e159ff0

andrewphelpsj force-pushed the remodel-new-api-refactor branch from 4f1135d to e159ff0 Compare January 14, 2025 17:26

alfonsosanchezbeato merged commit 5305352 into canonical:master Jan 14, 2025
54 of 60 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

o/devicestate: refactor remodeling code to use new snapstate API #14898

o/devicestate: refactor remodeling code to use new snapstate API #14898

andrewphelpsj commented Jan 6, 2025

pedronis left a comment

pedronis Jan 9, 2025

pedronis left a comment

pedronis Jan 10, 2025

pedronis Jan 10, 2025

andrewphelpsj Jan 10, 2025

pedronis Jan 10, 2025

bboozzoo Jan 13, 2025

andrewphelpsj Jan 13, 2025

bboozzoo Jan 13, 2025

pedronis left a comment

codecov bot commented Jan 13, 2025 •

edited

Loading

bboozzoo left a comment

bboozzoo commented Jan 14, 2025

github-actions bot commented Jan 14, 2025 •

edited

Loading

andrewphelpsj commented Jan 14, 2025

		@@ -1597,11 +1561,29 @@ func createRecoverySystemTasks(st *state.State, label string, snapSetupTasks []s
		// that is represented by the snap.SideInfo.
		type LocalSnap struct {

o/devicestate: refactor remodeling code to use new snapstate API #14898

o/devicestate: refactor remodeling code to use new snapstate API #14898

Conversation

andrewphelpsj commented Jan 6, 2025

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

pedronis left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 13, 2025 • edited Loading

Codecov Report

bboozzoo left a comment

Choose a reason for hiding this comment

bboozzoo commented Jan 14, 2025

github-actions bot commented Jan 14, 2025 • edited Loading

No spread failures

andrewphelpsj commented Jan 14, 2025

codecov bot commented Jan 13, 2025 •

edited

Loading

github-actions bot commented Jan 14, 2025 •

edited

Loading