o/snapstate,servicestate: use service-control task for service action…

…s (9/9) (canonical#8960)

This PR finalizes changes to the semantics of services as follows:
- when units for services are created on snap install/refresh (in link handler), systemd units are only created on disk but not immediately enabled.
- units are enabled later on start-snap-services. These two changes are reflected in wrappers/services.go and changes to AddSnapServies and StartServices.
- service commands executed from snap and snapctl are handled by new service-control-task. This unifies service handling for these commands as there are no ad-hoc calls to systemctl, but everything goes through wrappers. There is one task per snap in case of service commands affecting multiple snaps. The task carries snap name and affected services (if the list is empty, then all services of the snap are considered).
- for compatibility with old snapd though, old-style exec-command tasks are created alongside new type of tasks, but they have ignore flag set on them so new snapd skips them (logic for that landed already in cmdstate).
- disabled/enabled services are tracked with ServicesDisabledByHooks/ServicesEnabledByHooks in SnapState. This is needed for hooks that run between link-snap and start-snap-services, and may effect state of the services.

daemon/api.go

@@ -2461,16 +2461,19 @@ func postApps(c *Command, r *http.Request, user *auth.UserState) Response {
 		return InternalError("no services found")
 	}
 
-	tss, err := servicestateControl(st, appInfos, &inst, nil)
+	// do not pass flags - only create service-control tasks, do not create
+	// exec-command tasks for old snapd. These are not needed since we are
+	// handling momentary snap service commands.
+	st.Lock()
+	defer st.Unlock()
+	tss, err := servicestateControl(st, appInfos, &inst, nil, nil)
 	if err != nil {
 		// TODO: use errToResponse here too and introduce a proper error kind ?
 		if _, ok := err.(servicestate.ServiceActionConflictError); ok {
 			return Conflict(err.Error())
 		}
 		return BadRequest(err.Error())
 	}
-	st.Lock()
-	defer st.Unlock()
 	// names received in the request can be snap or snap.app, we need to
 	// extract the actual snap names before associating them with a change
 	chg := newChange(st, "service-control", fmt.Sprintf("Running service command"), tss, namesToSnapNames(&inst))

daemon/api_test.go

@@ -359,9 +359,10 @@ var modelDefaults = map[string]interface{}{
 	"kernel":       "kernel",
 }
 
-func (s *apiBaseSuite) fakeServiceControl(st *state.State, appInfos []*snap.AppInfo, inst *servicestate.Instruction, context *hookstate.Context) ([]*state.TaskSet, error) {
-	st.Lock()
-	defer st.Unlock()
+func (s *apiBaseSuite) fakeServiceControl(st *state.State, appInfos []*snap.AppInfo, inst *servicestate.Instruction, flags *servicestate.Flags, context *hookstate.Context) ([]*state.TaskSet, error) {
+	if flags != nil {
+		panic("flags are not expected")
+	}
 
 	if s.serviceControlError != nil {
 		return nil, s.serviceControlError

daemon/export_test.go

@@ -65,7 +65,7 @@ func MockShutdownTimeout(tm time.Duration) (restore func()) {
 	}
 }
 
-func MockServicestateControl(f func(st *state.State, appInfos []*snap.AppInfo, inst *servicestate.Instruction, context *hookstate.Context) ([]*state.TaskSet, error)) (restore func()) {
+func MockServicestateControl(f func(st *state.State, appInfos []*snap.AppInfo, inst *servicestate.Instruction, flags *servicestate.Flags, context *hookstate.Context) ([]*state.TaskSet, error)) (restore func()) {
 	old := servicestateControl
 	servicestateControl = f
 	return func() {

overlord/configstate/configcore/vitality.go

@@ -110,7 +110,7 @@ func handleVitalityConfiguration(tr config.Conf, opts *fsOnlyContext) error {
 			}
 
 			opts := &wrappers.AddSnapServicesOptions{VitalityRank: rank}
-			if err := wrappers.AddSnapServices(info, disabledSvcs, opts, progress.Null); err != nil {
+			if err := wrappers.AddSnapServices(info, opts, progress.Null); err != nil {
 				return err
 			}
 		}
@@ -120,8 +120,9 @@ func handleVitalityConfiguration(tr config.Conf, opts *fsOnlyContext) error {
 		if err != nil {
 			return err
 		}
+		flags := &wrappers.StartServicesFlags{Enable: true}
 		tm := timings.New(nil)
-		if err = wrappers.StartServices(startupOrdered, nil, nil, progress.Null, tm); err != nil {
+		if err = wrappers.StartServices(startupOrdered, disabledSvcs, flags, progress.Null, tm); err != nil {
 			return err
 		}
 	}

overlord/configstate/configcore/vitality_test.go

@@ -101,9 +101,8 @@ func (s *vitalitySuite) TestConfigureVitalityWithValidSnap(c *C) {
 	svcName := "snap.test-snap.foo.service"
 	c.Check(s.systemctlArgs, DeepEquals, [][]string{
 		{"is-enabled", "snap.test-snap.foo.service"},
-		{"enable", "snap.test-snap.foo.service"},
 		{"daemon-reload"},
-		{"is-enabled", "snap.test-snap.foo.service"},
+		{"enable", "snap.test-snap.foo.service"},
 		{"start", "snap.test-snap.foo.service"},
 	})
 	svcPath := filepath.Join(dirs.SnapServicesDir, svcName)

overlord/hookstate/context.go

@@ -252,3 +252,14 @@ func (c *Context) Done() error {
 func (c *Context) IsEphemeral() bool {
 	return c.task == nil
 }
+
+// ChangeID returns change ID for non-ephemeral context
+// or empty string otherwise.
+func (c *Context) ChangeID() string {
+	if task, ok := c.Task(); ok {
+		if chg := task.Change(); chg != nil {
+			return chg.ID()
+		}
+	}
+	return ""
+}

overlord/hookstate/context_test.go

@@ -164,3 +164,23 @@ func (s *contextSuite) TestEphemeralContextGetSet(c *C) {
 	// Test another non-existing key, but after the context data was created.
 	c.Check(context.Get("baz", &output), NotNil)
 }
+
+func (s *contextSuite) TestChangeID(c *C) {
+	context, err := NewContext(nil, s.state, &HookSetup{Snap: "test-snap"}, nil, "")
+	c.Assert(err, IsNil)
+	c.Check(context.ChangeID(), Equals, "")
+
+	s.state.Lock()
+	defer s.state.Unlock()
+
+	task := s.state.NewTask("foo", "")
+	context, err = NewContext(task, s.state, &HookSetup{Snap: "test-snap"}, nil, "")
+	c.Assert(err, IsNil)
+	c.Check(context.ChangeID(), Equals, "")
+
+	chg := s.state.NewChange("bar", "")
+	chg.AddTask(task)
+	context, err = NewContext(task, s.state, &HookSetup{Snap: "test-snap"}, nil, "")
+	c.Assert(err, IsNil)
+	c.Check(context.ChangeID(), Equals, chg.ID())
+}

overlord/hookstate/ctlcmd/export_test.go

@@ -30,7 +30,7 @@ import (
 
 var AttributesTask = attributesTask
 
-func MockServicestateControlFunc(f func(*state.State, []*snap.AppInfo, *servicestate.Instruction, *hookstate.Context) ([]*state.TaskSet, error)) (restore func()) {
+func MockServicestateControlFunc(f func(*state.State, []*snap.AppInfo, *servicestate.Instruction, *servicestate.Flags, *hookstate.Context) ([]*state.TaskSet, error)) (restore func()) {
 	old := servicestateControl
 	servicestateControl = f
 	return func() { servicestateControl = old }

overlord/hookstate/ctlcmd/helpers.go

@@ -141,8 +141,11 @@ func runServiceCommand(context *hookstate.Context, inst *servicestate.Instructio
 		return err
 	}
 
+	flags := &servicestate.Flags{CreateExecCommandTasks: true}
 	// passing context so we can ignore self-conflicts with the current change
-	tts, err := servicestateControl(st, appInfos, inst, context)
+	st.Lock()
+	tts, err := servicestateControl(st, appInfos, inst, flags, context)
+	st.Unlock()
 	if err != nil {
 		return err
 	}

overlord/hookstate/ctlcmd/services_test.go

@@ -130,7 +130,7 @@ apps:
 `
 
 func mockServiceChangeFunc(testServiceControlInputs func(appInfos []*snap.AppInfo, inst *servicestate.Instruction)) func() {
-	return ctlcmd.MockServicestateControlFunc(func(st *state.State, appInfos []*snap.AppInfo, inst *servicestate.Instruction, context *hookstate.Context) ([]*state.TaskSet, error) {
+	return ctlcmd.MockServicestateControlFunc(func(st *state.State, appInfos []*snap.AppInfo, inst *servicestate.Instruction, flags *servicestate.Flags, context *hookstate.Context) ([]*state.TaskSet, error) {
 		testServiceControlInputs(appInfos, inst)
 		return nil, fmt.Errorf("forced error")
 	})
@@ -385,15 +385,17 @@ func (s *servicectlSuite) TestQueuedCommands(c *C) {
 	s.st.Lock()
 	defer s.st.Unlock()
 
-	expectedTaskKinds := append(installTaskKinds, "exec-command", "exec-command", "exec-command")
-	for i := 1; i <= 2; i++ {
-		laneTasks := chg.LaneTasks(i)
+	expectedTaskKinds := append(installTaskKinds, "exec-command", "service-control", "exec-command", "service-control", "exec-command", "service-control")
+	checkLaneTasks := func(lane int) {
+		laneTasks := chg.LaneTasks(lane)
 		c.Assert(taskKinds(laneTasks), DeepEquals, expectedTaskKinds)
 		c.Check(laneTasks[12].Summary(), Matches, `Run configure hook of .* snap if present`)
 		c.Check(laneTasks[14].Summary(), Equals, "stop of [test-snap.test-service]")
-		c.Check(laneTasks[15].Summary(), Equals, "start of [test-snap.test-service]")
-		c.Check(laneTasks[16].Summary(), Equals, "restart of [test-snap.test-service]")
+		c.Check(laneTasks[16].Summary(), Equals, "start of [test-snap.test-service]")
+		c.Check(laneTasks[18].Summary(), Equals, "restart of [test-snap.test-service]")
 	}
+	checkLaneTasks(1)
+	checkLaneTasks(2)
 }
 
 func (s *servicectlSuite) testQueueCommandsOrdering(c *C, finalTaskKind string) {
@@ -419,30 +421,62 @@ func (s *servicectlSuite) testQueueCommandsOrdering(c *C, finalTaskKind string)
 	s.st.Lock()
 	defer s.st.Unlock()
 
-	finalTaskWt := finalTask.WaitTasks()
-	c.Assert(finalTaskWt, HasLen, 2)
+	var finalWaitTasks []string
+	for _, t := range finalTask.WaitTasks() {
+		taskInfo := fmt.Sprintf("%s:%s", t.Kind(), t.Summary())
+		finalWaitTasks = append(finalWaitTasks, taskInfo)
 
-	for _, t := range finalTaskWt {
-		// mark-seeded tasks should wait for both exec-command tasks
-		c.Check(t.Kind(), Equals, "exec-command")
-		var argv []string
-		c.Assert(t.Get("argv", &argv), IsNil)
-		c.Check(argv, HasLen, 3)
-
-		commandWt := make(map[string]bool)
+		var wait []string
+		var hasRunHook bool
 		for _, wt := range t.WaitTasks() {
-			commandWt[wt.Kind()] = true
+			if wt.Kind() != "run-hook" {
+				taskInfo = fmt.Sprintf("%s:%s", wt.Kind(), wt.Summary())
+				wait = append(wait, taskInfo)
+			} else {
+				hasRunHook = true
+			}
 		}
-		// exec-command for "stop" should wait for configure hook task, "start" should wait for "stop" and "configure" hook task.
-		switch argv[1] {
-		case "stop":
-			c.Check(commandWt, DeepEquals, map[string]bool{"run-hook": true})
-		case "start":
-			c.Check(commandWt, DeepEquals, map[string]bool{"run-hook": true, "exec-command": true})
+		c.Assert(hasRunHook, Equals, true)
+
+		switch t.Kind() {
+		case "exec-command":
+			var argv []string
+			c.Assert(t.Get("argv", &argv), IsNil)
+			c.Check(argv, HasLen, 3)
+			switch argv[1] {
+			case "stop":
+				c.Check(wait, HasLen, 0)
+			case "start":
+				c.Check(wait, DeepEquals, []string{
+					`exec-command:stop of [test-snap.test-service]`,
+					`service-control:Run service command "stop" for services ["test-service"] of snap "test-snap"`})
+			default:
+				c.Fatalf("unexpected command: %q", argv[1])
+			}
+		case "service-control":
+			var sa servicestate.ServiceAction
+			c.Assert(t.Get("service-action", &sa), IsNil)
+			c.Check(sa.Services, DeepEquals, []string{"test-service"})
+			switch sa.Action {
+			case "stop":
+				c.Check(wait, DeepEquals, []string{
+					"exec-command:stop of [test-snap.test-service]"})
+			case "start":
+				c.Check(wait, DeepEquals, []string{
+					"exec-command:start of [test-snap.test-service]",
+					"exec-command:stop of [test-snap.test-service]",
+					`service-control:Run service command "stop" for services ["test-service"] of snap "test-snap"`})
+			}
 		default:
-			c.Fatalf("unexpected command: %q", argv[1])
+			c.Fatalf("unexpected task: %s", t.Kind())
 		}
+
 	}
+	c.Check(finalWaitTasks, DeepEquals, []string{
+		`exec-command:stop of [test-snap.test-service]`,
+		`service-control:Run service command "stop" for services ["test-service"] of snap "test-snap"`,
+		`exec-command:start of [test-snap.test-service]`,
+		`service-control:Run service command "start" for services ["test-service"] of snap "test-snap"`})
 	c.Check(finalTask.HaltTasks(), HasLen, 0)
 }
 
@@ -498,14 +532,17 @@ func (s *servicectlSuite) TestQueuedCommandsUpdateMany(c *C) {
 	s.st.Lock()
 	defer s.st.Unlock()
 
-	expectedTaskKinds := append(refreshTaskKinds, "exec-command", "exec-command", "exec-command")
+	expectedTaskKinds := append(refreshTaskKinds, "exec-command", "service-control", "exec-command", "service-control", "exec-command", "service-control")
 	for i := 1; i <= 2; i++ {
 		laneTasks := chg.LaneTasks(i)
 		c.Assert(taskKinds(laneTasks), DeepEquals, expectedTaskKinds)
 		c.Check(laneTasks[17].Summary(), Matches, `Run configure hook of .* snap if present`)
 		c.Check(laneTasks[19].Summary(), Equals, "stop of [test-snap.test-service]")
-		c.Check(laneTasks[20].Summary(), Equals, "start of [test-snap.test-service]")
-		c.Check(laneTasks[21].Summary(), Equals, "restart of [test-snap.test-service]")
+		c.Check(laneTasks[20].Summary(), Equals, `Run service command "stop" for services ["test-service"] of snap "test-snap"`)
+		c.Check(laneTasks[21].Summary(), Equals, "start of [test-snap.test-service]")
+		c.Check(laneTasks[22].Summary(), Equals, `Run service command "start" for services ["test-service"] of snap "test-snap"`)
+		c.Check(laneTasks[23].Summary(), Equals, "restart of [test-snap.test-service]")
+		c.Check(laneTasks[24].Summary(), Equals, `Run service command "restart" for services ["test-service"] of snap "test-snap"`)
 	}
 }
 
@@ -539,11 +576,11 @@ func (s *servicectlSuite) TestQueuedCommandsSingleLane(c *C) {
 	defer s.st.Unlock()
 
 	laneTasks := chg.LaneTasks(0)
-	c.Assert(taskKinds(laneTasks), DeepEquals, append(installTaskKinds, "exec-command", "exec-command", "exec-command"))
+	c.Assert(taskKinds(laneTasks), DeepEquals, append(installTaskKinds, "exec-command", "service-control", "exec-command", "service-control", "exec-command", "service-control"))
 	c.Check(laneTasks[12].Summary(), Matches, `Run configure hook of .* snap if present`)
 	c.Check(laneTasks[14].Summary(), Equals, "stop of [test-snap.test-service]")
-	c.Check(laneTasks[15].Summary(), Equals, "start of [test-snap.test-service]")
-	c.Check(laneTasks[16].Summary(), Equals, "restart of [test-snap.test-service]")
+	c.Check(laneTasks[16].Summary(), Equals, "start of [test-snap.test-service]")
+	c.Check(laneTasks[18].Summary(), Equals, "restart of [test-snap.test-service]")
 }
 
 func (s *servicectlSuite) TestTwoServices(c *C) {

overlord/overlord.go

@@ -45,6 +45,7 @@ import (
 	"github.com/snapcore/snapd/overlord/hookstate"
 	"github.com/snapcore/snapd/overlord/ifacestate"
 	"github.com/snapcore/snapd/overlord/patch"
+	"github.com/snapcore/snapd/overlord/servicestate"
 	"github.com/snapcore/snapd/overlord/snapshotstate"
 	"github.com/snapcore/snapd/overlord/snapstate"
 	_ "github.com/snapcore/snapd/overlord/snapstate/policy"
@@ -89,16 +90,17 @@ type Overlord struct {
 	// restarts
 	restartBehavior RestartBehavior
 	// managers
-	inited    bool
-	startedUp bool
-	runner    *state.TaskRunner
-	snapMgr   *snapstate.SnapManager
-	assertMgr *assertstate.AssertManager
-	ifaceMgr  *ifacestate.InterfaceManager
-	hookMgr   *hookstate.HookManager
-	deviceMgr *devicestate.DeviceManager
-	cmdMgr    *cmdstate.CommandManager
-	shotMgr   *snapshotstate.SnapshotManager
+	inited     bool
+	startedUp  bool
+	runner     *state.TaskRunner
+	snapMgr    *snapstate.SnapManager
+	serviceMgr *servicestate.ServiceManager
+	assertMgr  *assertstate.AssertManager
+	ifaceMgr   *ifacestate.InterfaceManager
+	hookMgr    *hookstate.HookManager
+	deviceMgr  *devicestate.DeviceManager
+	cmdMgr     *cmdstate.CommandManager
+	shotMgr    *snapshotstate.SnapshotManager
 	// proxyConf mediates the http proxy config
 	proxyConf func(req *http.Request) (*url.URL, error)
 }
@@ -157,6 +159,9 @@ func New(restartBehavior RestartBehavior) (*Overlord, error) {
 	}
 	o.addManager(snapMgr)
 
+	serviceMgr := servicestate.Manager(s, o.runner)
+	o.addManager(serviceMgr)
+
 	assertMgr, err := assertstate.Manager(s, o.runner)
 	if err != nil {
 		return nil, err
@@ -204,6 +209,8 @@ func (o *Overlord) addManager(mgr StateManager) {
 		o.hookMgr = x
 	case *snapstate.SnapManager:
 		o.snapMgr = x
+	case *servicestate.ServiceManager:
+		o.serviceMgr = x
 	case *assertstate.AssertManager:
 		o.assertMgr = x
 	case *ifacestate.InterfaceManager:

overlord/patch/patch5.go

@@ -71,7 +71,7 @@ func patch5(st *state.State) error {
 			return err
 		}
 
-		err = wrappers.AddSnapServices(info, nil, nil, log)
+		err = wrappers.AddSnapServices(info, nil, log)
 		if err != nil {
 			return err
 		}

overlord/servicestate/service_control.go

@@ -91,23 +91,43 @@ func (m *ServiceManager) doServiceControl(t *state.Task, _ *tomb.Tomb) error {
 
 	switch sc.Action {
 	case "stop":
+		disable := sc.ActionModifier == "disable"
 		flags := &wrappers.StopServicesFlags{
-			Disable: sc.ActionModifier == "disable",
+			Disable: disable,
 		}
 		if err := wrappers.StopServices(services, flags, snap.StopReasonOther, meter, perfTimings); err != nil {
 			return err
 		}
+		if disable {
+			changed, err := updateSnapstateServices(&snapst, nil, services)
+			if err != nil {
+				return err
+			}
+			if changed {
+				snapstate.Set(st, sc.SnapName, &snapst)
+			}
+		}
 	case "start":
 		startupOrdered, err := snap.SortServices(services)
 		if err != nil {
 			return err
 		}
+		enable := sc.ActionModifier == "enable"
 		flags := &wrappers.StartServicesFlags{
-			Enable: sc.ActionModifier == "enable",
+			Enable: enable,
 		}
 		if err := wrappers.StartServices(startupOrdered, nil, flags, meter, perfTimings); err != nil {
 			return err
 		}
+		if enable {
+			changed, err := updateSnapstateServices(&snapst, startupOrdered, nil)
+			if err != nil {
+				return err
+			}
+			if changed {
+				snapstate.Set(st, sc.SnapName, &snapst)
+			}
+		}
 	case "restart":
 		return wrappers.RestartServices(services, nil, meter, perfTimings)
 	case "reload-or-restart":