From d0188b24c5080e57c5d92054813cf09b8551fa70 Mon Sep 17 00:00:00 2001
From: Tiexin Guo <guotiexin@gmail.com>
Date: Wed, 26 Jun 2024 16:29:33 +0800
Subject: [PATCH 01/10] feat: use lanes when starting services

---
 internals/daemon/api_services.go        |  6 +--
 internals/overlord/servstate/request.go | 69 ++++++++++++++++++++++---
 2 files changed, 66 insertions(+), 9 deletions(-)

diff --git a/internals/daemon/api_services.go b/internals/daemon/api_services.go
index e2b9d58a2..c9dba863c 100644
--- a/internals/daemon/api_services.go
+++ b/internals/daemon/api_services.go
@@ -111,7 +111,7 @@ func v1PostServices(c *Command, r *http.Request, _ *UserState) Response {
 		if err != nil {
 			break
 		}
-		taskSet, err = servstate.Start(st, services)
+		taskSet, err = servstate.Start(st, services, servmgr)
 	case "stop":
 		services, err = servmgr.StopOrder(payload.Services)
 		if err != nil {
@@ -134,7 +134,7 @@ func v1PostServices(c *Command, r *http.Request, _ *UserState) Response {
 			break
 		}
 		var startTasks *state.TaskSet
-		startTasks, err = servstate.Start(st, services)
+		startTasks, err = servstate.Start(st, services, servmgr)
 		if err != nil {
 			break
 		}
@@ -154,7 +154,7 @@ func v1PostServices(c *Command, r *http.Request, _ *UserState) Response {
 			break
 		}
 		var startTasks *state.TaskSet
-		startTasks, err = servstate.Start(st, startNames)
+		startTasks, err = servstate.Start(st, startNames, servmgr)
 		if err != nil {
 			break
 		}
diff --git a/internals/overlord/servstate/request.go b/internals/overlord/servstate/request.go
index 49c23f4f3..bca5d8d3a 100644
--- a/internals/overlord/servstate/request.go
+++ b/internals/overlord/servstate/request.go
@@ -4,6 +4,7 @@ import (
 	"fmt"
 
 	"github.com/canonical/pebble/internals/overlord/state"
+	"github.com/canonical/pebble/internals/plan"
 )
 
 // ServiceRequest holds the details required to perform service tasks.
@@ -11,21 +12,77 @@ type ServiceRequest struct {
 	Name string
 }
 
+func get_or_create_lane(s *state.State, service *plan.Service, service_lane_mapping map[string]int) int {
+	// if the service has been mapped to a lane
+	if lane, ok := service_lane_mapping[service.Name]; ok {
+		return lane
+	}
+
+	// if any dependency has been mapped to a lane
+	all_dependencies := append(append(service.Requires, service.Before...), service.After...)
+	for _, dependency := range all_dependencies {
+		if lane, ok := service_lane_mapping[dependency]; ok {
+			return lane
+		}
+	}
+
+	// neither the service itself nor any of its dependencies is mapped to an existing lane
+	return s.NewLane()
+}
+
+func joinLane(s *state.State, task *state.Task, service *plan.Service, service_lane_mapping map[string]int, lane_tasks_mapping map[int][]*state.Task) {
+	lane := get_or_create_lane(s, service, service_lane_mapping)
+	
+	task.JoinLane(lane)
+
+	// map task to lane
+	if _, ok := lane_tasks_mapping[lane]; !ok {
+		lane_tasks_mapping[lane] = nil
+	}
+	lane_tasks_mapping[lane] = append(lane_tasks_mapping[lane], task)
+
+	service_lane_mapping[service.Name] = lane
+	all_dependencies := append(append(service.Requires, service.Before...), service.After...)
+	for _, dependency := range all_dependencies {
+		service_lane_mapping[dependency] = lane
+	}
+}
+
+func handleWaitFor(lane_tasks_mapping map[int][]*state.Task) {
+	for _, tasks := range lane_tasks_mapping {
+		for i := 1; i < len(tasks); i++ {
+			tasks[i].WaitFor(tasks[i-1])
+		}
+	}
+}
+
 // Start creates and returns a task set for starting the given services.
-func Start(s *state.State, services []string) (*state.TaskSet, error) {
+func Start(s *state.State, names []string, m *ServiceManager) (*state.TaskSet, error) {
+	fmt.Println(names)
+	services := m.getPlan().Services
+	service_lane_mapping := make(map[string]int)
+	lane_tasks_mapping := make(map[int][]*state.Task)
+
 	var tasks []*state.Task
-	for _, name := range services {
+
+	for _, name := range names {
+		service, ok := services[name]
+		if !ok {
+			return nil, fmt.Errorf("service %q does not exist", name)
+		}
+
 		task := s.NewTask("start", fmt.Sprintf("Start service %q", name))
 		req := ServiceRequest{
 			Name: name,
 		}
 		task.Set("service-request", &req)
-		if len(tasks) > 0 {
-			// TODO Allow non-dependent services to start in parallel.
-			task.WaitFor(tasks[len(tasks)-1])
-		}
+		joinLane(s, task, service, service_lane_mapping, lane_tasks_mapping)
+
 		tasks = append(tasks, task)
 	}
+
+	handleWaitFor(lane_tasks_mapping)
+
 	return state.NewTaskSet(tasks...), nil
 }
 

From 9182407ed421a1846eefccc82d45a1cbf594a4d0 Mon Sep 17 00:00:00 2001
From: Tiexin Guo <guotiexin@gmail.com>
Date: Wed, 26 Jun 2024 16:30:19 +0800
Subject: [PATCH 02/10] test: add test for newly added lane feature for
 starting services

---
 internals/overlord/servstate/request_test.go | 109 ++++++++++++++++++-
 1 file changed, 107 insertions(+), 2 deletions(-)

diff --git a/internals/overlord/servstate/request_test.go b/internals/overlord/servstate/request_test.go
index b915ee471..92702113a 100644
--- a/internals/overlord/servstate/request_test.go
+++ b/internals/overlord/servstate/request_test.go
@@ -1,4 +1,4 @@
-// Copyright (c) 2014-2020 Canonical Ltd
+// Copyright (c) 2014-2024 Canonical Ltd
 //
 // This program is free software: you can redistribute it and/or modify
 // it under the terms of the GNU General Public License version 3 as
@@ -18,13 +18,73 @@ import (
 	. "gopkg.in/check.v1"
 
 	"github.com/canonical/pebble/internals/overlord/servstate"
+	. "github.com/canonical/pebble/internals/testutil"
 )
 
 func (s *S) TestStart(c *C) {
+	s.newServiceManager(c)
+	layer := `
+services:
+    one:
+        override: replace
+        command: /bin/sh -c "echo one; sleep 10"
+        startup: enabled
+
+    two:
+        override: replace
+        command: /bin/sh -c "echo two; sleep 10"
+        startup: enabled
+`
+	s.planAddLayer(c, layer)
+	s.planChanged(c)
+
+	s.st.Lock()
+	defer s.st.Unlock()
+
+	tset, err := servstate.Start(s.st, []string{"one", "two"}, s.manager)
+	c.Assert(err, IsNil)
+
+	tasks := tset.Tasks()
+	c.Assert(len(tasks), Equals, 2)
+
+	c.Assert(tasks[0].Kind(), Equals, "start")
+	req, err := servstate.TaskServiceRequest(tasks[0])
+	c.Assert(err, IsNil)
+	c.Assert(req.Name, Equals, "one")
+
+	c.Assert(tasks[1].Kind(), Equals, "start")
+	req, err = servstate.TaskServiceRequest(tasks[1])
+	c.Assert(err, IsNil)
+	c.Assert(req.Name, Equals, "two")
+
+	c.Assert(tasks[0].Lanes()[0], IntNotEqual, tasks[1].Lanes()[0])
+}
+
+func (s *S) TestStartInTheSameLaneAfter(c *C) {
+	s.newServiceManager(c)
+	layer := `
+services:
+    one:
+        override: replace
+        command: /bin/sh -c "echo one; sleep 10"
+        startup: enabled
+
+    two:
+        override: replace
+        command: /bin/sh -c "echo two; sleep 10"
+        startup: enabled
+        requires:
+            - one
+        after:
+            - one
+`
+	s.planAddLayer(c, layer)
+	s.planChanged(c)
+
 	s.st.Lock()
 	defer s.st.Unlock()
 
-	tset, err := servstate.Start(s.st, []string{"one", "two"})
+	tset, err := servstate.Start(s.st, []string{"one", "two"}, s.manager)
 	c.Assert(err, IsNil)
 
 	tasks := tset.Tasks()
@@ -39,6 +99,51 @@ func (s *S) TestStart(c *C) {
 	req, err = servstate.TaskServiceRequest(tasks[1])
 	c.Assert(err, IsNil)
 	c.Assert(req.Name, Equals, "two")
+
+	c.Assert(tasks[0].Lanes()[0], Equals, tasks[1].Lanes()[0])
+}
+
+func (s *S) TestStartInTheSameLaneBefore(c *C) {
+	s.newServiceManager(c)
+	layer := `
+services:
+    one:
+        override: replace
+        command: /bin/sh -c "echo one; sleep 10"
+        startup: enabled
+        requires:
+            - two
+        before:
+            - two
+
+    two:
+        override: replace
+        command: /bin/sh -c "echo two; sleep 10"
+        startup: enabled
+`
+	s.planAddLayer(c, layer)
+	s.planChanged(c)
+
+	s.st.Lock()
+	defer s.st.Unlock()
+
+	tset, err := servstate.Start(s.st, []string{"one", "two"}, s.manager)
+	c.Assert(err, IsNil)
+
+	tasks := tset.Tasks()
+	c.Assert(len(tasks), Equals, 2)
+
+	c.Assert(tasks[0].Kind(), Equals, "start")
+	req, err := servstate.TaskServiceRequest(tasks[0])
+	c.Assert(err, IsNil)
+	c.Assert(req.Name, Equals, "one")
+
+	c.Assert(tasks[1].Kind(), Equals, "start")
+	req, err = servstate.TaskServiceRequest(tasks[1])
+	c.Assert(err, IsNil)
+	c.Assert(req.Name, Equals, "two")
+
+	c.Assert(tasks[0].Lanes()[0], Equals, tasks[1].Lanes()[0])
 }
 
 func (s *S) TestStop(c *C) {

From 29d17db2cf7a391613d795e68096fb74264d9771 Mon Sep 17 00:00:00 2001
From: Tiexin Guo <guotiexin@gmail.com>
Date: Wed, 26 Jun 2024 16:30:31 +0800
Subject: [PATCH 03/10] test: fix failed manager test

---
 internals/overlord/servstate/manager_test.go | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/internals/overlord/servstate/manager_test.go b/internals/overlord/servstate/manager_test.go
index 9798a516d..cd0f2959f 100644
--- a/internals/overlord/servstate/manager_test.go
+++ b/internals/overlord/servstate/manager_test.go
@@ -1820,7 +1820,7 @@ func (s *S) testServiceLogs(c *C, outputs map[string]string) {
 
 func (s *S) startServices(c *C, services []string) *state.Change {
 	s.st.Lock()
-	ts, err := servstate.Start(s.st, services)
+	ts, err := servstate.Start(s.st, services, s.manager)
 	c.Check(err, IsNil)
 	chg := s.st.NewChange("test", "Start test")
 	chg.AddAll(ts)

From d38d26157744e217219741d057ae9d23fcffcf30 Mon Sep 17 00:00:00 2001
From: Tiexin Guo <guotiexin@gmail.com>
Date: Wed, 26 Jun 2024 17:15:37 +0800
Subject: [PATCH 04/10] chore: fmt

---
 internals/overlord/servstate/request.go | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/internals/overlord/servstate/request.go b/internals/overlord/servstate/request.go
index bca5d8d3a..7e012b1a6 100644
--- a/internals/overlord/servstate/request.go
+++ b/internals/overlord/servstate/request.go
@@ -32,7 +32,7 @@ func get_or_create_lane(s *state.State, service *plan.Service, service_lane_mapp
 
 func joinLane(s *state.State, task *state.Task, service *plan.Service, service_lane_mapping map[string]int, lane_tasks_mapping map[int][]*state.Task) {
 	lane := get_or_create_lane(s, service, service_lane_mapping)
-	
+
 	task.JoinLane(lane)
 
 	// map task to lane

From ca1a9d47a0d0bfc10642683d6341910508341189 Mon Sep 17 00:00:00 2001
From: Tiexin Guo <guotiexin@gmail.com>
Date: Thu, 27 Jun 2024 11:23:28 +0800
Subject: [PATCH 05/10] chore: refactor

---
 internals/overlord/servstate/request.go | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/internals/overlord/servstate/request.go b/internals/overlord/servstate/request.go
index 7e012b1a6..00fb4d90b 100644
--- a/internals/overlord/servstate/request.go
+++ b/internals/overlord/servstate/request.go
@@ -41,6 +41,7 @@ func joinLane(s *state.State, task *state.Task, service *plan.Service, service_l
 	}
 	lane_tasks_mapping[lane] = append(lane_tasks_mapping[lane], task)
 
+	// map the service's dependencies to the same lane
 	service_lane_mapping[service.Name] = lane
 	all_dependencies := append(append(service.Requires, service.Before...), service.After...)
 	for _, dependency := range all_dependencies {
@@ -58,7 +59,6 @@ func handleWaitFor(lane_tasks_mapping map[int][]*state.Task) {
 
 // Start creates and returns a task set for starting the given services.
 func Start(s *state.State, names []string, m *ServiceManager) (*state.TaskSet, error) {
-	fmt.Println(names)
 	services := m.getPlan().Services
 	service_lane_mapping := make(map[string]int)
 	lane_tasks_mapping := make(map[int][]*state.Task)

From 5d6037fcff44652971f539522c9c6e253ab8003a Mon Sep 17 00:00:00 2001
From: Tiexin Guo <guotiexin@gmail.com>
Date: Wed, 24 Jul 2024 11:47:47 +0800
Subject: [PATCH 06/10] feat: move lanes logic from servstate to plan

---
 internals/daemon/api_services.go             |  53 +++++----
 internals/overlord/servstate/manager.go      |  50 +++++---
 internals/overlord/servstate/manager_test.go |  83 +++++++------
 internals/overlord/servstate/request.go      | 115 ++++++-------------
 internals/overlord/servstate/request_test.go |   8 +-
 internals/plan/plan.go                       |  70 ++++++++++-
 internals/plan/plan_test.go                  |  32 ++++--
 7 files changed, 239 insertions(+), 172 deletions(-)

diff --git a/internals/daemon/api_services.go b/internals/daemon/api_services.go
index c9dba863c..1728be0a0 100644
--- a/internals/daemon/api_services.go
+++ b/internals/daemon/api_services.go
@@ -104,37 +104,38 @@ func v1PostServices(c *Command, r *http.Request, _ *UserState) Response {
 	defer st.Unlock()
 
 	var taskSet *state.TaskSet
+	var lanes [][]string
 	var services []string
 	switch payload.Action {
 	case "start", "autostart":
-		services, err = servmgr.StartOrder(payload.Services)
+		lanes, err = servmgr.StartOrder(payload.Services)
 		if err != nil {
 			break
 		}
-		taskSet, err = servstate.Start(st, services, servmgr)
+		taskSet, err = servstate.Start(st, lanes)
 	case "stop":
-		services, err = servmgr.StopOrder(payload.Services)
+		lanes, err = servmgr.StopOrder(payload.Services)
 		if err != nil {
 			break
 		}
-		taskSet, err = servstate.Stop(st, services)
+		taskSet, err = servstate.Stop(st, lanes)
 	case "restart":
-		services, err = servmgr.StopOrder(payload.Services)
+		lanes, err = servmgr.StopOrder(payload.Services)
 		if err != nil {
 			break
 		}
-		services = intersectOrdered(payload.Services, services)
+		lanes = intersectOrdered(payload.Services, lanes)
 		var stopTasks *state.TaskSet
-		stopTasks, err = servstate.Stop(st, services)
+		stopTasks, err = servstate.Stop(st, lanes)
 		if err != nil {
 			break
 		}
-		services, err = servmgr.StartOrder(payload.Services)
+		lanes, err = servmgr.StartOrder(payload.Services)
 		if err != nil {
 			break
 		}
 		var startTasks *state.TaskSet
-		startTasks, err = servstate.Start(st, services, servmgr)
+		startTasks, err = servstate.Start(st, lanes)
 		if err != nil {
 			break
 		}
@@ -143,7 +144,7 @@ func v1PostServices(c *Command, r *http.Request, _ *UserState) Response {
 		taskSet.AddAll(stopTasks)
 		taskSet.AddAll(startTasks)
 	case "replan":
-		var stopNames, startNames []string
+		var stopNames, startNames [][]string
 		stopNames, startNames, err = servmgr.Replan()
 		if err != nil {
 			break
@@ -154,7 +155,7 @@ func v1PostServices(c *Command, r *http.Request, _ *UserState) Response {
 			break
 		}
 		var startTasks *state.TaskSet
-		startTasks, err = servstate.Start(st, startNames, servmgr)
+		startTasks, err = servstate.Start(st, startNames)
 		if err != nil {
 			break
 		}
@@ -165,11 +166,15 @@ func v1PostServices(c *Command, r *http.Request, _ *UserState) Response {
 
 		// Populate a list of services affected by the replan for summary.
 		replanned := make(map[string]bool)
-		for _, v := range stopNames {
-			replanned[v] = true
+		for _, row := range stopNames {
+			for _, v := range row {
+				replanned[v] = true
+			}
 		}
-		for _, v := range startNames {
-			replanned[v] = true
+		for _, row := range startNames {
+			for _, v := range row {
+				replanned[v] = true
+			}
 		}
 		for k := range replanned {
 			services = append(services, k)
@@ -186,6 +191,9 @@ func v1PostServices(c *Command, r *http.Request, _ *UserState) Response {
 	// Use the original requested service name for the summary, not the
 	// resolved one. But do use the resolved set for the count.
 	var summary string
+	for _, names := range lanes {
+		services = append(services, names...)
+	}
 	switch {
 	case len(taskSet.Tasks()) == 0:
 		// Can happen with a replan that has no services to stop/start. A
@@ -222,16 +230,21 @@ func v1PostService(c *Command, r *http.Request, _ *UserState) Response {
 
 // intersectOrdered returns the intersection of left and right where
 // the right's ordering is persisted in the resulting set.
-func intersectOrdered(left []string, orderedRight []string) []string {
+func intersectOrdered(left []string, orderedRight [][]string) [][]string {
 	m := map[string]bool{}
 	for _, v := range left {
 		m[v] = true
 	}
-	var out []string
-	for _, v := range orderedRight {
-		if m[v] {
-			out = append(out, v)
+
+	var out [][]string
+	for _, row := range orderedRight {
+		var intersectRow []string
+		for _, v := range row {
+			if m[v] {
+				intersectRow = append(intersectRow, v)
+			}
 		}
+		out = append(out, intersectRow)
 	}
 	return out
 }
diff --git a/internals/overlord/servstate/manager.go b/internals/overlord/servstate/manager.go
index ae1ff9e08..2ac0b9555 100644
--- a/internals/overlord/servstate/manager.go
+++ b/internals/overlord/servstate/manager.go
@@ -204,19 +204,28 @@ func (m *ServiceManager) DefaultServiceNames() ([]string, error) {
 		}
 	}
 
-	return currentPlan.StartOrder(names)
+	lanes, err := currentPlan.StartOrder(names)
+	if err != nil {
+		return nil, err
+	}
+
+	var result []string
+	for _, row := range lanes {
+		result = append(result, row...)
+	}
+	return result, err
 }
 
 // StartOrder returns the provided services, together with any required
-// dependencies, in the proper order for starting them all up.
-func (m *ServiceManager) StartOrder(services []string) ([]string, error) {
+// dependencies, in the proper order, put in lanes, for starting them all up.
+func (m *ServiceManager) StartOrder(services []string) ([][]string, error) {
 	currentPlan := m.getPlan()
 	return currentPlan.StartOrder(services)
 }
 
 // StopOrder returns the provided services, together with any dependants,
-// in the proper order for stopping them all.
-func (m *ServiceManager) StopOrder(services []string) ([]string, error) {
+// in the proper order, put in lanes, for stopping them all.
+func (m *ServiceManager) StopOrder(services []string) ([][]string, error) {
 	currentPlan := m.getPlan()
 	return currentPlan.StopOrder(services)
 }
@@ -253,7 +262,7 @@ func (m *ServiceManager) ServiceLogs(services []string, last int) (map[string]se
 
 // Replan returns a list of services to stop and services to start because
 // their plans had changed between when they started and this call.
-func (m *ServiceManager) Replan() ([]string, []string, error) {
+func (m *ServiceManager) Replan() ([][]string, [][]string, error) {
 	currentPlan := m.getPlan()
 	m.servicesLock.Lock()
 	defer m.servicesLock.Unlock()
@@ -278,7 +287,7 @@ func (m *ServiceManager) Replan() ([]string, []string, error) {
 		}
 	}
 
-	stop, err := currentPlan.StopOrder(stop)
+	stopLanes, err := currentPlan.StopOrder(stop)
 	if err != nil {
 		return nil, nil, err
 	}
@@ -288,12 +297,12 @@ func (m *ServiceManager) Replan() ([]string, []string, error) {
 		}
 	}
 
-	start, err = currentPlan.StartOrder(start)
+	startLanes, err := currentPlan.StartOrder(start)
 	if err != nil {
 		return nil, nil, err
 	}
 
-	return stop, start, nil
+	return stopLanes, startLanes, nil
 }
 
 func (m *ServiceManager) SendSignal(services []string, signal string) error {
@@ -342,8 +351,9 @@ func (m *ServiceManager) CheckFailed(name string) {
 // exit. If it starts just before, it would continue to run after the service
 // manager is terminated. If it starts just after (before the main process
 // exits), it would generate a runtime error as the reaper would already be dead.
-// This function returns a slice of service names to stop, in dependency order.
-func servicesToStop(m *ServiceManager) ([]string, error) {
+// This function returns a slice of service names to stop, in dependency order,
+// put in separate lanes.
+func servicesToStop(m *ServiceManager) ([][]string, error) {
 	currentPlan := m.getPlan()
 	// Get all service names in plan.
 	services := make([]string, 0, len(currentPlan.Services))
@@ -360,12 +370,18 @@ func servicesToStop(m *ServiceManager) ([]string, error) {
 	// Filter down to only those that are running or in backoff
 	m.servicesLock.Lock()
 	defer m.servicesLock.Unlock()
-	var notStopped []string
-	for _, name := range stop {
-		s := m.services[name]
-		if s != nil && (s.state == stateRunning || s.state == stateBackoff) {
-			notStopped = append(notStopped, name)
+	var result [][]string
+	for _, services := range stop {
+		var notStopped []string
+		for _, name := range services {
+			s := m.services[name]
+			if s != nil && (s.state == stateRunning || s.state == stateBackoff) {
+				notStopped = append(notStopped, name)
+			}
+		}
+		if len(notStopped) > 0 {
+			result = append(result, notStopped)
 		}
 	}
-	return notStopped, nil
+	return result, nil
 }
diff --git a/internals/overlord/servstate/manager_test.go b/internals/overlord/servstate/manager_test.go
index cd0f2959f..b99691bf2 100644
--- a/internals/overlord/servstate/manager_test.go
+++ b/internals/overlord/servstate/manager_test.go
@@ -290,7 +290,7 @@ services:
 
 	_, _, err := s.manager.Replan()
 	c.Assert(err, IsNil)
-	s.startServices(c, []string{"test9"})
+	s.startServices(c, [][]string{{"test9"}})
 	s.waitUntilService(c, "test9", func(service *servstate.ServiceInfo) bool {
 		return service.Current == servstate.StatusActive
 	})
@@ -312,13 +312,13 @@ services:
 	_, _, err := s.manager.Replan()
 	c.Assert(err, IsNil)
 
-	s.startServices(c, []string{"test6"})
+	s.startServices(c, [][]string{{"test6"}})
 	s.waitUntilService(c, "test6", func(service *servstate.ServiceInfo) bool {
 		return service.Current == servstate.StatusActive
 	})
 
 	startTime := time.Now()
-	chg := s.stopServices(c, []string{"test6"})
+	chg := s.stopServices(c, [][]string{{"test6"}})
 	s.st.Lock()
 	c.Check(chg.Status(), Equals, state.DoneStatus, Commentf("Error: %v", chg.Err()))
 	s.st.Unlock()
@@ -351,8 +351,17 @@ services:
 
 	stops, starts, err := s.manager.Replan()
 	c.Assert(err, IsNil)
-	c.Check(stops, DeepEquals, []string{"test2"})
-	c.Check(starts, DeepEquals, []string{"test1", "test2"})
+	for _, row := range stops {
+		if len(row) > 0 {
+			c.Check(row, DeepEquals, []string{"test2", "test1"})
+		}
+	}
+
+	for _, row := range starts {
+		if len(row) > 0 {
+			c.Check(row, DeepEquals, []string{"test1", "test2"})
+		}
+	}
 
 	s.stopTestServices(c)
 }
@@ -441,7 +450,7 @@ func (s *S) TestStartBadCommand(c *C) {
 	s.planAddLayer(c, testPlanLayer)
 	s.planChanged(c)
 
-	chg := s.startServices(c, []string{"test3"})
+	chg := s.startServices(c, [][]string{{"test3"}})
 
 	s.st.Lock()
 	c.Check(chg.Status(), Equals, state.ErrorStatus)
@@ -479,7 +488,7 @@ services:
 	))
 	s.planChanged(c)
 
-	chg := s.startServices(c, []string{"usrtest"})
+	chg := s.startServices(c, [][]string{{"usrtest"}})
 	s.st.Lock()
 	c.Assert(chg.Err(), IsNil)
 	s.st.Unlock()
@@ -511,7 +520,7 @@ func (s *S) TestUserGroupFails(c *C) {
 	})
 	defer restore()
 
-	chg := s.startServices(c, []string{"test5"})
+	chg := s.startServices(c, [][]string{{"test5"}})
 
 	s.st.Lock()
 	c.Check(chg.Status(), Equals, state.ErrorStatus)
@@ -567,7 +576,7 @@ services:
 	os.Setenv("USER", "y")
 
 	// Start service and ensure it has enough time to write to log.
-	chg := s.startServices(c, []string{"usrgrp"})
+	chg := s.startServices(c, [][]string{{"usrgrp"}})
 	s.waitUntilService(c, "usrgrp", func(svc *servstate.ServiceInfo) bool {
 		return svc.Current == servstate.StatusActive
 	})
@@ -585,7 +594,7 @@ func (s *S) TestStartFastExitCommand(c *C) {
 	s.planAddLayer(c, testPlanLayer)
 	s.planChanged(c)
 
-	chg := s.startServices(c, []string{"test4"})
+	chg := s.startServices(c, [][]string{{"test4"}})
 
 	s.st.Lock()
 	c.Check(chg.Status(), Equals, state.ErrorStatus)
@@ -623,7 +632,7 @@ func (s *S) TestServices(c *C) {
 	})
 
 	// Start a service and ensure it's marked active
-	s.startServices(c, []string{"test2"})
+	s.startServices(c, [][]string{{"test2"}})
 
 	services, err = s.manager.Services(nil)
 	c.Assert(err, IsNil)
@@ -669,7 +678,7 @@ services:
 	c.Assert(err, IsNil)
 
 	// Start "envtest" service
-	chg := s.startServices(c, []string{"envtest"})
+	chg := s.startServices(c, [][]string{{"envtest"}})
 	s.st.Lock()
 	c.Check(chg.Status(), Equals, state.DoneStatus, Commentf("Error: %v", chg.Err()))
 	s.st.Unlock()
@@ -731,7 +740,7 @@ services:
 	s.planChanged(c)
 
 	// Start the "test2" service
-	chg := s.startServices(c, []string{"test2"})
+	chg := s.startServices(c, [][]string{{"test2"}})
 	// Wait until "test2" service completes the echo command (so that we
 	// know the log buffer contains stdout).
 	s.waitForDoneCheck(c, "test2")
@@ -792,7 +801,7 @@ services:
 	s.planChanged(c)
 
 	// Start service and wait till it starts up the first time.
-	chg := s.startServices(c, []string{"test2"})
+	chg := s.startServices(c, [][]string{{"test2"}})
 	s.waitUntilService(c, "test2", func(svc *servstate.ServiceInfo) bool {
 		return svc.Current == servstate.StatusActive
 	})
@@ -807,7 +816,7 @@ services:
 	})
 
 	// Ensure it can be stopped successfully.
-	chg = s.stopServices(c, []string{"test2"})
+	chg = s.stopServices(c, [][]string{{"test2"}})
 	s.st.Lock()
 	c.Check(chg.Status(), Equals, state.DoneStatus, Commentf("Error: %v", chg.Err()))
 	s.st.Unlock()
@@ -868,7 +877,7 @@ checks:
 	checkMgr.PlanChanged(s.plan)
 
 	// Start service and wait till it starts up
-	s.startServices(c, []string{"test2"})
+	s.startServices(c, [][]string{{"test2"}})
 
 	s.waitForDoneCheck(c, "test2")
 
@@ -964,7 +973,7 @@ checks:
 	checkMgr.PlanChanged(s.plan)
 
 	// Start service and wait till it starts up
-	s.startServices(c, []string{"test2"})
+	s.startServices(c, [][]string{{"test2"}})
 
 	s.waitForDoneCheck(c, "test2")
 
@@ -1054,7 +1063,7 @@ checks:
 	checkMgr.PlanChanged(s.plan)
 
 	// Start service and wait till it starts up (output file is written to)
-	s.startServices(c, []string{"test2"})
+	s.startServices(c, [][]string{{"test2"}})
 
 	s.waitForDoneCheck(c, "test2")
 
@@ -1139,7 +1148,7 @@ checks:
 	checkMgr.PlanChanged(s.plan)
 
 	// Start service and wait till it starts up (output file is written to)
-	s.startServices(c, []string{"test2"})
+	s.startServices(c, [][]string{{"test2"}})
 
 	s.waitForDoneCheck(c, "test2")
 
@@ -1179,7 +1188,7 @@ services:
 	s.planChanged(c)
 
 	// Start service and wait till it starts up the first time.
-	s.startServices(c, []string{"test2"})
+	s.startServices(c, [][]string{{"test2"}})
 	s.waitUntilService(c, "test2", func(svc *servstate.ServiceInfo) bool {
 		return svc.Current == servstate.StatusActive
 	})
@@ -1210,7 +1219,7 @@ services:
 	s.planChanged(c)
 
 	// Start service and wait till it starts up the first time.
-	s.startServices(c, []string{"test2"})
+	s.startServices(c, [][]string{{"test2"}})
 	s.waitUntilService(c, "test2", func(svc *servstate.ServiceInfo) bool {
 		return svc.Current == servstate.StatusActive
 	})
@@ -1241,7 +1250,7 @@ services:
 	s.planChanged(c)
 
 	// Start service and wait till it starts up the first time.
-	s.startServices(c, []string{"test2"})
+	s.startServices(c, [][]string{{"test2"}})
 	s.waitUntilService(c, "test2", func(svc *servstate.ServiceInfo) bool {
 		return svc.Current == servstate.StatusActive
 	})
@@ -1272,7 +1281,7 @@ services:
 	s.planChanged(c)
 
 	// Start service and wait till it starts up the first time.
-	s.startServices(c, []string{"test2"})
+	s.startServices(c, [][]string{{"test2"}})
 	s.waitUntilService(c, "test2", func(svc *servstate.ServiceInfo) bool {
 		return svc.Current == servstate.StatusActive
 	})
@@ -1303,7 +1312,7 @@ services:
 	s.planChanged(c)
 
 	// Start service and wait till it starts up the first time.
-	s.startServices(c, []string{"test2"})
+	s.startServices(c, [][]string{{"test2"}})
 	s.waitUntilService(c, "test2", func(svc *servstate.ServiceInfo) bool {
 		return svc.Current == servstate.StatusActive
 	})
@@ -1471,7 +1480,7 @@ services:
 	))
 	s.planChanged(c)
 
-	s.startServices(c, []string{"test2"})
+	s.startServices(c, [][]string{{"test2"}})
 
 	// The child process creates a grandchild and will print the
 	// PID of the grandchild for us to inspect here. We need to
@@ -1547,7 +1556,7 @@ func (s *S) TestStopRunning(c *C) {
 	s.planAddLayer(c, testPlanLayer)
 	s.planChanged(c)
 
-	s.startServices(c, []string{"test2"})
+	s.startServices(c, [][]string{{"test2"}})
 	s.waitUntilService(c, "test2", func(svc *servstate.ServiceInfo) bool {
 		return svc.Current == servstate.StatusActive
 	})
@@ -1607,7 +1616,7 @@ services:
 
 	// Service command should run in current directory (package directory)
 	// if "working-dir" config option not set.
-	chg := s.startServices(c, []string{"nowrkdir"})
+	chg := s.startServices(c, [][]string{{"nowrkdir"}})
 	s.st.Lock()
 	c.Assert(chg.Err(), IsNil)
 	s.st.Unlock()
@@ -1640,7 +1649,7 @@ services:
 	))
 	s.planChanged(c)
 
-	chg := s.startServices(c, []string{"wrkdir"})
+	chg := s.startServices(c, [][]string{{"wrkdir"}})
 	s.st.Lock()
 	c.Assert(chg.Err(), IsNil)
 	s.st.Unlock()
@@ -1675,7 +1684,7 @@ services:
 	s.planChanged(c)
 
 	// Start service and wait for it to be started
-	chg := s.startServices(c, []string{"waitdelay"})
+	chg := s.startServices(c, [][]string{{"waitdelay"}})
 	s.st.Lock()
 	c.Assert(chg.Err(), IsNil)
 	s.st.Unlock()
@@ -1685,7 +1694,7 @@ services:
 
 	// Try to stop the service; it will only stop if WaitDelay logic is working,
 	// otherwise the goroutine waiting for the child's stdout will never finish.
-	chg = s.stopServices(c, []string{"waitdelay"})
+	chg = s.stopServices(c, [][]string{{"waitdelay"}})
 	s.st.Lock()
 	c.Assert(chg.Err(), IsNil)
 	s.st.Unlock()
@@ -1818,9 +1827,9 @@ func (s *S) testServiceLogs(c *C, outputs map[string]string) {
 	s.stopTestServices(c)
 }
 
-func (s *S) startServices(c *C, services []string) *state.Change {
+func (s *S) startServices(c *C, lanes [][]string) *state.Change {
 	s.st.Lock()
-	ts, err := servstate.Start(s.st, services, s.manager)
+	ts, err := servstate.Start(s.st, lanes)
 	c.Check(err, IsNil)
 	chg := s.st.NewChange("test", "Start test")
 	chg.AddAll(ts)
@@ -1829,9 +1838,9 @@ func (s *S) startServices(c *C, services []string) *state.Change {
 	return chg
 }
 
-func (s *S) stopServices(c *C, services []string) *state.Change {
+func (s *S) stopServices(c *C, lanes [][]string) *state.Change {
 	s.st.Lock()
-	ts, err := servstate.Stop(s.st, services)
+	ts, err := servstate.Stop(s.st, lanes)
 	c.Check(err, IsNil)
 	chg := s.st.NewChange("test", "Stop test")
 	chg.AddAll(ts)
@@ -1848,7 +1857,7 @@ func (s *S) serviceByName(c *C, name string) *servstate.ServiceInfo {
 }
 
 func (s *S) startTestServices(c *C, logCheck bool) {
-	chg := s.startServices(c, []string{"test1", "test2"})
+	chg := s.startServices(c, [][]string{{"test1", "test2"}})
 	s.st.Lock()
 	c.Check(chg.Status(), Equals, state.DoneStatus, Commentf("Error: %v", chg.Err()))
 	s.st.Unlock()
@@ -1872,7 +1881,7 @@ func (s *S) stopTestServices(c *C) {
 	cmds := s.manager.RunningCmds()
 	c.Check(cmds, HasLen, 2)
 
-	chg := s.stopServices(c, []string{"test1", "test2"})
+	chg := s.stopServices(c, [][]string{{"test1", "test2"}})
 
 	// Ensure processes are gone indeed.
 	c.Assert(cmds, HasLen, 2)
@@ -1894,7 +1903,7 @@ func (s *S) stopTestServicesAlreadyDead(c *C) {
 	cmds := s.manager.RunningCmds()
 	c.Check(cmds, HasLen, 0)
 
-	chg := s.stopServices(c, []string{"test1", "test2"})
+	chg := s.stopServices(c, [][]string{{"test1", "test2"}})
 
 	c.Assert(cmds, HasLen, 0)
 
diff --git a/internals/overlord/servstate/request.go b/internals/overlord/servstate/request.go
index 00fb4d90b..7746741e7 100644
--- a/internals/overlord/servstate/request.go
+++ b/internals/overlord/servstate/request.go
@@ -4,7 +4,6 @@ import (
 	"fmt"
 
 	"github.com/canonical/pebble/internals/overlord/state"
-	"github.com/canonical/pebble/internals/plan"
 )
 
 // ServiceRequest holds the details required to perform service tasks.
@@ -12,94 +11,46 @@ type ServiceRequest struct {
 	Name string
 }
 
-func get_or_create_lane(s *state.State, service *plan.Service, service_lane_mapping map[string]int) int {
-	// if the service has been mapped to a lane
-	if lane, ok := service_lane_mapping[service.Name]; ok {
-		return lane
-	}
-
-	// if any dependency has been mapped to a lane
-	all_dependencies := append(append(service.Requires, service.Before...), service.After...)
-	for _, dependency := range all_dependencies {
-		if lane, ok := service_lane_mapping[dependency]; ok {
-			return lane
-		}
-	}
-
-	// neither the service itself nor any of its dependencies is mapped to an existing lane
-	return s.NewLane()
-}
-
-func joinLane(s *state.State, task *state.Task, service *plan.Service, service_lane_mapping map[string]int, lane_tasks_mapping map[int][]*state.Task) {
-	lane := get_or_create_lane(s, service, service_lane_mapping)
-
-	task.JoinLane(lane)
-
-	// map task to lane
-	if _, ok := lane_tasks_mapping[lane]; !ok {
-		lane_tasks_mapping[lane] = nil
-	}
-	lane_tasks_mapping[lane] = append(lane_tasks_mapping[lane], task)
-
-	// map the service's dependencies to the same lane
-	service_lane_mapping[service.Name] = lane
-	all_dependencies := append(append(service.Requires, service.Before...), service.After...)
-	for _, dependency := range all_dependencies {
-		service_lane_mapping[dependency] = lane
-	}
-}
-
-func handleWaitFor(lane_tasks_mapping map[int][]*state.Task) {
-	for _, tasks := range lane_tasks_mapping {
-		for i := 1; i < len(tasks); i++ {
-			tasks[i].WaitFor(tasks[i-1])
-		}
-	}
-}
-
 // Start creates and returns a task set for starting the given services.
-func Start(s *state.State, names []string, m *ServiceManager) (*state.TaskSet, error) {
-	services := m.getPlan().Services
-	service_lane_mapping := make(map[string]int)
-	lane_tasks_mapping := make(map[int][]*state.Task)
-
+func Start(s *state.State, lanes [][]string) (*state.TaskSet, error) {
 	var tasks []*state.Task
-
-	for _, name := range names {
-		service, ok := services[name]
-		if !ok {
-			return nil, fmt.Errorf("service %q does not exist", name)
+	for _, services := range lanes {
+		lane := s.NewLane()
+		for i, name := range services {
+			task := s.NewTask("start", fmt.Sprintf("Start service %q", name))
+			req := ServiceRequest{
+				Name: name,
+			}
+			task.Set("service-request", &req)
+			task.JoinLane(lane)
+			// Wait for the previous task in the same lane.
+			if i > 0 {
+				task.WaitFor(tasks[len(tasks)-1])
+			}
+			tasks = append(tasks, task)
 		}
-
-		task := s.NewTask("start", fmt.Sprintf("Start service %q", name))
-		req := ServiceRequest{
-			Name: name,
-		}
-		task.Set("service-request", &req)
-		joinLane(s, task, service, service_lane_mapping, lane_tasks_mapping)
-
-		tasks = append(tasks, task)
 	}
-
-	handleWaitFor(lane_tasks_mapping)
-
 	return state.NewTaskSet(tasks...), nil
 }
 
 // Stop creates and returns a task set for stopping the given services.
-func Stop(s *state.State, services []string) (*state.TaskSet, error) {
+func Stop(s *state.State, lanes [][]string) (*state.TaskSet, error) {
 	var tasks []*state.Task
-	for _, name := range services {
-		task := s.NewTask("stop", fmt.Sprintf("Stop service %q", name))
-		req := ServiceRequest{
-			Name: name,
-		}
-		task.Set("service-request", &req)
-		if len(tasks) > 1 {
-			// TODO Allow non-dependent services to stop in parallel.
-			task.WaitFor(tasks[len(tasks)-1])
+	for _, services := range lanes {
+		lane := s.NewLane()
+		for i, name := range services {
+			task := s.NewTask("stop", fmt.Sprintf("Stop service %q", name))
+			req := ServiceRequest{
+				Name: name,
+			}
+			task.Set("service-request", &req)
+			task.JoinLane(lane)
+			// Wait for the previous task in the same lane.
+			if i > 0 {
+				task.WaitFor(tasks[len(tasks)-1])
+			}
+			tasks = append(tasks, task)
 		}
-		tasks = append(tasks, task)
 	}
 	return state.NewTaskSet(tasks...), nil
 }
@@ -107,18 +58,18 @@ func Stop(s *state.State, services []string) (*state.TaskSet, error) {
 // StopRunning creates and returns a task set for stopping all running
 // services. It returns a nil *TaskSet if there are no services to stop.
 func StopRunning(s *state.State, m *ServiceManager) (*state.TaskSet, error) {
-	services, err := servicesToStop(m)
+	lanes, err := servicesToStop(m)
 	if err != nil {
 		return nil, err
 	}
-	if len(services) == 0 {
+	if len(lanes) == 0 {
 		return nil, nil
 	}
 
 	// One change to stop them all.
 	s.Lock()
 	defer s.Unlock()
-	taskSet, err := Stop(s, services)
+	taskSet, err := Stop(s, lanes)
 	if err != nil {
 		return nil, err
 	}
diff --git a/internals/overlord/servstate/request_test.go b/internals/overlord/servstate/request_test.go
index 92702113a..19a96450c 100644
--- a/internals/overlord/servstate/request_test.go
+++ b/internals/overlord/servstate/request_test.go
@@ -41,7 +41,7 @@ services:
 	s.st.Lock()
 	defer s.st.Unlock()
 
-	tset, err := servstate.Start(s.st, []string{"one", "two"}, s.manager)
+	tset, err := servstate.Start(s.st, [][]string{{"one"}, {"two"}})
 	c.Assert(err, IsNil)
 
 	tasks := tset.Tasks()
@@ -84,7 +84,7 @@ services:
 	s.st.Lock()
 	defer s.st.Unlock()
 
-	tset, err := servstate.Start(s.st, []string{"one", "two"}, s.manager)
+	tset, err := servstate.Start(s.st, [][]string{{"one", "two"}})
 	c.Assert(err, IsNil)
 
 	tasks := tset.Tasks()
@@ -127,7 +127,7 @@ services:
 	s.st.Lock()
 	defer s.st.Unlock()
 
-	tset, err := servstate.Start(s.st, []string{"one", "two"}, s.manager)
+	tset, err := servstate.Start(s.st, [][]string{{"one", "two"}})
 	c.Assert(err, IsNil)
 
 	tasks := tset.Tasks()
@@ -150,7 +150,7 @@ func (s *S) TestStop(c *C) {
 	s.st.Lock()
 	defer s.st.Unlock()
 
-	tset, err := servstate.Stop(s.st, []string{"one", "two"})
+	tset, err := servstate.Stop(s.st, [][]string{{"one", "two"}})
 	c.Assert(err, IsNil)
 
 	tasks := tset.Tasks()
diff --git a/internals/plan/plan.go b/internals/plan/plan.go
index 2d5f3bcdc..1466ba55e 100644
--- a/internals/plan/plan.go
+++ b/internals/plan/plan.go
@@ -924,16 +924,78 @@ func (p *Plan) Validate() error {
 // services to be properly started, in the order that they must be started.
 // An error is returned when a provided service name does not exist, or there
 // is an order cycle involving the provided service or its dependencies.
-func (p *Plan) StartOrder(names []string) ([]string, error) {
-	return order(p.Services, names, false)
+func (p *Plan) StartOrder(names []string) ([][]string, error) {
+	orderedNames, err := order(p.Services, names, false)
+	if err != nil {
+		return nil, err
+	}
+
+	return createLanes(orderedNames, p.Services)
 }
 
 // StopOrder returns the required services that must be stopped for the named
 // services to be properly stopped, in the order that they must be stopped.
 // An error is returned when a provided service name does not exist, or there
 // is an order cycle involving the provided service or its dependencies.
-func (p *Plan) StopOrder(names []string) ([]string, error) {
-	return order(p.Services, names, true)
+func (p *Plan) StopOrder(names []string) ([][]string, error) {
+	orderedNames, err := order(p.Services, names, true)
+	if err != nil {
+		return nil, err
+	}
+
+	return createLanes(orderedNames, p.Services)
+}
+
+func getOrCreateLane(current_lane int, service *Service, serviceLaneMapping map[string]int) int {
+	// if the service has been mapped to a lane
+	if lane, ok := serviceLaneMapping[service.Name]; ok {
+		return lane
+	}
+
+	// if any dependency has been mapped to a lane
+	for _, dependency := range service.Requires {
+		if lane, ok := serviceLaneMapping[dependency]; ok {
+			return lane
+		}
+	}
+
+	// neither the service itself nor any of its dependencies is mapped to an existing lane
+	return current_lane + 1
+}
+
+func mapServiceToLane(service *Service, lane int, serviceLaneMapping map[string]int) {
+	serviceLaneMapping[service.Name] = lane
+
+	// map the service's dependencies to the same lane
+	for _, dependency := range service.Requires {
+		serviceLaneMapping[dependency] = lane
+	}
+}
+
+func createLanes(names []string, services map[string]*Service) ([][]string, error) {
+	serviceLaneMapping := make(map[string]int)
+
+	// Map all services into lanes.
+	var lane = 0
+	for _, name := range names {
+		service, ok := services[name]
+		if !ok {
+			return nil, &FormatError{
+				Message: fmt.Sprintf("service %q does not exist", name),
+			}
+		}
+
+		lane = getOrCreateLane(lane, service, serviceLaneMapping)
+		mapServiceToLane(service, lane, serviceLaneMapping)
+	}
+
+	// Create lanes
+	lanes := make([][]string, lane+1)
+	for _, service := range names {
+		lane := serviceLaneMapping[service]
+		lanes[lane] = append(lanes[lane], service)
+	}
+	return lanes, nil
 }
 
 func order(services map[string]*Service, names []string, stop bool) ([]string, error) {
diff --git a/internals/plan/plan_test.go b/internals/plan/plan_test.go
index 3a99fcdf4..cba18afef 100644
--- a/internals/plan/plan_test.go
+++ b/internals/plan/plan_test.go
@@ -1427,15 +1427,23 @@ func (s *S) TestParseLayer(c *C) {
 			if err == nil {
 				for name, order := range test.start {
 					p := plan.Plan{Services: result.Services}
-					names, err := p.StartOrder([]string{name})
+					lanes, err := p.StartOrder([]string{name})
 					c.Assert(err, IsNil)
-					c.Assert(names, DeepEquals, order)
+					for _, names := range lanes {
+						if len(names) > 0 {
+							c.Assert(names, DeepEquals, order)
+						}
+					}
 				}
 				for name, order := range test.stop {
 					p := plan.Plan{Services: result.Services}
-					names, err := p.StopOrder([]string{name})
+					lanes, err := p.StopOrder([]string{name})
 					c.Assert(err, IsNil)
-					c.Assert(names, DeepEquals, order)
+					for _, names := range lanes {
+						if len(names) > 0 {
+							c.Assert(names, DeepEquals, order)
+						}
+					}
 				}
 			}
 			if err == nil {
@@ -1575,15 +1583,23 @@ func (s *S) TestReadDir(c *C) {
 			if err == nil {
 				for name, order := range test.start {
 					p := plan.Plan{Services: result.Services}
-					names, err := p.StartOrder([]string{name})
+					lanes, err := p.StartOrder([]string{name})
 					c.Assert(err, IsNil)
-					c.Assert(names, DeepEquals, order)
+					for _, names := range lanes {
+						if len(names) > 0 {
+							c.Assert(names, DeepEquals, order)
+						}
+					}
 				}
 				for name, order := range test.stop {
 					p := plan.Plan{Services: result.Services}
-					names, err := p.StopOrder([]string{name})
+					lanes, err := p.StopOrder([]string{name})
 					c.Assert(err, IsNil)
-					c.Assert(names, DeepEquals, order)
+					for _, names := range lanes {
+						if len(names) > 0 {
+							c.Assert(names, DeepEquals, order)
+						}
+					}
 				}
 			}
 		}

From 1ef278b6a108785b852e3c35c4ab159442ee2f48 Mon Sep 17 00:00:00 2001
From: Tiexin Guo <guotiexin@gmail.com>
Date: Wed, 24 Jul 2024 12:27:33 +0800
Subject: [PATCH 07/10] chore: refactor and add tests

---
 internals/daemon/api_services.go             | 10 ++-
 internals/overlord/servstate/manager.go      |  6 +-
 internals/overlord/servstate/manager_test.go |  2 +-
 internals/overlord/servstate/request_test.go |  4 +-
 internals/plan/plan.go                       |  8 +-
 internals/plan/plan_test.go                  | 92 +++++++++++++++++++-
 6 files changed, 107 insertions(+), 15 deletions(-)

diff --git a/internals/daemon/api_services.go b/internals/daemon/api_services.go
index 1728be0a0..44028ccc2 100644
--- a/internals/daemon/api_services.go
+++ b/internals/daemon/api_services.go
@@ -1,4 +1,4 @@
-// Copyright (c) 2014-2020 Canonical Ltd
+// Copyright (c) 2014-2024 Canonical Ltd
 //
 // This program is free software: you can redistribute it and/or modify
 // it under the terms of the GNU General Public License version 3 as
@@ -191,8 +191,8 @@ func v1PostServices(c *Command, r *http.Request, _ *UserState) Response {
 	// Use the original requested service name for the summary, not the
 	// resolved one. But do use the resolved set for the count.
 	var summary string
-	for _, names := range lanes {
-		services = append(services, names...)
+	for _, row := range lanes {
+		services = append(services, row...)
 	}
 	switch {
 	case len(taskSet.Tasks()) == 0:
@@ -244,7 +244,9 @@ func intersectOrdered(left []string, orderedRight [][]string) [][]string {
 				intersectRow = append(intersectRow, v)
 			}
 		}
-		out = append(out, intersectRow)
+		if len(intersectRow) > 0 {
+			out = append(out, intersectRow)
+		}
 	}
 	return out
 }
diff --git a/internals/overlord/servstate/manager.go b/internals/overlord/servstate/manager.go
index 2ac0b9555..0fc1e20fa 100644
--- a/internals/overlord/servstate/manager.go
+++ b/internals/overlord/servstate/manager.go
@@ -260,8 +260,8 @@ func (m *ServiceManager) ServiceLogs(services []string, last int) (map[string]se
 	return iterators, nil
 }
 
-// Replan returns a list of services to stop and services to start because
-// their plans had changed between when they started and this call.
+// Replan returns a list of services in lanes to stop and services to start
+// because their plans had changed between when they started and this call.
 func (m *ServiceManager) Replan() ([][]string, [][]string, error) {
 	currentPlan := m.getPlan()
 	m.servicesLock.Lock()
@@ -352,7 +352,7 @@ func (m *ServiceManager) CheckFailed(name string) {
 // manager is terminated. If it starts just after (before the main process
 // exits), it would generate a runtime error as the reaper would already be dead.
 // This function returns a slice of service names to stop, in dependency order,
-// put in separate lanes.
+// put in lanes.
 func servicesToStop(m *ServiceManager) ([][]string, error) {
 	currentPlan := m.getPlan()
 	// Get all service names in plan.
diff --git a/internals/overlord/servstate/manager_test.go b/internals/overlord/servstate/manager_test.go
index b99691bf2..e4dd860c9 100644
--- a/internals/overlord/servstate/manager_test.go
+++ b/internals/overlord/servstate/manager_test.go
@@ -1,4 +1,4 @@
-// Copyright (c) 2014-2020 Canonical Ltd
+// Copyright (c) 2014-2024 Canonical Ltd
 //
 // This program is free software: you can redistribute it and/or modify
 // it under the terms of the GNU General Public License version 3 as
diff --git a/internals/overlord/servstate/request_test.go b/internals/overlord/servstate/request_test.go
index 19a96450c..36beefc5a 100644
--- a/internals/overlord/servstate/request_test.go
+++ b/internals/overlord/servstate/request_test.go
@@ -68,13 +68,13 @@ services:
         override: replace
         command: /bin/sh -c "echo one; sleep 10"
         startup: enabled
+        requires:
+            - two
 
     two:
         override: replace
         command: /bin/sh -c "echo two; sleep 10"
         startup: enabled
-        requires:
-            - one
         after:
             - one
 `
diff --git a/internals/plan/plan.go b/internals/plan/plan.go
index 1466ba55e..700f71e4b 100644
--- a/internals/plan/plan.go
+++ b/internals/plan/plan.go
@@ -1,4 +1,4 @@
-// Copyright (c) 2021 Canonical Ltd
+// Copyright (c) 2024 Canonical Ltd
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -946,7 +946,7 @@ func (p *Plan) StopOrder(names []string) ([][]string, error) {
 	return createLanes(orderedNames, p.Services)
 }
 
-func getOrCreateLane(current_lane int, service *Service, serviceLaneMapping map[string]int) int {
+func getOrCreateLane(currentLane int, service *Service, serviceLaneMapping map[string]int) int {
 	// if the service has been mapped to a lane
 	if lane, ok := serviceLaneMapping[service.Name]; ok {
 		return lane
@@ -960,7 +960,7 @@ func getOrCreateLane(current_lane int, service *Service, serviceLaneMapping map[
 	}
 
 	// neither the service itself nor any of its dependencies is mapped to an existing lane
-	return current_lane + 1
+	return currentLane + 1
 }
 
 func mapServiceToLane(service *Service, lane int, serviceLaneMapping map[string]int) {
@@ -976,7 +976,7 @@ func createLanes(names []string, services map[string]*Service) ([][]string, erro
 	serviceLaneMapping := make(map[string]int)
 
 	// Map all services into lanes.
-	var lane = 0
+	var lane = -1
 	for _, name := range names {
 		service, ok := services[name]
 		if !ok {
diff --git a/internals/plan/plan_test.go b/internals/plan/plan_test.go
index cba18afef..b7a23bf21 100644
--- a/internals/plan/plan_test.go
+++ b/internals/plan/plan_test.go
@@ -1,4 +1,4 @@
-// Copyright (c) 2020 Canonical Ltd
+// Copyright (c) 2024 Canonical Ltd
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -1955,3 +1955,93 @@ func (s *S) TestPebbleLabelPrefixReserved(c *C) {
 	_, err := plan.ParseLayer(0, "pebble-foo", []byte("{}"))
 	c.Check(err, ErrorMatches, `cannot use reserved label prefix "pebble-"`)
 }
+
+func (s *S) TestStartStopOrderSingleLane(c *C) {
+	layer := &plan.Layer{
+		Summary:     "services with dependencies in the same lane",
+		Description: "a simple layer",
+		Services: map[string]*plan.Service{
+			"srv1": {
+				Name:     "srv1",
+				Override: "replace",
+				Command:  `cmd`,
+				Requires: []string{"srv2"},
+				Before: []string{"srv2"},
+				Startup:  plan.StartupEnabled,
+			},
+			"srv2": {
+				Name:     "srv2",
+				Override: "replace",
+				Command:  `cmd`,
+				Requires: []string{"srv3"},
+				Before: []string{"srv3"},
+				Startup:  plan.StartupEnabled,
+			},
+			"srv3": {
+				Name:     "srv3",
+				Override: "replace",
+				Command:  `cmd`,
+				Startup:  plan.StartupEnabled,
+			},
+		},
+		Checks:     map[string]*plan.Check{},
+		LogTargets: map[string]*plan.LogTarget{},
+	}
+
+	p := plan.Plan{Services: layer.Services}
+	
+	lanes, err := p.StartOrder([]string{"srv1", "srv2", "srv3"})
+	c.Assert(err, IsNil)
+	c.Assert(len(lanes), Equals, 1)
+	c.Assert(lanes[0], DeepEquals, []string{"srv1", "srv2", "srv3"})
+
+	lanes, err = p.StopOrder([]string{"srv1", "srv2", "srv3"})
+	c.Assert(err, IsNil)
+	c.Assert(len(lanes), Equals, 1)
+	c.Assert(lanes[0], DeepEquals, []string{"srv3", "srv2", "srv1"})
+}
+
+func (s *S) TestStartStopOrderMultipleLanes(c *C) {
+	layer := &plan.Layer{
+		Summary:     "services with no dependencies in different lanes",
+		Description: "a simple layer",
+		Services: map[string]*plan.Service{
+			"srv1": {
+				Name:     "srv1",
+				Override: "replace",
+				Command:  `cmd`,
+				Startup:  plan.StartupEnabled,
+			},
+			"srv2": {
+				Name:     "srv2",
+				Override: "replace",
+				Command:  `cmd`,
+				Startup:  plan.StartupEnabled,
+			},
+			"srv3": {
+				Name:     "srv3",
+				Override: "replace",
+				Command:  `cmd`,
+				Startup:  plan.StartupEnabled,
+			},
+		},
+		Checks:     map[string]*plan.Check{},
+		LogTargets: map[string]*plan.LogTarget{},
+	}
+
+	p := plan.Plan{Services: layer.Services}
+	
+	lanes, err := p.StartOrder([]string{"srv1", "srv2", "srv3"})
+	c.Assert(err, IsNil)
+	c.Assert(len(lanes), Equals, 3)
+	c.Assert(lanes[0], DeepEquals, []string{"srv1"})
+	c.Assert(lanes[1], DeepEquals, []string{"srv2"})
+	c.Assert(lanes[2], DeepEquals, []string{"srv3"})
+
+	lanes, err = p.StopOrder([]string{"srv1", "srv2", "srv3"})
+	c.Assert(err, IsNil)
+	c.Assert(len(lanes), Equals, 3)
+	c.Assert(lanes[0], DeepEquals, []string{"srv1"})
+	c.Assert(lanes[1], DeepEquals, []string{"srv2"})
+	c.Assert(lanes[2], DeepEquals, []string{"srv3"})
+}

From b4ca4b7c77488b819c1595689e581666082dc8a9 Mon Sep 17 00:00:00 2001
From: Tiexin Guo <guotiexin@gmail.com>
Date: Wed, 24 Jul 2024 12:33:39 +0800
Subject: [PATCH 08/10] chore: fmt

---
 internals/plan/plan_test.go | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/internals/plan/plan_test.go b/internals/plan/plan_test.go
index b7a23bf21..d334699e6 100644
--- a/internals/plan/plan_test.go
+++ b/internals/plan/plan_test.go
@@ -1966,7 +1966,7 @@ func (s *S) TestStartStopOrderSingleLane(c *C) {
 				Override: "replace",
 				Command:  `cmd`,
 				Requires: []string{"srv2"},
-				Before: []string{"srv2"},
+				Before:   []string{"srv2"},
 				Startup:  plan.StartupEnabled,
 			},
 			"srv2": {
@@ -1974,7 +1974,7 @@ func (s *S) TestStartStopOrderSingleLane(c *C) {
 				Override: "replace",
 				Command:  `cmd`,
 				Requires: []string{"srv3"},
-				Before: []string{"srv3"},
+				Before:   []string{"srv3"},
 				Startup:  plan.StartupEnabled,
 			},
 			"srv3": {
@@ -1989,7 +1989,7 @@ func (s *S) TestStartStopOrderSingleLane(c *C) {
 	}
 
 	p := plan.Plan{Services: layer.Services}
-	
+
 	lanes, err := p.StartOrder([]string{"srv1", "srv2", "srv3"})
 	c.Assert(err, IsNil)
 	c.Assert(len(lanes), Equals, 1)
@@ -2030,7 +2030,7 @@ func (s *S) TestStartStopOrderMultipleLanes(c *C) {
 	}
 
 	p := plan.Plan{Services: layer.Services}
-	
+
 	lanes, err := p.StartOrder([]string{"srv1", "srv2", "srv3"})
 	c.Assert(err, IsNil)
 	c.Assert(len(lanes), Equals, 3)

From 430b7085afb4da2c502663e9c48c5bdd3db2ad8c Mon Sep 17 00:00:00 2001
From: Tiexin Guo <guotiexin@gmail.com>
Date: Thu, 25 Jul 2024 19:06:38 +0800
Subject: [PATCH 09/10] chore: refactor after code review

---
 internals/daemon/api_services.go             | 28 ++++++++++----------
 internals/overlord/servstate/manager.go      |  4 +--
 internals/overlord/servstate/manager_test.go | 13 ++-------
 internals/overlord/servstate/request_test.go |  3 +--
 4 files changed, 19 insertions(+), 29 deletions(-)

diff --git a/internals/daemon/api_services.go b/internals/daemon/api_services.go
index 44028ccc2..dfc045c6b 100644
--- a/internals/daemon/api_services.go
+++ b/internals/daemon/api_services.go
@@ -144,18 +144,18 @@ func v1PostServices(c *Command, r *http.Request, _ *UserState) Response {
 		taskSet.AddAll(stopTasks)
 		taskSet.AddAll(startTasks)
 	case "replan":
-		var stopNames, startNames [][]string
-		stopNames, startNames, err = servmgr.Replan()
+		var stopLanes, startLanes [][]string
+		stopLanes, startLanes, err = servmgr.Replan()
 		if err != nil {
 			break
 		}
 		var stopTasks *state.TaskSet
-		stopTasks, err = servstate.Stop(st, stopNames)
+		stopTasks, err = servstate.Stop(st, stopLanes)
 		if err != nil {
 			break
 		}
 		var startTasks *state.TaskSet
-		startTasks, err = servstate.Start(st, startNames)
+		startTasks, err = servstate.Start(st, startLanes)
 		if err != nil {
 			break
 		}
@@ -166,13 +166,13 @@ func v1PostServices(c *Command, r *http.Request, _ *UserState) Response {
 
 		// Populate a list of services affected by the replan for summary.
 		replanned := make(map[string]bool)
-		for _, row := range stopNames {
-			for _, v := range row {
+		for _, lane := range stopLanes {
+			for _, v := range lane {
 				replanned[v] = true
 			}
 		}
-		for _, row := range startNames {
-			for _, v := range row {
+		for _, lane := range startLanes {
+			for _, v := range lane {
 				replanned[v] = true
 			}
 		}
@@ -237,15 +237,15 @@ func intersectOrdered(left []string, orderedRight [][]string) [][]string {
 	}
 
 	var out [][]string
-	for _, row := range orderedRight {
-		var intersectRow []string
-		for _, v := range row {
+	for _, lane := range orderedRight {
+		var intersectLane []string
+		for _, v := range lane {
 			if m[v] {
-				intersectRow = append(intersectRow, v)
+				intersectLane = append(intersectLane, v)
 			}
 		}
-		if len(intersectRow) > 0 {
-			out = append(out, intersectRow)
+		if len(intersectLane) > 0 {
+			out = append(out, intersectLane)
 		}
 	}
 	return out
diff --git a/internals/overlord/servstate/manager.go b/internals/overlord/servstate/manager.go
index 0fc1e20fa..43a379d27 100644
--- a/internals/overlord/servstate/manager.go
+++ b/internals/overlord/servstate/manager.go
@@ -210,8 +210,8 @@ func (m *ServiceManager) DefaultServiceNames() ([]string, error) {
 	}
 
 	var result []string
-	for _, row := range lanes {
-		result = append(result, row...)
+	for _, lane := range lanes {
+		result = append(result, lane...)
 	}
 	return result, err
 }
diff --git a/internals/overlord/servstate/manager_test.go b/internals/overlord/servstate/manager_test.go
index e4dd860c9..391e41628 100644
--- a/internals/overlord/servstate/manager_test.go
+++ b/internals/overlord/servstate/manager_test.go
@@ -351,17 +351,8 @@ services:
 
 	stops, starts, err := s.manager.Replan()
 	c.Assert(err, IsNil)
-	for _, row := range stops {
-		if len(row) > 0 {
-			c.Check(row, DeepEquals, []string{"test2", "test1"})
-		}
-	}
-
-	for _, row := range starts {
-		if len(row) > 0 {
-			c.Check(row, DeepEquals, []string{"test1", "test2"})
-		}
-	}
+	c.Check(stops, DeepEquals, [][]string{{"test2", "test1"}})
+	c.Check(starts, DeepEquals, [][]string{{"test1", "test2"}})
 
 	s.stopTestServices(c)
 }
diff --git a/internals/overlord/servstate/request_test.go b/internals/overlord/servstate/request_test.go
index 36beefc5a..c1bd8ecfd 100644
--- a/internals/overlord/servstate/request_test.go
+++ b/internals/overlord/servstate/request_test.go
@@ -18,7 +18,6 @@ import (
 	. "gopkg.in/check.v1"
 
 	"github.com/canonical/pebble/internals/overlord/servstate"
-	. "github.com/canonical/pebble/internals/testutil"
 )
 
 func (s *S) TestStart(c *C) {
@@ -57,7 +56,7 @@ services:
 	c.Assert(err, IsNil)
 	c.Assert(req.Name, Equals, "two")
 
-	c.Assert(tasks[0].Lanes()[0], IntNotEqual, tasks[1].Lanes()[0])
+	c.Assert(tasks[0].Lanes()[0], Not(Equals), tasks[1].Lanes()[0])
 }
 
 func (s *S) TestStartInTheSameLaneAfter(c *C) {

From 37bc8bac4d7b6a84895c9faff5c42df18333dcca Mon Sep 17 00:00:00 2001
From: Tiexin Guo <guotiexin@gmail.com>
Date: Fri, 26 Jul 2024 10:05:27 +0800
Subject: [PATCH 10/10] chore: refactor

---
 internals/plan/plan.go | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/internals/plan/plan.go b/internals/plan/plan.go
index 700f71e4b..1b0a7ad87 100644
--- a/internals/plan/plan.go
+++ b/internals/plan/plan.go
@@ -949,18 +949,22 @@ func (p *Plan) StopOrder(names []string) ([][]string, error) {
 func getOrCreateLane(currentLane int, service *Service, serviceLaneMapping map[string]int) int {
 	// if the service has been mapped to a lane
 	if lane, ok := serviceLaneMapping[service.Name]; ok {
+		mapServiceToLane(service, lane, serviceLaneMapping)
 		return lane
 	}
 
 	// if any dependency has been mapped to a lane
 	for _, dependency := range service.Requires {
 		if lane, ok := serviceLaneMapping[dependency]; ok {
+			mapServiceToLane(service, lane, serviceLaneMapping)
 			return lane
 		}
 	}
 
 	// neither the service itself nor any of its dependencies is mapped to an existing lane
-	return currentLane + 1
+	lane := currentLane + 1
+	mapServiceToLane(service, lane, serviceLaneMapping)
+	return lane
 }
 
 func mapServiceToLane(service *Service, lane int, serviceLaneMapping map[string]int) {
@@ -986,7 +990,6 @@ func createLanes(names []string, services map[string]*Service) ([][]string, erro
 		}
 
 		lane = getOrCreateLane(lane, service, serviceLaneMapping)
-		mapServiceToLane(service, lane, serviceLaneMapping)
 	}
 
 	// Create lanes