o/snapstate: install components from the store #14092

andrewphelpsj · 2024-06-17T21:20:05Z

This PR enables us to install components and a snap from the store at the same time. Note that installing components from the store for an already installed snap is not yet handled yet, as that interaction with the store is more complex, requiring the usage of both channel and revision.

First relevant commit is 97dad45, based on #14070.

alfonsosanchezbeato

Some comments, see below.

Is it possible to have with these changes now a spread test using the store?

alfonsosanchezbeato · 2024-06-18T14:06:36Z

overlord/snapstate/snapstate.go

+			// if we are removing the snap, we can assume that we should remove
+			// the component too
+			RemoveComponentPath: snapsup.RemoveSnapPath,
+			SkipSecurity:        true,


But we need the security profiles for component hooks generated, when is that happening? We need to know the hooks shipped in the component for that, is that considered? In any case, a comment regarding this here would be good to have.

The setup-profiles task that is created for the snap handles generating those profiles here: https://github.com/snapcore/snapd/blob/master/overlord/ifacestate/handlers.go?plain=1#L184. I'll add comment.

alfonsosanchezbeato · 2024-06-18T14:34:41Z

overlord/snapstate/snapstate.go

@@ -3901,7 +3934,7 @@ func RevertToRevision(st *state.State, name string, rev snap.Revision, flags Fla
 		PlugsOnly:   len(info.Slots) == 0,
 		InstanceKey: snapst.InstanceKey,
 	}
-	return doInstall(st, &snapst, snapsup, 0, fromChange, nil)
+	return doInstall(st, &snapst, *snapsup, nil, 0, fromChange, nil)


Should this have a TODO as we will need components here as well?

alfonsosanchezbeato · 2024-06-18T14:34:54Z

overlord/snapstate/snapstate.go

