o/snapstate: install components from the store (#14092)

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

* o/snapstate: create validate-component task when installing component from the store

* o/snapstate: add arg to doInstall for components

* o/snapstate: install components from store in StoreTarget

* o/snapstate: add component fields to fakeSnappyBackend ops

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

* o/snapstate: add testing channel that adds a component to snap that is being installed

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

* o/snapstate: extend doInstall to handle components

* o/snapstate: refactor addTask in doInstall to update prev pointer to eliminate manual tracking of last task

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

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

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

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

* o/snapstate: replace pointer params to doInstall with copies

* o/snapstate: unexport componentTarget

* o/snapstate: add TODOs about handling components

* o/snapstate: add comment explaining why we do not create setup-profiles task for new components when installed with snap

* o/snapstate: remove usage of new go stdlib function

* o/snapstate: fully initialize ComponentSetup in store's InstallGoal implementation

* o/snapstate: rename componentInstallFlags.SkipSecurity to SkipProfiles

* o/snapstate: move componentTarget struct

* o/snapstate: eliminate componentTarget type

* store, o/snapstate: swap out resources slice in SnapAction for flag in RefreshOptions

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

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

* o/snapstate: rename variable for readability

overlord/snapstate/backend_test.go

@@ -60,6 +60,11 @@ type fakeOp struct {
 	sinfo snap.SideInfo
 	stype snap.Type
 
+	componentName     string
+	componentPath     string
+	componentRev      snap.Revision
+	componentSideInfo snap.ComponentSideInfo
+
 	curSnaps []store.CurrentSnap
 	action   store.SnapAction
 
@@ -328,6 +333,17 @@ func (f *fakeStore) snap(spec snapSpec) (*snap.Info, error) {
 				DaemonScope: "user",
 			},
 		}
+	case "channel-for-components":
+		info.Components = map[string]*snap.Component{
+			"test-component": {
+				Type: snap.TestComponent,
+				Name: "test-component",
+			},
+			"kernel-modules-component": {
+				Type: snap.KernelModulesComponent,
+				Name: "kernel-modules-component",
+			},
+		}
 	case "channel-for-dbus-activation":
 		slot := &snap.SlotInfo{
 			Snap:      info,
@@ -658,10 +674,14 @@ func (f *fakeStore) SnapAction(ctx context.Context, currentSnaps []*store.Curren
 				info.Channel = ""
 			}
 			info.InstanceKey = instanceKey
+
 			sar := store.SnapActionResult{
-				Info:      info,
-				Resources: f.snapResources(info),
+				Info: info,
+			}
+			if opts.IncludeResources {
+				sar.Resources = f.snapResources(info)
 			}
+
 			if strings.HasSuffix(snapName, "-with-default-track") && strutil.ListContains([]string{"stable", "candidate", "beta", "edge"}, a.Channel) {
 				sar.RedirectChannel = "2.0/" + a.Channel
 			}
@@ -1371,7 +1391,7 @@ func (f *fakeSnappyBackend) RemoveSnapDataDir(info *snap.Info, otherInstances bo
 	f.appendOp(&fakeOp{
 		op:             "remove-snap-data-dir",
 		name:           info.InstanceName(),
-		path:           snap.BaseDataDir(info.SnapName()),
+		path:           snap.BaseDataDir(info.InstanceName()),
 		otherInstances: otherInstances,
 	})
 	return f.maybeErrForLastOp()
@@ -1389,7 +1409,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()),
 		otherInstances: otherInstances,
 	})
 	return f.maybeErrForLastOp()
@@ -1440,12 +1460,21 @@ func (f *fakeSnappyBackend) CurrentInfo(curInfo *snap.Info) {
 	})
 }
 
