chore: replace immutable hash with container spec comparison

internal/machine/docker/server.go

@@ -341,7 +341,7 @@ func (s *Server) CreateServiceContainer(
 	if err := json.Unmarshal(req.ServiceSpec, &spec); err != nil {
 		return nil, status.Errorf(codes.InvalidArgument, "unmarshal service spec: %v", err)
 	}
-	spec.ApplyDefaults()
+	spec = spec.SetDefaults()
 	if err := spec.Validate(); err != nil {
 		return nil, status.Errorf(codes.InvalidArgument, "invalid service spec: %v", err)
 	}
@@ -355,23 +355,16 @@ func (s *Server) CreateServiceContainer(
 		containerName = fmt.Sprintf("%s-%s", spec.Name, suffix)
 	}
 
-	// TODO: do not set the immutable hash as container label once container diff uses the spec stored in DB.
-	specHash, err := spec.ImmutableHash()
-	if err != nil {
-		return nil, fmt.Errorf("calculate immutable hash for service spec: %w", err)
-	}
-
 	config := &container.Config{
 		Cmd:        spec.Container.Command,
 		Entrypoint: spec.Container.Entrypoint,
 		Hostname:   containerName,
 		Image:      spec.Container.Image,
 		Labels: map[string]string{
-			api.LabelServiceID:       req.ServiceId,
-			api.LabelServiceName:     spec.Name,
-			api.LabelServiceMode:     spec.Mode,
-			api.LabelServiceSpecHash: specHash,
-			api.LabelManaged:         "",
+			api.LabelServiceID:   req.ServiceId,
+			api.LabelServiceName: spec.Name,
+			api.LabelServiceMode: spec.Mode,
+			api.LabelManaged:     "",
 		},
 	}
 	if spec.Mode == "" {
@@ -379,6 +372,7 @@ func (s *Server) CreateServiceContainer(
 	}
 
 	// TODO: do not set the ports as container labels once migrated to retrieve them from the spec in DB.
+	var err error
 	if len(spec.Ports) > 0 {
 		encodedPorts := make([]string, len(spec.Ports))
 		for i, p := range spec.Ports {

pkg/api/container.go

@@ -11,12 +11,11 @@ import (
 )
 
 const (
-	LabelManaged         = "uncloud.managed"
-	LabelServiceID       = "uncloud.service.id"
-	LabelServiceName     = "uncloud.service.name"
-	LabelServiceMode     = "uncloud.service.mode"
-	LabelServicePorts    = "uncloud.service.ports"
-	LabelServiceSpecHash = "uncloud.service.spec-hash"
+	LabelManaged      = "uncloud.managed"
+	LabelServiceID    = "uncloud.service.id"
+	LabelServiceName  = "uncloud.service.name"
+	LabelServiceMode  = "uncloud.service.mode"
+	LabelServicePorts = "uncloud.service.ports"
 )
 
 type Container struct {

pkg/api/service.go

@@ -1,8 +1,6 @@
 package api
 
 import (
-	"crypto/sha256"
-	"encoding/hex"
 	"encoding/json"
 	"fmt"
 	"maps"
@@ -37,6 +35,8 @@ func ValidateServiceID(id string) bool {
 	return serviceIDRegexp.MatchString(id)
 }
 
+// ServiceSpec defines the desired state of a service.
+// ATTENTION: after changing this struct, verify if deploy.EvalContainerSpecChange needs to be updated.
 type ServiceSpec struct {
 	Container ContainerSpec
 	// Mode is the replication mode of the service. Default is ServiceModeReplicated if empty.
@@ -48,18 +48,19 @@ type ServiceSpec struct {
 	Replicas uint `json:",omitempty"`
 }
 
-func (s *ServiceSpec) ApplyDefaults() {
-	if s.Mode == "" {
-		s.Mode = ServiceModeReplicated
+func (s *ServiceSpec) SetDefaults() ServiceSpec {
+	spec := s.Clone()
+
+	if spec.Mode == "" {
+		spec.Mode = ServiceModeReplicated
 	}
 	// Ensure the replicated service has at least one replica.
-	if s.Mode == ServiceModeReplicated && s.Replicas == 0 {
-		s.Replicas = 1
+	if spec.Mode == ServiceModeReplicated && spec.Replicas == 0 {
+		spec.Replicas = 1
 	}
+	spec.Container = spec.Container.SetDefaults()
 
-	if s.Container.PullPolicy == "" {
-		s.Container.PullPolicy = PullPolicyMissing
-	}
+	return spec
 }
 
 func (s *ServiceSpec) Validate() error {
@@ -87,68 +88,6 @@ func (s *ServiceSpec) Validate() error {
 	return nil
 }
 
-// ImmutableHash returns a hash of the immutable parts of the ServiceSpec that require container recreation if changed.
-func (s *ServiceSpec) ImmutableHash() (string, error) {
-	var err error
-	// Serialise and sort the ports to ensure the hash is consistent.
-	ports := make([]string, len(s.Ports))
-	for i, p := range s.Ports {
-		ports[i], err = p.String()
-		if err != nil {
-			return "", fmt.Errorf("encode service port spec: %w", err)
-		}
-	}
-	slices.Sort(ports)
-
-	volumes := make([]string, 0, len(s.Container.Volumes))
-	volumes = append(volumes, s.Container.Volumes...)
-	slices.Sort(volumes)
-
-	hashSpec := immutableHashSpec{
-		Command:    s.Container.Command,
-		Entrypoint: s.Container.Entrypoint,
-		Image:      s.Container.Image,
-		Init:       s.Container.Init,
-		Ports:      ports,
-		Volumes:    volumes,
-	}
-
-	data, err := json.Marshal(hashSpec)
-	if err != nil {
-		return "", fmt.Errorf("marshal immutable hash spec: %w", err)
-	}
-
-	hasher := sha256.New()
-	if _, err = hasher.Write(data); err != nil {
-		return "", fmt.Errorf("write to SHA256 hasher: %w", err)
-	}
-
-	return hex.EncodeToString(hasher.Sum(nil)), nil
-}
-
-// immutableHashSpec contains only the immutable fields from ServiceSpec that require container recreation if changed.
-type immutableHashSpec struct {
-	Command    []string `json:",omitempty"`
-	Entrypoint []string `json:",omitempty"`
-	Image      string
-	Init       *bool `json:",omitempty"`
-	// Ports are set as labels on the container which are immutable.
-	// TODO: store ingress ports in the cluster store instead of as labels which will allow changing them without
-	//  recreating the container.
-	Ports   []string `json:",omitempty"`
-	Volumes []string `json:",omitempty"`
-}
-
-// Equals returns true if the service spec is equal to the given spec ignoring the number of replicas.
-func (s *ServiceSpec) Equals(spec ServiceSpec) bool {
-	// TODO: ignore order of ports.
-	sCopy := *s
-	// Ignore the number of replicas when comparing.
-	sCopy.Replicas = 0
-	spec.Replicas = 0
-	return reflect.DeepEqual(*s, spec)
-}
-
 func (s *ServiceSpec) Clone() ServiceSpec {
 	spec := *s
 
@@ -161,6 +100,8 @@ func (s *ServiceSpec) Clone() ServiceSpec {
 	return spec
 }
 
+// ContainerSpec defines the desired state of a container in a service.
+// ATTENTION: after changing this struct, verify if deploy.EvalContainerSpecChange needs to be updated.
 type ContainerSpec struct {
 	// Command overrides the default CMD of the image to be executed when running a container.
 	Command []string
@@ -176,6 +117,16 @@ type ContainerSpec struct {
 	Volumes []string
 }
 
+// SetDefaults returns a copy of the container spec with default values set.
+func (s *ContainerSpec) SetDefaults() ContainerSpec {
+	spec := s.Clone()
+	if spec.PullPolicy == "" {
+		spec.PullPolicy = PullPolicyMissing
+	}
+
+	return spec
+}
+
 func (s *ContainerSpec) Validate() error {
 	if _, err := reference.ParseDockerRef(s.Image); err != nil {
 		return fmt.Errorf("invalid image: %w", err)
@@ -184,6 +135,16 @@ func (s *ContainerSpec) Validate() error {
 	return nil
 }
 
+func (s *ContainerSpec) Equals(spec ContainerSpec) bool {
+	orig := s.SetDefaults()
+	spec = spec.SetDefaults()
+
+	slices.Sort(orig.Volumes)
+	slices.Sort(spec.Volumes)
+
+	return reflect.DeepEqual(orig, spec)
+}
+
 func (s *ContainerSpec) Clone() ContainerSpec {
 	spec := *s
 

pkg/client/container.go

@@ -21,7 +21,7 @@ func (cli *Client) CreateContainer(
 ) (container.CreateResponse, error) {
 	var resp container.CreateResponse
 
-	spec.ApplyDefaults()
+	spec = spec.SetDefaults()
 	if err := spec.Validate(); err != nil {
 		return resp, fmt.Errorf("invalid service spec: %w", err)
 	}

pkg/client/deploy/container.go

@@ -1,8 +1,6 @@
 package deploy
 
 import (
-	"fmt"
-
 	"github.com/psviderski/uncloud/pkg/api"
 )
 
@@ -12,30 +10,30 @@ const ContainerUpToDate ContainerSpecStatus = "up-to-date"
 const ContainerNeedsUpdate ContainerSpecStatus = "needs-update"
 const ContainerNeedsRecreate ContainerSpecStatus = "needs-recreate"
 
-func CompareContainerToSpec(ctr api.ServiceContainer, spec api.ServiceSpec) (ContainerSpecStatus, error) {
-	// TODO: replace the hash comparison with a more detailed comparison of ctr.ServiceSpec and spec.
-	specHash, err := spec.ImmutableHash()
-	if err != nil {
-		return "", fmt.Errorf("calculate immutable hash for service spec: %w", err)
+func EvalContainerSpecChange(current api.ServiceSpec, new api.ServiceSpec) ContainerSpecStatus {
+	current = current.SetDefaults()
+	new = new.SetDefaults()
+
+	// Pull policy doesn't affect the container configuration.
+	new.Container.PullPolicy = current.Container.PullPolicy
+	if !current.Container.Equals(new.Container) {
+		return ContainerNeedsRecreate
 	}
 
-	// Is the hash label is unset, there is no easy way to compare its configuration with the spec,
-	// so let's recreate as well.
-	if ctr.Config.Labels[api.LabelServiceSpecHash] != specHash {
-		return ContainerNeedsRecreate, nil
+	if current.Mode != new.Mode {
+		return ContainerNeedsRecreate
+	}
+	if current.Name != new.Name {
+		return ContainerNeedsRecreate
 	}
 
 	// TODO: compare mutable properties such as memory or CPU limits when they are implemented.
 
-	// TODO: remove ports check when ports are stored in the local machine store instead of as labels.
-	ports, err := ctr.ServicePorts()
-	if err != nil {
-		return "", fmt.Errorf("get service ports: %w", err)
+	// TODO: change ports check to ContainerNeedsUpdate when ingress ports are stored only the machine DB instead
+	//  of as labels ans synced to the cluster store. Host ports changes should be handled as ContainerNeedsRecreate.
+	if !api.PortsEqual(current.Ports, new.Ports) {
+		return ContainerNeedsRecreate
 	}
 
-	if !api.PortsEqual(ports, spec.Ports) {
-		return ContainerNeedsRecreate, nil
-	}
-
-	return ContainerUpToDate, nil
+	return ContainerUpToDate
 }

pkg/client/deploy/strategy.go

@@ -100,10 +100,7 @@ func (s *RollingStrategy) planReplicated(
 				continue
 			}
 
-			status, err := CompareContainerToSpec(c.Container, spec)
-			if err != nil {
-				return plan, fmt.Errorf("compare container to spec: %w", err)
-			}
+			status := EvalContainerSpecChange(c.Container.ServiceSpec, spec)
 			containerSpecStatuses[c.Container.ID] = status
 
 			if status == ContainerUpToDate {
@@ -294,10 +291,7 @@ func reconcileGlobalContainer(
 			continue
 		}
 
-		status, err := CompareContainerToSpec(c.Container, spec)
-		if err != nil {
-			return nil, fmt.Errorf("compare container to spec: %w", err)
-		}
+		status := EvalContainerSpecChange(c.Container.ServiceSpec, spec)
 		if status == ContainerUpToDate {
 			// The container is already running with the same spec.
 			upToDate = true

test/e2e/assert.go

@@ -1,13 +1,16 @@
 package e2e
 
 import (
+	"strconv"
 	"testing"
 
 	mapset "github.com/deckarep/golang-set/v2"
+	"github.com/docker/docker/api/types/container"
+	"github.com/docker/go-connections/nat"
+	machinedocker "github.com/psviderski/uncloud/internal/machine/docker"
 	"github.com/psviderski/uncloud/pkg/api"
 	"github.com/psviderski/uncloud/pkg/client/deploy"
 	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
 )
 
 func assertServiceMatchesSpec(t *testing.T, svc api.Service, spec api.ServiceSpec) {
@@ -26,9 +29,54 @@ func assertServiceMatchesSpec(t *testing.T, svc api.Service, spec api.ServiceSpe
 }
 
 func assertContainerMatchesSpec(t *testing.T, ctr api.ServiceContainer, spec api.ServiceSpec) {
-	status, err := deploy.CompareContainerToSpec(ctr, spec)
-	require.NoError(t, err)
+	status := deploy.EvalContainerSpecChange(ctr.ServiceSpec, spec)
 	assert.Equal(t, deploy.ContainerUpToDate, status)
+
+	spec = spec.SetDefaults()
+	// Verify labels.
+	assert.Equal(t, spec.Name, ctr.Config.Labels[api.LabelServiceName])
+	assert.Equal(t, spec.Mode, ctr.Config.Labels[api.LabelServiceMode])
+	assert.Contains(t, ctr.Config.Labels, api.LabelManaged)
+
+	// Command and Entrypoint can only be compared if they are set in the spec.
+	// Otherwise, the container takes them from the image.
+	if spec.Container.Command != nil {
+		assert.EqualValues(t, spec.Container.Command, ctr.Config.Cmd)
+	}
+	if spec.Container.Entrypoint != nil {
+		assert.EqualValues(t, spec.Container.Entrypoint, ctr.Config.Entrypoint)
+	}
+
+	assert.Equal(t, spec.Container.Image, ctr.Config.Image)
+	assert.Equal(t, spec.Container.Init, ctr.HostConfig.Init)
+	assert.ElementsMatch(t, spec.Container.Volumes, ctr.HostConfig.Binds)
+
+	// Compare host ports.
+	portBindings := make(nat.PortMap)
+	for _, p := range spec.Ports {
+		if p.Mode != api.PortModeHost {
+			continue
+		}
+
+		port, err := nat.NewPort(p.Protocol, strconv.Itoa(int(p.ContainerPort)))
+		assert.NoError(t, err)
+
+		binding := nat.PortBinding{HostPort: strconv.Itoa(int(p.PublishedPort))}
+		if p.HostIP.IsValid() {
+			binding.HostIP = p.HostIP.String()
+		}
+		portBindings[port] = append(portBindings[port], binding)
+	}
+	assert.Equal(t, portBindings, ctr.HostConfig.PortBindings)
+
+	assert.Equal(t, container.RestartPolicy{
+		Name:              container.RestartPolicyAlways,
+		MaximumRetryCount: 0,
+	}, ctr.HostConfig.RestartPolicy)
+
+	// Verify network settings.
+	assert.Len(t, ctr.NetworkSettings.Networks, 1)
+	assert.Contains(t, ctr.NetworkSettings.Networks, machinedocker.NetworkName)
 }
 
 // serviceContainersByMachine returns a map of machine ID to service containers on that machine.