deploy.update_config.order support to start-first or stop-first when replacing containers (#248)

pkg/client/compose/service.go

@@ -97,6 +97,20 @@ func ServiceSpecFromCompose(project *types.Project, serviceName string) (api.Ser
 		default:
 			return spec, fmt.Errorf("unsupported deploy mode: '%s'", service.Deploy.Mode)
 		}
+
+		// Parse update_config.order
+		if cfg := service.Deploy.UpdateConfig; cfg != nil {
+			switch cfg.Order {
+			case "":
+				// No order specified, use default behavior.
+			case "start-first":
+				spec.UpdateConfig.Order = api.UpdateOrderStartFirst
+			case "stop-first":
+				spec.UpdateConfig.Order = api.UpdateOrderStopFirst
+			default:
+				return spec, fmt.Errorf("unsupported update_config.order: '%s'", cfg.Order)
+			}
+		}
 	}
 
 	// TODO: can service.tmpfs be handled as tmpfs volume mounts as well?

pkg/client/compose/service_test.go

@@ -787,6 +787,112 @@ services:
 	}
 }
 
+func TestServiceSpecFromCompose_UpdateConfig(t *testing.T) {
+	tests := []struct {
+		name        string
+		composeYAML string
+		expected    api.UpdateConfig
+		expectError bool
+	}{
+		{
+			name: "no update_config",
+			composeYAML: `
+services:
+  test:
+    image: nginx
+`,
+			expected: api.UpdateConfig{},
+		},
+		{
+			name: "update_config with stop-first order",
+			composeYAML: `
+services:
+  test:
+    image: postgres
+    deploy:
+      update_config:
+        order: stop-first
+`,
+			expected: api.UpdateConfig{
+				Order: api.UpdateOrderStopFirst,
+			},
+		},
+		{
+			name: "update_config with start-first order",
+			composeYAML: `
+services:
+  test:
+    image: nginx
+    deploy:
+      update_config:
+        order: start-first
+`,
+			expected: api.UpdateConfig{
+				Order: api.UpdateOrderStartFirst,
+			},
+		},
+		{
+			name: "update_config with invalid order",
+			composeYAML: `
+services:
+  test:
+    image: nginx
+    deploy:
+      update_config:
+        order: invalid-order
+`,
+			expectError: true,
+		},
+		{
+			name: "update_config with empty order",
+			composeYAML: `
+services:
+  test:
+    image: nginx
+    deploy:
+      update_config:
+        parallelism: 1
+`,
+			expected: api.UpdateConfig{},
+		},
+		{
+			name: "update_config with replicas and order",
+			composeYAML: `
+services:
+  test:
+    image: nginx
+    deploy:
+      replicas: 3
+      update_config:
+        order: stop-first
+`,
+			expected: api.UpdateConfig{
+				Order: api.UpdateOrderStopFirst,
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			project, err := LoadProjectFromContent(context.Background(), tt.composeYAML)
+			if tt.expectError {
+				require.Error(t, err)
+				return
+			}
+			require.NoError(t, err)
+
+			spec, err := ServiceSpecFromCompose(project, "test")
+			if tt.expectError {
+				assert.Error(t, err)
+				return
+			}
+			require.NoError(t, err)
+
+			assert.Equal(t, tt.expected, spec.UpdateConfig)
+		})
+	}
+}
+
 func TestServiceSpecFromCompose_XMachinesPlacement(t *testing.T) {
 	tests := []struct {
 		name        string

pkg/client/deploy/operation.go

@@ -158,6 +158,66 @@ func (o *CreateVolumeOperation) String() string {
 		o.MachineID, o.VolumeSpec.DockerVolumeName())
 }
 
+// ReplaceContainerOperation replaces an old container with a new one based on the specified update order.
+// For start-first: starts new container, then removes old container.
+// For stop-first: stops old container, starts new container, then removes old container.
+type ReplaceContainerOperation struct {
+	ServiceID    string
+	Spec         api.ServiceSpec
+	MachineID    string
+	OldContainer api.ServiceContainer
+	// Order specifies the update order: "start-first" or "stop-first".
+	Order string
+}
+
+func (o *ReplaceContainerOperation) Execute(ctx context.Context, cli Client) error {
+
+	stopFirst := o.Order == api.UpdateOrderStopFirst
+
+	if stopFirst {
+		if err := cli.StopContainer(ctx, o.ServiceID, o.OldContainer.ID, container.StopOptions{}); err != nil {
+			return fmt.Errorf("stop old container: %w", err)
+		}
+	}
+
+	// TODO: Rollback support - if new container fails to start, restart old container (#24)
+	// TODO: When parallelism is added, rollback becomes more complex - need to track which containers
+	//       were stopped and restore them all on failure
+	resp, err := cli.CreateContainer(ctx, o.ServiceID, o.Spec, o.MachineID)
+	if err != nil {
+		return fmt.Errorf("create container: %w", err)
+	}
+	if err = cli.StartContainer(ctx, o.ServiceID, resp.ID); err != nil {
+		return fmt.Errorf("start container: %w", err)
+	}
+
+	// For start-first, we need to stop before removing.
+	// For stop-first, the container is already stopped.
+	if !stopFirst {
+		if err := cli.StopContainer(ctx, o.ServiceID, o.OldContainer.ID, container.StopOptions{}); err != nil {
+			return fmt.Errorf("stop old container: %w", err)
+		}
+	}
+
+	if err := cli.RemoveContainer(ctx, o.ServiceID, o.OldContainer.ID, container.RemoveOptions{
+		RemoveVolumes: true,
+	}); err != nil {
+		return fmt.Errorf("remove old container: %w", err)
+	}
+
+	return nil
+}
+
+func (o *ReplaceContainerOperation) Format(resolver NameResolver) string {
+	return fmt.Sprintf("%s: Replace container [id=%s image=%s order=%s]",
+		resolver.MachineName(o.MachineID), o.OldContainer.ShortID(), o.Spec.Container.Image, o.Order)
+}
+
+func (o *ReplaceContainerOperation) String() string {
+	return fmt.Sprintf("ReplaceContainerOperation[machine_id=%s service_id=%s old_container_id=%s order=%s]",
+		o.MachineID, o.ServiceID, o.OldContainer.ID, o.Order)
+}
+
 // SequenceOperation is a composite operation that executes a sequence of operations in order.
 type SequenceOperation struct {
 	Operations []Operation

pkg/client/deploy/strategy.go

@@ -160,29 +160,16 @@ func (s *RollingStrategy) planReplicated(svc *api.Service, spec api.ServiceSpec)
 				continue
 			}
 			// TODO: handle ContainerNeedsUpdate when update of mutable fields on a container is supported.
-
-			conflictingPorts, portsErr := ctr.ConflictingServicePorts(spec.Ports)
-			if portsErr != nil || len(conflictingPorts) > 0 {
-				// Stop the malformed container or the container with conflicting ports.
-				plan.Operations = append(plan.Operations, &StopContainerOperation{
-					ServiceID:   plan.ServiceID,
-					ContainerID: ctr.ID,
-					MachineID:   m.Id,
-				})
-			}
 		}
 
