From 0e80f2e5b2d5a06eb87a6cd40f9a82d187bfb025 Mon Sep 17 00:00:00 2001 From: Pavel Sviderski Date: Mon, 31 Mar 2025 15:58:02 +1000 Subject: [PATCH] chore: replace immutable hash with container spec comparison --- internal/machine/docker/server.go | 18 ++--- pkg/api/container.go | 11 ++-- pkg/api/service.go | 105 ++++++++++-------------------- pkg/client/container.go | 2 +- pkg/client/deploy/container.go | 38 +++++------ pkg/client/deploy/strategy.go | 10 +-- test/e2e/assert.go | 54 ++++++++++++++- 7 files changed, 116 insertions(+), 122 deletions(-) diff --git a/internal/machine/docker/server.go b/internal/machine/docker/server.go index e0349b07..aea3f751 100644 --- a/internal/machine/docker/server.go +++ b/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 { diff --git a/pkg/api/container.go b/pkg/api/container.go index 3ce15ff5..9938b6b1 100644 --- a/pkg/api/container.go +++ b/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 { diff --git a/pkg/api/service.go b/pkg/api/service.go index 7254dfda..5eb26497 100644 --- a/pkg/api/service.go +++ b/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 diff --git a/pkg/client/container.go b/pkg/client/container.go index e9da408e..8b6dbaef 100644 --- a/pkg/client/container.go +++ b/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) } diff --git a/pkg/client/deploy/container.go b/pkg/client/deploy/container.go index 4925c9c1..fd25af27 100644 --- a/pkg/client/deploy/container.go +++ b/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 } diff --git a/pkg/client/deploy/strategy.go b/pkg/client/deploy/strategy.go index 016c2ff3..208c24f9 100644 --- a/pkg/client/deploy/strategy.go +++ b/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 diff --git a/test/e2e/assert.go b/test/e2e/assert.go index 828b6525..a0127e07 100644 --- a/test/e2e/assert.go +++ b/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.