Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 13 commits into from
Sep 13, 2024
Merged

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

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
f129277
o/snapstate: add currently installed components to the current snaps …
andrewphelpsj Jul 10, 2024
95f7eb9
o/snapstate: put options for component refresh test into struct
andrewphelpsj Jul 10, 2024
cea160f
o/snapstate: handle refreshing a snap that loses a currently installe…
andrewphelpsj Jul 12, 2024
eeb3824
o/snapstate: always initialize resources map that is sent to the stor…
andrewphelpsj Aug 9, 2024
14165a1
o/snapstate: build updatedCompRevisions with opts.postRefreshComponents
andrewphelpsj Aug 22, 2024
e21ad34
Merge branch 'master' of https://github.com/canonical/snapd into lose…
andrewphelpsj Sep 11, 2024
188128b
o/snapstate: handle refreshing a snap that only has component revisio…
andrewphelpsj Jul 10, 2024
bc036e4
o/snapstate: update doc comment on revisionSatisfied
andrewphelpsj Sep 11, 2024
3712562
o/snapstate: use firstInstall variable in conditional for readability
andrewphelpsj Sep 11, 2024
541066d
o/snapstate: add tests for doing/undoing refresh to current revision
andrewphelpsj Sep 12, 2024
6d38bec
Merge branch 'master' of https://github.com/canonical/snapd into refr…
andrewphelpsj Sep 12, 2024
9254023
o/snapstate: make names of fields in componentInstallTaskSet more exp…
andrewphelpsj Sep 12, 2024
d838bc8
Merge branch 'master' of https://github.com/canonical/snapd into refr…
andrewphelpsj Sep 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions overlord/snapstate/autorefresh_gating_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3181,18 +3181,21 @@ func (s *validationSetsSuite) TestAutoRefreshPhase1WithValidationSets(c *C) {
Revision: snap.R(1),
Epoch: snap.E("1*"),
RefreshedDate: refreshedDate,
Resources: make(map[string]snap.Revision),
}, {
InstanceName: "some-other-snap",
SnapID: "some-other-snap-id",
Revision: snap.R(1),
Epoch: snap.E("1*"),
RefreshedDate: refreshedDate,
Resources: make(map[string]snap.Revision),
}, {
InstanceName: "some-snap",
SnapID: "some-snap-id",
Revision: snap.R(1),
Epoch: snap.E("1*"),
RefreshedDate: refreshedDate,
Resources: make(map[string]snap.Revision),
}},
}, {
op: "storesvc-snap-action:action",
Expand Down
8 changes: 7 additions & 1 deletion overlord/snapstate/backend_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
}

Expand Down
18 changes: 9 additions & 9 deletions overlord/snapstate/component.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
beforeLink []*state.Task
linkTask *state.Task
postOpHookToDiscard []*state.Task
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does ToDiscard means here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The range of the tasks, [post-refresh/install hook -> discard). I'm happy to take suggestions on naming there.

Copy link
Collaborator

@pedronis pedronis Sep 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe all *state.Task field should have a name ending in "Task", and all the []*state.Task ones, one ending in "Tasks" ?

discardTask *state.Task
}

