o/snapstate: only return snaps that are changing revisions during ref…

…resh from snapstate.RefreshCandidates (canonical#14225)

* o/snapstate: only return snaps that are changing revisions during refresh from snapstate.RefreshCandidates

* tests: add spread tests for 'snap refresh --list'

* daemon: add missing architectures to snap info

* o/snapstate: rename refreshCandidates and storeUpdatePlan to storeUpdatePlan and storeUpdatePlanCore, respectively

This is the same naming convention as the previous introduced
refreshCandidates and refreshCandidatesCore.

* tests: fix shellcheck warning

daemon/api_find_test.go

@@ -84,6 +84,7 @@ func (s *findSuite) TestFindRefreshes(c *check.C) {
 		SideInfo: snap.SideInfo{
 			RealName: "store",
 		},
+		Architectures: []string{"all"},
 		Publisher: snap.StoreAccount{
 			ID:          "foo-id",
 			Username:    "foo",

overlord/snapstate/refreshhints.go

@@ -85,7 +85,7 @@ func (r *refreshHints) refresh() error {
 
 	var plan updatePlan
 	timings.Run(perfTimings, "refresh-candidates", "query store for refresh candidates", func(tm timings.Measurer) {
-		plan, err = refreshCandidates(auth.EnsureContextTODO(),
+		plan, err = storeUpdatePlan(auth.EnsureContextTODO(),
 			r.state, allSnaps, nil, nil, &store.RefreshOptions{RefreshManaged: refreshManaged}, Options{})
 	})
 	// TODO: we currently set last-refresh-hints even when there was an

overlord/snapstate/snapstate.go

@@ -1555,8 +1555,16 @@ func RefreshCandidates(st *state.State, user *auth.UserState) ([]*snap.Info, err
 		return nil, err
 	}
 
-	plan, err := refreshCandidates(context.TODO(), st, allSnaps, nil, user, nil, Options{})
-	return plan.targetInfos(), err
+	opts := Options{
+		PrereqTracker: snap.SimplePrereqTracker{},
+	}
+
+	plan, err := storeUpdatePlan(context.TODO(), st, allSnaps, nil, user, nil, opts)
+	if err != nil {
+		return nil, err
+	}
+
+	return plan.revisionChanges(st, opts)
 }
 
 // ValidateRefreshes allows to hook validation into the handling of refresh candidates.
@@ -1825,12 +1833,12 @@ type update struct {
 	Components []ComponentSetup
 }
 
-// satisfied returns true if the state of the snap on the system matches the
+// revisionSatisfied returns true if the state of the snap on the system matches the
 // state specified in the update. This method is primarily concerned with the
 // revision of the snap.
 //
 // TODO:COMPS: check if we need to change the state of components
-func (u *update) satisfied() bool {
+func (u *update) revisionSatisfied() bool {
 	if u.Setup.AlwaysUpdate || !u.SnapState.IsInstalled() {
 		return false
 	}
@@ -1940,7 +1948,7 @@ 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.satisfied() {
+		if up.revisionSatisfied() {
 			alreadySatisfied = append(alreadySatisfied, up)
 			continue
 		}
@@ -2216,7 +2224,7 @@ 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.satisfied()
+		updating[up.Setup.InstanceName()] = !up.revisionSatisfied()
 	}
 
 	// add explicitly auto-aliases only for snaps that are not updated
@@ -2554,7 +2562,7 @@ func autoRefreshPhase1(ctx context.Context, st *state.State, forGatingSnap strin
 
 	refreshOpts := &store.RefreshOptions{Scheduled: true}
 	// XXX: should we skip refreshCandidates if forGatingSnap isn't empty (meaning we're handling proceed from a snap)?
-	plan, err := refreshCandidates(ctx, st, allSnaps, nil, user, refreshOpts, Options{})
+	plan, err := storeUpdatePlan(ctx, st, allSnaps, nil, user, refreshOpts, Options{})
 	if err != nil {
 		// XXX: should we reset "refresh-candidates" to nil in state for some types
 		// of errors?

overlord/snapstate/snapstate_update_test.go

@@ -13487,3 +13487,33 @@ func (s *snapmgrTestSuite) TestUpdateStateConflictRemoved(c *C) {
 	c.Check(err, testutil.ErrorIs, &snapstate.ChangeConflictError{})
 	c.Assert(err, ErrorMatches, `snap "some-snap" has changes in progress`)
 }
+
+func (s *snapmgrTestSuite) TestRefreshCandidates(c *C) {
+	s.state.Lock()
+	defer s.state.Unlock()
+
+	snapstate.Set(s.state, "some-snap", &snapstate.SnapState{
+		Active:          true,
+		TrackingChannel: "latest/stable",
+		Sequence: snapstatetest.NewSequenceFromSnapSideInfos([]*snap.SideInfo{
+			{RealName: "some-snap", SnapID: "some-snap-id", Revision: snap.R(2)},
+		}),
+		Current:  snap.R(2),
+		SnapType: "app",
+	})
+
+	snapstate.Set(s.state, "some-other-snap", &snapstate.SnapState{
+		Active:          true,
+		TrackingChannel: "channel-for-7/stable",
+		Sequence: snapstatetest.NewSequenceFromSnapSideInfos([]*snap.SideInfo{
+			{RealName: "some-other-snap", SnapID: "some-other-snap-id", Revision: snap.R(7)},
+		}),
+		Current:  snap.R(7),
+		SnapType: "app",
+	})
+
+	candidates, err := snapstate.RefreshCandidates(s.state, nil)
+	c.Assert(err, IsNil)
+	c.Assert(candidates, HasLen, 1)
+	c.Check(candidates[0].InstanceName(), Equals, "some-snap")
+}

overlord/snapstate/storehelpers.go

@@ -471,7 +471,7 @@ func collectCurrentSnaps(snapStates map[string]*SnapState, holds map[string][]st
 	return curSnaps, nil
 }
 
-// refreshCandidates is a wrapper for storeUpdatePlan.
+// storeUpdatePlan is a wrapper for storeUpdatePlanCore.
 //
 // It addresses the case where the store doesn't return refresh candidates for
 // snaps with already existing monitored refresh-candidates due to inconsistent
@@ -481,14 +481,14 @@ func collectCurrentSnaps(snapStates map[string]*SnapState, holds map[string][]st
 //
 // Note: This wrapper is a short term solution and should be removed once a better
 // solution is reached.
-func refreshCandidates(ctx context.Context, st *state.State, allSnaps map[string]*SnapState, requested map[string]StoreUpdate, user *auth.UserState, refreshOpts *store.RefreshOptions, opts Options) (updatePlan, error) {
+func storeUpdatePlan(ctx context.Context, st *state.State, allSnaps map[string]*SnapState, requested map[string]StoreUpdate, user *auth.UserState, refreshOpts *store.RefreshOptions, opts Options) (updatePlan, error) {
 	// initialize options before using
 	refreshOpts, err := refreshOptions(st, refreshOpts)
 	if err != nil {
 		return updatePlan{}, err
 	}
 
-	plan, err := storeUpdatePlan(ctx, st, allSnaps, requested, user, refreshOpts, opts)
+	plan, err := storeUpdatePlanCore(ctx, st, allSnaps, requested, user, refreshOpts, opts)
 	if err != nil {
 		return updatePlan{}, err
 	}
@@ -547,7 +547,7 @@ func refreshCandidates(ctx context.Context, st *state.State, allSnaps map[string
 		// we already started a pre-download for this snap, so no extra
 		// load is being exerted on the store.
 		refreshOpts.Scheduled = false
-		extraPlan, err := storeUpdatePlan(ctx, st, allSnaps, missingRequests, user, refreshOpts, opts)
+		extraPlan, err := storeUpdatePlanCore(ctx, st, allSnaps, missingRequests, user, refreshOpts, opts)
 		if err != nil {
 			return updatePlan{}, err
 		}
@@ -557,7 +557,7 @@ func refreshCandidates(ctx context.Context, st *state.State, allSnaps map[string
 	return plan, nil
 }
 
-func storeUpdatePlan(
+func storeUpdatePlanCore(
 	ctx context.Context,
 	st *state.State,
 	allSnaps map[string]*SnapState,

overlord/snapstate/target.go

@@ -796,6 +796,62 @@ func (p *updatePlan) targetInfos() []*snap.Info {
 	return infos
 }
 
+// updates returns the updates that should be applied to the system's state for
+// this plan.
+func (p *updatePlan) updates(st *state.State, opts Options) ([]update, error) {
+	updates := make([]update, 0, len(p.targets))
+	for _, t := range p.targets {
+		opts.PrereqTracker.Add(t.info)
+
+		snapsup, compsups, err := t.setups(st, opts)
+		if err != nil {
+			if !p.refreshAll() {
+				return nil, err
+			}
+
+			logger.Noticef("cannot refresh snap %q: %v", t.info.InstanceName(), err)
+			continue
+		}
+
+		updates = append(updates, update{
+			Setup:      snapsup,
+			SnapState:  t.snapst,
+			Components: compsups,
+		})
+	}
+	return updates, nil
+}
+
+// revisionChanges returns the snaps that will have their revisions changed by
+// the updates in this plan.
+func (p *updatePlan) revisionChanges(st *state.State, opts Options) ([]*snap.Info, error) {
+	updates, err := p.updates(st, opts)
+	if err != nil {
+		return nil, err
+	}
+
+	targetByName := make(map[string]target, len(p.targets))
+	for _, t := range p.targets {
+		targetByName[t.info.InstanceName()] = t
+	}
+
+	changes := make([]*snap.Info, 0, len(updates))
+	for _, up := range updates {
+		if up.revisionSatisfied() {
+			continue
+		}
+
+		t, ok := targetByName[up.SnapState.InstanceName()]
+		// this should never happen
+		if !ok {
+			return nil, fmt.Errorf("internal error: update %q not found in targets", up.SnapState.InstanceName())
+		}
+
+		changes = append(changes, t.info)
+	}
+	return changes, nil
+}
+
 // filter applies the given function to each target in the update plan and
 // removes any targets for which the function returns false.
 func (p *updatePlan) filter(f func(t target) (bool, error)) error {
@@ -992,25 +1048,9 @@ func updateFromPlan(st *state.State, plan updatePlan, opts Options) ([]string, *
 	// it is sad that we have to split up updatePlan like this, but doUpdate is
 	// used in places where we don't have a snap.Info, so we cannot pass an
 	// updatePlan to doUpdate
-	updates := make([]update, 0, len(plan.targets))
-	for _, t := range plan.targets {
-		opts.PrereqTracker.Add(t.info)
-
-		snapsup, compsups, err := t.setups(st, opts)
-		if err != nil {
-			if !plan.refreshAll() {
-				return nil, nil, err
-			}
-
-			logger.Noticef("cannot refresh snap %q: %v", t.info.InstanceName(), err)
-			continue
-		}
-
-		updates = append(updates, update{
-			Setup:      snapsup,
-			SnapState:  t.snapst,
-			Components: compsups,
-		})
+	updates, err := plan.updates(st, opts)
+	if err != nil {
+		return nil, nil, err
 	}
 
 	updated, uts, err := doPotentiallySplitUpdate(st, plan.requested, updates, opts)
@@ -1086,7 +1126,7 @@ func (s *storeUpdateGoal) toUpdate(ctx context.Context, st *state.State, opts Op
 	}
 
 	refreshOpts := &store.RefreshOptions{Scheduled: opts.Flags.IsAutoRefresh}
-	plan, err := refreshCandidates(ctx, st, allSnaps, s.snaps, user, refreshOpts, opts)
+	plan, err := storeUpdatePlan(ctx, st, allSnaps, s.snaps, user, refreshOpts, opts)
 	if err != nil {
 		return updatePlan{}, err
 	}

tests/main/refresh-all/task.yaml

@@ -45,6 +45,7 @@ execute: |
     fi
 
     echo "Precondition check for the fake store"
+    snap refresh --list 2>&1 | MATCH "All snaps up to date."
     snap refresh 2>&1 | MATCH "All snaps up to date."
 
     echo "When the store is configured to make them refreshable"
@@ -53,6 +54,12 @@ execute: |
     "$TESTSTOOLS"/store-state init-fake-refreshes  "$BLOB_DIR" test-snapd-python-webserver
     retry -n 4 --wait 0.5 test -e "$BLOB_DIR"/test-snapd-python-webserver*fake1*.snap
 
+    # make sure that "snap refresh --list" correctly shows the new revisions
+    snap refresh --list > refresh-list.out 2>&1
+    MATCH 'test-snapd-python-webserver\s+16.04-3\+fake1\s+7' < refresh-list.out
+    MATCH 'test-snapd-tools\s+1.0\+fake1\s+10' < refresh-list.out
+    MATCH 'test-snapd-tools_instance\s+1.0\+fake1\s+10' < refresh-list.out
+
     echo "And a refresh is performed"
     snap refresh