o/snapstate: simplify updating of refresh-candidates (#13626)

* o/snapstate: stop using setNewRefreshCandidates() when pruning Signed-off-by: Maciej Borzecki <[email protected]> * o/snapstate: drop calls to setNewRefreshCandidates from gated auto refresh Signed-off-by: Maciej Borzecki <[email protected]> * o/snapstate: drop unused refresh-candidates manipulation code Signed-off-by: Maciej Borzecki <[email protected]> --------- Signed-off-by: Maciej Borzecki <[email protected]>
canonical · Feb 26, 2024 · de66103 · de66103
1 parent acd3ee9
commit de66103
Show file tree

Hide file tree

Showing 4 changed files with 90 additions and 39 deletions.
diff --git a/overlord/snapstate/autorefresh_gating_test.go b/overlord/snapstate/autorefresh_gating_test.go
@@ -1467,6 +1467,14 @@ func (s *autorefreshGatingSuite) TestAutoRefreshPhase1(c *C) {
 			RealName: "snap-c",
 			Revision: snap.R(5),
 		},
+	}, {
+		Architectures: []string{"all"},
+		SnapType:      snap.TypeApp,
+		SideInfo: snap.SideInfo{
+			// this one will be monitored for running apps
+			RealName: "snap-f",
+			Revision: snap.R(6),
+		},
 	}}
 
 	st := s.state
@@ -1478,6 +1486,7 @@ func (s *autorefreshGatingSuite) TestAutoRefreshPhase1(c *C) {
 	mockInstalledSnap(c, s.state, snapCyaml, noHook)
 	mockInstalledSnap(c, s.state, baseSnapByaml, noHook)
 	mockInstalledSnap(c, s.state, snapDyaml, noHook)
+	mockInstalledSnap(c, s.state, snapFyaml, noHook)
 
 	restore := snapstatetest.MockDeviceModel(DefaultModel())
 	defer restore()
@@ -1492,10 +1501,19 @@ func (s *autorefreshGatingSuite) TestAutoRefreshPhase1(c *C) {
 		"snap-a": {"gating-snap"},
 		"snap-d": {"gating-snap"},
 	})
-
+	// we're already monitoring one of the snaps
+	st.Cache("monitored-snaps", map[string]context.CancelFunc{
+		"snap-f": func() {},
+	})
+	st.Set("refresh-candidates", map[string]*snapstate.RefreshCandidate{
+		"snap-f": {
+			SnapSetup: snapstate.SnapSetup{SideInfo: &snap.SideInfo{RealName: "snap-f", Revision: snap.R(6)}},
+			Monitored: true,
+		},
+	})
 	names, tss, err := snapstate.AutoRefreshPhase1(context.TODO(), st, "")
 	c.Assert(err, IsNil)
-	c.Check(names, DeepEquals, []string{"base-snap-b", "snap-a", "snap-c"})
+	c.Check(names, DeepEquals, []string{"base-snap-b", "snap-a", "snap-c", "snap-f"})
 	c.Assert(tss, HasLen, 2)
 
 	c.Assert(tss[0].Tasks(), HasLen, 1)
@@ -1542,6 +1560,21 @@ func (s *autorefreshGatingSuite) TestAutoRefreshPhase1(c *C) {
 				DownloadInfo: &snap.DownloadInfo{},
 			},
 		},
+		"snap-f": {
+			SnapSetup: snapstate.SnapSetup{
+				Type:      "app",
+				PlugsOnly: true,
+				Flags: snapstate.Flags{
+					IsAutoRefresh: true,
+				},
+				SideInfo: &snap.SideInfo{
+					RealName: "snap-f",
+					Revision: snap.R(6),
+				},
+				DownloadInfo: &snap.DownloadInfo{},
+			},
+			Monitored: true,
+		},
 	})
 
 	c.Assert(tss[1].Tasks(), HasLen, 2)