@@ -2087,7 +2120,7 @@ func doUpdate(ctx context.Context, st *state.State, names []string, updates []mi

 		// Do not set any default restart boundaries, we do it when we have access to all
 		// the task-sets in preparation for single-reboot.
-		ts, err := doInstall(st, snapst, snapsup, noRestartBoundaries, fromChange, inUseFor(deviceCtx))
+		ts, err := doInstall(st, snapst, *snapsup, nil, noRestartBoundaries, fromChange, inUseFor(deviceCtx))


Should this have a TODO as we will need components here as well?

alfonsosanchezbeato · 2024-06-18T15:53:56Z

overlord/snapstate/target.go

+	info *snap.ComponentInfo
+}
+
+func (c *componentTarget) compsup() ComponentSetup {


Why do you need to construct this instead of returning c.setup? Cannot it be fully filled in componentFromResource?

It is shared here since there will be another implementation of UpdateGoal (the file one) which will also return a target.

But I do not see any mention to UpdateGoal in this PR (or am I missing something?), maybe it will become clear in the next PR, but atm I find this a bit confusing, maybe this part of the change could go in next one?

Sorry, I meant InstallGoal. Too many PRs in my head 🙂. I can move this to the next PR, which includes an InstallGoal implementation that installs from local files.

alfonsosanchezbeato · 2024-06-18T16:06:42Z

overlord/snapstate/target.go

+		info: &snap.ComponentInfo{
+			Component: compName,
+			Type:      comp.Type,
+			Version:   sar.Version,


Should we sanity chech the version?

snap.Component doesn't carry a Version, is there something else you were thinking to compare it to?

It would need to be read from component.yaml, I guess we can skip. Not sure though if what we put in store.SnapActionResult is checked against snap.yaml or not.

andrewphelpsj · 2024-06-18T16:18:34Z

@alfonsosanchezbeato as for the spread test, not yet without implementing some of the CLI aspects changes. Would you like that as a part of this PR?

alfonsosanchezbeato · 2024-06-18T16:24:35Z

@alfonsosanchezbeato as for the spread test, not yet without implementing some of the CLI aspects changes. Would you like that as a part of this PR?

Nope, we can do it in a follow-up, maybe as you say when we have api/cli changes.

pedronis

did a pass, some comments/questions, some might be for the store

overlord/snapstate/component.go

overlord/snapstate/handlers_components.go

pedronis · 2024-06-20T14:48:44Z

overlord/snapstate/snapstate_remove_test.go

@@ -438,7 +438,7 @@ func (s *snapmgrTestSuite) TestParallelInstanceRemoveRunThrough(c *C) {
 		{
 			op:             "remove-snap-data-dir",
 			name:           "some-snap_instance",
-			path:           filepath.Join(dirs.SnapDataDir, "some-snap"),
+			path:           filepath.Join(dirs.SnapDataDir, "some-snap_instance"),


why this change?

It lines up with this change here: https://github.com/snapcore/snapd/pull/14092/files#r1647912200.

It doesn't have to happen, but I noticed that it was inconsistent when writing the new tests here.

overlord/snapstate/target.go

pedronis · 2024-06-20T14:56:13Z

overlord/snapstate/target.go

 func installActionForStoreTarget(t StoreSnap, opts Options, enforcedSets func() (*snapasserts.ValidationSets, error)) (*store.SnapAction, error) {
 	action := &store.SnapAction{
 		Action:       "install",
 		InstanceName: t.InstanceName,
 		Channel:      t.RevOpts.Channel,
 		Revision:     t.RevOpts.Revision,
 		CohortKey:    t.RevOpts.CohortKey,
+		Resources:    t.Components,


what's the reason we need to pass this along? for metrics?

It is ultimately used to indicate in the store package that we should request resource info from the store. It could (and maybe should?) be a boolean value. It is a bit weird, since the option is a per-request toggle (just a query param), but right here I have it expressed as a per-action concept. But the StoreService.SnapAction API doesn't have any sort of flags right now.

overlord/snapstate/target.go

andrewphelpsj · 2024-06-20T17:25:44Z

overlord/snapstate/backend_test.go

@@ -1372,7 +1405,7 @@ func (f *fakeSnappyBackend) RemoveSnapDir(s snap.PlaceInfo, otherInstances bool)
 	f.appendOp(&fakeOp{
 		op:             "remove-snap-dir",
 		name:           s.InstanceName(),
-		path:           snap.BaseDir(s.SnapName()),
+		path:           snap.BaseDir(s.InstanceName()),


This change here I think is better representative of what actually happens in RemoveSnapDataDir (the real implementation).

pedronis

thanks for the changes, one question

pedronis · 2024-06-21T15:38:08Z

overlord/snapstate/snapstate.go

-	prev = linkSnap
+
+	for _, t := range tasksAfterLinkSnap {
+		addTask(t)


does this match where we specced after link hooks for components should run vs snap own hooks?

The component hook tasks aren't created yet. Should I add a TODO here for once #13775 gets merged?

As discussed, we are not adding hook tasks yet in doInstallComponent(), but that will eventually happen. In any case, if componentTasksForInstallWithSnap() keeps the tasks order things should be fine, maybe for the moment we could just add a comment there commenting that order needs to be kept.

alfonsosanchezbeato

LGTM, couple of minor comments

alfonsosanchezbeato · 2024-06-21T19:17:07Z

overlord/snapstate/target.go

+		return ComponentSetup{}, fmt.Errorf("inconsistent component type (%q in snap, %q in component)", typ, sar.Type)
+	}
+
+	compName := naming.NewComponentRef(info.SnapName(), name)


nitpick: maybe cref, there are too many *name variables here

alfonsosanchezbeato · 2024-06-21T19:28:30Z

overlord/snapstate/snapstate.go

-	prev = linkSnap
+
+	for _, t := range tasksAfterLinkSnap {
+		addTask(t)


As discussed, we are not adding hook tasks yet in doInstallComponent(), but that will eventually happen. In any case, if componentTasksForInstallWithSnap() keeps the tasks order things should be fine, maybe for the moment we could just add a comment there commenting that order needs to be kept.

pedronis

thanks

… from the store

…s being installed

…in test backend

…eliminate manual tracking of last task

…order

…es task for new components when installed with snap

…mplementation

…n RefreshOptions

…his PR

andrewphelpsj requested review from pedronis and alfonsosanchezbeato June 17, 2024 21:20

andrewphelpsj force-pushed the install-component-from-store branch from 432a5bc to 6c002e5 Compare June 17, 2024 21:33

andrewphelpsj mentioned this pull request Jun 18, 2024

o/snapstate: install components and snaps from file #14095

Merged

alfonsosanchezbeato reviewed Jun 18, 2024

View reviewed changes

andrewphelpsj requested a review from alfonsosanchezbeato June 18, 2024 16:30

pedronis reviewed Jun 20, 2024

View reviewed changes

andrewphelpsj commented Jun 20, 2024

View reviewed changes

andrewphelpsj requested a review from pedronis June 20, 2024 18:10

pedronis reviewed Jun 21, 2024

View reviewed changes

andrewphelpsj requested a review from pedronis June 21, 2024 18:15

alfonsosanchezbeato approved these changes Jun 21, 2024

View reviewed changes

pedronis approved these changes Jun 24, 2024

View reviewed changes

andrewphelpsj force-pushed the install-component-from-store branch from 2e9883d to 7e5c074 Compare June 24, 2024 15:10

andrewphelpsj added 14 commits June 25, 2024 16:01

o/snapstate: replace some args to doInstallComponent with a flags type

af15f20

o/snapstate: create validate-component task when installing component…

0df0a6a

… from the store

o/snapstate: add arg to doInstall for components

3201a16

o/snapstate: install components from store in StoreTarget

111e4d8

o/snapstate: add component fields to fakeSnappyBackend ops

46e2970

o/snapstate: add helpers for checking tasks for installing components

e3d653c

o/snapstate: add testing channel that adds a component to snap that i…

109b029

…s being installed

o/snapstate: use instance name in path to that is set for remove ops …

d4c8b86

…in test backend

o/snapstate: extend doInstall to handle components

bef60d3

o/snapstate: refactor addTask in doInstall to update prev pointer to …

bdcbf7b

…eliminate manual tracking of last task

o/snapstate: test tasks created when installing component from the store

2c9920d

o/snapstate: remove component files when snap file is also ephemeral

fbc4392

o/snapstate: change component types in error strings to be more clear

230a849

o/snapstate: always install components for each snap in a consistent …

e1b50f3

…order

andrewphelpsj added 13 commits June 25, 2024 16:01

o/snapstate: replace pointer params to doInstall with copies

f3bbbdb

o/snapstate: unexport componentTarget

78896a9

o/snapstate: add TODOs about handling components

217c9a0

o/snapstate: add comment explaining why we do not create setup-profil…

09d783d

…es task for new components when installed with snap

o/snapstate: remove usage of new go stdlib function

50f175e

o/snapstate: fully initialize ComponentSetup in store's InstallGoal i…

32fab00

…mplementation

o/snapstate: rename componentInstallFlags.SkipSecurity to SkipProfiles

b349603

o/snapstate: move componentTarget struct

3ad8551

o/snapstate: eliminate componentTarget type

dceb95a

store, o/snapstate: swap out resources slice in SnapAction for flag i…

289bff7

…n RefreshOptions

o/snapstate: remove this change for now, it should not be a part of t…

4da6b71

…his PR

o/snapstate: add TODO about ordering component and snap hook tasks

c36a3a1

o/snapstate: rename variable for readability

6c61b19

andrewphelpsj force-pushed the install-component-from-store branch from 7e5c074 to 6c61b19 Compare June 25, 2024 20:01

alfonsosanchezbeato merged commit 411ac6c into canonical:master Jun 26, 2024
45 of 51 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

o/snapstate: install components from the store #14092

o/snapstate: install components from the store #14092

andrewphelpsj commented Jun 17, 2024

alfonsosanchezbeato left a comment •

edited

Loading

alfonsosanchezbeato Jun 18, 2024

andrewphelpsj Jun 18, 2024

alfonsosanchezbeato Jun 18, 2024

alfonsosanchezbeato Jun 18, 2024

alfonsosanchezbeato Jun 18, 2024

andrewphelpsj Jun 18, 2024 •

edited

Loading

alfonsosanchezbeato Jun 18, 2024

andrewphelpsj Jun 18, 2024

alfonsosanchezbeato Jun 18, 2024

andrewphelpsj Jun 18, 2024

alfonsosanchezbeato Jun 18, 2024

andrewphelpsj commented Jun 18, 2024

alfonsosanchezbeato commented Jun 18, 2024 •

edited

Loading

pedronis left a comment

pedronis Jun 20, 2024

andrewphelpsj Jun 20, 2024

pedronis Jun 20, 2024

andrewphelpsj Jun 20, 2024

andrewphelpsj Jun 20, 2024

pedronis left a comment •

edited

Loading

pedronis Jun 21, 2024

andrewphelpsj Jun 21, 2024

alfonsosanchezbeato Jun 21, 2024

alfonsosanchezbeato left a comment

alfonsosanchezbeato Jun 21, 2024

alfonsosanchezbeato Jun 21, 2024

pedronis left a comment

o/snapstate: install components from the store #14092

o/snapstate: install components from the store #14092

Conversation

andrewphelpsj commented Jun 17, 2024

alfonsosanchezbeato left a comment • 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

andrewphelpsj Jun 18, 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

andrewphelpsj commented Jun 18, 2024

alfonsosanchezbeato commented Jun 18, 2024 • edited Loading

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 • 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

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

pedronis left a comment

Choose a reason for hiding this comment

alfonsosanchezbeato left a comment •

edited

Loading

andrewphelpsj Jun 18, 2024 •

edited

Loading

alfonsosanchezbeato commented Jun 18, 2024 •

edited

Loading

pedronis left a comment •

edited

Loading