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

overlord/snapstate/backend_test.go

@@ -523,7 +523,7 @@ func (f *fakeStore) lookupRefresh(cand refreshCand) (*snap.Info, error) {
 		confinement = snap.ClassicConfinement
 	case "channel-for-devmode/stable":
 		confinement = snap.DevModeConfinement
-	case "channel-for-components":
+	case "channel-for-components", "channel-for-components-only-component-refresh":
 		components = map[string]*snap.Component{
 			"test-component": {
 				Type: snap.TestComponent,
@@ -628,6 +628,12 @@ func (f *fakeStore) lookupRefresh(cand refreshCand) (*snap.Info, error) {
 		return info, nil
 	}
 
+	// this is a special case for testing the case where we only refresh
+	// component revisions
+	if cand.channel == "channel-for-components-only-component-refresh" {
+		return info, nil
+	}
+
 	return nil, store.ErrNoUpdateAvailable
 }
 

overlord/snapstate/component.go

@@ -256,18 +256,18 @@ type componentInstallFlags struct {
 }
 
 type componentInstallTaskSet struct {
-	compSetupTaskID    string
-	beforeLink         []*state.Task
-	linkTask           *state.Task
-	postOpHookAndAfter []*state.Task
-	discardTask        *state.Task
+	compSetupTaskID        string
+	beforeLinkTasks        []*state.Task
+	linkTask               *state.Task
+	postHookToDiscardTasks []*state.Task
+	discardTask            *state.Task
 }
 
 func (c *componentInstallTaskSet) taskSet() *state.TaskSet {
-	tasks := make([]*state.Task, 0, len(c.beforeLink)+1+len(c.postOpHookAndAfter)+1)
-	tasks = append(tasks, c.beforeLink...)
+	tasks := make([]*state.Task, 0, len(c.beforeLinkTasks)+1+len(c.postHookToDiscardTasks)+1)
+	tasks = append(tasks, c.beforeLinkTasks...)
 	tasks = append(tasks, c.linkTask)
-	tasks = append(tasks, c.postOpHookAndAfter...)
+	tasks = append(tasks, c.postHookToDiscardTasks...)
 	if c.discardTask != nil {
 		tasks = append(tasks, c.discardTask)
 	}
@@ -345,13 +345,13 @@ func doInstallComponent(st *state.State, snapst *SnapState, compSetup ComponentS
 		compSetupTaskID: prepare.ID(),
 	}
 
-	componentTS.beforeLink = append(componentTS.beforeLink, prepare)
+	componentTS.beforeLinkTasks = append(componentTS.beforeLinkTasks, prepare)
 
 	if fromStore {
 		validate := st.NewTask("validate-component", fmt.Sprintf(
 			i18n.G("Fetch and check assertions for component %q%s"), compSetup.ComponentName(), revisionStr),
 		)
-		componentTS.beforeLink = append(componentTS.beforeLink, validate)
+		componentTS.beforeLinkTasks = append(componentTS.beforeLinkTasks, validate)
 		addTask(validate)
 	}
 
@@ -360,7 +360,7 @@ func doInstallComponent(st *state.State, snapst *SnapState, compSetup ComponentS
 		mount := st.NewTask("mount-component",
 			fmt.Sprintf(i18n.G("Mount component %q%s"),
 				compSi.Component, revisionStr))
-		componentTS.beforeLink = append(componentTS.beforeLink, mount)
+		componentTS.beforeLinkTasks = append(componentTS.beforeLinkTasks, mount)
 		addTask(mount)
 	} else {
 		if compSetup.RemoveComponentPath {
@@ -379,7 +379,7 @@ func doInstallComponent(st *state.State, snapst *SnapState, compSetup ComponentS
 
 	if !snapsup.Revert && compInstalled {
 		preRefreshHook := SetupPreRefreshComponentHook(st, snapsup.InstanceName(), compSi.Component.ComponentName)
-		componentTS.beforeLink = append(componentTS.beforeLink, preRefreshHook)
+		componentTS.beforeLinkTasks = append(componentTS.beforeLinkTasks, preRefreshHook)
 		addTask(preRefreshHook)
 	}
 
@@ -396,7 +396,7 @@ func doInstallComponent(st *state.State, snapst *SnapState, compSetup ComponentS
 		unlink := st.NewTask("unlink-current-component", fmt.Sprintf(i18n.G(
 			"Make current revision for component %q unavailable"),
 			compSi.Component))
-		componentTS.beforeLink = append(componentTS.beforeLink, unlink)
+		componentTS.beforeLinkTasks = append(componentTS.beforeLinkTasks, unlink)
 		addTask(unlink)
 	}
 
@@ -405,7 +405,7 @@ func doInstallComponent(st *state.State, snapst *SnapState, compSetup ComponentS
 		setupSecurity = st.NewTask("setup-profiles", fmt.Sprintf(i18n.G("Setup component %q%s security profiles"), compSi.Component, revisionStr))
 		setupSecurity.Set("component-setup-task", prepare.ID())
 		setupSecurity.Set("snap-setup-task", snapSetupTaskID)
-		componentTS.beforeLink = append(componentTS.beforeLink, setupSecurity)
+		componentTS.beforeLinkTasks = append(componentTS.beforeLinkTasks, setupSecurity)
 	}
 	if setupSecurity != nil {
 		// note that we don't use addTask here because this task is shared and
@@ -428,7 +428,7 @@ func doInstallComponent(st *state.State, snapst *SnapState, compSetup ComponentS
 	} else {
 		postOpHook = SetupPostRefreshComponentHook(st, snapsup.InstanceName(), compSi.Component.ComponentName)
 	}
-	componentTS.postOpHookAndAfter = append(componentTS.postOpHookAndAfter, postOpHook)
+	componentTS.postHookToDiscardTasks = append(componentTS.postHookToDiscardTasks, postOpHook)
 	addTask(postOpHook)
 
 	if !compSetup.MultiComponentInstall && kmodSetup == nil && compSetup.CompType == snap.KernelModulesComponent {
@@ -437,7 +437,7 @@ func doInstallComponent(st *state.State, snapst *SnapState, compSetup ComponentS
 				compSi.Component, revisionStr))
 		kmodSetup.Set("component-setup-task", prepare.ID())
 		kmodSetup.Set("snap-setup-task", snapSetupTaskID)
-		componentTS.postOpHookAndAfter = append(componentTS.postOpHookAndAfter, kmodSetup)
+		componentTS.postHookToDiscardTasks = append(componentTS.postHookToDiscardTasks, kmodSetup)
 	}
 	if kmodSetup != nil {
 		// note that we don't use addTask here because this task is shared and

overlord/snapstate/handlers.go

@@ -2714,7 +2714,8 @@ func (m *SnapManager) undoLinkSnap(t *state.Task, _ *tomb.Tomb) error {
 		return err
 	}
 
-	if len(snapst.Sequence.Revisions) == 1 {
+	firstInstall := oldCurrent.Unset()
+	if firstInstall {
 		// XXX: shouldn't these two just log and carry on? this is an undo handler...
 		timings.Run(perfTimings, "discard-snap-namespace", fmt.Sprintf("discard the namespace of snap %q", snapsup.InstanceName()), func(tm timings.Measurer) {
 			err = m.backend.DiscardSnapNamespace(snapsup.InstanceName())
@@ -2801,7 +2802,6 @@ func (m *SnapManager) undoLinkSnap(t *state.Task, _ *tomb.Tomb) error {
 	}
 
 	pb := NewTaskProgressAdapterLocked(t)
-	firstInstall := oldCurrent.Unset()
 	linkCtx := backend.LinkContext{
 		FirstInstall:      firstInstall,
 		IsUndo:            true,

overlord/snapstate/handlers_components.go

@@ -523,12 +523,16 @@
 	defer st.Unlock()
 
 	// snapSt is a copy of the current state
-	compSetup, _, snapSt, err := compSetupAndState(t)
+	compSetup, snapsup, snapSt, err := compSetupAndState(t)
 	if err != nil {
 		return err
 	}
 	cref := compSetup.CompSideInfo.Component
 
+	if err := saveCurrentKernelModuleComponents(t, snapsup, snapSt); err != nil {
+		return err
+	}
+
 	// Expected to be installed
 	snapInfo, err := snapSt.CurrentInfo()
 	if err != nil {
@@ -561,12 +565,6 @@
 		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 {
-		return err
-	}
-
 	cref := compSetup.CompSideInfo.Component
 	// Remove component for the specified revision
 	if err := m.unlinkComponent(

overlord/snapstate/handlers_components_link_test.go

@@ -289,7 +289,7 @@ func (s *linkCompSnapSuite) TestDoUnlinkCurrentComponent(c *C) {
 	setStateWithOneComponent(s.state, snapName, snapRev, compName, compRev)
 	s.state.Unlock()
 
-	s.testDoUnlinkComponent(c, snapName, snapRev, compName, compRev, "unlink-current-component", nil)
+	s.testDoUnlinkComponent(c, snapName, snapRev, compName, compRev, "unlink-current-component", []*snap.ComponentSideInfo{})
 }
 
 func (s *linkCompSnapSuite) TestDoUnlinkCurrentComponentOtherCompPresent(c *C) {
@@ -303,11 +303,11 @@ func (s *linkCompSnapSuite) TestDoUnlinkCurrentComponentOtherCompPresent(c *C) {
 	csi1 := snap.NewComponentSideInfo(naming.NewComponentRef(snapName, compName), compRev)
 	csi2 := snap.NewComponentSideInfo(naming.NewComponentRef(snapName, "other-comp"), snap.R(33))
 	cs1 := sequence.NewComponentState(csi1, snap.TestComponent)
-	cs2 := sequence.NewComponentState(csi2, snap.TestComponent)
+	cs2 := sequence.NewComponentState(csi2, snap.KernelModulesComponent)
 	setStateWithComponents(s.state, snapName, snapRev, []*sequence.ComponentState{cs1, cs2})
 	s.state.Unlock()
 
-	s.testDoUnlinkComponent(c, snapName, snapRev, compName, compRev, "unlink-current-component", nil)
+	s.testDoUnlinkComponent(c, snapName, snapRev, compName, compRev, "unlink-current-component", []*snap.ComponentSideInfo{cs2.SideInfo})
 }
 
 func (s *linkCompSnapSuite) TestDoUnlinkCurrentComponentTwoTasks(c *C) {
@@ -471,19 +471,13 @@ func (s *linkCompSnapSuite) TestDoUnlinkComponent(c *C) {
 
 	s.state.Lock()
 
-	kmodCsi := &snap.ComponentSideInfo{
-		Component: naming.NewComponentRef(snapName, "kmod-comp"),
-		Revision:  snap.R(10),
-	}
-
 	// State must contain the component. Note that in this case
 	// the snap does not need to be active.
 	ssi := &snap.SideInfo{RealName: snapName, Revision: snapRev,
 		SnapID: "some-snap-id"}
 	csi := snap.NewComponentSideInfo(naming.NewComponentRef(snapName, compName), compRev)
 	comps := []*sequence.ComponentState{
 		sequence.NewComponentState(csi, snap.TestComponent),
-		sequence.NewComponentState(kmodCsi, snap.KernelModulesComponent),
 	}
 	snapstate.Set(s.state, snapName, &snapstate.SnapState{
 		Active: false,
@@ -495,7 +489,7 @@ func (s *linkCompSnapSuite) TestDoUnlinkComponent(c *C) {
 
 	s.state.Unlock()
 
-	s.testDoUnlinkComponent(c, snapName, snapRev, compName, compRev, "unlink-component", []*snap.ComponentSideInfo{kmodCsi})
+	s.testDoUnlinkComponent(c, snapName, snapRev, compName, compRev, "unlink-component", nil)
 }
 
 func (s *linkCompSnapSuite) TestDoUnlinkThenUndoUnlinkComponent(c *C) {

overlord/snapstate/snapstate.go

@@ -453,8 +453,14 @@
 		addTask(t)
 	}
 
-	// run refresh hooks when updating existing snap, otherwise run install hook further down.
-	runRefreshHooks := snapst.IsInstalled() && !snapsup.Flags.Revert
+	// if the snap is already installed, and the revision we are refreshing to
+	// is the same as the current revision, and we're not forcing an update,
+	// then we know that we're really modifying the state of components.
+	componentOnlyUpdate := snapst.IsInstalled() && snapsup.Revision() == snapst.Current && !snapsup.AlwaysUpdate
+
+	// run refresh hooks when updating existing snap, otherwise run install hook
+	// further down.
+	runRefreshHooks := snapst.IsInstalled() && !componentOnlyUpdate && !snapsup.Flags.Revert
 	if runRefreshHooks {
 		preRefreshHook := SetupPreRefreshHook(st, snapsup.InstanceName())
 		addTask(preRefreshHook)
@@ -620,8 +626,6 @@
 	startSnapServices := st.NewTask("start-snap-services", fmt.Sprintf(i18n.G("Start snap %q%s services"), snapsup.InstanceName(), revisionStr))
 	addTask(startSnapServices)
 
-	// TODO:COMPS: test discarding components during a snap refresh (coming
-	// soon!)
 	for _, t := range tasksBeforeDiscard {
 		addTask(t)
 	}
@@ -774,9 +778,9 @@
 
 		compSetupIDs = append(compSetupIDs, componentTS.compSetupTaskID)
 
-		tasksBeforePreRefreshHook = append(tasksBeforePreRefreshHook, componentTS.beforeLink...)
+		tasksBeforePreRefreshHook = append(tasksBeforePreRefreshHook, componentTS.beforeLinkTasks...)
 		tasksAfterLinkSnap = append(tasksAfterLinkSnap, componentTS.linkTask)
-		tasksAfterPostOpHook = append(tasksAfterPostOpHook, componentTS.postOpHookAndAfter...)
+		tasksAfterPostOpHook = append(tasksAfterPostOpHook, componentTS.postHookToDiscardTasks...)
 		if componentTS.discardTask != nil {
 			tasksBeforeDiscard = append(tasksBeforeDiscard, componentTS.discardTask)
 		}
@@ -1876,17 +1880,35 @@
 	Components []ComponentSetup
 }
 
-// revisionSatisfied returns true if the state of the snap on the system matches the
-// state specified in the update. This method is primarily concerned with the
-// revision of the snap.
-//
-// TODO:COMPS: check if we need to change the state of components
-func (u *update) revisionSatisfied() bool {
+// revisionSatisfied returns true if the state of the snap on the system matches
+// the state specified in the update. This method checks if the currently
+// installed snap and components match what is specified in the update.
+func (u *update) revisionSatisfied() (bool, error) {
 	if u.Setup.AlwaysUpdate || !u.SnapState.IsInstalled() {
-		return false
+		return false, nil
+	}
+
+	if u.SnapState.Current != u.Setup.Revision() {
+		return false, nil
+	}
+
+	comps, err := u.SnapState.CurrentComponentInfos()
+	if err != nil {
+		return false, err
+	}
+
+	currentCompRevs := make(map[string]snap.Revision, len(comps))
+	for _, comp := range comps {
+		currentCompRevs[comp.Component.ComponentName] = comp.Revision
 	}
 
-	return u.SnapState.Current == u.Setup.Revision()
+	for _, comp := range u.Components {
+		if currentCompRevs[comp.CompSideInfo.Component.ComponentName] != comp.Revision() {
+			return false, nil
+		}
+	}
+
+	return true, nil
 }
 
 func doPotentiallySplitUpdate(st *state.State, requested []string, updates []update, opts Options) ([]string, *UpdateTaskSets, error) {
@@ -1991,7 +2013,12 @@
 	// and bases and then other snaps
 	for _, up := range updates {
 		// if the update is already satisfied, then we can skip it
-		if up.revisionSatisfied() {
+		ok, err := up.revisionSatisfied()
+		if err != nil {
+			return nil, false, nil, err
+		}
+
+		if ok {
 			alreadySatisfied = append(alreadySatisfied, up)
 			continue
 		}
@@ -2267,7 +2294,12 @@
 	// snaps with updates
 	updating := make(map[string]bool, len(updates))
 	for _, up := range updates {
-		updating[up.Setup.InstanceName()] = !up.revisionSatisfied()
+		ok, err := up.revisionSatisfied()
+		if err != nil {
+			return nil, nil, nil, err
+		}
+
+		updating[up.Setup.InstanceName()] = !ok
 	}
 
 	// add explicitly auto-aliases only for snaps that are not updated

overlord/snapstate/snapstate_install_test.go

@@ -6430,8 +6430,22 @@ func undoOps(instanceName string, newSequence, prevSequence *sequence.RevisionSi
 	for i := len(newComponents) - 1; i >= 0; i-- {
 		csi := newComponents[i].SideInfo
 
-		containerName := fmt.Sprintf("%s+%s", instanceName, csi.Component.ComponentName)
-		filename := fmt.Sprintf("%s_%v.comp", containerName, csi.Revision)
+		compName := csi.Component.ComponentName
+		compRev := csi.Revision
+
+		containerName := fmt.Sprintf("%s+%s", instanceName, compName)
+		filename := fmt.Sprintf("%s_%v.comp", containerName, compRev)
+
+		// if the snap revision isn't changing, then we need to re-link the old
+		// component
+		if snapRevision == prevRevision {
+			oldCS := prevSequence.FindComponent(csi.Component)
+
+			ops = append(ops, []fakeOp{{
+				op:   "link-component",
+				path: snap.ComponentMountDir(compName, oldCS.SideInfo.Revision, instanceName),
+			}}...)
+		}
 
 		ops = append(ops, []fakeOp{{
 			op:                "undo-setup-component",
@@ -6444,16 +6458,18 @@ func undoOps(instanceName string, newSequence, prevSequence *sequence.RevisionSi
 		}}...)
 	}
 
-	ops = append(ops, []fakeOp{{
-		op:    "undo-setup-snap",
-		name:  instanceName,
-		stype: "app",
-		path:  snapMount,
-	}, {
-		op:   "remove-snap-dir",
-		name: instanceName,
-		path: filepath.Join(dirs.SnapMountDir, instanceName),
-	}}...)
+	if snapRevision != prevRevision {
+		ops = append(ops, []fakeOp{{
+			op:    "undo-setup-snap",
+			name:  instanceName,
+			stype: "app",
+			path:  snapMount,
+		}, {
+			op:   "remove-snap-dir",
+			name: instanceName,
+			path: filepath.Join(dirs.SnapMountDir, instanceName),
+		}}...)
+	}
 
 	return ops
 }