-
Notifications
You must be signed in to change notification settings - Fork 576
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.
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
o/snapstate: properly handle components when refreshing to revision that has been on the system before #14498
Changes from 3 commits
880d889
10420aa
93b98c2
aa707be
2ac5298
6014886
6dc918b
7ceabfe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm confused by this comment, aren't the component linked to a given snap revision, why would we remove components when switch around by revision? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, they are linked by snap revision. Consider this case:
When we move from snap revision 2 -> 1, we need to unlink component a (and discard it, if nothing else references it) and install component c. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am a bit confused about the comment too, should "target snap might have components that are installed that we don't want any more" be "target revision might have components that were installed in the past but that we don't want any more"? Or maybe state more clearly that this is about checking a revision that was active in the past and still around but not currently. I always find misleading when we talk about installed revisions for non-active but present revisions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see what you mean, I'll update it to be explicit when I'm talking about something that was installed in the past. |
||
func removeExtraComponentsTasks(st *state.State, snapst *SnapState, newRevision snap.Revision, compsups []ComponentSetup) ( | ||
andrewphelpsj marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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() | ||
andrewphelpsj marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will this confuse findSnapSetupTask ? that's one example but I fear we might have a few bits of logic that assume there is mainly one task in a snap lane with snap-setup attached to it vs id? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've change this in 6014886. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't know why I didn't do that in the first place. |
||
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,13 +526,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) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mmh, test don't seem to hit this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the code, we can't reach that case. Removed.