func (c *componentInstallTaskSet) taskSet() *state.TaskSet {
tasks := make([]*state.Task, 0, len(c.beforeLink)+1+len(c.postOpHookAndAfter)+1)
tasks := make([]*state.Task, 0, len(c.beforeLink)+1+len(c.postOpHookToDiscard)+1)
tasks = append(tasks, c.beforeLink...)
tasks = append(tasks, c.linkTask)
tasks = append(tasks, c.postOpHookAndAfter...)
tasks = append(tasks, c.postOpHookToDiscard...)
if c.discardTask != nil {
tasks = append(tasks, c.discardTask)
}
Expand Down Expand Up @@ -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.postOpHookToDiscard = append(componentTS.postOpHookToDiscard, postOpHook)
addTask(postOpHook)

if !compSetup.MultiComponentInstall && kmodSetup == nil && compSetup.CompType == snap.KernelModulesComponent {
Expand All @@ -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.postOpHookToDiscard = append(componentTS.postOpHookToDiscard, kmodSetup)
}
if kmodSetup != nil {
// note that we don't use addTask here because this task is shared and
Expand Down
2 changes: 1 addition & 1 deletion overlord/snapstate/handlers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2714,7 +2714,7 @@ func (m *SnapManager) undoLinkSnap(t *state.Task, _ *tomb.Tomb) error {
return err
}

if len(snapst.Sequence.Revisions) == 1 {
if oldCurrent.Unset() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this mean doing this only on firstInstall ? maybe is a good idea to call the name the predicate like that and use the var? I'm not entirely sure I understand the need for this change

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

During a refresh where we only refresh components, we treat it a lot like snap refresh --revision=n snapname, where n is the current revision of the snap. In that case, this undo handler wouldn't do the right thing. We don't want to treat it as if the snap is completely removed, since it isn't.

Naming the predicate to firstInstall is a good idea, I'll do that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added tests for this case here: 541066d

// 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())
Expand Down
12 changes: 5 additions & 7 deletions overlord/snapstate/handlers_components.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -523,12 +523,16 @@ func (m *SnapManager) doUnlinkCurrentComponent(t *state.Task, _ *tomb.Tomb) (err
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 {
Expand Down Expand Up @@ -561,12 +565,6 @@ func (m *SnapManager) doUnlinkComponent(t *state.Task, _ *tomb.Tomb) (err error)
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 {
Copy link
Member Author

@andrewphelpsj andrewphelpsj Sep 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was in the wrong place. We now do this in these 3 places:

link-component
unlink-current-component
link-snap

Previously we had it in unlink-component, rather than unlink-current-component. unlink-component isn't run during a snap refresh, that is only run when we are explicitly removing a snap (with its components)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we sure we do not need to save the kernel-modules components here too? For instance, if we are removing one of these components, to get the state restored on rollback. I'm not sure if in that case we do an unlink current component.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case we use "unlink-current-component", since we don't support removing components for anything but the current snap revision.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See here: https://github.com/canonical/snapd/blob/master/overlord/snapstate/component.go?plain=1#L560

return err
}

cref := compSetup.CompSideInfo.Component
// Remove component for the specified revision
if err := m.unlinkComponent(
Expand Down
14 changes: 4 additions & 10 deletions overlord/snapstate/handlers_components_link_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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) {
Expand Down Expand Up @@ -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,
Expand All @@ -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) {
Expand Down
54 changes: 44 additions & 10 deletions overlord/snapstate/snapstate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -453,8 +453,14 @@ func doInstall(st *state.State, snapst *SnapState, snapsup SnapSetup, compsups [
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)
Expand Down Expand Up @@ -620,8 +626,6 @@ func doInstall(st *state.State, snapst *SnapState, snapsup SnapSetup, compsups [
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)
}
Expand Down Expand Up @@ -776,7 +780,7 @@ func splitComponentTasksForInstall(

tasksBeforePreRefreshHook = append(tasksBeforePreRefreshHook, componentTS.beforeLink...)
tasksAfterLinkSnap = append(tasksAfterLinkSnap, componentTS.linkTask)
tasksAfterPostOpHook = append(tasksAfterPostOpHook, componentTS.postOpHookAndAfter...)
tasksAfterPostOpHook = append(tasksAfterPostOpHook, componentTS.postOpHookToDiscard...)
if componentTS.discardTask != nil {
tasksBeforeDiscard = append(tasksBeforeDiscard, componentTS.discardTask)
}
Expand Down Expand Up @@ -1881,12 +1885,32 @@ type update struct {
// revision of the snap.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems the doc comment for this needs changes, also I wonder if turning this around and calling it revisionDiverged wouldn't be clear?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally think that "satisfied" works better here as a method on update, since it reads as the update has already been satisfied. Using diverged implies something different to me, almost as if something has unexpectedly changed.

//
// TODO:COMPS: check if we need to change the state of components
andrewphelpsj marked this conversation as resolved.
Show resolved Hide resolved
func (u *update) revisionSatisfied() bool {
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
}

return u.SnapState.Current == u.Setup.Revision()
currentCompRevs := make(map[string]snap.Revision, len(comps))
for _, comp := range comps {
currentCompRevs[comp.Component.ComponentName] = comp.Revision
}

for _, comp := range u.Components {
if currentCompRevs[comp.CompSideInfo.Component.ComponentName] != comp.Revision() {
return false, nil
}
}
alfonsosanchezbeato marked this conversation as resolved.
Show resolved Hide resolved

return true, nil
}

func doPotentiallySplitUpdate(st *state.State, requested []string, updates []update, opts Options) ([]string, *UpdateTaskSets, error) {
Expand Down Expand Up @@ -1991,7 +2015,12 @@ func doUpdate(st *state.State, requested []string, updates []update, opts Option
// 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
}
Expand Down Expand Up @@ -2267,7 +2296,12 @@ func autoAliasesUpdate(st *state.State, requested []string, updates []update) (c
// 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
Expand Down
Loading
Loading