o/snapstate, o/s/backend: restart stopped services on error in stopSnapServices (#16861)

Transactionality improvement for `"stop-snap-services"` [[SNAPDENG-36594](https://warthogs.atlassian.net/browse/SNAPDENG-36594)]
* When `stopSnapServices` encounters an error mid-flight (e.g. during refresh), restart any services that were already stopped so that the snap services remain functional
* Disabled services are now queried before stopping and passed to `backend.StartServices`, so that in case of error leading to undo will only start previously enabled services.
* For remove/disable stop reasons the target is to have services stopped, so undo is skipped. **Note:** currently `undoStopSnapServices` still restarts all enabled services for a remove/disable if a future task in the change fails. We plan to change that as part of snap removal robustness improvements (at least for remove).


Add `UndoTracker` for rollback on in-flight error [[SNAPDENG-36593](https://warthogs.atlassian.net/browse/SNAPDENG-36593)]
* To better manage transactionality, add `UndoTracker` a LIFO cleanup stack that lets do handlers incrementally register undo closures for each step. On error (excluding `*state.Wait` and `*state.Retry`), the closure returned by `NewUndoTracker` executes the undos in reverse order, rolling back partial progress without knowing the undo handler's implementation details.
* The do handlers decide whether the undoes need to be registered with state lock held or released by passing either the `UndoTracker.Locked()` or `UndoTracker.Unlocked()`
* `NullUndoer` and `TODOUndoer` are also added to make it clear where undos are not needed or needed but not yet setup. This also helps in removing the need for non-nil `Undoer` check before registering undos.
* For `backend.StopServices`, the `Unlocked` adapter is used to register undoes

Author: Mohit-Chachada

overlord/snapstate/backend.go

@@ -91,7 +91,8 @@ type managerBackend interface {
 	LinkSnap(info *snap.Info, dev snap.Device, linkCtx backend.LinkContext, tm timings.Measurer) error
 	LinkComponent(cpi snap.ContainerPlaceInfo, snapRev snap.Revision) error
 	StartServices(svcs []*snap.AppInfo, disabledSvcs *wrappers.DisabledServices, meter progress.Meter, tm timings.Measurer) error
-	StopServices(svcs []*snap.AppInfo, removedSvcs map[string]*snap.AppInfo, reason snap.ServiceStopReason, meter progress.Meter, tm timings.Measurer) error
+	// TODO: reduce the number of arguments here, perhaps by grouping some
+	StopServices(svcs []*snap.AppInfo, removedSvcs map[string]*snap.AppInfo, disabledSvcs *wrappers.DisabledServices, reason snap.ServiceStopReason, undoer backend.Undoer, meter progress.Meter, tm timings.Measurer) error
 	QueryDisabledServices(info *snap.Info, pb progress.Meter) (*wrappers.DisabledServices, error)
 	MaybeSetNextBoot(info *snap.Info, dev snap.Device, isUndo bool) (boot.RebootInfo, error)
 

overlord/snapstate/backend/backend.go

@@ -30,6 +30,11 @@ type Backend struct {
 	preseed bool
 }
 
+// Undoer collects undo actions to reverse system changes on error.
+type Undoer interface {
+	AddUndo(f func() error)
+}
+
 // Candidate is a test hook.
 func (b Backend) Candidate(*snap.SideInfo) {}
 

overlord/snapstate/backend/export_test.go

@@ -27,6 +27,7 @@ import (
 	"github.com/snapcore/snapd/osutil/sys"
 	"github.com/snapcore/snapd/snap"
 	"github.com/snapcore/snapd/testutil"
+	"github.com/snapcore/snapd/timings"
 	"github.com/snapcore/snapd/wrappers"
 )
 
@@ -76,3 +77,11 @@ func MockKernelEnsureKernelDriversTree(f func(kMntPts kernel.MountPoints, compsM
 func MockCgroupKillSnapProcesses(f func(ctx context.Context, snapName string) error) func() {
 	return testutil.Mock(&cgroupKillSnapProcesses, f)
 }
+
+func MockWrappersStartServices(f func(apps []*snap.AppInfo, disabledSvcs *wrappers.DisabledServices, opts *wrappers.StartServicesOptions, inter wrappers.Interacter, tm timings.Measurer) error) func() {
+	return testutil.Mock(&wrappersStartServices, f)
+}
+
+func MockWrappersStopServices(f func(svcs []*snap.AppInfo, removedSvcs map[string]*snap.AppInfo, opts *wrappers.StopServicesOptions, reason snap.ServiceStopReason, inter wrappers.Interacter, tm timings.Measurer) error) func() {
+	return testutil.Mock(&wrappersStopServices, f)
+}

overlord/snapstate/backend/link.go

@@ -39,6 +39,8 @@ import (
 )
 
 var wrappersAddSnapdSnapServices = wrappers.AddSnapdSnapServices
+var wrappersStartServices = wrappers.StartServices
+var wrappersStopServices = wrappers.StopServices
 var cgroupKillSnapProcesses = cgroup.KillSnapProcesses
 
 // LinkContext carries additional information about the current or the previous
@@ -244,11 +246,25 @@ func (b Backend) LinkComponent(cpi snap.ContainerPlaceInfo, snapRev snap.Revisio
 
 func (b Backend) StartServices(apps []*snap.AppInfo, disabledSvcs *wrappers.DisabledServices, meter progress.Meter, tm timings.Measurer) error {
 	opts := &wrappers.StartServicesOptions{Enable: true}
-	return wrappers.StartServices(apps, disabledSvcs, opts, meter, tm)
+	return wrappersStartServices(apps, disabledSvcs, opts, meter, tm)
 }
 
-func (b Backend) StopServices(apps []*snap.AppInfo, removedSvcs map[string]*snap.AppInfo, reason snap.ServiceStopReason, meter progress.Meter, tm timings.Measurer) error {
-	return wrappers.StopServices(apps, removedSvcs, nil, reason, meter, tm)
+func (b Backend) StopServices(apps []*snap.AppInfo, removedSvcs map[string]*snap.AppInfo, disabledSvcs *wrappers.DisabledServices, reason snap.ServiceStopReason, undoer Undoer, meter progress.Meter, tm timings.Measurer) error {
+	// Register the undo before stopping so that services are
+	// started again even when StopServices fails partway through
+	// (some services stopped, then an error on a later one).
+	undoer.AddUndo(func() error {
+		// Services need to be sorted according to their Before
+		// and After requirements
+		startupOrdered, err := snap.SortServices(apps)
+		if err != nil {
+			return fmt.Errorf("cannot sort services for undo: %v", err)
+		}
+		// StartServices filters out disabled services, so only
+		// previously enabled services will be started again.
+		return b.StartServices(startupOrdered, disabledSvcs, meter, tm)
+	})
+	return wrappersStopServices(apps, removedSvcs, nil, reason, meter, tm)
 }
 
 func (b Backend) generateWrappers(s *snap.Info, linkCtx LinkContext) error {

overlord/snapstate/backend/link_test.go

@@ -1218,6 +1218,65 @@ func (s *linkSuite) TestKillSnapApps(c *C) {
 	c.Assert(called, Equals, 1)
 }
 
+func (s *linkSuite) TestStartServices(c *C) {
+	var called int
+	restore := backend.MockWrappersStartServices(func(apps []*snap.AppInfo, disabledSvcs *wrappers.DisabledServices, opts *wrappers.StartServicesOptions, inter wrappers.Interacter, tm timings.Measurer) error {
+		called++
+		c.Assert(apps, HasLen, 1)
+		c.Check(apps[0].Name, Equals, "svc")
+		return nil
+	})
+	defer restore()
+
+	apps := []*snap.AppInfo{{Name: "svc"}}
+	err := s.be.StartServices(apps, nil, progress.Null, s.perfTimings)
+	c.Assert(err, IsNil)
+	c.Assert(called, Equals, 1)
+}
+
+type nullUndoer struct{}
+
+func (nu nullUndoer) AddUndo(f func() error) {}
+
+func (s *linkSuite) TestStopServices(c *C) {
+	var called int
+	restore := backend.MockWrappersStopServices(func(svcs []*snap.AppInfo, removedSvcs map[string]*snap.AppInfo, opts *wrappers.StopServicesOptions, reason snap.ServiceStopReason, inter wrappers.Interacter, tm timings.Measurer) error {
+		called++
+		c.Assert(svcs, HasLen, 1)
+		c.Check(svcs[0].Name, Equals, "svc")
+		return nil
+	})
+	defer restore()
+
+	apps := []*snap.AppInfo{{Name: "svc"}}
+	err := s.be.StopServices(apps, nil, nil, snap.StopReasonRefresh, &nullUndoer{}, progress.Null, s.perfTimings)
+	c.Assert(err, IsNil)
+	c.Assert(called, Equals, 1)
+}
+
+type fakeUndoer struct {
+	undoFuncs []func() error
+}
+
+func (u *fakeUndoer) AddUndo(f func() error) {
+	u.undoFuncs = append(u.undoFuncs, f)
+}
+
+func (s *linkSuite) TestStopServicesWithNotNilUndoerRegistersUndo(c *C) {
+	restore := backend.MockWrappersStopServices(func(svcs []*snap.AppInfo, removedSvcs map[string]*snap.AppInfo, opts *wrappers.StopServicesOptions, reason snap.ServiceStopReason, inter wrappers.Interacter, tm timings.Measurer) error {
+		c.Assert(svcs, HasLen, 1)
+		c.Check(svcs[0].Name, Equals, "svc")
+		return errors.New("mock StopServices error")
+	})
+	defer restore()
+
+	undoer := &fakeUndoer{}
+	apps := []*snap.AppInfo{{Name: "svc"}}
+	err := s.be.StopServices(apps, nil, nil, snap.StopReasonRefresh, undoer, progress.Null, s.perfTimings)
+	c.Assert(err, ErrorMatches, "mock StopServices error")
+	c.Assert(undoer.undoFuncs, HasLen, 1)
+}
+
 func (s *linkSuite) TestLinkSnapNilStateUnlockerError(c *C) {
 	err := s.be.LinkSnap(nil, nil, backend.LinkContext{}, nil)
 	c.Assert(err, ErrorMatches, "internal error: LinkContext.StateUnlocker cannot be nil")

overlord/snapstate/backend_test.go

@@ -1482,7 +1482,7 @@ func (f *fakeSnappyBackend) StartServices(svcs []*snap.AppInfo, disabledSvcs *wr
 	return f.maybeErrForLastOp()
 }
 
-func (f *fakeSnappyBackend) StopServices(svcs []*snap.AppInfo, rmSvcs map[string]*snap.AppInfo, reason snap.ServiceStopReason, meter progress.Meter, tm timings.Measurer) error {
+func (f *fakeSnappyBackend) StopServices(svcs []*snap.AppInfo, rmSvcs map[string]*snap.AppInfo, disabledSvcs *wrappers.DisabledServices, reason snap.ServiceStopReason, undoer backend.Undoer, meter progress.Meter, tm timings.Measurer) error {
 	meter.Notify("stop-services")
 
 	var svcNames []string
@@ -1504,6 +1504,14 @@ func (f *fakeSnappyBackend) StopServices(svcs []*snap.AppInfo, rmSvcs map[string
 		sort.Strings(rmSvcNames)
 	}
 
+	undoer.AddUndo(func() error {
+		startupOrdered, err := snap.SortServices(svcs)
+		if err != nil {
+			return fmt.Errorf("cannot sort services for undo: %v", err)
+		}
+		return f.StartServices(startupOrdered, disabledSvcs, meter, tm)
+	})
+
 	f.appendOp(&fakeOp{
 		op:              fmt.Sprintf("stop-snap-services:%s", reason),
 		path:            svcSnapMountDir(svcs),

overlord/snapstate/handlers.go

@@ -3547,7 +3547,7 @@ func (m *SnapManager) undoStartSnapServices(t *state.Task, _ *tomb.Tomb) error {
 
 	// stop the services
 	st.Unlock()
-	err = m.backend.StopServices(svcs, nil, stopReason, progress.Null, perfTimings)
+	err = m.backend.StopServices(svcs, nil, nil, stopReason, NullUndoer, progress.Null, perfTimings)
 	st.Lock()
 	if err != nil {
 		return err
@@ -3556,11 +3556,30 @@ func (m *SnapManager) undoStartSnapServices(t *state.Task, _ *tomb.Tomb) error {
 	return nil
 }
 
-func (m *SnapManager) stopSnapServices(t *state.Task, _ *tomb.Tomb) error {
+func (m *SnapManager) stopSnapServices(t *state.Task, _ *tomb.Tomb) (retErr error) {
 	st := t.State()
 	st.Lock()
 	defer st.Unlock()
 
+	var stopReason snap.ServiceStopReason
+	if err := t.Get("stop-reason", &stopReason); err != nil && !errors.Is(err, state.ErrNoState) {
+		return err
+	}
+
+	// For remove/disable, the end goal is to have services stopped, so undo is skipped
+	// to avoid restarting the services in case of error. This aligns with making
+	// remove/disable best effort towards achieving their end goal.
+	// On the other hand, for example for refresh the end goal is to have the services
+	// running (just from a different revision), so undo is needed to restart the
+	// services in case of error.
+	undoerUnlocked := NullUndoer
+	skipUndo := stopReason == snap.StopReasonRemove || stopReason == snap.StopReasonDisable
+	if !skipUndo {
+		ut, undoOnError := NewUndoTracker(t, &retErr)
+		defer undoOnError()
+		undoerUnlocked = ut.Unlocked()
+	}
+
 	perfTimings := state.TimingsForTask(t)
 	defer perfTimings.Save(st)
 
@@ -3578,11 +3597,6 @@ func (m *SnapManager) stopSnapServices(t *state.Task, _ *tomb.Tomb) error {
 		return nil
 	}
 
-	var stopReason snap.ServiceStopReason
-	if err := t.Get("stop-reason", &stopReason); err != nil && !errors.Is(err, state.ErrNoState) {
-		return err
-	}
-
 	pb := NewTaskProgressAdapterUnlocked(t)
 	st.Unlock()
 	defer st.Lock()
@@ -3606,19 +3620,22 @@ func (m *SnapManager) stopSnapServices(t *state.Task, _ *tomb.Tomb) error {
 		}
 	}
 
-	// stop the services
-	err = m.backend.StopServices(svcs, rmSvcs, stopReason, pb, perfTimings)
+	// Query disabled services before stopping. This serves two purposes:
+	// 1. The undoer passes it to StartServices to skip disabled services,
+	//    so only previously enabled services are started again on error.
+	// 2. The result is persisted in the task state for undoStopSnapServices
+	//    to similarly know which services should remain disabled when starting
+	//    services again during undo.
+	// backend.StopServices does not change which services are disabled as it uses
+	// the default StopServicesOptions.Disable = false opts, so a single
+	// query before the stop is sufficient for both uses.
+	disabledServices, err := m.queryDisabledServices(currentInfo, pb)
 	if err != nil {
 		return err
 	}
 
-	// get the disabled services after we stopped all the services.
-	// this list is not meant to save what services are disabled at any given
-	// time, specifically just what services are disabled while systemd loses
-	// track of the services. this list is also used to determine what services are enabled
-	// when we start services of a new revision of the snap in
-	// start-snap-services handler.
-	disabledServices, err := m.queryDisabledServices(currentInfo, pb)
+	// stop the services
+	err = m.backend.StopServices(svcs, rmSvcs, disabledServices, stopReason, undoerUnlocked, pb, perfTimings)
 	if err != nil {
 		return err
 	}
@@ -3800,7 +3817,8 @@ func (m *SnapManager) doKillSnapApps(t *state.Task, _ *tomb.Tomb) (retErr error)
 	pb := NewTaskProgressAdapterUnlocked(t)
 
 	// Make sure snap services are stopped because they may have started through snapctl
-	err = m.backend.StopServices(svcs, nil, snap.ServiceStopReason(reason), pb, perfTimings)
+	// TODO: replace TODOUndoer with an UndoTracker for non-remove reason
+	err = m.backend.StopServices(svcs, nil, nil, snap.ServiceStopReason(reason), TODOUndoer, pb, perfTimings)
 	if err != nil {
 		return err
 	}

overlord/snapstate/snapstate_test.go

@@ -7995,15 +7995,15 @@ func (s *snapmgrTestSuite) TestStopSnapServicesUndo(c *C) {
 	c.Check(t.Status(), Equals, state.UndoneStatus)
 
 	expected := fakeOps{
+		{
+			op:               "current-snap-service-states",
+			disabledServices: []string{"svc1"},
+		},
 		{
 			op:       "stop-snap-services:",
 			path:     filepath.Join(dirs.SnapMountDir, "hello-snap/1"),
 			services: []string{"svc1", "svc2"},
 		},
-		{
-			op:               "current-snap-service-states",
-			disabledServices: []string{"svc1"},
-		},
 		{
 			op:               "start-snap-services",
 			services:         []string{"svc1", "svc2"},
@@ -8067,14 +8067,14 @@ func (s *snapmgrTestSuite) TestStopSnapServicesErrInUndo(c *C) {
 	c.Check(t.Status(), Equals, state.ErrorStatus)
 
 	expected := fakeOps{
+		{
+			op: "current-snap-service-states",
+		},
 		{
 			op:       "stop-snap-services:",
 			path:     filepath.Join(dirs.SnapMountDir, "hello-snap/1"),
 			services: []string{"svc1", "svc2"},
 		},
-		{
-			op: "current-snap-service-states",
-		},
 		{
 			// failed after this op
 			op:       "start-snap-services",
@@ -8085,6 +8085,81 @@ func (s *snapmgrTestSuite) TestStopSnapServicesErrInUndo(c *C) {
 	c.Check(s.fakeBackend.ops, DeepEquals, expected)
 }
 
+func (s *snapmgrTestSuite) testStopSnapServicesOnStopFailure(c *C, reason snap.ServiceStopReason) {
+	s.state.Lock()
+	defer s.state.Unlock()
+
+	prevCurrentlyDisabled := s.fakeBackend.servicesCurrentlyDisabled
+	s.fakeBackend.servicesCurrentlyDisabled = []string{"svc1"}
+
+	defer func() {
+		s.fakeBackend.servicesCurrentlyDisabled = prevCurrentlyDisabled
+	}()
+
+	si := &snap.SideInfo{RealName: "hello-snap", SnapID: "hello-snap-id", Revision: snap.R(1)}
+	snaptest.MockSnap(c, servicesSnap, si)
+
+	snapstate.Set(s.state, "hello-snap", &snapstate.SnapState{
+		Active:   true,
+		Sequence: snapstatetest.NewSequenceFromSnapSideInfos([]*snap.SideInfo{si}),
+		Current:  si.Revision,
+		SnapType: "app",
+	})
+
+	snapstate.MockSnapReadInfo(snap.ReadInfo)
+
+	chg := s.state.NewChange("services..", "")
+	t := s.state.NewTask("stop-snap-services", "")
+	sup := &snapstate.SnapSetup{SideInfo: si}
+	t.Set("snap-setup", sup)
+	t.Set("stop-reason", reason)
+	chg.AddTask(t)
+
+	// Mock error in StopServices
+	s.fakeBackend.maybeInjectErr = func(op *fakeOp) error {
+		if op.op == fmt.Sprintf("stop-snap-services:%s", reason) {
+			return fmt.Errorf("mock stop failure")
+		}
+		return nil
+	}
+
+	s.settle(c)
+
+	c.Check(chg.Status(), Equals, state.ErrorStatus)
+	c.Check(t.Status(), Equals, state.ErrorStatus)
+
+	expected := fakeOps{
+		{
+			op:               "current-snap-service-states",
+			disabledServices: []string{"svc1"},
+		},
+		{
+			op:       fmt.Sprintf("stop-snap-services:%s", reason),
+			path:     filepath.Join(dirs.SnapMountDir, "hello-snap/1"),
+			services: []string{"svc1", "svc2"},
+		},
+	}
+	skipUndo := reason == snap.StopReasonRemove || reason == snap.StopReasonDisable
+	if !skipUndo {
+		expected = append(expected, fakeOp{
+			// Should be triggered due to mock error in StopServices
+			op:               "start-snap-services",
+			services:         []string{"svc1", "svc2"},
+			disabledServices: []string{"svc1"},
+			path:             filepath.Join(dirs.SnapMountDir, "hello-snap/1"),
+		})
+	}
+	c.Check(s.fakeBackend.ops, DeepEquals, expected)
+}
+
+func (s *snapmgrTestSuite) TestStopSnapServicesStartsStoppedServicesOnStopFailureForRefresh(c *C) {
+	s.testStopSnapServicesOnStopFailure(c, snap.StopReasonRefresh)
+}
+
+func (s *snapmgrTestSuite) TestStopSnapServicesDoesNotStartStoppedServicesOnStopFailureForRemove(c *C) {
+	s.testStopSnapServicesOnStopFailure(c, snap.StopReasonRemove)
+}
+
 func (s *snapmgrTestSuite) TestEnsureAutoRefreshesAreDelayed(c *C) {
 	s.state.Lock()
 	defer s.state.Unlock()