overlord/snapstate: on multi-snap refresh make sure bases and core are finished before dependent snaps #5018
Conversation
…ly their base if those are being updated in the same change, otherwise the current link of core or the base could go away and break running hooks or services of the snap
Codecov Report
@@ Coverage Diff @@
## master #5018 +/- ##
==========================================
+ Coverage 79.13% 79.16% +0.02%
==========================================
Files 479 479
Lines 35541 35569 +28
==========================================
+ Hits 28127 28157 +30
+ Misses 5185 5184 -1
+ Partials 2229 2228 -1
Continue to review full report at Codecov.
|
| @@ -744,6 +745,16 @@ func doUpdate(st *state.State, names []string, updates []*snap.Info, params func | |||
| reportUpdated[snapName] = true | |||
| } | |||
|
|
|||
| // first core, bases, then rest | |||
There was a problem hiding this comment.
Nice, this is a very good way to order snaps by kind.
There was a problem hiding this comment.
On second thought, do we need to put gadget ahead of apps here? Also kernel (probably)
There was a problem hiding this comment.
not for the current problem, gadget or kernel shouldn't block running hooks in other snaps. Kernel provokes a reboot but that's a different issue.
| // first core, bases, then rest | ||
| sort.Sort(byKind(updates)) | ||
| prereqs := make(map[string]*state.TaskSet) | ||
| waitPrereq := func(ts *state.TaskSet, prereqName string) { |
There was a problem hiding this comment.
So waitPrereq is a helper that given snap name, adds the prerequisites of that snap, if any, to the given task set.
overlord/snapstate/snapstate.go
Outdated
| @@ -786,6 +797,20 @@ func doUpdate(st *state.State, names []string, updates []*snap.Info, params func | |||
| } | |||
| ts.JoinLane(st.NewLane()) | |||
|
|
|||
| if update.Type == snap.TypeOS || update.Type == snap.TypeBase { | |||
There was a problem hiding this comment.
Can you please add a comment that this loop iterates over updates ordered by the priority and this makes this newly added code safe, that is, that we always have prerequisites by the time we need to read them.
There was a problem hiding this comment.
that's was the intention of the comments inside the two if sides, but it seems they are not clear enough
…e finished before dependent snaps (snapcore#5018) This makes sure non-base snaps wait for core (source of snapctl etc) and possibly their base if those are being updated in the same multi-snap refresh change, otherwise the current link of core or the base could go away and break running hooks or services of the snap during the snap own refresh process. The issue is more general than this, but this covers the auto-refresh case for example. TODO: in general we should raise conflicts between changes where a snap is being operated on in one change, and their base or core is going through unlink-snap or unlink-current-snap in another.
This makes sure non-base snaps wait for core (source of snapctl etc) and possibly their base if those are being updated in the same multi-snap refresh change, otherwise the current link of core or the base could go away and break running hooks or services of the snap during the snap own refresh process.
The issue is more general than this, but this covers the auto-refresh case for example.
TODO: in general we should raise conflicts between changes where a snap is being operated on in one change, and their base or core is going through unlink-snap or unlink-current-snap in another.