chore: global reconciliation: stop all running conflicting containers, not only replace the first one

2026-08-26 11:03:34 +00:00 · 2026-02-09 22:29:25 +00:00
parent 215f21439d
commit 6e59657fe2
2 changed files with 188 additions and 0 deletions
@@ -301,6 +301,23 @@ func reconcileGlobalContainer(
 	}

 	if containerToReplace != nil {
+		// Stop any other running containers that have conflicting ports before replacing the container.
+		// This handles the edge case where multiple running containers exist (due to bugs or interrupted deployments)
+		// and more than one has ports that conflict with the new spec.
+		for _, c := range containers {
+			if c.Container.ID == containerToReplace.Container.ID || !c.Container.State.Running {
+				continue
+			}
+			conflictingPorts, err := c.Container.ConflictingServicePorts(spec.Ports)
+			if err != nil || len(conflictingPorts) > 0 {
+				ops = append(ops, &StopContainerOperation{
+					ServiceID:   serviceID,
+					ContainerID: c.Container.ID,
+					MachineID:   machineID,
+				})
+			}
+		}
+
 		// Replace the running container with a new one.
 		order := determineUpdateOrder(containerToReplace.Container, spec)
 		ops = append(ops, &ReplaceContainerOperation{
@@ -4,6 +4,8 @@ import (
 	"testing"

 	"github.com/docker/docker/api/types/container"
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 	"github.com/psviderski/uncloud/pkg/api"
 	"github.com/stretchr/testify/assert"
 )
@@ -249,3 +251,172 @@ func TestDetermineUpdateOrder(t *testing.T) {
 		})
 	}
 }
+
+func TestReconcileGlobalContainer(t *testing.T) {
+	container1 := api.ServiceContainer{
+		Container: api.Container{
+			InspectResponse: container.InspectResponse{
+				ContainerJSONBase: &container.ContainerJSONBase{
+					ID:    "container-1",
+					State: &container.State{Running: true},
+				},
+				Config: &container.Config{
+					Labels: map[string]string{
+						api.LabelServicePorts: "8080:8080/tcp@host",
+					},
+				},
+			},
+		},
+	}
+	container2WithPort9090 := api.ServiceContainer{
+		Container: api.Container{
+			InspectResponse: container.InspectResponse{
+				ContainerJSONBase: &container.ContainerJSONBase{
+					ID:    "container-2",
+					State: &container.State{Running: true},
+				},
+				Config: &container.Config{
+					Labels: map[string]string{
+						api.LabelServicePorts: "9090:9090/tcp@host",
+					},
+				},
+			},
+		},
+	}
+	container2WithPort3000 := api.ServiceContainer{
+		Container: api.Container{
+			InspectResponse: container.InspectResponse{
+				ContainerJSONBase: &container.ContainerJSONBase{
+					ID:    "container-2",
+					State: &container.State{Running: true},
+				},
+				Config: &container.Config{
+					Labels: map[string]string{
+						api.LabelServicePorts: "3000:3000/tcp@host",
+					},
+				},
+			},
+		},
+	}
+
+	tests := []struct {
+		name          string
+		containers    []api.MachineServiceContainer
+		spec          api.ServiceSpec
+		forceRecreate bool
+		expectedOps   []Operation
+	}{
+		{
+			name:       "no containers creates new",
+			containers: nil,
+			spec: api.ServiceSpec{
+				Container: api.ContainerSpec{Image: "nginx:latest"},
+			},
+			expectedOps: []Operation{
+				&RunContainerOperation{
+					ServiceID: "service-1",
+					MachineID: "machine-1",
+				},
+			},
+		},
+		{
+			name: "single running container with port conflict uses replace",
+			containers: []api.MachineServiceContainer{
+				{MachineID: "machine-1", Container: container1},
+			},
+			spec: api.ServiceSpec{
+				Container: api.ContainerSpec{Image: "nginx:latest"},
+				Ports: []api.PortSpec{
+					{ContainerPort: 8080, PublishedPort: 8080, Protocol: "tcp", Mode: api.PortModeHost},
+				},
+			},
+			expectedOps: []Operation{
+				&ReplaceContainerOperation{
+					ServiceID:    "service-1",
+					MachineID:    "machine-1",
+					OldContainer: container1,
+					Order:        api.UpdateOrderStopFirst,
+				},
+			},
+		},
+		{
+			name: "multiple running containers with different conflicting ports stops extras before replace",
+			containers: []api.MachineServiceContainer{
+				{MachineID: "machine-1", Container: container1},
+				{MachineID: "machine-1", Container: container2WithPort9090},
+			},
+			spec: api.ServiceSpec{
+				Container: api.ContainerSpec{Image: "nginx:latest"},
+				Ports: []api.PortSpec{
+					{ContainerPort: 8080, PublishedPort: 8080, Protocol: "tcp", Mode: api.PortModeHost},
+					{ContainerPort: 9090, PublishedPort: 9090, Protocol: "tcp", Mode: api.PortModeHost},
+				},
+			},
+			expectedOps: []Operation{
+				&StopContainerOperation{
+					ServiceID:   "service-1",
+					ContainerID: "container-2",
+					MachineID:   "machine-1",
+				},
+				&ReplaceContainerOperation{
+					ServiceID:    "service-1",
+					MachineID:    "machine-1",
+					OldContainer: container1,
+					Order:        api.UpdateOrderStopFirst,
+				},
+				&RemoveContainerOperation{
+					MachineID: "machine-1",
+					Container: container2WithPort9090,
+				},
+			},
+		},
+		{
+			name: "multiple running containers but only one has port conflict",
+			containers: []api.MachineServiceContainer{
+				{MachineID: "machine-1", Container: container1},
+				{MachineID: "machine-1", Container: container2WithPort3000},
+			},
+			spec: api.ServiceSpec{
+				Container: api.ContainerSpec{Image: "nginx:latest"},
+				Ports: []api.PortSpec{
+					{ContainerPort: 8080, PublishedPort: 8080, Protocol: "tcp", Mode: api.PortModeHost},
+				},
+			},
+			// Container-2 has no conflicting ports, so no StopContainerOperation for it.
+			expectedOps: []Operation{
+				&ReplaceContainerOperation{
+					ServiceID:    "service-1",
+					MachineID:    "machine-1",
+					OldContainer: container1,
+					Order:        api.UpdateOrderStopFirst,
+				},
+				&RemoveContainerOperation{
+					MachineID: "machine-1",
+					Container: container2WithPort3000,
+				},
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ops, err := reconcileGlobalContainer(tt.containers, tt.spec, "service-1", "machine-1", tt.forceRecreate)
+			assert.NoError(t, err)
+			assertOperationsEqual(t, tt.expectedOps, ops)
+		})
+	}
+}
+
+// assertOperationsEqual compares expected and actual operations, ignoring the Spec field
+// which is passed separately to the function and not the focus of these tests.
+func assertOperationsEqual(t *testing.T, expected, actual []Operation) {
+	t.Helper()
+	opts := cmp.Options{
+		cmpopts.IgnoreFields(RunContainerOperation{}, "Spec"),
+		cmpopts.IgnoreFields(ReplaceContainerOperation{}, "Spec"),
+		cmpopts.IgnoreUnexported(api.Container{}),
+	}
+	if diff := cmp.Diff(expected, actual, opts); diff != "" {
+		t.Errorf("operations mismatch (-expected +actual):\n%s", diff)
+	}
+}