From 77decbd4f87d592d250c6476d7addcbeb3c87dd9 Mon Sep 17 00:00:00 2001
From: Tiexin Guo <guotiexin@gmail.com>
Date: Thu, 28 Nov 2024 12:24:17 +0800
Subject: [PATCH] fix: mapping service to lanes when services are in random
 order (#528)

If services are defined in random order in the layer config, for
example, svc1, svc2, then svc3, but svc3 depends on svc1, when mapping
these services into lanes, the maximum lane count is incorrectly
calculated.

This PR fixes this issue and closes
https://github.com/canonical/pebble/issues/525.

Manual test:

Layer config:

```yaml
summary: a simple layer
services:
  test1:
    override: replace
    command: bash -c "sleep 1000"
    startup: enabled
  test2:
    override: replace
    command: bash -c "sleep 1000"
    startup: enabled
  test3:
    override: replace
    command: bash -c "sleep 1000"
    startup: enabled
    requires:
      - test1
```

Result:

```bash
$ ./pebble run
2024-11-28T03:00:55.321Z [pebble] Started daemon.
2024-11-28T03:00:55.329Z [pebble] POST /v1/services 2.313041ms 202
2024-11-28T03:00:55.331Z [pebble] Service "test1" starting: bash -c "sleep 1000"
2024-11-28T03:00:55.332Z [pebble] Service "test2" starting: bash -c "sleep 1000"
2024-11-28T03:00:56.343Z [pebble] Service "test3" starting: bash -c "sleep 1000"
2024-11-28T03:00:57.351Z [pebble] GET /v1/changes/1/wait 2.022350876s 200
2024-11-28T03:00:57.352Z [pebble] Started default services with change 1.
^C2024-11-28T03:00:58.497Z [pebble] Exiting on interrupt signal.
2024-11-28T03:00:58.500Z [pebble] Stopping all running services.
2024-11-28T03:00:58.505Z [pebble] Service "test2" stopped
2024-11-28T03:00:58.505Z [pebble] Service "test1" stopped
2024-11-28T03:00:58.512Z [pebble] Service "test3" stopped
```

A [test
case](https://github.com/canonical/pebble/pull/528/files#diff-f3aedc0e48287c8fa95ea71f4a1bd0a350d89ab1c5559ee7db3298d44207e93bR2076)
is also added to cover this scenario.
---
 internals/plan/plan.go      |  8 ++++--
 internals/plan/plan_test.go | 53 +++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 2 deletions(-)

diff --git a/internals/plan/plan.go b/internals/plan/plan.go
index 3d65675fe..d2a9a4f3f 100644
--- a/internals/plan/plan.go
+++ b/internals/plan/plan.go
@@ -1133,7 +1133,8 @@ func createLanes(names []string, services map[string]*Service) ([][]string, erro
 	serviceLaneMapping := make(map[string]int)
 
 	// Map all services into lanes.
-	var lane = -1
+	lane := -1
+	maxLane := 0
 	for _, name := range names {
 		service, ok := services[name]
 		if !ok {
@@ -1143,10 +1144,13 @@ func createLanes(names []string, services map[string]*Service) ([][]string, erro
 		}
 
 		lane = getOrCreateLane(lane, service, serviceLaneMapping)
+		if lane > maxLane {
+			maxLane = lane
+		}
 	}
 
 	// Create lanes
-	lanes := make([][]string, lane+1)
+	lanes := make([][]string, maxLane+1)
 	for _, service := range names {
 		lane := serviceLaneMapping[service]
 		lanes[lane] = append(lanes[lane], service)
diff --git a/internals/plan/plan_test.go b/internals/plan/plan_test.go
index 000e0c30e..0e607ece4 100644
--- a/internals/plan/plan_test.go
+++ b/internals/plan/plan_test.go
@@ -2073,6 +2073,59 @@ func (s *S) TestStartStopOrderMultipleLanes(c *C) {
 	c.Assert(lanes[2], DeepEquals, []string{"srv3"})
 }
 
+func (s *S) TestStartStopOrderMultipleLanesRandomOrder(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,
+			},
+			"srv4": {
+				Name:     "srv4",
+				Override: "replace",
+				Command:  `cmd`,
+				Startup:  plan.StartupEnabled,
+				Requires: []string{"srv1"},
+				After:    []string{"srv1"},
+			},
+		},
+		Checks:     map[string]*plan.Check{},
+		LogTargets: map[string]*plan.LogTarget{},
+	}
+
+	p := plan.Plan{Services: layer.Services}
+
+	lanes, err := p.StartOrder([]string{"srv1", "srv2", "srv3", "srv4"})
+	c.Assert(err, IsNil)
+	c.Assert(len(lanes), Equals, 3)
+	c.Assert(lanes[0], DeepEquals, []string{"srv1", "srv4"})
+	c.Assert(lanes[1], DeepEquals, []string{"srv2"})
+	c.Assert(lanes[2], DeepEquals, []string{"srv3"})
+
+	lanes, err = p.StopOrder([]string{"srv1", "srv2", "srv3", "srv4"})
+	c.Assert(err, IsNil)
+	c.Assert(len(lanes), Equals, 3)
+	c.Assert(lanes[0], DeepEquals, []string{"srv4", "srv1"})
+	c.Assert(lanes[1], DeepEquals, []string{"srv2"})
+	c.Assert(lanes[2], DeepEquals, []string{"srv3"})
+}
+
 // TestSectionFieldStability detects changes in plan.Layer and plan.Plan
 // YAML fields, and fails on any change that could break hardcoded section
 // fields in the code. On failure, please carefully inspect and update