-func (f *fakeSnappyBackend) ForeignTask(kind string, status state.Status, snapsup *snapstate.SnapSetup) error {
-	f.appendOp(&fakeOp{
+func (f *fakeSnappyBackend) ForeignTask(kind string, status state.Status, snapsup *snapstate.SnapSetup, compsup *snapstate.ComponentSetup) error {
+	op := &fakeOp{
 		op:    kind + ":" + status.String(),
 		name:  snapsup.InstanceName(),
 		revno: snapsup.Revision(),
-	})
+	}
+
+	if compsup != nil {
+		op.componentName = compsup.ComponentName()
+		op.componentPath = compsup.CompPath
+		op.componentRev = compsup.Revision()
+		op.componentSideInfo = *compsup.CompSideInfo
+	}
+
+	f.appendOp(op)
 	return f.maybeErrForLastOp()
 }
 

overlord/snapstate/component.go

@@ -71,15 +71,21 @@ func InstallComponentPath(st *state.State, csi *snap.ComponentSideInfo, info *sn
 		CompType:     compInfo.Type,
 		CompPath:     path,
 	}
-	// The file passed around is temporary, make sure it gets removed.
-	// TODO probably this should be part of a flags type in the future.
-	removeComponentPath := true
-	return doInstallComponent(st, &snapst, compSetup, snapsup, removeComponentPath, "")
+
+	return doInstallComponent(st, &snapst, compSetup, snapsup, componentInstallFlags{
+		// The file passed around is temporary, make sure it gets removed.
+		RemoveComponentPath: true,
+	}, "")
+}
+
+type componentInstallFlags struct {
+	RemoveComponentPath bool
+	SkipProfiles        bool
 }
 
 // doInstallComponent might be called with the owner snap installed or not.
 func doInstallComponent(st *state.State, snapst *SnapState, compSetup *ComponentSetup,
-	snapsup *SnapSetup, removeComponentPath bool, fromChange string) (*state.TaskSet, error) {
+	snapsup *SnapSetup, flags componentInstallFlags, fromChange string) (*state.TaskSet, error) {
 
 	// TODO check for experimental flag that will hide temporarily components
 
@@ -100,23 +106,23 @@ func doInstallComponent(st *state.State, snapst *SnapState, compSetup *Component
 
 	// Check if we already have the revision in the snaps folder (alters tasks).
 	// Note that this will search for all snap revisions in the system.
-	revisionIsPresent := snapst.IsComponentRevPresent(compSi) == true
+	revisionIsPresent := snapst.IsComponentRevPresent(compSi)
 	revisionStr := fmt.Sprintf(" (%s)", compSi.Revision)
 
-	var prepare, prev *state.Task
+	fromStore := compSetup.CompPath == "" && !revisionIsPresent
+
+	var prepare *state.Task
 	// if we have a local revision here we go back to that
-	if compSetup.CompPath != "" || revisionIsPresent {
-		prepare = st.NewTask("prepare-component",
-			fmt.Sprintf(i18n.G("Prepare component %q%s"),
-				compSetup.CompPath, revisionStr))
+	if fromStore {
+		prepare = st.NewTask("download-component", fmt.Sprintf(i18n.G("Download component %q%s"), compSetup.ComponentName(), revisionStr))
 	} else {
-		prepare = st.NewTask("download-component", fmt.Sprintf(i18n.G("Download component %q%s"), compSetup, revisionStr))
+		prepare = st.NewTask("prepare-component", fmt.Sprintf(i18n.G("Prepare component %q%s"), compSetup.CompPath, revisionStr))
 	}
 	prepare.Set("component-setup", compSetup)
 	prepare.Set("snap-setup", snapsup)
 
 	tasks := []*state.Task{prepare}
-	prev = prepare
+	prev := prepare
 
 	addTask := func(t *state.Task) {
 		t.Set("component-setup-task", prepare.ID())
@@ -126,8 +132,12 @@ func doInstallComponent(st *state.State, snapst *SnapState, compSetup *Component
 		prev = t
 	}
 
-	// TODO task to fetch and check assertions for component if from store
-	// (equivalent to "validate-snap")
+	if fromStore {
+		validate := st.NewTask("validate-component", fmt.Sprintf(
+			i18n.G("Fetch and check assertions for component %q%s"), compSetup.ComponentName(), revisionStr),
+		)
+		addTask(validate)
+	}
 
 	// Task that copies the file and creates mount units
 	if !revisionIsPresent {
@@ -136,7 +146,7 @@ func doInstallComponent(st *state.State, snapst *SnapState, compSetup *Component
 				compSi.Component, revisionStr))
 		addTask(mount)
 	} else {
-		if removeComponentPath {
+		if flags.RemoveComponentPath {
 			// If the revision is local, we will not need the
 			// temporary snap. This can happen when e.g.
 			// side-loading a local revision again. The path is
@@ -167,9 +177,10 @@ func doInstallComponent(st *state.State, snapst *SnapState, compSetup *Component
 	}
 
 	// security
-	setupSecurity := st.NewTask("setup-profiles", fmt.Sprintf(i18n.G("Setup component %q%s security profiles"), compSi.Component, revisionStr))
-	addTask(setupSecurity)
-	prev = setupSecurity
+	if !flags.SkipProfiles {
+		setupSecurity := st.NewTask("setup-profiles", fmt.Sprintf(i18n.G("Setup component %q%s security profiles"), compSi.Component, revisionStr))
+		addTask(setupSecurity)
+	}
 
 	// finalize (sets SnapState)
 	linkSnap := st.NewTask("link-component",

overlord/snapstate/component_install_test.go

@@ -45,6 +45,8 @@ const (
 	compTypeIsKernMods
 	// Current component is discarded at the end
 	compCurrentIsDiscarded
+	// Component is being installed with a snap, so skip setup-profiles
+	compOptSkipSecurity
 )
 
 // opts is a bitset with compOpt* as possible values.
@@ -54,8 +56,9 @@ func expectedComponentInstallTasks(opts int) []string {
 	if opts&compOptIsLocal != 0 {
 		startTasks = []string{"prepare-component"}
 	} else {
-		startTasks = []string{"download-component"}
+		startTasks = []string{"download-component", "validate-component"}
 	}
+
 	// Revision is not the same as the current one installed
 	if opts&compOptRevisionPresent == 0 {
 		startTasks = append(startTasks, "mount-component")
@@ -68,7 +71,9 @@ func expectedComponentInstallTasks(opts int) []string {
 		startTasks = append(startTasks, "unlink-current-component")
 	}
 
-	startTasks = append(startTasks, "setup-profiles")
+	if opts&compOptSkipSecurity == 0 {
+		startTasks = append(startTasks, "setup-profiles")
+	}
 
 	// link-component is always present
 	startTasks = append(startTasks, "link-component")