overlord/snapstate: figure out satisfied installation targets when intstalling from store#16705
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the store install goal logic in overlord/snapstate so that snaps already present on the system (when SkipIfPresent is used) are treated as “satisfied” and do not generate installation tasks—allowing InstallMany to succeed with no tasks when everything is already installed.
Changes:
- Track and surface “satisfied” snaps during
storeInstallGoal.validateAndPrune, and avoid sending store install actions when nothing remains to install. - Extend
targetwith asatisfiedflag and updateInstallWithGoalto skip task creation for satisfied targets while still returning theirsnap.Info. - Update/add unit tests to cover empty installs and installs where some/all snaps are already present.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
overlord/snapstate/target.go |
Adds satisfied-target handling to pruning and install planning, and skips task creation for already-installed snaps. |
overlord/snapstate/snapstate_install_test.go |
Updates expectations for empty install lists and adds coverage for partially/fully satisfied InstallMany calls. |
Comments suppressed due to low confidence (2)
overlord/snapstate/target.go:772
validateAndPruneappends tos.satisfiedbut never resets it. If astoreInstallGoalinstance is reused across calls, previously satisfied snaps will leak into subsequent operations and produce duplicate/incorrect satisfied targets. Consider clearings.satisfiedat the start ofvalidateAndPrune(similar to hows.snapsis rewritten) or makingtoInstall/validateAndPrunetreat this field as strictly local state.
snapst, ok := installedSnaps[sn.InstanceName]
if ok && snapst.IsInstalled() {
if !sn.SkipIfPresent {
return &snap.AlreadyInstalledError{Snap: sn.InstanceName}
}
s.satisfied = append(s.satisfied, snapst)
continue
}
overlord/snapstate/snapstate_install_test.go:6623
- The comment says this is a taskset for "other-snap installation", but in this test
other-snapis already present and the single taskset should correspond to installingsome-snap. Please update the comment to match the assertion below.
// one taskset for other-snap installation
c.Assert(tss, HasLen, 1)
| } | ||
|
|
||
| return installs, err | ||
| return append(installs, satisfiedTargets...), err |
There was a problem hiding this comment.
toInstall appends satisfiedTargets after installs, which can reorder results vs the original request order when a satisfied snap appears before a snap that still needs installation (e.g. [present, missing] becomes [missing, present]). This breaks the stated ordering guarantee on storeInstallGoal.snaps and can affect task ordering/lane grouping. Consider rebuilding the final []target in the original requested order (e.g. keep an ordered list of requested instance names and map name->target for both installed and store results).
| return append(installs, satisfiedTargets...), err | |
| // Combine installs and already-satisfied snaps into a single slice and impose | |
| // a deterministic ordering that does not depend on whether the snap needs | |
| // installation. This avoids reordering relative to each other based solely | |
| // on satisfaction state. | |
| allTargets := make([]target, 0, len(installs)+len(satisfiedTargets)) | |
| allTargets = append(allTargets, installs...) | |
| allTargets = append(allTargets, satisfiedTargets...) | |
| sort.SliceStable(allTargets, func(i, j int) bool { | |
| return allTargets[i].info.InstanceName() < allTargets[j].info.InstanceName() | |
| }) | |
| return allTargets, err |
overlord/snapstate/target.go
Outdated
| if t.satisfied { | ||
| infos = append(infos, t.info) | ||
| continue | ||
| } |
There was a problem hiding this comment.
Satisfied targets are still included in installInfos (built before the loop that skips t.satisfied) and therefore participate in checkDiskSpace/installSize. Since CurrentInfo() (used for satisfied targets) does not populate Info.DownloadInfo, this can trigger internal error: download info missing when disk-space checking is enabled. Exclude satisfied targets from installInfos (and disk-space checks) or otherwise ensure they have valid download info.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #16705 +/- ##
=======================================
Coverage 77.54% 77.54%
=======================================
Files 1360 1360
Lines 187369 187335 -34
Branches 2446 2446
=======================================
- Hits 145289 145274 -15
+ Misses 33304 33288 -16
+ Partials 8776 8773 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
bboozzoo
force-pushed
the
bboozzoo/snapstate-already-satisfied-install-many
branch
from
99d6c8e to
bab7912
Compare
|
Tue Mar 3 11:05:43 UTC 2026 Failures:Preparing:
Executing:
Restoring:
Skipped tests from snapd-testing-skip
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
overlord/snapstate/snapstate_install_test.go:6623
- The comment says "one taskset for other-snap installation" but this test is asserting the task set installs
some-snap(andother-snapis the already-present snap). Updating the comment will avoid confusion when reading/fixing this test later.
// one taskset for other-snap installation
c.Assert(tss, HasLen, 1)
…stalling from store The store installation goal supports skipping snaps which are already satisfied. In an extreme case, all requested snaps could already be installed in the system, which should not produce any installation tasks. Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>
bboozzoo
force-pushed
the
bboozzoo/snapstate-already-satisfied-install-many
branch
from
bab7912 to
e091519
Compare
pedronis
left a comment
pedronis
left a comment
There was a problem hiding this comment.
do the places that call InstallMany do the right thing we they get empty tss and tasks?
The store installation goal supports skipping snaps which are already satisfied. In an extreme case, all requested snaps could already be installed in the system, which should not produce any installation tasks.
Thanks for helping us make a better snapd!
Have you signed the license agreement and read the contribution guide?