diff --git a/overlord/hookstate/ctlcmd/helpers.go b/overlord/hookstate/ctlcmd/helpers.go index a6c5edc1788..ff4db1d1425 100644 --- 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 index b6a953e2b07..ea44f26c1d6 100644 --- 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 index cc9ef4755a7..24aba6008bb 100644 --- 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() {