o/hookstate/ctlcmd: queue service commands if run from default-config…

…ure hook (canonical#13960) * o/hookstate/ctlcmd: queue service command if run from default-configure hook Queue "snapctl restart ..." and "snapctl start ..." commands to be run after default-configure similar to configure hook. This is to avoid a problem where the service doesn't see a new value if it uses "snapctl get ...", because it's still not commited by default-configure hook. Fixes: https://bugs.launchpad.net/snapd/+bug/2047949 Signed-off-by: Zeyad Gouda <[email protected]> * o/hookstate/ctlcmd: refactor TestQueuedCommands into two tests And add comments explaining tasks relative order. Signed-off-by: Zeyad Gouda <[email protected]> * o/hookstate/ctlcmd: fix typo in TestQueuedCommandsDefaultConfigureHook Signed-off-by: Zeyad Gouda <[email protected]> * o/hookstate/ctlcmd: inject default-configure hook commands after start-snap-services Signed-off-by: Zeyad Gouda <[email protected]> * o/hookstate/ctlcmd: fix queuing commands for default-configure hook Multiple snaps could be installed in a single transaction where all snap tasksets are in a single lane. The old simplistic approach of looking up the first "start-snap-services" fails when we have multiple tasks for multiple snaps in the same lane. A test is added to trigger this corner case, and the fix just checks the snap name associated with the "start-snap-services" tasks. Signed-off-by: Zeyad Gouda <[email protected]> * o/hookstate/ctlcmd: explain why configure hook is not tested for single transaction Signed-off-by: Zeyad Gouda <[email protected]> --------- Signed-off-by: Zeyad Gouda <[email protected]>
pedronis · Jul 4, 2024 · 4669c7c · 4669c7c
1 parent 5b632be
commit 4669c7c
Show file tree

Hide file tree

Showing 3 changed files with 270 additions and 15 deletions.
diff --git a/overlord/hookstate/ctlcmd/helpers.go b/overlord/hookstate/ctlcmd/helpers.go
@@ -85,19 +85,20 @@ func getServiceInfos(st *state.State, snapName string, serviceNames []string) ([
 
 var servicestateControl = servicestate.Control
 
-func queueCommand(context *hookstate.Context, tts []*state.TaskSet) error {
+func prepareQueueCommand(context *hookstate.Context, tts []*state.TaskSet) (change *state.Change, tasks []*state.Task, err error) {
+	hookName := context.HookName()
+	if hookName != "configure" && hookName != "default-configure" {
+		return nil, nil, fmt.Errorf(`internal error: unexpected %q hook`, hookName)
+	}
+
 	hookTask, ok := context.Task()
 	if !ok {
-		return fmt.Errorf("attempted to queue command with ephemeral context")
+		return nil, nil, fmt.Errorf("attempted to queue command with ephemeral context")
 	}
 
-	st := context.State()
-	st.Lock()
-	defer st.Unlock()
-
-	change := hookTask.Change()
+	change = hookTask.Change()
 	hookTaskLanes := hookTask.Lanes()
-	tasks := change.LaneTasks(hookTaskLanes...)
+	tasks = change.LaneTasks(hookTaskLanes...)
 
 	// When installing or updating multiple snaps, there is one lane per snap.
 	// We want service command to join respective lane (it's the lane the hook belongs to).
@@ -111,9 +112,27 @@ func queueCommand(context *hookstate.Context, tts []*state.TaskSet) error {
 		}
 	}
 
+	return change, tasks, err
+}
+
+// queueConfigureHookCommand queues service command after all tasks, except for final tasks which must come after service commands.
+func queueConfigureHookCommand(context *hookstate.Context, tts []*state.TaskSet) error {
+	st := context.State()
+	st.Lock()
+	defer st.Unlock()
+
+	change, tasks, err := prepareQueueCommand(context, tts)
+	if err != nil {
+		return err
+	}
+
+	// Note: Multiple snaps could be installed in single transaction mode
+	// where all snap tasksets are in a single lane.
+	// This is non-issue for configure hook since the command tasks are
+	// queued at the very end of the change unlike the default-configure
+	// hook.
 	for _, ts := range tts {
 		for _, t := range tasks {
-			// queue service command after all tasks, except for final tasks which must come after service commands
 			if finalTasks[t.Kind()] {
 				t.WaitAll(ts)
 			} else {
@@ -122,6 +141,48 @@ func queueCommand(context *hookstate.Context, tts []*state.TaskSet) error {
 		}
 		change.AddAll(ts)
 	}
+
+	// As this can be run from what was originally the last task of a change,
+	// make sure the tasks added to the change are considered immediately.
+	st.EnsureBefore(0)
+
+	return nil
+}
+
+// queueDefaultConfigureHookCommand queues service command exactly after start-snap-services.
+//
+// This is possible because the default-configure hook is run on first-install only and right
+// after start-snap-services is the nearest we can queue the service commands safely to make
+// sure all the needed state is setup properly.
+func queueDefaultConfigureHookCommand(context *hookstate.Context, tts []*state.TaskSet) error {
+	st := context.State()
+	st.Lock()
+	defer st.Unlock()
+
+	_, tasks, err := prepareQueueCommand(context, tts)
+	if err != nil {
+		return err
+	}
+
+	for _, t := range tasks {
+		if t.Kind() == "start-snap-services" {
+			snapsup, err := snapstate.TaskSnapSetup(t)
+			if err != nil {
+				return err
+			}
+			// Multiple snaps could be installed in single transaction mode
+			// where all snap tasksets are in a single lane.
+			// Check that the task belongs to the relevant snap.
+			if snapsup.InstanceName() != context.InstanceName() {
+				continue
+			}
+			for _, ts := range tts {
+				snapstate.InjectTasks(t, ts)
+			}
+			break
+		}
+	}
+
 	// As this can be run from what was originally the last task of a change,
 	// make sure the tasks added to the change are considered immediately.
 	st.EnsureBefore(0)
@@ -149,8 +210,14 @@ func runServiceCommand(context *hookstate.Context, inst *servicestate.Instructio
 		return err
 	}
 
-	if !context.IsEphemeral() && context.HookName() == "configure" {
-		return queueCommand(context, tts)
+	if !context.IsEphemeral() {
+		// queue service command for default-configure and configure hooks.
+		switch context.HookName() {
+		case "configure":
+			return queueConfigureHookCommand(context, tts)
+		case "default-configure":
+			return queueDefaultConfigureHookCommand(context, tts)
+		}
 	}
 
 	st.Lock()

diff --git a/overlord/hookstate/ctlcmd/services_test.go b/overlord/hookstate/ctlcmd/services_test.go
@@ -24,6 +24,7 @@ import (
 	"fmt"
 	"os/user"
 	"sort"
+	"strings"
 
 	. "gopkg.in/check.v1"
 
@@ -512,7 +513,194 @@ func (s *servicectlSuite) TestQueuedCommands(c *C) {
 	checkLaneTasks(2)
 }
 
-func (s *servicectlSuite) testQueueCommandsOrdering(c *C, finalTaskKind string) {
+func (s *servicectlSuite) testQueuedCommandsOrdering(c *C, hook string, singleTransaction bool) {
+	var transaction = client.TransactionPerSnap
+	if singleTransaction {
+		transaction = client.TransactionAllSnaps
+	}
+
+	s.st.Lock()
+
+	chg := s.st.NewChange("install change", "install change")
+	installed, tts, err := snapstate.InstallMany(s.st, []string{"one", "two"}, nil, 0, &snapstate.Flags{Transaction: transaction})
+	c.Assert(err, IsNil)
+	c.Check(installed, DeepEquals, []string{"one", "two"})
+	c.Assert(tts, HasLen, 2)
+	c.Assert(taskKinds(tts[0].Tasks()), DeepEquals, installTaskKinds)
+	c.Assert(taskKinds(tts[1].Tasks()), DeepEquals, installTaskKinds)
+	chg.AddAll(tts[0])
+	chg.AddAll(tts[1])
+
+	// Mock snaps as installed for the ctlcmd.Run calls below
+	for _, snapName := range installed {
+		var testSnapYaml = `name: %s
+version: 1.0
+apps:
+ test-service:
+  command: bin/service
+  daemon: simple
+`
+		info := snaptest.MockSnapCurrent(c, fmt.Sprintf(testSnapYaml, snapName), &snap.SideInfo{
+			Revision: snap.R(1),
+		})
+		snapstate.Set(s.st, info.InstanceName(), &snapstate.SnapState{
+			Active: true,
+			Sequence: snapstatetest.NewSequenceFromSnapSideInfos([]*snap.SideInfo{
+				{
+					RealName: info.SnapName(),
+					Revision: info.Revision,
+					SnapID:   snapName + "-id",
+				},
+			}),
+			Current: info.Revision,
+		})
+	}
+
+	s.st.Unlock()
+
+	hookTasks := make([]*state.Task, 2)
+	for i, ts := range tts {
+		tsTasks := ts.Tasks()
+		switch hook {
+		case "default-configure":
+			// default-configure hook task is the 4th to last task (check installTaskKinds)
+			hookTasks[i] = tsTasks[len(tsTasks)-4]
+		case "configure":
+			// configure hook is 2nd to last task (check installTaskKinds)
+			hookTasks[i] = tsTasks[len(tsTasks)-2]
+		default:
+			c.Errorf("unexpected hook %q", hook)
+		}
+		c.Assert(hookTasks[i].Kind(), Equals, "run-hook")
+
+		s.st.Lock()
+		var setup *hookstate.HookSetup
+		c.Assert(hookTasks[i].Get("hook-setup", &setup), IsNil)
+		s.st.Unlock()
+
+		c.Assert(setup.Hook, Equals, hook)
+
+		// reuse existing services
+		setup = &hookstate.HookSetup{Snap: installed[i], Revision: snap.R(1), Hook: hook}
+		context, err := hookstate.NewContext(hookTasks[i], hookTasks[i].State(), setup, s.mockHandler, "")
+		c.Assert(err, IsNil)
+
+		// simulate running service commands inside the default-configure hook
+		_, _, err = ctlcmd.Run(context, []string{"stop", fmt.Sprintf("%s.test-service", installed[i])}, 0)
+		c.Assert(err, IsNil)
+		_, _, err = ctlcmd.Run(context, []string{"start", fmt.Sprintf("%s.test-service", installed[i])}, 0)
+		c.Assert(err, IsNil)
+	}
+
+	s.st.Lock()
+	defer s.st.Unlock()
+
+	cmdTasksPerSnap := make(map[string][]*state.Task, 2)
+	for _, t := range chg.Tasks() {
+		if t.Kind() != "exec-command" && t.Kind() != "service-control" {
+			continue
+		}
+		var snapName string
+		if strings.Contains(t.Summary(), "one") {
+			snapName = "one"
+		} else if strings.Contains(t.Summary(), "two") {
+			snapName = "two"
+		} else {
+			c.Errorf("unexpected task summary: %q", t.Summary())
+		}
+		cmdTasksPerSnap[snapName] = append(cmdTasksPerSnap[snapName], t)
+	}
+	c.Assert(cmdTasksPerSnap, HasLen, 2)
+
+	// check command tasks for snap "one"
+	c.Assert(taskKinds(cmdTasksPerSnap["one"]), DeepEquals, []string{"exec-command", "service-control", "exec-command", "service-control"})
+	c.Check(cmdTasksPerSnap["one"][0].Summary(), Equals, "stop of [one.test-service]")
+	c.Check(cmdTasksPerSnap["one"][2].Summary(), Equals, "start of [one.test-service]")
+	// check command tasks for snap "two"
+	c.Assert(taskKinds(cmdTasksPerSnap["two"]), DeepEquals, []string{"exec-command", "service-control", "exec-command", "service-control"})
+	c.Check(cmdTasksPerSnap["two"][0].Summary(), Equals, "stop of [two.test-service]")
+	c.Check(cmdTasksPerSnap["two"][2].Summary(), Equals, "start of [two.test-service]")
+
+	var expectedHaltTaskKinds, expectedWaitTaskKinds []string
+	switch hook {
+	case "default-configure":
+		// service command tasks are injected after start-snap-services
+		expectedWaitTaskKinds = []string{"start-snap-services"}
+		expectedHaltTaskKinds = []string{"run-hook[configure]", "run-hook[check-health]"}
+	case "configure":
+		// service command tasks are queued after all tasks
+		expectedWaitTaskKinds = installTaskKinds
+		expectedHaltTaskKinds = []string{}
+	}
+
+	var snapNameFromTask = func(t *state.Task) string {
+		if t.Kind() == "run-hook" {
+			var setup *hookstate.HookSetup
+			c.Assert(t.Get("hook-setup", &setup), IsNil)
+			return setup.Snap
+		}
+		setup, err := snapstate.TaskSnapSetup(t)
+		c.Assert(err, IsNil)
+		return setup.InstanceName()
+	}
+
+	for snapName, cmdTasks := range cmdTasksPerSnap {
+		for _, t := range cmdTasks {
+			var filteredHaltTasks, filteredWaitTasks []*state.Task
+			for _, wt := range t.WaitTasks() {
+				// filter out command tasks
+				if wt.Kind() == "exec-command" || wt.Kind() == "service-control" {
+					continue
+				}
+				// Check task is for the correct snap
+				c.Assert(snapNameFromTask(wt), Equals, snapName)
+				filteredWaitTasks = append(filteredWaitTasks, wt)
+			}
+			for _, ht := range t.HaltTasks() {
+				// filter out command tasks
+				if ht.Kind() == "exec-command" || ht.Kind() == "service-control" {
+					continue
+				}
+				// Check task is for the correct snap
+				c.Assert(snapNameFromTask(ht), Equals, snapName)
+				filteredHaltTasks = append(filteredHaltTasks, ht)
+			}
+			c.Assert(taskKinds(filteredWaitTasks), DeepEquals, expectedWaitTaskKinds)
+			c.Assert(taskKinds(filteredHaltTasks), DeepEquals, expectedHaltTaskKinds)
+		}
+	}
+}
+
+func (s *servicectlSuite) TestQueuedCommandsOrderingDefaultConfigureHook(c *C) {
+	const hook = "default-configure"
+	const singleTransaction = false
+	s.testQueuedCommandsOrdering(c, hook, singleTransaction)
+}
+
+func (s *servicectlSuite) TestQueuedCommandsOrderingDefaultConfigureHookSingleTransaction(c *C) {
+	const hook = "default-configure"
+	const singleTransaction = true
+	s.testQueuedCommandsOrdering(c, hook, singleTransaction)
+}
+
+func (s *servicectlSuite) TestQueuedCommandsOrderingConfigureHook(c *C) {
+	const hook = "configure"
+	const singleTransaction = false
+	s.testQueuedCommandsOrdering(c, hook, singleTransaction)
+}
+
+// NOTE: It is tricky to get snap name for all task kinds in the case of configure hook
+// so this test is left commented out just for clarity, but it will fail.
+// This is a non-issue for configure hook since the command tasks are queued at the very
+// end of the change unlike the default-configure hook.
+//
+// func (s *servicectlSuite) TestQueuedCommandsOrderingConfigureHookSingleTransaction(c *C) {
+// 	const hook = "configure"
+// 	const singleTransaction = true
+// 	s.testQueuedCommandsOrdering(c, hook, singleTransaction)
+// }
+
+func (s *servicectlSuite) testQueueCommandsConfigureHookFinalTask(c *C, finalTaskKind string) {
 	s.st.Lock()
 
 	chg := s.st.NewChange("seeding change", "seeding change")
@@ -595,11 +783,11 @@ func (s *servicectlSuite) testQueueCommandsOrdering(c *C, finalTaskKind string)
 }
 
 func (s *servicectlSuite) TestQueuedCommandsRunBeforeMarkSeeded(c *C) {
-	s.testQueueCommandsOrdering(c, "mark-seeded")
+	s.testQueueCommandsConfigureHookFinalTask(c, "mark-seeded")
 }
 
 func (s *servicectlSuite) TestQueuedCommandsRunBeforeSetModel(c *C) {
-	s.testQueueCommandsOrdering(c, "set-model")
+	s.testQueueCommandsConfigureHookFinalTask(c, "set-model")
 }
 
 func (s *servicectlSuite) TestQueuedCommandsUpdateMany(c *C) {

diff --git a/overlord/hookstate/ctlcmd/start.go b/overlord/hookstate/ctlcmd/start.go
@@ -30,7 +30,7 @@ var (
 	shortStartHelp = i18n.G("Start services")
 	longStartHelp  = i18n.G(`
 The start command starts the given services of the snap. If executed from the
-"configure" hook, the services will be started after the hook finishes.`)
+"configure" hook or "default-configure" hook, the services will be started after the hook finishes.`)
 )
 
 func init() {