From 880d889ca614978d85df97c053a113937b8bcb8d Mon Sep 17 00:00:00 2001 From: Andrew Phelps Date: Mon, 22 Jul 2024 13:05:41 -0400 Subject: [PATCH 1/8] o/snapstate: properly handle components when refreshing to revision that has been on the system before --- overlord/snapstate/backend_test.go | 13 ++ overlord/snapstate/component.go | 33 ++--- overlord/snapstate/snapstate_update_test.go | 145 +++++++++++++++++--- overlord/snapstate/storehelpers.go | 40 +++--- 4 files changed, 178 insertions(+), 53 deletions(-) diff --git a/overlord/snapstate/backend_test.go b/overlord/snapstate/backend_test.go index b95b6a645c0..d11e0e0318e 100644 --- a/overlord/snapstate/backend_test.go +++ b/overlord/snapstate/backend_test.go @@ -506,6 +506,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)) } @@ -1117,6 +1119,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..b22b42afec8 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,10 +154,16 @@ 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) { + var si *snap.SideInfo + if snapRev.Unset() { + si = snapst.CurrentSideInfo() + } else { + 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 == "" { @@ -184,8 +185,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_update_test.go b/overlord/snapstate/snapstate_update_test.go index d322098afd9..f532e2880ce 100644 --- a/overlord/snapstate/snapstate_update_test.go +++ b/overlord/snapstate/snapstate_update_test.go @@ -13796,10 +13796,10 @@ func (s *snapmgrTestSuite) TestUpdateStateConflictRemoved(c *C) { func (s *snapmgrTestSuite) TestUpdateWithComponentsBackToPrevRevision(c *C) { const ( - snapName = "some-snap" + snapName = "snap-with-components" instanceKey = "key" - snapID = "some-snap-id" - channel = "channel-for-components" + snapID = "snap-with-components-id" + channel = "channel-for-components-only-component-refresh" ) components := []string{"test-component", "kernel-modules-component"} @@ -13810,9 +13810,12 @@ func (s *snapmgrTestSuite) TestUpdateWithComponentsBackToPrevRevision(c *C) { sort.Strings(components) - s.fakeStore.snapResourcesFn = func(info *snap.Info) []store.SnapResourceResult { - c.Fatalf("unexpected call to snapResourcesFn") - return nil + compNameToType := func(name string) snap.ComponentType { + typ := strings.TrimSuffix(name, "-component") + if typ == name { + c.Fatalf("unexpected component name %q", name) + } + return snap.ComponentType(typ) } // we start without the auxiliary store info (or with an older one) @@ -13824,6 +13827,7 @@ func (s *snapmgrTestSuite) TestUpdateWithComponentsBackToPrevRevision(c *C) { SnapID: snapID, Channel: channel, } + snaptest.MockSnapInstance(c, instanceName, fmt.Sprintf("name: %s", snapName), ¤tSI) restore := snapstate.MockRevisionDate(nil) @@ -13832,11 +13836,6 @@ 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. - snapstate.ReplaceStore(s.state, &storetest.Store{}) - if instanceKey != "" { tr := config.NewTransaction(s.state) tr.Set("core", "experimental.parallel-instances", true) @@ -13853,7 +13852,14 @@ func (s *snapmgrTestSuite) TestUpdateWithComponentsBackToPrevRevision(c *C) { seq := snapstatetest.NewSequenceFromSnapSideInfos([]*snap.SideInfo{&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), @@ -13868,7 +13874,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, @@ -13877,9 +13883,32 @@ 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) + } + + s.fakeStore.snapResourcesFn = func(info *snap.Info) []store.SnapResourceResult { + c.Assert(info.InstanceName(), DeepEquals, instanceName) + var results []store.SnapResourceResult + for i, compName := range components { + results = append(results, store.SnapResourceResult{ + DownloadInfo: snap.DownloadInfo{ + DownloadURL: "http://example.com/" + compName, + }, + Name: compName, + Revision: i + 2, + Type: fmt.Sprintf("component/%s", compNameToType(compName)), + Version: "1.0", + CreatedAt: "2024-01-01T00:00:00Z", + }) + } + return results } s.AddCleanup(snapstate.MockReadComponentInfo(func( @@ -13926,11 +13955,71 @@ 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, + Channel: channel, + ResourceInstall: true, + }, + revno: prevSnapRev, + userID: 1, + }, + } + + 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, @@ -13967,12 +14056,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), }) } @@ -13987,11 +14076,11 @@ 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, }) } @@ -14018,7 +14107,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, @@ -14042,6 +14131,20 @@ func (s *snapmgrTestSuite) TestUpdateWithComponentsBackToPrevRevision(c *C) { c.Assert(snapst.Active, Equals, true) c.Assert(snapst.Sequence.Revisions, HasLen, 2) + // the original revision's components should be replaced with the new + // components + seq.Revisions[0].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: compNameToType(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]) diff --git a/overlord/snapstate/storehelpers.go b/overlord/snapstate/storehelpers.go index 624e56e6424..8915eafcc59 100644 --- a/overlord/snapstate/storehelpers.go +++ b/overlord/snapstate/storehelpers.go @@ -738,13 +738,23 @@ 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 } revOpts.setChannelIfUnset(snapst.TrackingChannel) + compsups, err := componentSetupsForInstall(ctx, st, compsToInstall, *snapst, si.Revision, revOpts.Channel, opts) + if err != nil { + return updatePlan{}, err + } + // make sure that we switch the current channel of the snap that we're // switching to info.Channel = revOpts.Channel @@ -763,32 +773,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( From 10420aa79b359ca7fb172bd9a16c1d6d0c4eea99 Mon Sep 17 00:00:00 2001 From: Andrew Phelps Date: Tue, 30 Jul 2024 10:25:51 -0400 Subject: [PATCH 2/8] o/snapstate: make sure to unlink and discard components that are already installed for a snap revision that we don't want anymore --- overlord/snapstate/backend_test.go | 8 ++ overlord/snapstate/snapstate.go | 95 +++++++++++++++++++++ overlord/snapstate/snapstate_update_test.go | 93 +++++++++++++++----- 3 files changed, 176 insertions(+), 20 deletions(-) diff --git a/overlord/snapstate/backend_test.go b/overlord/snapstate/backend_test.go index d11e0e0318e..29745b63f7f 100644 --- a/overlord/snapstate/backend_test.go +++ b/overlord/snapstate/backend_test.go @@ -535,6 +535,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" { diff --git a/overlord/snapstate/snapstate.go b/overlord/snapstate/snapstate.go index 0cdf61b0cc8..133b3aa1d86 100644 --- a/overlord/snapstate/snapstate.go +++ b/overlord/snapstate/snapstate.go @@ -52,6 +52,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" @@ -273,6 +274,89 @@ func isCoreSnap(snapName string) bool { return snapName == defaultCoreSnapName } +// removeExtraComponentsTasks creates tasks that will remove unwanted components +// that are currently installed alongside the snap revision that is about to be +// installed. 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 has already been installed on the system. The target snap +// might have components that are installed that we don't want any more. +func removeExtraComponentsTasks(st *state.State, snapst *SnapState, newRevision snap.Revision, compsups []ComponentSetup) ( + unlinkTasks, discardTasks []*state.Task, err error, +) { + idx := snapst.LastIndex(newRevision) + if idx < 0 { + return nil, nil, nil + } + si := snapst.Sequence.Revisions[idx].Snap + + keep := make(map[naming.ComponentRef]bool, len(compsups)) + for _, compsup := range compsups { + keep[compsup.CompSideInfo.Component] = true + } + + snapst.CurrentComponentSideInfos() + + linkedForRevision, err := snapst.ComponentInfosForRevision(newRevision) + if err != nil { + return nil, nil, err + } + + // this is just a throwaway SnapSetup we create here so that + // unlink-component knows which snap revision we're unlinking the component + // from. 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 currently installed snap revision. + snapsup := SnapSetup{ + SideInfo: &snap.SideInfo{ + RealName: si.RealName, + SnapID: si.SnapID, + Revision: si.Revision, + }, + InstanceKey: snapst.InstanceKey, + Type: snap.Type(snapst.SnapType), + } + + 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 == newRevision { + return nil, nil, errors.New("internal error: cannot lose a component during a refresh without a snap revision change") + } + + unlink := st.NewTask("unlink-component", fmt.Sprintf( + i18n.G("Unlink component %q for snap revision %s"), ci.Component, snapsup.Revision(), + )) + + unlink.Set("snap-setup", snapsup) + 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("snap-setup-task", unlink.ID()) + 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) @@ -442,6 +526,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, ) @@ -449,6 +542,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 f532e2880ce..118ce37d7a7 100644 --- a/overlord/snapstate/snapstate_update_test.go +++ b/overlord/snapstate/snapstate_update_test.go @@ -13810,14 +13810,6 @@ func (s *snapmgrTestSuite) TestUpdateWithComponentsBackToPrevRevision(c *C) { sort.Strings(components) - compNameToType := func(name string) snap.ComponentType { - typ := strings.TrimSuffix(name, "-component") - if typ == name { - c.Fatalf("unexpected component name %q", name) - } - return snap.ComponentType(typ) - } - // we start without the auxiliary store info (or with an older one) c.Check(snapstate.AuxStoreInfoFilename(snapID), testutil.FileAbsent) @@ -13849,7 +13841,14 @@ func (s *snapmgrTestSuite) TestUpdateWithComponentsBackToPrevRevision(c *C) { Channel: channel, } - seq := snapstatetest.NewSequenceFromSnapSideInfos([]*snap.SideInfo{&prevSI, ¤tSI}) + 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)) newKmodComps := make([]*snap.ComponentSideInfo, 0, len(components)) @@ -13889,21 +13888,50 @@ func (s *snapmgrTestSuite) TestUpdateWithComponentsBackToPrevRevision(c *C) { }) 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 components { + 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", compNameToType(compName)), + Type: fmt.Sprintf("component/%s", componentNameToType(c, compName)), Version: "1.0", CreatedAt: "2024-01-01T00:00:00Z", }) @@ -13987,6 +14015,13 @@ func (s *snapmgrTestSuite) TestUpdateWithComponentsBackToPrevRevision(c *C) { }, } + 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), @@ -14084,6 +14119,23 @@ func (s *snapmgrTestSuite) TestUpdateWithComponentsBackToPrevRevision(c *C) { }) } + // 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, @@ -14129,26 +14181,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[0].Components = nil + 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: compNameToType(comp), + 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) { @@ -14230,8 +14283,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) From 93b98c20d5bf8f0e407573a5c71659be13e1e393 Mon Sep 17 00:00:00 2001 From: Andrew Phelps Date: Tue, 30 Jul 2024 08:43:05 -0400 Subject: [PATCH 3/8] o/snapstate: make sure to set tracking channel after talking to the store --- overlord/snapstate/snapstate_update_test.go | 5 ++++- overlord/snapstate/storehelpers.go | 7 +++++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/overlord/snapstate/snapstate_update_test.go b/overlord/snapstate/snapstate_update_test.go index 118ce37d7a7..081fe9fba73 100644 --- a/overlord/snapstate/snapstate_update_test.go +++ b/overlord/snapstate/snapstate_update_test.go @@ -14007,8 +14007,11 @@ func (s *snapmgrTestSuite) TestUpdateWithComponentsBackToPrevRevision(c *C) { InstanceName: instanceName, Revision: prevSnapRev, SnapID: snapID, - Channel: channel, 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, diff --git a/overlord/snapstate/storehelpers.go b/overlord/snapstate/storehelpers.go index 8915eafcc59..4dc3d30f986 100644 --- a/overlord/snapstate/storehelpers.go +++ b/overlord/snapstate/storehelpers.go @@ -748,13 +748,16 @@ func storeUpdatePlanCore( return updatePlan{}, err } - revOpts.setChannelIfUnset(snapst.TrackingChannel) - 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 // switching to info.Channel = revOpts.Channel From aa707be542866eed4ac71307fc090b2ab01ab983 Mon Sep 17 00:00:00 2001 From: Andrew Phelps Date: Mon, 16 Sep 2024 12:10:44 -0400 Subject: [PATCH 4/8] o/snapstate: use targetRevision instead of newRevision to refer to revision we are moving to --- overlord/snapstate/snapstate.go | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/overlord/snapstate/snapstate.go b/overlord/snapstate/snapstate.go index 133b3aa1d86..84716bf1997 100644 --- a/overlord/snapstate/snapstate.go +++ b/overlord/snapstate/snapstate.go @@ -275,18 +275,18 @@ func isCoreSnap(snapName string) bool { } // removeExtraComponentsTasks creates tasks that will remove unwanted components -// that are currently installed alongside the snap revision that is about to be -// installed. 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. +// 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 has already been installed on the system. The target snap // might have components that are installed that we don't want any more. -func removeExtraComponentsTasks(st *state.State, snapst *SnapState, newRevision snap.Revision, compsups []ComponentSetup) ( +func removeExtraComponentsTasks(st *state.State, snapst *SnapState, targetRevision snap.Revision, compsups []ComponentSetup) ( unlinkTasks, discardTasks []*state.Task, err error, ) { - idx := snapst.LastIndex(newRevision) + idx := snapst.LastIndex(targetRevision) if idx < 0 { return nil, nil, nil } @@ -297,9 +297,7 @@ func removeExtraComponentsTasks(st *state.State, snapst *SnapState, newRevision keep[compsup.CompSideInfo.Component] = true } - snapst.CurrentComponentSideInfos() - - linkedForRevision, err := snapst.ComponentInfosForRevision(newRevision) + linkedForRevision, err := snapst.ComponentInfosForRevision(targetRevision) if err != nil { return nil, nil, err } @@ -329,7 +327,7 @@ func removeExtraComponentsTasks(st *state.State, snapst *SnapState, newRevision // 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 == newRevision { + if snapst.Current == targetRevision { return nil, nil, errors.New("internal error: cannot lose a component during a refresh without a snap revision change") } From 2ac5298f866841cfb5c9755052b64c685315d1d0 Mon Sep 17 00:00:00 2001 From: Andrew Phelps Date: Mon, 16 Sep 2024 12:22:23 -0400 Subject: [PATCH 5/8] o/snapstate: add back test for refreshing to old revision without components This test ensures that we don't hit the store when we have no components to check on. --- overlord/snapstate/snapstate_update_test.go | 189 ++++++++++++++++++++ 1 file changed, 189 insertions(+) diff --git a/overlord/snapstate/snapstate_update_test.go b/overlord/snapstate/snapstate_update_test.go index 081fe9fba73..001b5b9f833 100644 --- a/overlord/snapstate/snapstate_update_test.go +++ b/overlord/snapstate/snapstate_update_test.go @@ -13794,6 +13794,195 @@ func (s *snapmgrTestSuite) TestUpdateStateConflictRemoved(c *C) { c.Assert(err, ErrorMatches, `snap "some-snap" has changes in progress`) } +func (s *snapmgrTestSuite) TestUpdateBackToPrevRevision(c *C) { + const ( + snapName = "some-snap" + instanceKey = "key" + snapID = "some-snap-id" + channel = "some-channel" + ) + + currentSnapRev := snap.R(11) + prevSnapRev := snap.R(7) + instanceName := snap.InstanceName(snapName, instanceKey) + + s.fakeStore.snapResourcesFn = func(info *snap.Info) []store.SnapResourceResult { + c.Fatalf("unexpected call to snapResourcesFn") + return nil + } + + // 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() + + // 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 != "" { + 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, + } + + 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" From 60148863dba30553f4a3d413b4ec9f23ef2ecc8f Mon Sep 17 00:00:00 2001 From: Andrew Phelps Date: Tue, 17 Sep 2024 09:04:35 -0400 Subject: [PATCH 6/8] o/snapstate: use snapsup from main chain of tasks in tasks for removing extra already-present components --- overlord/snapstate/snapstate.go | 25 ++++++------------------- 1 file changed, 6 insertions(+), 19 deletions(-) diff --git a/overlord/snapstate/snapstate.go b/overlord/snapstate/snapstate.go index 84716bf1997..5f8629de7be 100644 --- a/overlord/snapstate/snapstate.go +++ b/overlord/snapstate/snapstate.go @@ -290,7 +290,6 @@ func removeExtraComponentsTasks(st *state.State, snapst *SnapState, targetRevisi if idx < 0 { return nil, nil, nil } - si := snapst.Sequence.Revisions[idx].Snap keep := make(map[naming.ComponentRef]bool, len(compsups)) for _, compsup := range compsups { @@ -302,21 +301,6 @@ func removeExtraComponentsTasks(st *state.State, snapst *SnapState, targetRevisi return nil, nil, err } - // this is just a throwaway SnapSetup we create here so that - // unlink-component knows which snap revision we're unlinking the component - // from. 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 currently installed snap revision. - snapsup := SnapSetup{ - SideInfo: &snap.SideInfo{ - RealName: si.RealName, - SnapID: si.SnapID, - Revision: si.Revision, - }, - InstanceKey: snapst.InstanceKey, - Type: snap.Type(snapst.SnapType), - } - for _, ci := range linkedForRevision { if keep[ci.Component] { continue @@ -331,11 +315,15 @@ func removeExtraComponentsTasks(st *state.State, snapst *SnapState, targetRevisi 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 + // currently installed 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, snapsup.Revision(), + i18n.G("Unlink component %q for snap revision %s"), ci.Component, targetRevision, )) - unlink.Set("snap-setup", snapsup) unlink.Set("component-setup", ComponentSetup{ CompSideInfo: &ci.ComponentSideInfo, CompType: ci.Type, @@ -346,7 +334,6 @@ func removeExtraComponentsTasks(st *state.State, snapst *SnapState, targetRevisi discard := st.NewTask("discard-component", fmt.Sprintf( i18n.G("Discard previous revision for component %q"), ci.Component, )) - discard.Set("snap-setup-task", unlink.ID()) discard.Set("component-setup-task", unlink.ID()) discardTasks = append(discardTasks, discard) } From 6dc918b1584709a69e974615201331825f42c90e Mon Sep 17 00:00:00 2001 From: Andrew Phelps Date: Tue, 17 Sep 2024 09:15:04 -0400 Subject: [PATCH 7/8] o/snapstate: remove branch that we cannot go into --- overlord/snapstate/component.go | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/overlord/snapstate/component.go b/overlord/snapstate/component.go index b22b42afec8..7efdcd4b444 100644 --- a/overlord/snapstate/component.go +++ b/overlord/snapstate/component.go @@ -155,16 +155,11 @@ func componentSetupsForInstall(ctx context.Context, st *state.State, names []str } func installComponentAction(st *state.State, snapst SnapState, snapRev snap.Revision, channel string, opts Options) (*store.SnapAction, error) { - var si *snap.SideInfo - if snapRev.Unset() { - si = snapst.CurrentSideInfo() - } else { - 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] + 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") From 7ceabfe5205f8af030f2692d58fd742582bf2a65 Mon Sep 17 00:00:00 2001 From: Andrew Phelps Date: Thu, 19 Sep 2024 17:12:31 -0400 Subject: [PATCH 8/8] o/snapstate: clear up some confusing language in comments --- overlord/snapstate/snapstate.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/overlord/snapstate/snapstate.go b/overlord/snapstate/snapstate.go index 5f8629de7be..43f191d3a50 100644 --- a/overlord/snapstate/snapstate.go +++ b/overlord/snapstate/snapstate.go @@ -281,8 +281,9 @@ func isCoreSnap(snapName string) bool { // components that are not in compsups. // // This is mostly relevant when we're moving from one snap revision to another -// snap revision that has already been installed on the system. The target snap -// might have components that are installed that we don't want any more. +// 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, ) { @@ -317,7 +318,7 @@ func removeExtraComponentsTasks(st *state.State, snapst *SnapState, targetRevisi // 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 - // currently installed snap revision. unlink-component differs from + // 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(