-		// Run a new container.
-		plan.Operations = append(plan.Operations, &RunContainerOperation{
-			ServiceID: plan.ServiceID,
-			Spec:      spec,
-			MachineID: m.Id,
-		})
-
-		// Remove the old container.
-		plan.Operations = append(plan.Operations, &RemoveContainerOperation{
-			MachineID: m.Id,
-			Container: ctr,
+		// Replace the old container with a new one.
+		order := determineUpdateOrder(ctr, spec)
+		plan.Operations = append(plan.Operations, &ReplaceContainerOperation{
+			ServiceID:    plan.ServiceID,
+			Spec:         spec,
+			MachineID:    m.Id,
+			OldContainer: ctr,
+			Order:        order,
 		})
 	}
 
@@ -304,43 +291,83 @@ func reconcileGlobalContainer(
 	}
 
 	// The machine has containers but none of them match the new spec.
-	// Stop the old running containers that have conflicting ports with the new spec before running a new one.
-	for _, c := range containers {
+	// Find the first running container to replace (there should typically be only one).
+	var containerToReplace *api.MachineServiceContainer
+	for i, c := range containers {
 		if c.Container.State.Running {
-			conflictingPorts, err := c.Container.ConflictingServicePorts(spec.Ports)
-			if err != nil {
-				return nil, fmt.Errorf("check conflicting ports: %w", err)
-			}
-
-			if len(conflictingPorts) > 0 {
-				// Stop the running container with conflicting ports.
-				ops = append(ops, &StopContainerOperation{
-					ServiceID:   serviceID,
-					ContainerID: c.Container.ID,
-					MachineID:   c.MachineID,
-				})
-			}
+			containerToReplace = &containers[i]
+			break
 		}
 	}
 
-	// Run a new container.
-	ops = append(ops, &RunContainerOperation{
-		ServiceID: serviceID,
-		Spec:      spec,
-		MachineID: machineID,
-	})
-
-	// Remove the old containers.
-	for _, c := range containers {
-		ops = append(ops, &RemoveContainerOperation{
-			MachineID: c.MachineID,
-			Container: c.Container,
+	if containerToReplace != nil {
+		// Replace the running container with a new one.
+		order := determineUpdateOrder(containerToReplace.Container, spec)
+		ops = append(ops, &ReplaceContainerOperation{
+			ServiceID:    serviceID,
+			Spec:         spec,
+			MachineID:    machineID,
+			OldContainer: containerToReplace.Container,
+			Order:        order,
 		})
+
+		// Remove any other containers (there shouldn't be any in normal operation).
+		for _, c := range containers {
+			if c.Container.ID == containerToReplace.Container.ID {
+				continue
+			}
+			ops = append(ops, &RemoveContainerOperation{
+				MachineID: c.MachineID,
+				Container: c.Container,
+			})
+		}
+	} else {
+		// No running containers, create a new one and remove all stopped containers.
+		ops = append(ops, &RunContainerOperation{
+			ServiceID: serviceID,
+			Spec:      spec,
+			MachineID: machineID,
+		})
+		for _, c := range containers {
+			ops = append(ops, &RemoveContainerOperation{
+				MachineID: c.MachineID,
+				Container: c.Container,
+			})
+		}
 	}
 
 	return ops, nil
 }
 
+// determineUpdateOrder determines the update order for replacing a container based on the service spec
+// and current container state. The order can be explicitly set in UpdateConfig, or automatically determined:
+// - If the user explicitly set order, respect it
+// - Services with port conflicts require stop-first (ports must be freed first)
+// - Single-replica services with data volumes default to stop-first (prevents data corruption)
+// - Multi-replica services use start-first (concurrent access already happening)
+// - All other services default to start-first (minimizes downtime)
+func determineUpdateOrder(oldContainer api.ServiceContainer, spec api.ServiceSpec) string {
+	// User explicitly set order - respect it
+	if spec.UpdateConfig.Order != "" {
+		return spec.UpdateConfig.Order
+	}
+
+	// Port conflicts require stop-first
+	conflictingPorts, err := oldContainer.ConflictingServicePorts(spec.Ports)
+	if err != nil || len(conflictingPorts) > 0 {
+		return api.UpdateOrderStopFirst
+	}
+
+	// Single-replica services with data volumes default to stop-first to prevent data corruption.
+	// Multi-replica services already have concurrent access, so start-first is safe.
+	if spec.Replicas <= 1 && len(spec.MountedDockerVolumes()) > 0 {
+		return api.UpdateOrderStopFirst
+	}
+
+	// Default: start-first for minimal downtime
+	return api.UpdateOrderStartFirst
+}
+
 // newEmptyPlan creates a new empty plan for a service deployment with initialised service ID and name.
 func newEmptyPlan(svc *api.Service, spec api.ServiceSpec) (Plan, error) {
 	var plan Plan

pkg/client/deploy/strategy_test.go

@@ -0,0 +1,251 @@
+package deploy
+
+import (
+	"testing"
+
+	"github.com/docker/docker/api/types/container"
+	"github.com/psviderski/uncloud/pkg/api"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestDetermineUpdateOrder(t *testing.T) {
+	tests := []struct {
+		name         string
+		oldContainer api.ServiceContainer
+		spec         api.ServiceSpec
+		expected     string
+	}{
+		{
+			name: "explicit stop-first order",
+			oldContainer: api.ServiceContainer{
+				Container: api.Container{
+					InspectResponse: container.InspectResponse{
+						Config: &container.Config{Labels: map[string]string{}},
+					},
+				},
+			},
+			spec: api.ServiceSpec{
+				UpdateConfig: api.UpdateConfig{
+					Order: api.UpdateOrderStopFirst,
+				},
+			},
+			expected: api.UpdateOrderStopFirst,
+		},
+		{
+			name: "explicit start-first order",
+			oldContainer: api.ServiceContainer{
+				Container: api.Container{
+					InspectResponse: container.InspectResponse{
+						Config: &container.Config{Labels: map[string]string{}},
+					},
+				},
+			},
+			spec: api.ServiceSpec{
+				UpdateConfig: api.UpdateConfig{
+					Order: api.UpdateOrderStartFirst,
+				},
+			},
+			expected: api.UpdateOrderStartFirst,
+		},
+		{
+			name: "explicit start-first overrides volume default",
+			oldContainer: api.ServiceContainer{
+				Container: api.Container{
+					InspectResponse: container.InspectResponse{
+						Config: &container.Config{Labels: map[string]string{}},
+					},
+				},
+			},
+			spec: api.ServiceSpec{
+				UpdateConfig: api.UpdateConfig{
+					Order: api.UpdateOrderStartFirst,
+				},
+				Volumes: []api.VolumeSpec{
+					{
+						Name: "data",
+						Type: api.VolumeTypeVolume,
+						VolumeOptions: &api.VolumeOptions{
+							Name: "data",
+						},
+					},
+				},
+				Container: api.ContainerSpec{
+					VolumeMounts: []api.VolumeMount{
+						{
+							VolumeName:    "data",
+							ContainerPath: "/data",
+						},
+					},
+				},
+			},
+			expected: api.UpdateOrderStartFirst,
+		},
+		{
+			name: "single-replica service with volume defaults to stop-first",
+			oldContainer: api.ServiceContainer{
+				Container: api.Container{
+					InspectResponse: container.InspectResponse{
+						Config: &container.Config{Labels: map[string]string{}},
+					},
+				},
+			},
+			spec: api.ServiceSpec{
+				Replicas: 1,
+				Volumes: []api.VolumeSpec{
+					{
+						Name: "db-data",
+						Type: api.VolumeTypeVolume,
+						VolumeOptions: &api.VolumeOptions{
+							Name: "db-data",
+						},
+					},
+				},
+				Container: api.ContainerSpec{
+					VolumeMounts: []api.VolumeMount{
+						{
+							VolumeName:    "db-data",
+							ContainerPath: "/var/lib/postgresql/data",
+						},
+					},
+				},
+			},
+			expected: api.UpdateOrderStopFirst,
+		},
+		{
+			name: "multi-replica service with volume defaults to start-first",
+			oldContainer: api.ServiceContainer{
+				Container: api.Container{
+					InspectResponse: container.InspectResponse{
+						Config: &container.Config{Labels: map[string]string{}},
+					},
+				},
+			},
+			spec: api.ServiceSpec{
+				Replicas: 3,
+				Volumes: []api.VolumeSpec{
+					{
+						Name: "app-data",
+						Type: api.VolumeTypeVolume,
+						VolumeOptions: &api.VolumeOptions{
+							Name: "app-data",
+						},
+					},
+				},
+				Container: api.ContainerSpec{
+					VolumeMounts: []api.VolumeMount{
+						{
+							VolumeName:    "app-data",
+							ContainerPath: "/data",
+						},
+					},
+				},
+			},
+			expected: api.UpdateOrderStartFirst,
+		},
+		{
+			name: "service with bind mount defaults to start-first",
+			oldContainer: api.ServiceContainer{
+				Container: api.Container{
+					InspectResponse: container.InspectResponse{
+						Config: &container.Config{Labels: map[string]string{}},
+					},
+				},
+			},
+			spec: api.ServiceSpec{
+				Volumes: []api.VolumeSpec{
+					{
+						Name: "config",
+						Type: api.VolumeTypeBind,
+						BindOptions: &api.BindOptions{
+							HostPath: "/etc/app/config",
+						},
+					},
+				},
+				Container: api.ContainerSpec{
+					VolumeMounts: []api.VolumeMount{
+						{
+							VolumeName:    "config",
+							ContainerPath: "/config",
+						},
+					},
+				},
+			},
+			expected: api.UpdateOrderStartFirst,
+		},
+		{
+			name: "service with tmpfs mount defaults to start-first",
+			oldContainer: api.ServiceContainer{
+				Container: api.Container{
+					InspectResponse: container.InspectResponse{
+						Config: &container.Config{Labels: map[string]string{}},
+					},
+				},
+			},
+			spec: api.ServiceSpec{
+				Volumes: []api.VolumeSpec{
+					{
+						Name: "tmp",
+						Type: api.VolumeTypeTmpfs,
+					},
+				},
+				Container: api.ContainerSpec{
+					VolumeMounts: []api.VolumeMount{
+						{
+							VolumeName:    "tmp",
+							ContainerPath: "/tmp",
+						},
+					},
+				},
+			},
+			expected: api.UpdateOrderStartFirst,
+		},
+		{
+			name: "stateless service defaults to start-first",
+			oldContainer: api.ServiceContainer{
+				Container: api.Container{
+					InspectResponse: container.InspectResponse{
+						Config: &container.Config{Labels: map[string]string{}},
+					},
+				},
+			},
+			spec: api.ServiceSpec{
+				Container: api.ContainerSpec{
+					Image: "nginx:latest",
+				},
+			},
+			expected: api.UpdateOrderStartFirst,
+		},
+		{
+			name: "port conflict forces stop-first",
+			oldContainer: api.ServiceContainer{
+				Container: api.Container{
+					InspectResponse: container.InspectResponse{
+						Config: &container.Config{
+							Labels: map[string]string{
+								api.LabelServicePorts: `[{"container_port":8080,"published_port":8080,"protocol":"tcp","mode":"host"}]`,
+							},
+						},
+					},
+				},
+			},
+			spec: api.ServiceSpec{
+				Ports: []api.PortSpec{
+					{
+						ContainerPort: 8080,
+						PublishedPort: 8080,
+						Protocol:      "tcp",
+						Mode:          api.PortModeHost,
+					},
+				},
+			},
+			expected: api.UpdateOrderStopFirst,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result := determineUpdateOrder(tt.oldContainer, tt.spec)
+			assert.Equal(t, tt.expected, result)
+		})
+	}
+}