o/snapstate, o/s/backend: move SortServices into Backend.StartServices

Author: Mohit-Chachada

overlord/snapstate/backend/link.go

@@ -245,24 +245,24 @@ 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 {
+	// Services need to be sorted according to their Before
+	// and After requirements
+	startupOrdered, err := snap.SortServices(apps)
+	if err != nil {
+		return err
+	}
 	opts := &wrappers.StartServicesOptions{Enable: true}
-	return wrappersStartServices(apps, disabledSvcs, opts, meter, tm)
+	return wrappersStartServices(startupOrdered, disabledSvcs, opts, 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 b.StartServices(apps, disabledSvcs, meter, tm)
 	})
 	return wrappersStopServices(apps, removedSvcs, nil, reason, meter, tm)
 }

overlord/snapstate/backend/link_test.go

@@ -1234,6 +1234,44 @@ func (s *linkSuite) TestStartServices(c *C) {
 	c.Assert(called, Equals, 1)
 }
 
+func (s *linkSuite) TestStartServicesSortsServices(c *C) {
+	var sortedNames []string
+	restore := backend.MockWrappersStartServices(func(apps []*snap.AppInfo, disabledSvcs *wrappers.DisabledServices, opts *wrappers.StartServicesOptions, inter wrappers.Interacter, tm timings.Measurer) error {
+		sortedNames = make([]string, len(apps))
+		for i, app := range apps {
+			sortedNames[i] = app.Name
+		}
+		return nil
+	})
+	defer restore()
+
+	svc1 := &snap.AppInfo{Name: "svc1", Before: []string{"svc3"}}
+	svc2 := &snap.AppInfo{Name: "svc2", After: []string{"svc1"}}
+	svc3 := &snap.AppInfo{Name: "svc3", Before: []string{"svc2"}}
+
+	// pass in unsorted order
+	apps := []*snap.AppInfo{svc1, svc2, svc3}
+	err := s.be.StartServices(apps, nil, progress.Null, s.perfTimings)
+	c.Assert(err, IsNil)
+	// wrappers.StartServices should receive them sorted
+	c.Check(sortedNames, DeepEquals, []string{"svc1", "svc3", "svc2"})
+}
+
+func (s *linkSuite) TestStartServicesFailsOnCycle(c *C) {
+	restore := backend.MockWrappersStartServices(func(apps []*snap.AppInfo, disabledSvcs *wrappers.DisabledServices, opts *wrappers.StartServicesOptions, inter wrappers.Interacter, tm timings.Measurer) error {
+		c.Fatal("wrappers.StartServices should not be called when sorting fails")
+		return nil
+	})
+	defer restore()
+
+	svc1 := &snap.AppInfo{Name: "svc1", After: []string{"svc2"}}
+	svc2 := &snap.AppInfo{Name: "svc2", After: []string{"svc1"}}
+
+	apps := []*snap.AppInfo{svc1, svc2}
+	err := s.be.StartServices(apps, nil, progress.Null, s.perfTimings)
+	c.Assert(err, ErrorMatches, "applications are part of a before/after cycle: .*")
+}
+
 type nullUndoer struct{}
 
 func (nu nullUndoer) AddUndo(f func() error) {}

overlord/snapstate/backend_test.go

@@ -1460,13 +1460,17 @@ func svcSnapMountDir(svcs []*snap.AppInfo) string {
 }
 
 func (f *fakeSnappyBackend) StartServices(svcs []*snap.AppInfo, disabledSvcs *wrappers.DisabledServices, meter progress.Meter, tm timings.Measurer) error {
-	services := make([]string, 0, len(svcs))
-	for _, svc := range svcs {
+	startupOrdered, err := snap.SortServices(svcs)
+	if err != nil {
+		return err
+	}
+	services := make([]string, 0, len(startupOrdered))
+	for _, svc := range startupOrdered {
 		services = append(services, svc.Name)
 	}
 	op := fakeOp{
 		op:       "start-snap-services",
-		path:     svcSnapMountDir(svcs),
+		path:     svcSnapMountDir(startupOrdered),
 		services: services,
 	}
 	// only add the services to the op if there's something to add
@@ -1505,11 +1509,7 @@ func (f *fakeSnappyBackend) StopServices(svcs []*snap.AppInfo, rmSvcs map[string
 	}
 
 	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)
+		return f.StartServices(svcs, disabledSvcs, meter, tm)
 	})
 
 	f.appendOp(&fakeOp{

overlord/snapstate/handlers.go

@@ -3505,15 +3505,10 @@ func (m *SnapManager) startSnapServices(t *state.Task, _ *tomb.Tomb) error {
 		return nil
 	}
 
-	startupOrdered, err := snap.SortServices(svcs)
-	if err != nil {
-		return err
-	}
-
 	pb := NewTaskProgressAdapterUnlocked(t)
 
 	st.Unlock()
-	err = m.backend.StartServices(startupOrdered, &wrappers.DisabledServices{
+	err = m.backend.StartServices(svcs, &wrappers.DisabledServices{
 		SystemServices: missingSvcsOverview.FoundSystemServices,
 		UserServices:   missingSvcsOverview.FoundUserServices,
 	}, pb, perfTimings)
@@ -3715,11 +3710,6 @@ func (m *SnapManager) undoStopSnapServices(t *state.Task, _ *tomb.Tomb) error {
 		return nil
 	}
 
-	startupOrdered, err := snap.SortServices(svcs)
-	if err != nil {
-		return err
-	}
-
 	var oldLastActiveDisabledServices []string
 	var oldLastActiveDisabledUserServices map[int][]string
 	if err := t.Get("old-last-active-disabled-services", &oldLastActiveDisabledServices); err != nil && !errors.Is(err, state.ErrNoState) {
@@ -3738,7 +3728,7 @@ func (m *SnapManager) undoStopSnapServices(t *state.Task, _ *tomb.Tomb) error {
 	}
 
 	st.Unlock()
-	err = m.backend.StartServices(startupOrdered, &disabledServices, progress.Null, perfTimings)
+	err = m.backend.StartServices(svcs, &disabledServices, progress.Null, perfTimings)
 	st.Lock()
 	if err != nil {
 		return err