@@ -1566,10 +1599,12 @@ func (s *autorefreshGatingSuite) TestAutoRefreshPhase1(c *C) {
 	// check that refresh-candidates in the state were updated
 	var candidates map[string]*snapstate.RefreshCandidate
 	c.Assert(st.Get("refresh-candidates", &candidates), IsNil)
-	c.Assert(candidates, HasLen, 3)
+	c.Assert(candidates, HasLen, 4)
 	c.Check(candidates["snap-a"], NotNil)
 	c.Check(candidates["base-snap-b"], NotNil)
 	c.Check(candidates["snap-c"], NotNil)
+	c.Assert(candidates["snap-f"], NotNil)
+	c.Check(candidates["snap-f"].Monitored, Equals, true)
 
 	// check that after autoRefreshPhase1 any held snaps that are not in refresh
 	// candidates got removed.

diff --git a/overlord/snapstate/refreshhints.go b/overlord/snapstate/refreshhints.go
@@ -260,9 +260,15 @@ func pruneRefreshCandidates(st *state.State, snaps ...string) error {
 
 	for _, snapName := range snaps {
 		delete(candidates, snapName)
+		abortMonitoring(st, snapName)
+	}
+
+	if len(candidates) == 0 {
+		st.Set("refresh-candidates", nil)
+	} else {
+		st.Set("refresh-candidates", candidates)
 	}
 
-	setNewRefreshCandidates(st, candidates)
 	return nil
 }
 
@@ -318,37 +324,3 @@ func updateRefreshCandidates(st *state.State, hints map[string]*refreshCandidate
 	st.Set("refresh-candidates", oldHints)
 	return nil
 }
-
-// setNewRefreshCandidates is used to set/replace "refresh-candidates" making
-// sure that any snap that is no longer a candidate has its monitoring stopped.
-// Must always be used when replacing the full "refresh-candidates"
-func setNewRefreshCandidates(st *state.State, hints map[string]*refreshCandidate) {
-	stopMonitoringOutdatedCandidates(st, hints)
-	if len(hints) == 0 {
-		st.Set("refresh-candidates", nil)
-		return
-	}
-	st.Set("refresh-candidates", hints)
-}
-
-// stopMonitoringOutdatedCandidates aborts the monitoring for snaps for which a
-// refresh candidate has been removed (possibly because the channel was reverted
-// to an older version)
-func stopMonitoringOutdatedCandidates(st *state.State, hints map[string]*refreshCandidate) {
-	var oldHints map[string]*refreshCandidate
-	if err := st.Get("refresh-candidates", &oldHints); err != nil {
-		if errors.Is(err, &state.NoStateError{}) {
-			// nothing to abort
-			return
-		}
-
-		logger.Noticef("cannot abort removed refresh candidates: %v", err)
-		return
-	}
-
-	for oldCand := range oldHints {
-		if _, ok := hints[oldCand]; !ok {
-			abortMonitoring(st, oldCand)
-		}
-	}
-}
diff --git a/overlord/snapstate/refreshhints_test.go b/overlord/snapstate/refreshhints_test.go
@@ -374,9 +374,27 @@ func (s *refreshHintsTestSuite) TestPruneRefreshCandidates(c *C) {
 				},
 			},
 		},
+		"snap-f": {
+			SnapSetup: snapstate.SnapSetup{
+				Type: "app",
+				SideInfo: &snap.SideInfo{
+					RealName: "snap-c",
+					Revision: snap.R(1),
+				},
+			},
+			Monitored: true,
+		},
 	}
 	st.Set("refresh-candidates", candidates)
 
+	abortCalled := false
+
+	st.Cache("monitored-snaps", map[string]context.CancelFunc{
+		"snap-f": func() {
+			abortCalled = true
+		},
+	})
+
 	c.Assert(snapstate.PruneRefreshCandidates(st, "snap-a"), IsNil)
 
 	var candidates2 map[string]*snapstate.RefreshCandidate
@@ -387,6 +405,13 @@ func (s *refreshHintsTestSuite) TestPruneRefreshCandidates(c *C) {
 	c.Check(ok, Equals, true)
 	_, ok = candidates2["snap-c"]
 	c.Check(ok, Equals, true)
+	_, ok = candidates2["snap-f"]
+	c.Check(ok, Equals, true)
+	m := st.Cached("monitored-snaps")
+	monitored, ok := m.(map[string]context.CancelFunc)
+	c.Assert(ok, Equals, true)
+	c.Check(monitored["snap-f"], NotNil)
+	c.Check(abortCalled, Equals, false)
 
 	var candidates3 map[string]*snapstate.RefreshCandidate
 	c.Assert(snapstate.PruneRefreshCandidates(st, "snap-b"), IsNil)
@@ -397,6 +422,25 @@ func (s *refreshHintsTestSuite) TestPruneRefreshCandidates(c *C) {
 	c.Check(ok, Equals, false)
 	_, ok = candidates3["snap-c"]
 	c.Check(ok, Equals, true)
+	_, ok = candidates3["snap-f"]
+	c.Check(ok, Equals, true)
+	m = st.Cached("monitored-snaps")
+	monitored, ok = m.(map[string]context.CancelFunc)
+	c.Assert(ok, Equals, true)
+	c.Check(monitored["snap-f"], NotNil)
+	c.Check(abortCalled, Equals, false)
+
+	var candidates4 map[string]*snapstate.RefreshCandidate
+	c.Assert(snapstate.PruneRefreshCandidates(st, "snap-f"), IsNil)
+	c.Assert(st.Get("refresh-candidates", &candidates4), IsNil)
+	_, ok = candidates4["snap-c"]
+	c.Check(ok, Equals, true)
+	_, ok = candidates4["snap-f"]
+	c.Check(ok, Equals, false)
+	m = st.Cached("monitored-snaps")
+	// this is expected as the monitoring handler normally does the cleanup
+	c.Assert(m, NotNil)
+	c.Check(abortCalled, Equals, true)
 }
 
 func (s *refreshHintsTestSuite) TestPruneRefreshCandidatesIncorrectFormat(c *C) {

diff --git a/overlord/snapstate/snapstate.go b/overlord/snapstate/snapstate.go
@@ -2735,7 +2735,7 @@ func autoRefreshPhase1(ctx context.Context, st *state.State, forGatingSnap strin
 	if err != nil {
 		return nil, nil, err
 	}
-	setNewRefreshCandidates(st, hints)
+	updateRefreshCandidates(st, hints, nil)
 
 	// prune affecting snaps that are not in refresh candidates from hold state.
 	if err := pruneGating(st, hints); err != nil {