From 112e58d32851886ac7af9fa08addb027660e8380 Mon Sep 17 00:00:00 2001 From: Andrew Phelps <136256549+andrewphelpsj@users.noreply.github.com> Date: Thu, 19 Sep 2024 22:35:01 -0400 Subject: [PATCH] o/snapstate: properly handle components when refreshing to revision that has been on the system before (#14498) --- overlord/snapstate/backend_test.go | 21 ++ overlord/snapstate/component.go | 28 +- overlord/snapstate/snapstate.go | 81 ++++ overlord/snapstate/snapstate_update_test.go | 398 ++++++++++++++++++-- overlord/snapstate/storehelpers.go | 43 ++- 5 files changed, 514 insertions(+), 57 deletions(-) diff --git a/overlord/snapstate/backend_test.go b/overlord/snapstate/backend_test.go index 624a8572511..d2de65ee39b 100644 --- a/overlord/snapstate/backend_test.go +++ b/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 diff --git a/overlord/snapstate/component.go b/overlord/snapstate/component.go index 1fbf0560d67..7efdcd4b444 100644 --- a/overlord/snapstate/component.go +++ b/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 diff --git a/overlord/snapstate/snapstate.go b/overlord/snapstate/snapstate.go index 061af2fbe1a..04f576cd8c0 100644 --- a/overlord/snapstate/snapstate.go +++ b/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,6 +513,15 @@ 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, ) @@ -450,6 +529,8 @@ func doInstall(st *state.State, snapst *SnapState, snapsup SnapSetup, compsups [ return nil, err } + tasksBeforeDiscard = append(tasksBeforeDiscard, discardExtraComps...) + for _, t := range tasksBeforePreRefreshHook { addTask(t) } diff --git a/overlord/snapstate/snapstate_update_test.go b/overlord/snapstate/snapstate_update_test.go index 9452c7ada10..bc12421b91f 100644 --- a/overlord/snapstate/snapstate_update_test.go +++ b/overlord/snapstate/snapstate_update_test.go @@ -13899,22 +13899,18 @@ func (s *snapmgrTestSuite) TestUpdateStateConflictRemoved(c *C) { c.Assert(err, ErrorMatches, `snap "some-snap" has changes in progress`) } -func (s *snapmgrTestSuite) TestUpdateWithComponentsBackToPrevRevision(c *C) { +func (s *snapmgrTestSuite) TestUpdateBackToPrevRevision(c *C) { const ( snapName = "some-snap" instanceKey = "key" snapID = "some-snap-id" - channel = "channel-for-components" + channel = "some-channel" ) - components := []string{"test-component", "kernel-modules-component"} - currentSnapRev := snap.R(11) prevSnapRev := snap.R(7) instanceName := snap.InstanceName(snapName, instanceKey) - sort.Strings(components) - s.fakeStore.snapResourcesFn = func(info *snap.Info) []store.SnapResourceResult { c.Fatalf("unexpected call to snapResourcesFn") return nil @@ -13937,9 +13933,8 @@ func (s *snapmgrTestSuite) TestUpdateWithComponentsBackToPrevRevision(c *C) { s.state.Lock() defer s.state.Unlock() - // right now, we don't expect to hit the store for this case. we might if we - // choose to start checking the store for an updated list of compatible - // components. + // since we don't have any components to check on, we shouldn't hit the + // store at all snapstate.ReplaceStore(s.state, &storetest.Store{}) if instanceKey != "" { @@ -13957,8 +13952,207 @@ func (s *snapmgrTestSuite) TestUpdateWithComponentsBackToPrevRevision(c *C) { seq := snapstatetest.NewSequenceFromSnapSideInfos([]*snap.SideInfo{&prevSI, ¤tSI}) + snapstate.Set(s.state, instanceName, &snapstate.SnapState{ + Active: true, + Sequence: seq, + Current: currentSI.Revision, + SnapType: "app", + TrackingChannel: channel, + InstanceKey: instanceKey, + }) + + ts, err := snapstate.Update(s.state, instanceName, &snapstate.RevisionOptions{ + Revision: prevSnapRev, + }, s.user.ID, snapstate.Flags{}) + c.Assert(err, IsNil) + + chg := s.state.NewChange("refresh", "refresh a snap") + chg.AddAll(ts) + + // check unlink-reason + unlinkTask := findLastTask(chg, "unlink-current-snap") + c.Assert(unlinkTask, NotNil) + var unlinkReason string + unlinkTask.Get("unlink-reason", &unlinkReason) + c.Check(unlinkReason, Equals, "refresh") + + // local modifications, edge must be set + te := ts.MaybeEdge(snapstate.LastBeforeLocalModificationsEdge) + c.Assert(te, NotNil) + c.Assert(te.Kind(), Equals, "prepare-snap") + + s.settle(c) + + c.Assert(chg.Err(), IsNil, Commentf("change tasks:\n%s", printTasks(chg.Tasks()))) + + expected := fakeOps{ + { + op: "remove-snap-aliases", + name: instanceName, + }, + { + op: "run-inhibit-snap-for-unlink", + name: instanceName, + inhibitHint: "refresh", + }, + { + op: "unlink-snap", + path: filepath.Join(dirs.SnapMountDir, instanceName, currentSnapRev.String()), + }, + { + op: "copy-data", + path: filepath.Join(dirs.SnapMountDir, instanceName, prevSnapRev.String()), + old: filepath.Join(dirs.SnapMountDir, instanceName, currentSnapRev.String()), + }, + { + op: "setup-snap-save-data", + path: filepath.Join(dirs.SnapDataSaveDir, instanceName), + }, + { + op: "setup-profiles:Doing", + name: instanceName, + revno: prevSnapRev, + }, + { + op: "candidate", + sinfo: snap.SideInfo{ + RealName: snapName, + SnapID: snapID, + Channel: channel, + Revision: prevSnapRev, + }, + }, + { + op: "link-snap", + path: filepath.Join(dirs.SnapMountDir, instanceName, prevSnapRev.String()), + }, + { + op: "auto-connect:Doing", + name: instanceName, + revno: prevSnapRev, + }, + { + op: "update-aliases", + }, + { + op: "cleanup-trash", + name: instanceName, + revno: prevSnapRev, + }, + } + + // start with an easier-to-read error if this fails: + c.Assert(s.fakeBackend.ops.Ops(), DeepEquals, expected.Ops()) + c.Assert(s.fakeBackend.ops, DeepEquals, expected) + + task := ts.Tasks()[1] + + // verify snapSetup info + var snapsup snapstate.SnapSetup + err = task.Get("snap-setup", &snapsup) + c.Assert(err, IsNil) + c.Assert(snapsup, DeepEquals, snapstate.SnapSetup{ + Channel: channel, + UserID: s.user.ID, + + SnapPath: filepath.Join(dirs.SnapBlobDir, fmt.Sprintf("%s_%v.snap", instanceName, prevSnapRev)), + SideInfo: snapsup.SideInfo, + Type: snap.TypeApp, + Version: "some-snapVer", + PlugsOnly: true, + Flags: snapstate.Flags{ + Transaction: client.TransactionPerSnap, + }, + InstanceKey: instanceKey, + PreUpdateKernelModuleComponents: []*snap.ComponentSideInfo{}, + }) + c.Assert(snapsup.SideInfo, DeepEquals, &snap.SideInfo{ + RealName: snapName, + Revision: prevSnapRev, + Channel: channel, + SnapID: snapID, + }) + + // verify snaps in the system state + var snapst snapstate.SnapState + err = snapstate.Get(s.state, instanceName, &snapst) + c.Assert(err, IsNil) + + c.Assert(snapst.LastRefreshTime, NotNil) + c.Assert(snapst.Active, Equals, true) + c.Assert(snapst.Sequence.Revisions, HasLen, 2) + + // link-snap should put the revision we refreshed to at the end of the + // sequence. in this case, swapping their positions. + c.Assert(snapst.Sequence.Revisions[1], DeepEquals, seq.Revisions[0]) + c.Assert(snapst.Sequence.Revisions[0], DeepEquals, seq.Revisions[1]) +} + +func (s *snapmgrTestSuite) TestUpdateWithComponentsBackToPrevRevision(c *C) { + const ( + snapName = "snap-with-components" + instanceKey = "key" + snapID = "snap-with-components-id" + channel = "channel-for-components-only-component-refresh" + ) + + components := []string{"test-component", "kernel-modules-component"} + + currentSnapRev := snap.R(11) + prevSnapRev := snap.R(7) + instanceName := snap.InstanceName(snapName, instanceKey) + + sort.Strings(components) + + // we start without the auxiliary store info (or with an older one) + c.Check(snapstate.AuxStoreInfoFilename(snapID), testutil.FileAbsent) + + currentSI := snap.SideInfo{ + RealName: snapName, + Revision: currentSnapRev, + SnapID: snapID, + Channel: channel, + } + + snaptest.MockSnapInstance(c, instanceName, fmt.Sprintf("name: %s", snapName), ¤tSI) + + restore := snapstate.MockRevisionDate(nil) + defer restore() + + s.state.Lock() + defer s.state.Unlock() + + if instanceKey != "" { + tr := config.NewTransaction(s.state) + tr.Set("core", "experimental.parallel-instances", true) + tr.Commit() + } + + prevSI := snap.SideInfo{ + RealName: snapName, + Revision: prevSnapRev, + SnapID: snapID, + Channel: channel, + } + + otherSI := snap.SideInfo{ + RealName: snapName, + Revision: snap.R(99), + SnapID: snapID, + Channel: channel, + } + + seq := snapstatetest.NewSequenceFromSnapSideInfos([]*snap.SideInfo{&otherSI, &prevSI, ¤tSI}) + currentKmodComps := make([]*snap.ComponentSideInfo, 0, len(components)) - prevKmodComps := make([]*snap.ComponentSideInfo, 0, len(components)) + newKmodComps := make([]*snap.ComponentSideInfo, 0, len(components)) + currentResources := make(map[string]snap.Revision, len(components)) + + // we have three sets of revisions we are working with: + // 1. current component revisions (+1 to index) + // 2. previous component revisions (+3 to index) + // 3. new component revisions (that is connected with the original snap + // revision, via the store) (+2 to index) for i, comp := range components { prevCsi := snap.ComponentSideInfo{ Component: naming.NewComponentRef(snapName, comp), @@ -13973,7 +14167,7 @@ func (s *snapmgrTestSuite) TestUpdateWithComponentsBackToPrevRevision(c *C) { currentCsi := snap.ComponentSideInfo{ Component: naming.NewComponentRef(snapName, comp), - Revision: snap.R(i + 2), + Revision: snap.R(i + 3), } err = seq.AddComponentForRevision(currentSnapRev, &sequence.ComponentState{ SideInfo: ¤tCsi, @@ -13982,9 +14176,61 @@ func (s *snapmgrTestSuite) TestUpdateWithComponentsBackToPrevRevision(c *C) { c.Assert(err, IsNil) if strings.HasPrefix(comp, string(snap.KernelModulesComponent)) { - prevKmodComps = append(prevKmodComps, &prevCsi) + newKmodComps = append(newKmodComps, &snap.ComponentSideInfo{ + Component: naming.NewComponentRef(snapName, comp), + Revision: snap.R(i + 2), + }) currentKmodComps = append(currentKmodComps, ¤tCsi) } + currentResources[comp] = snap.R(i + 3) + } + + availableComponents := make([]string, len(components)) + copy(availableComponents, components) + availableComponents = append(availableComponents, "test-component-extra", "test-component-present-in-sequence") + + // test-component-extra is installed for just the revision we're moving to, + // it should be unlinked and discarded + extraCsi := snap.ComponentSideInfo{ + Component: naming.NewComponentRef(snapName, "test-component-extra"), + Revision: snap.R(len(availableComponents) + 1), + } + err := seq.AddComponentForRevision(prevSnapRev, &sequence.ComponentState{ + SideInfo: &extraCsi, + CompType: componentNameToType(c, extraCsi.Component.ComponentName), + }) + c.Assert(err, IsNil) + + // test-component-present-in-sequence is installed for the revision we're + // moving to and another revision. it should be unlinked, but not discarded. + presentInSeqCsi := snap.ComponentSideInfo{ + Component: naming.NewComponentRef(snapName, "test-component-present-in-sequence"), + Revision: snap.R(len(availableComponents) + 1), + } + for _, si := range []*snap.SideInfo{&prevSI, &otherSI} { + err := seq.AddComponentForRevision(si.Revision, &sequence.ComponentState{ + SideInfo: &presentInSeqCsi, + CompType: componentNameToType(c, presentInSeqCsi.Component.ComponentName), + }) + c.Assert(err, IsNil) + } + + s.fakeStore.snapResourcesFn = func(info *snap.Info) []store.SnapResourceResult { + c.Assert(info.InstanceName(), DeepEquals, instanceName) + var results []store.SnapResourceResult + for i, compName := range availableComponents { + results = append(results, store.SnapResourceResult{ + DownloadInfo: snap.DownloadInfo{ + DownloadURL: "http://example.com/" + compName, + }, + Name: compName, + Revision: i + 2, + Type: fmt.Sprintf("component/%s", componentNameToType(c, compName)), + Version: "1.0", + CreatedAt: "2024-01-01T00:00:00Z", + }) + } + return results } s.AddCleanup(snapstate.MockReadComponentInfo(func( @@ -14031,11 +14277,81 @@ func (s *snapmgrTestSuite) TestUpdateWithComponentsBackToPrevRevision(c *C) { c.Assert(chg.Err(), IsNil, Commentf("change tasks:\n%s", printTasks(chg.Tasks()))) + fi, err := os.Stat(snap.MountFile(instanceName, currentSnapRev)) + c.Assert(err, IsNil) + expected := fakeOps{ { - op: "remove-snap-aliases", - name: instanceName, + op: "storesvc-snap-action", + curSnaps: []store.CurrentSnap{{ + InstanceName: instanceName, + SnapID: snapID, + Revision: currentSnapRev, + TrackingChannel: channel, + RefreshedDate: fi.ModTime(), + Epoch: snap.E("1*"), + Resources: currentResources, + }}, + userID: 1, }, + { + op: "storesvc-snap-action:action", + action: store.SnapAction{ + Action: "refresh", + InstanceName: instanceName, + Revision: prevSnapRev, + SnapID: snapID, + ResourceInstall: true, + // note that channel is empty here since we can't provide a + // channel in this case, since we don't know if the revision is + // a part of the channel + Channel: "", + }, + revno: prevSnapRev, + userID: 1, + }, + } + + for _, unlinked := range []snap.ComponentSideInfo{extraCsi, presentInSeqCsi} { + expected = append(expected, fakeOp{ + op: "unlink-component", + path: snap.ComponentMountDir(unlinked.Component.ComponentName, unlinked.Revision, instanceName), + }) + } + + for i, compName := range components { + csi := snap.ComponentSideInfo{ + Component: naming.NewComponentRef(snapName, compName), + Revision: snap.R(i + 2), + } + + containerName := fmt.Sprintf("%s+%s", instanceName, compName) + filename := fmt.Sprintf("%s_%v.comp", containerName, csi.Revision) + + expected = append(expected, []fakeOp{{ + op: "storesvc-download", + name: csi.Component.String(), + }, { + op: "validate-component:Doing", + name: instanceName, + revno: prevSnapRev, + componentName: compName, + componentPath: filepath.Join(dirs.SnapBlobDir, filename), + componentRev: csi.Revision, + componentSideInfo: csi, + }, { + op: "setup-component", + containerName: containerName, + containerFileName: filename, + }}...) + } + + expected = append(expected, fakeOp{ + op: "remove-snap-aliases", + name: instanceName, + }) + + expected = append(expected, fakeOps{ { op: "run-inhibit-snap-for-unlink", name: instanceName, @@ -14072,12 +14388,12 @@ func (s *snapmgrTestSuite) TestUpdateWithComponentsBackToPrevRevision(c *C) { op: "link-snap", path: filepath.Join(dirs.SnapMountDir, instanceName, prevSnapRev.String()), }, - } + }...) for i, compName := range components { expected = append(expected, fakeOp{ op: "link-component", - path: snap.ComponentMountDir(compName, snap.R(i+1), instanceName), + path: snap.ComponentMountDir(compName, snap.R(i+2), instanceName), }) } @@ -14092,14 +14408,31 @@ func (s *snapmgrTestSuite) TestUpdateWithComponentsBackToPrevRevision(c *C) { }, }...) - if len(prevKmodComps) > 0 || len(currentKmodComps) > 0 { + if len(newKmodComps) > 0 || len(currentKmodComps) > 0 { expected = append(expected, fakeOp{ op: "prepare-kernel-modules-components", currentComps: currentKmodComps, - finalComps: prevKmodComps, + finalComps: newKmodComps, }) } + // note that test-present-in-both-component is not discarded since it is + // still referenced by the original snap revision + discardedContainerName := fmt.Sprintf("%s+%s", instanceName, extraCsi.Component.ComponentName) + discardedFilename := fmt.Sprintf("%s_%v.comp", discardedContainerName, extraCsi.Revision) + expected = append(expected, []fakeOp{ + { + op: "undo-setup-component", + containerName: discardedContainerName, + containerFileName: discardedFilename, + }, + { + op: "remove-component-dir", + containerName: discardedContainerName, + containerFileName: discardedFilename, + }, + }...) + expected = append(expected, fakeOp{ op: "cleanup-trash", name: instanceName, @@ -14123,7 +14456,7 @@ func (s *snapmgrTestSuite) TestUpdateWithComponentsBackToPrevRevision(c *C) { SnapPath: filepath.Join(dirs.SnapBlobDir, fmt.Sprintf("%s_%v.snap", instanceName, prevSnapRev)), SideInfo: snapsup.SideInfo, Type: snap.TypeApp, - Version: "some-snapVer", + Version: "snap-with-componentsVer", PlugsOnly: true, Flags: snapstate.Flags{ Transaction: client.TransactionPerSnap, @@ -14145,12 +14478,27 @@ func (s *snapmgrTestSuite) TestUpdateWithComponentsBackToPrevRevision(c *C) { c.Assert(snapst.LastRefreshTime, NotNil) c.Assert(snapst.Active, Equals, true) - c.Assert(snapst.Sequence.Revisions, HasLen, 2) + c.Assert(snapst.Sequence.Revisions, HasLen, 3) + + // the original revision's components should be replaced with the new + // components + seq.Revisions[1].Components = nil + for i, comp := range components { + err := seq.AddComponentForRevision(prevSnapRev, &sequence.ComponentState{ + SideInfo: &snap.ComponentSideInfo{ + Component: naming.NewComponentRef(snapName, comp), + Revision: snap.R(i + 2), + }, + CompType: componentNameToType(c, comp), + }) + c.Assert(err, IsNil) + } // link-snap should put the revision we refreshed to at the end of the - // sequence. in this case, swapping their positions. - c.Assert(snapst.Sequence.Revisions[1], DeepEquals, seq.Revisions[0]) - c.Assert(snapst.Sequence.Revisions[0], DeepEquals, seq.Revisions[1]) + // sequence + c.Assert(snapst.Sequence.Revisions[2], DeepEquals, seq.Revisions[1]) + c.Assert(snapst.Sequence.Revisions[1], DeepEquals, seq.Revisions[2]) + c.Assert(snapst.Sequence.Revisions[0], DeepEquals, seq.Revisions[0]) } func (s *snapmgrTestSuite) TestUpdateWithComponentsRunThrough(c *C) { @@ -14232,8 +14580,8 @@ type updateWithComponentsOpts struct { } func componentNameToType(c *C, name string) snap.ComponentType { - typ := strings.TrimSuffix(name, "-component") - if typ == name { + typ, _, ok := strings.Cut(name, "-component") + if !ok { c.Fatalf("unexpected component name %q", name) } return snap.ComponentType(typ) diff --git a/overlord/snapstate/storehelpers.go b/overlord/snapstate/storehelpers.go index 624e56e6424..4dc3d30f986 100644 --- a/overlord/snapstate/storehelpers.go +++ b/overlord/snapstate/storehelpers.go @@ -738,11 +738,24 @@ func storeUpdatePlanCore( return updatePlan{}, err } - components, err := componentTargetsFromLocalRevision(snapst, si.Revision) + // here, we attempt to refresh components that are currently installed. + // first, we take the list of currently installed components and remove + // any components that are not available in the target snap revision. + // then we check with the store to get the revisions of the desired + // components. + compsToInstall, err := currentComponentsAvailableInRevision(snapst, info) if err != nil { return updatePlan{}, err } + compsups, err := componentSetupsForInstall(ctx, st, compsToInstall, *snapst, si.Revision, revOpts.Channel, opts) + if err != nil { + return updatePlan{}, err + } + + // this must happen after the call to componentSetupsForInstall, since + // we can't set the channel to the tracking channel if we don't know + // that the requested revision is part of this channel revOpts.setChannelIfUnset(snapst.TrackingChannel) // make sure that we switch the current channel of the snap that we're @@ -763,32 +776,30 @@ func storeUpdatePlanCore( // installed AlwaysUpdate: !revOpts.Revision.Unset(), }, - components: components, + components: compsups, }) } return plan, nil } -func componentTargetsFromLocalRevision(snapst *SnapState, snapRev snap.Revision) ([]ComponentSetup, error) { - // TODO:COMPS: for now, go back to the components that were already - // installed with this revision. to be more robust, we should refresh - // only the components that are installed with the current revision of - // the snap. this means we'll need to check with the store for which - // revisions now available for that snap. - compInfos, err := snapst.ComponentInfosForRevision(snapRev) +func currentComponentsAvailableInRevision(snapst *SnapState, info *snap.Info) ([]string, error) { + if len(info.Components) == 0 { + return nil, nil + } + + current, err := snapst.CurrentComponentInfos() if err != nil { return nil, err } - components := make([]ComponentSetup, 0, len(compInfos)) - for _, compInfo := range compInfos { - components = append(components, ComponentSetup{ - CompSideInfo: &compInfo.ComponentSideInfo, - CompType: compInfo.Type, - }) + var intersection []string + for _, comp := range current { + if _, ok := info.Components[comp.Component.ComponentName]; ok { + intersection = append(intersection, comp.Component.ComponentName) + } } - return components, nil + return intersection, nil } func collectCurrentSnapsAndActions(