o/snapstate: properly handle components when refreshing to revision t…

…hat has been on the system before (#14498)

overlord/snapstate/backend_test.go

@@ -518,6 +518,8 @@ func (f *fakeStore) lookupRefresh(cand refreshCand) (*snap.Info, error) {
 		base = "some-base"
 	case "provenance-snap-id":
 		name = "provenance-snap"
+	case "snap-with-components-id":
+		name = "snap-with-components"
 	default:
 		panic(fmt.Sprintf("refresh: unknown snap-id: %s", cand.snapID))
 	}
@@ -545,6 +547,14 @@ func (f *fakeStore) lookupRefresh(cand refreshCand) (*snap.Info, error) {
 				Type: snap.KernelModulesComponent,
 				Name: "kernel-modules-component",
 			},
+			"test-component-extra": {
+				Type: snap.TestComponent,
+				Name: "test-component-extra",
+			},
+			"test-component-present-in-sequence": {
+				Type: snap.TestComponent,
+				Name: "test-component-present-in-sequence",
+			},
 		}
 	}
 	if name == "some-snap-now-classic" {
@@ -1130,6 +1140,17 @@ func (f *fakeSnappyBackend) ReadInfo(name string, si *snap.SideInfo) (*snap.Info
 		info.SnapType = snap.TypeOS
 	case "snapd":
 		info.SnapType = snap.TypeSnapd
+	case "snap-with-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 "services-snap":
 		var err error
 		// fix services after/before so that there is only one solution

overlord/snapstate/component.go

@@ -55,7 +55,7 @@ func InstallComponents(ctx context.Context, st *state.State, names []string, inf
 		}
 	}
 
-	compsups, err := componentSetupsForInstall(ctx, st, names, info, opts)
+	compsups, err := componentSetupsForInstall(ctx, st, names, snapst, snapst.Current, snapst.TrackingChannel, opts)
 	if err != nil {
 		return nil, err
 	}
@@ -111,14 +111,9 @@ func InstallComponents(ctx context.Context, st *state.State, names []string, inf
 	return append(tss, ts), nil
 }
 
-func componentSetupsForInstall(ctx context.Context, st *state.State, names []string, info *snap.Info, opts Options) ([]ComponentSetup, error) {
-	var snapst SnapState
-	err := Get(st, info.InstanceName(), &snapst)
-	if err != nil {
-		if errors.Is(err, state.ErrNoState) {
-			return nil, &snap.NotInstalledError{Snap: info.InstanceName()}
-		}
-		return nil, err
+func componentSetupsForInstall(ctx context.Context, st *state.State, names []string, snapst SnapState, snapRev snap.Revision, channel string, opts Options) ([]ComponentSetup, error) {
+	if len(names) == 0 {
+		return nil, nil
 	}
 
 	current, err := currentSnaps(st)
@@ -132,7 +127,7 @@ func componentSetupsForInstall(ctx context.Context, st *state.State, names []str
 		return nil, err
 	}
 
-	action, err := installComponentAction(st, snapst, opts)
+	action, err := installComponentAction(st, snapst, snapRev, channel, opts)
 	if err != nil {
 		return nil, err
 	}
@@ -159,11 +154,12 @@ func componentSetupsForInstall(ctx context.Context, st *state.State, names []str
 	return componentTargetsFromActionResult("install", sars[0], names)
 }
 
-func installComponentAction(st *state.State, snapst SnapState, opts Options) (*store.SnapAction, error) {
-	si := snapst.CurrentSideInfo()
-	if si == nil {
-		return nil, errors.New("internal error: cannot install components for a snap that is not installed")
+func installComponentAction(st *state.State, snapst SnapState, snapRev snap.Revision, channel string, opts Options) (*store.SnapAction, error) {
+	index := snapst.LastIndex(snapRev)
+	if index == -1 {
+		return nil, fmt.Errorf("internal error: cannot find snap revision %s in sequence", snapRev)
 	}
+	si := snapst.Sequence.SideInfos()[index]
 
 	if si.SnapID == "" {
 		return nil, errors.New("internal error: cannot install components for a snap that is unknown to the store")
@@ -184,8 +180,8 @@ func installComponentAction(st *state.State, snapst SnapState, opts Options) (*s
 	// that we make sure to get back components that are compatible with the
 	// currently installed snap
 	revOpts := RevisionOptions{
-		Channel:  snapst.TrackingChannel,
-		Revision: snapst.Current,
+		Revision: si.Revision,
+		Channel:  channel,
 	}
 
 	// TODO:COMPS: handle validation sets here

overlord/snapstate/snapstate.go

@@ -53,6 +53,7 @@ import (
 	"github.com/snapcore/snapd/release"
 	"github.com/snapcore/snapd/snap"
 	"github.com/snapcore/snapd/snap/channel"
+	"github.com/snapcore/snapd/snap/naming"
 	"github.com/snapcore/snapd/snapdenv"
 	"github.com/snapcore/snapd/store"
 	"github.com/snapcore/snapd/strutil"
@@ -274,6 +275,75 @@ func isCoreSnap(snapName string) bool {
 	return snapName == defaultCoreSnapName
 }
 
+// removeExtraComponentsTasks creates tasks that will remove unwanted components
+// that are currently installed alongside the target snap revision. If the new
+// snap revision is not in the sequence, then we don't have anything to do. If
+// the revision is in the sequence, then we generate tasks that will unlink
+// components that are not in compsups.
+//
+// This is mostly relevant when we're moving from one snap revision to another
+// snap revision that was installed in past and is still in the sequence. The
+// target snap might have had components that were installed alongside it in the
+// past, and they are not wanted anymore.
+func removeExtraComponentsTasks(st *state.State, snapst *SnapState, targetRevision snap.Revision, compsups []ComponentSetup) (
+	unlinkTasks, discardTasks []*state.Task, err error,
+) {
+	idx := snapst.LastIndex(targetRevision)
+	if idx < 0 {
+		return nil, nil, nil
+	}
+
+	keep := make(map[naming.ComponentRef]bool, len(compsups))
+	for _, compsup := range compsups {
+		keep[compsup.CompSideInfo.Component] = true
+	}
+
+	linkedForRevision, err := snapst.ComponentInfosForRevision(targetRevision)
+	if err != nil {
+		return nil, nil, err
+	}
+
+	for _, ci := range linkedForRevision {
+		if keep[ci.Component] {
+			continue
+		}
+
+		// note that we shouldn't ever be able to lose components during a
+		// refresh without a snap revision change. this might be able to happen
+		// once we introduce components and validation sets? if that is the
+		// case, we'll need to take care here to use "unlink-current-component"
+		// and point it to the correct snap setup task.
+		if snapst.Current == targetRevision {
+			return nil, nil, errors.New("internal error: cannot lose a component during a refresh without a snap revision change")
+		}
+
+		// note that we don't need to worry about kernel module components here,
+		// since the components that we are removing are not associated with the
+		// current snap revision. unlink-component differs from
+		// unlink-current-component in that it doesn't save the state of kernel
+		// module components on the the SnapSetup.
+		unlink := st.NewTask("unlink-component", fmt.Sprintf(
+			i18n.G("Unlink component %q for snap revision %s"), ci.Component, targetRevision,
+		))
+
+		unlink.Set("component-setup", ComponentSetup{
+			CompSideInfo: &ci.ComponentSideInfo,
+			CompType:     ci.Type,
+		})
+		unlinkTasks = append(unlinkTasks, unlink)
+
+		if !snapst.Sequence.IsComponentRevInRefSeqPtInAnyOtherSeqPt(ci.Component, idx) {
+			discard := st.NewTask("discard-component", fmt.Sprintf(
+				i18n.G("Discard previous revision for component %q"), ci.Component,
+			))
+			discard.Set("component-setup-task", unlink.ID())
+			discardTasks = append(discardTasks, discard)
+		}
+	}
+
+	return unlinkTasks, discardTasks, nil
+}
+
 func doInstall(st *state.State, snapst *SnapState, snapsup SnapSetup, compsups []ComponentSetup, flags int, fromChange string, inUseCheck func(snap.Type) (boot.InUseFunc, error)) (*state.TaskSet, error) {
 	tr := config.NewTransaction(st)
 	experimentalRefreshAppAwareness, err := features.Flag(tr, features.RefreshAppAwareness)
@@ -443,13 +513,24 @@ func doInstall(st *state.State, snapst *SnapState, snapsup SnapSetup, compsups [
 		}
 	}
 
+	removeExtraComps, discardExtraComps, err := removeExtraComponentsTasks(st, snapst, snapsup.Revision(), compsups)
+	if err != nil {
+		return nil, err
+	}
+
+	for _, t := range removeExtraComps {
+		addTask(t)
+	}
+
 	tasksBeforePreRefreshHook, tasksAfterLinkSnap, tasksAfterPostOpHook, tasksBeforeDiscard, compSetupIDs, err := splitComponentTasksForInstall(
 		compsups, st, snapst, snapsup, prepare.ID(), fromChange,
 	)
 	if err != nil {
 		return nil, err
 	}
 
+	tasksBeforeDiscard = append(tasksBeforeDiscard, discardExtraComps...)
+
 	for _, t := range tasksBeforePreRefreshHook {
 		addTask(t)
 	}