o/snapstate: refactor prerequisites task handler to enable proper seed-refresh integration#17152
o/snapstate: refactor prerequisites task handler to enable proper seed-refresh integration#17152andrewphelpsj wants to merge 8 commits into
Conversation
|
Here are some example diagrams that show the graphs for installing two snaps that both pull in the same base as a prerequisite. This is the graph after the changes in this branch: This is the comically large graph from before the changes in this branch: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #17152 +/- ##
==========================================
- Coverage 79.15% 79.12% -0.04%
==========================================
Files 1376 1383 +7
Lines 192837 193274 +437
Branches 2466 2466
==========================================
+ Hits 152640 152922 +282
- Misses 31024 31171 +147
- Partials 9173 9181 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Wed Jun 10 21:36:44 UTC 2026 Failures:Preparing:
Executing:
Restoring:
Skipped tests from snapd-testing-skipIf you wish to have any of the below tests run in your PR, in your PR description, add 'unskip:' followed by a copy-and-pasted list (without variants) of the below tests you wish to run (unskip plus test list must be valid yaml)
|
andrewphelpsj
force-pushed
the
prereq-refactor
branch
3 times, most recently
from
b6e6be4 to
755a207
Compare
|
I rewrote history here to break down the refactor into smaller incremental changes that work towards the final change. Hopefully should make reviewing a bit easier. |
pedronis
left a comment
pedronis
left a comment
There was a problem hiding this comment.
looked at the first 4 commits, I have enough questions already that is probably a good idea to stop there for now, some things are a bit confusing
| NoReRefresh: true, | ||
|
|
||
| // we're calling an API facing call which would otherwise be normally | ||
| // expected to produce a delayed effects taskset, but since the desire | ||
| // is to inject the tasksets into the current change, set the flag to | ||
| // avoid generating one | ||
| NoDelayedSideEffects: true, |
There was a problem hiding this comment.
this commit is strange because I don't see a place that were are removing in here where these Flags were set
| Base: "none", | ||
| PrereqContentAttrs: map[string][]string{"prereq1": {"some-content"}}, | ||
| // set devmode to prove that prerequisites don't inherit these flags | ||
| Flags: snapstate.Flags{DevMode: true}, |
There was a problem hiding this comment.
do we need to double the test?
| // tasks are ordered by the remodeling code. specifically, all snap | ||
| // downloads during a remodel happen prior to snap installation. thus, | ||
| // we cannot wait for snaps to be installed here. see remodelTasks for | ||
| // more information on how the tasks are ordered. |
There was a problem hiding this comment.
this comment got already strange in previous refactor I think but now it's really confusing, returning early here really means not doing prereq, this needs to be reformulated
| return false, retry | ||
| } | ||
|
|
||
| // if this snap is already waiting behind the in-flight refresh, this |
There was a problem hiding this comment.
we should probably be a bit more explicit here than say "this snap"
| // well. | ||
| // | ||
| // snapd is special, we'll always wait for that to be fully done before | ||
| // progressing this task |
There was a problem hiding this comment.
this wasn't true before, snapd passed in false ??
There was a problem hiding this comment.
the comment placement is a bit odd
| } | ||
|
|
||
| func installOneBaseOrRequired(t *state.Task, snapName string, contentAttrs []string, requireTypeBase bool, channel string, onInFlight error, userID int, flags Flags, deviceCtx DeviceContext) (*state.TaskSet, error) { | ||
| func skipOrRetryPrereq(prereqs *state.Task, snapName string, required bool) (bool, error) { |
There was a problem hiding this comment.
the result needs some names, and function probably a comment
| // well. | ||
| // | ||
| // snapd is special, we'll always wait for that to be fully done before | ||
| // progressing this task |
There was a problem hiding this comment.
the comment placement is a bit odd
| return false, err | ||
| } | ||
|
|
||
| retry := &state.Retry{After: prerequisitesRetryTimeout} |
There was a problem hiding this comment.
return a retry from this is a bit too subtle now
| if link.Change().ID() == prereqs.Change().ID() { | ||
| return skip, nil | ||
| } | ||
| // not in this change, poll until that task is done |
There was a problem hiding this comment.
in this case we would conflict that's why we need to retry anyway, the old code was doing that differently
|
|
||
| // snap is being installed, retry later | ||
| return true, nil | ||
| required := snapName == "snapd" || requireTypeBase |
There was a problem hiding this comment.
it's unclear that this new flag is useful, because anyway the function then need to look at snapd as name anyway. So maybe we should pass the old flag and to something slightly different inside the helper
andrewphelpsj
force-pushed
the
prereq-refactor
branch
from
755a207 to
08ac2f2
Compare
…ct retry code for install/update
This change isn't needed, but does result in much nicer graphs for snap installations/refreshes that pull in prerequisites.
andrewphelpsj
force-pushed
the
prereq-refactor
branch
from
08ac2f2 to
fd8b48c
Compare
|
I've rewritten history so that things should be a bit more reviewable. |
pedronis
left a comment
pedronis
left a comment
There was a problem hiding this comment.
thanks for the tweaks, did another pass, some small comments and questions
| func serializeTaskSetBeforeInProgressChange(ts *state.TaskSet, chg *state.Change) { | ||
| tasks := make([]*state.Task, 0, len(chg.Tasks())) | ||
| for _, t := range chg.Tasks() { | ||
| if t.Status() == state.DoingStatus { |
There was a problem hiding this comment.
what if some subpart of the change is undoing?
There was a problem hiding this comment.
Good point, I've made this only consider tasks that still need to be done, in the do direction.
| // if the base being installed by the prerequisites task is already ordered | ||
| // behind the in-flight prerequisite link task in the same lane, this task | ||
| // does not need to wait for that prerequisite out-of-band as well. | ||
| waiting, err := snapWaitsForLinkInSameLane(prereqs, link) |
There was a problem hiding this comment.
so this can never apply to snapd itself?
There was a problem hiding this comment.
The specific reason why this shortcut was added was to prevent a deadlock that can occur due to some of the task ordering done by the seed-refresh code. That case can't happen to snapd, only bases.
This behavior was added in PR #16827.
Conceptually though, I don't think we should make it apply to snapd anyways. Most things should happen after snapd is fully refreshed.
| return nil, nil | ||
| } | ||
| // as a special case, we allow the core snap to satisfy a core16 requirement | ||
| if sn.InstanceName == "core16" { |
There was a problem hiding this comment.
maybe we can simply drop this code? core16 was removed
There was a problem hiding this comment.
We have this fallback in quite few places. If we want to drop it, they should probably be done all at once? And probably not in this PR.
| switch t.Status() { | ||
| case state.DoStatus: | ||
| case state.WaitStatus: | ||
| if t.WaitedStatus() != state.DoStatus { |
There was a problem hiding this comment.
the new test tests this continue?
There was a problem hiding this comment.
Yes, TestSerializeTaskSetBeforeInProgressChangeIncludesWaitStatusForDo hits this branch.
2 participants
This is an attempt at simplifying the current
prerequisitestask handler.This is somewhat of a full rewrite, so the diff might not be super helpful sadly. A main motivation was to simplify and combine some of the retry logic, since that was spread across a lot of the previous implementation.
No functional changes, though I've added a couple tests that caught regressions I introduced while working on the refactor.
I've also gotten rid of the
WaitAllusage that was used when organizing the task sets created byprerequisites. Now the graph is a bit more readable. I'll attach some examples.