o/snapstate: don't notify of pending refresh if it's manual (#12473)

* o/snapstate: don't notify of pending refresh if it's manual Don't trigger a notification that a refresh is pending due to a running snap if the refresh was triggered by the user. It's unnecessary since the error message will already carry the same information. We'll still trigger the notification if auto-refreshes are blocked. Signed-off-by: Miguel Pires <[email protected]> * o/snapstate: use SnapSetup to check for autorefresh Signed-off-by: Miguel Pires <[email protected]> * o/snapstate: reorder parameters in inhibitRefresh Signed-off-by: Miguel Pires <[email protected]> Signed-off-by: Miguel Pires <[email protected]>
canonical · Jan 12, 2023 · 8bb01e3 · 8bb01e3
1 parent 402677d
commit 8bb01e3
Show file tree

Hide file tree

Showing 6 changed files with 56 additions and 16 deletions.
diff --git a/overlord/snapstate/autorefresh.go b/overlord/snapstate/autorefresh.go
@@ -599,7 +599,7 @@ var asyncPendingRefreshNotification = func(context context.Context, client *user
 // Internally the snap state is updated to remember when the inhibition first
 // took place. Apps can inhibit refreshes for up to "maxInhibition", beyond
 // that period the refresh will go ahead despite application activity.
-func inhibitRefresh(st *state.State, snapst *SnapState, info *snap.Info, checker func(*snap.Info) error) error {
+func inhibitRefresh(st *state.State, snapst *SnapState, snapsup *SnapSetup, info *snap.Info, checker func(*snap.Info) error) error {
 	checkerErr := checker(info)
 	if checkerErr == nil {
 		return nil
@@ -641,8 +641,12 @@ func inhibitRefresh(st *state.State, snapst *SnapState, info *snap.Info, checker
 		checkerErr = nil
 	}
 
-	// Send the notification asynchronously to avoid holding the state lock.
-	asyncPendingRefreshNotification(context.TODO(), userclient.New(), refreshInfo)
+	// if the refresh is manual the error message already carries the information so don't notify
+	if snapsup.IsAutoRefresh {
+		// Send the notification asynchronously to avoid holding the state lock.
+		asyncPendingRefreshNotification(context.TODO(), userclient.New(), refreshInfo)
+	}
+
 	return checkerErr
 }
 

diff --git a/overlord/snapstate/autorefresh_test.go b/overlord/snapstate/autorefresh_test.go
@@ -942,7 +942,9 @@ func (s *autoRefreshTestSuite) TestInitialInhibitRefreshWithinInhibitWindow(c *C
 		Sequence: []*snap.SideInfo{si},
 		Current:  si.Revision,
 	}
-	err := snapstate.InhibitRefresh(s.state, snapst, info, func(si *snap.Info) error {
+	snapsup := &snapstate.SnapSetup{Flags: snapstate.Flags{IsAutoRefresh: true}}
+
+	err := snapstate.InhibitRefresh(s.state, snapst, snapsup, info, func(si *snap.Info) error {
 		return snapstate.NewBusySnapError(si, []int{123}, nil, nil)
 	})
 	c.Assert(err, ErrorMatches, `snap "pkg" has running apps or hooks, pids: 123`)
@@ -973,8 +975,9 @@ func (s *autoRefreshTestSuite) TestSubsequentInhibitRefreshWithinInhibitWindow(c
 		Current:              si.Revision,
 		RefreshInhibitedTime: &pastInstant,
 	}
+	snapsup := &snapstate.SnapSetup{Flags: snapstate.Flags{IsAutoRefresh: true}}
 
-	err := snapstate.InhibitRefresh(s.state, snapst, info, func(si *snap.Info) error {
+	err := snapstate.InhibitRefresh(s.state, snapst, snapsup, info, func(si *snap.Info) error {
 		return snapstate.NewBusySnapError(si, []int{123}, nil, nil)
 	})
 	c.Assert(err, ErrorMatches, `snap "pkg" has running apps or hooks, pids: 123`)
@@ -1003,9 +1006,38 @@ func (s *autoRefreshTestSuite) TestInhibitRefreshRefreshesWhenOverdue(c *C) {
 		Current:              si.Revision,
 		RefreshInhibitedTime: &pastInstant,
 	}
-	err := snapstate.InhibitRefresh(s.state, snapst, info, func(si *snap.Info) error {
+	snapsup := &snapstate.SnapSetup{Flags: snapstate.Flags{IsAutoRefresh: true}}
+
+	err := snapstate.InhibitRefresh(s.state, snapst, snapsup, info, func(si *snap.Info) error {
 		return &snapstate.BusySnapError{SnapInfo: si}
 	})
 	c.Assert(err, IsNil)
 	c.Check(notificationCount, Equals, 1)
 }
+
+func (s *autoRefreshTestSuite) TestInhibitNoNotificationOnManualRefresh(c *C) {
+	s.state.Lock()
+	defer s.state.Unlock()
+
+	restore := snapstate.MockAsyncPendingRefreshNotification(func(ctx context.Context, client *userclient.Client, refreshInfo *userclient.PendingSnapRefreshInfo) {
+		c.Fatal("shouldn't trigger pending refresh notification if refresh was manual")
+	})
+	defer restore()
+
+	pastInstant := time.Now().Add(-snapstate.MaxInhibition)
+
+	si := &snap.SideInfo{RealName: "pkg", Revision: snap.R(1)}
+	info := &snap.Info{SideInfo: *si}
+	snapst := &snapstate.SnapState{
+		Sequence:             []*snap.SideInfo{si},
+		Current:              si.Revision,
+		RefreshInhibitedTime: &pastInstant,
+	}
+	// manual refresh
+	snapsup := &snapstate.SnapSetup{Flags: snapstate.Flags{IsAutoRefresh: false}}
+
+	err := snapstate.InhibitRefresh(s.state, snapst, snapsup, info, func(si *snap.Info) error {
+		return &snapstate.BusySnapError{SnapInfo: si}
+	})
+	c.Assert(err, IsNil)
+}
diff --git a/overlord/snapstate/handlers.go b/overlord/snapstate/handlers.go
@@ -1033,7 +1033,7 @@ func (m *SnapManager) doUnlinkCurrentSnap(t *state.Task, _ *tomb.Tomb) (err erro
 		// held to prevent snap-run from advancing until UnlinkSnap, executed
 		// below, completes.
 		// XXX: should we skip it if type is snap.TypeSnapd?
-		lock, err := hardEnsureNothingRunningDuringRefresh(m.backend, st, snapst, oldInfo)
+		lock, err := hardEnsureNothingRunningDuringRefresh(m.backend, st, snapst, snapsup, oldInfo)
 		if err != nil {
 			return err
 		}

diff --git a/overlord/snapstate/refresh.go b/overlord/snapstate/refresh.go
@@ -204,13 +204,13 @@ func (err BusySnapError) Pids() []int {
 //
 // In practice, we either inhibit app startup and refresh the snap _or_ inhibit
 // the refresh change and continue running existing app processes.
-func hardEnsureNothingRunningDuringRefresh(backend managerBackend, st *state.State, snapst *SnapState, info *snap.Info) (*osutil.FileLock, error) {
+func hardEnsureNothingRunningDuringRefresh(backend managerBackend, st *state.State, snapst *SnapState, snapsup *SnapSetup, info *snap.Info) (*osutil.FileLock, error) {
 	return backend.RunInhibitSnapForUnlink(info, runinhibit.HintInhibitedForRefresh, func() error {
 		// In case of successful refresh inhibition the snap state is modified
 		// to indicate when the refresh was first inhibited. If the first
 		// refresh inhibition is outside of a grace period then refresh
 		// proceeds regardless of the existing processes.
-		return inhibitRefresh(st, snapst, info, HardNothingRunningRefreshCheck)
+		return inhibitRefresh(st, snapst, snapsup, info, HardNothingRunningRefreshCheck)
 	})
 }
 
@@ -225,14 +225,14 @@ func hardEnsureNothingRunningDuringRefresh(backend managerBackend, st *state.Sta
 // refresh was first postponed. Eventually the check does not fail, even if
 // non-service apps are running, because this mechanism only allows postponing
 // refreshes for a bounded amount of time.
-func softCheckNothingRunningForRefresh(st *state.State, snapst *SnapState, info *snap.Info) error {
+func softCheckNothingRunningForRefresh(st *state.State, snapst *SnapState, snapsup *SnapSetup, info *snap.Info) error {
 	// Grab per-snap lock to prevent new processes from starting. This is
 	// sufficient to perform the check, even though individual processes may
 	// fork or exit, we will have per-security-tag information about what is
 	// running.
 	return backend.WithSnapLock(info, func() error {
 		// Perform the soft refresh viability check, possibly writing to the state
 		// on failure.
-		return inhibitRefresh(st, snapst, info, SoftNothingRunningRefreshCheck)
+		return inhibitRefresh(st, snapst, snapsup, info, SoftNothingRunningRefreshCheck)
 	})
 }
diff --git a/overlord/snapstate/refresh_test.go b/overlord/snapstate/refresh_test.go
@@ -202,6 +202,7 @@ func (s *refreshSuite) TestDoSoftRefreshCheckAllowed(c *C) {
 	s.state.Lock()
 	defer s.state.Unlock()
 	snapst, info := s.addFakeInstalledSnap()
+	snapsup := &snapstate.SnapSetup{Flags: snapstate.Flags{IsAutoRefresh: true}}
 
 	// Pretend that snaps can refresh normally.
 	restore := snapstate.MockGenericRefreshCheck(func(info *snap.Info, canAppRunDuringRefresh func(app *snap.AppInfo) bool) error {
@@ -210,7 +211,7 @@ func (s *refreshSuite) TestDoSoftRefreshCheckAllowed(c *C) {
 	defer restore()
 
 	// Soft refresh should not fail.
-	err := snapstate.SoftCheckNothingRunningForRefresh(s.state, snapst, info)
+	err := snapstate.SoftCheckNothingRunningForRefresh(s.state, snapst, snapsup, info)
 	c.Assert(err, IsNil)
 
 	// In addition, the inhibition lock is not set.
@@ -224,6 +225,7 @@ func (s *refreshSuite) TestDoSoftRefreshCheckDisallowed(c *C) {
 	s.state.Lock()
 	defer s.state.Unlock()
 	snapst, info := s.addFakeInstalledSnap()
+	snapsup := &snapstate.SnapSetup{Flags: snapstate.Flags{IsAutoRefresh: true}}
 
 	// Pretend that snaps cannot refresh.
 	restore := snapstate.MockGenericRefreshCheck(func(info *snap.Info, canAppRunDuringRefresh func(app *snap.AppInfo) bool) error {
@@ -232,7 +234,7 @@ func (s *refreshSuite) TestDoSoftRefreshCheckDisallowed(c *C) {
 	defer restore()
 
 	// Soft refresh should fail with a proper error.
-	err := snapstate.SoftCheckNothingRunningForRefresh(s.state, snapst, info)
+	err := snapstate.SoftCheckNothingRunningForRefresh(s.state, snapst, snapsup, info)
 	c.Assert(err, ErrorMatches, `snap "pkg" has running apps or hooks, pids: 123`)
 
 	// Validity check: the inhibition lock was not set.
@@ -247,6 +249,7 @@ func (s *refreshSuite) TestDoHardRefreshFlowRefreshAllowed(c *C) {
 	s.state.Lock()
 	defer s.state.Unlock()
 	snapst, info := s.addFakeInstalledSnap()
+	snapsup := &snapstate.SnapSetup{Flags: snapstate.Flags{IsAutoRefresh: true}}
 
 	// Pretend that snaps can refresh normally.
 	restore := snapstate.MockGenericRefreshCheck(func(info *snap.Info, canAppRunDuringRefresh func(app *snap.AppInfo) bool) error {
@@ -255,7 +258,7 @@ func (s *refreshSuite) TestDoHardRefreshFlowRefreshAllowed(c *C) {
 	defer restore()
 
 	// Hard refresh should not fail and return a valid lock.
-	lock, err := snapstate.HardEnsureNothingRunningDuringRefresh(backend, s.state, snapst, info)
+	lock, err := snapstate.HardEnsureNothingRunningDuringRefresh(backend, s.state, snapst, snapsup, info)
 	c.Assert(err, IsNil)
 	c.Assert(lock, NotNil)
 	defer lock.Close()
@@ -275,6 +278,7 @@ func (s *refreshSuite) TestDoHardRefreshFlowRefreshDisallowed(c *C) {
 	s.state.Lock()
 	defer s.state.Unlock()
 	snapst, info := s.addFakeInstalledSnap()
+	snapsup := &snapstate.SnapSetup{Flags: snapstate.Flags{IsAutoRefresh: true}}
 
 	// Pretend that snaps cannot refresh.
 	restore := snapstate.MockGenericRefreshCheck(func(info *snap.Info, canAppRunDuringRefresh func(app *snap.AppInfo) bool) error {
@@ -283,7 +287,7 @@ func (s *refreshSuite) TestDoHardRefreshFlowRefreshDisallowed(c *C) {
 	defer restore()
 
 	// Hard refresh should fail and not return a lock.
-	lock, err := snapstate.HardEnsureNothingRunningDuringRefresh(backend, s.state, snapst, info)
+	lock, err := snapstate.HardEnsureNothingRunningDuringRefresh(backend, s.state, snapst, snapsup, info)
 	c.Assert(err, ErrorMatches, `snap "pkg" has running apps or hooks, pids: 123`)
 	c.Assert(lock, IsNil)
 

diff --git a/overlord/snapstate/snapstate.go b/overlord/snapstate/snapstate.go
@@ -370,7 +370,7 @@ func doInstall(st *state.State, snapst *SnapState, snapsup *SnapSetup, flags int
 			// Note that because we are modifying the snap state inside
 			// softCheckNothingRunningForRefresh, this block must be located
 			// after the conflict check done above.
-			if err := softCheckNothingRunningForRefresh(st, snapst, info); err != nil {
+			if err := softCheckNothingRunningForRefresh(st, snapst, snapsup, info); err != nil {
 				return nil, err
 			}
 		}