diff --git a/pkg/client/deploy/strategy.go b/pkg/client/deploy/strategy.go index bba531db..15e43877 100644 --- a/pkg/client/deploy/strategy.go +++ b/pkg/client/deploy/strategy.go @@ -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{ diff --git a/pkg/client/deploy/strategy_test.go b/pkg/client/deploy/strategy_test.go index 210866ea..086b00f6 100644 --- a/pkg/client/deploy/strategy_test.go +++ b/pkg/client/deploy/strategy_test.go @@ -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) + } +}