From a53fa2c1d8bd74b1411f08b24bc5c63cbfe1bd33 Mon Sep 17 00:00:00 2001 From: Pavel Sviderski Date: Fri, 14 Feb 2025 17:49:46 +1000 Subject: [PATCH] feat: do not redeploy global service if spec hasn't changed --- internal/api/container.go | 22 +++++++++-- internal/api/container_test.go | 46 +++++++++++++++++++++++ internal/api/service.go | 1 + internal/cli/client/container.go | 4 +- internal/cli/client/strategy.go | 5 ++- test/e2e/service_test.go | 63 ++++++++++++++++++-------------- 6 files changed, 107 insertions(+), 34 deletions(-) diff --git a/internal/api/container.go b/internal/api/container.go index fa93908f..200dc057 100644 --- a/internal/api/container.go +++ b/internal/api/container.go @@ -58,10 +58,24 @@ func (c *Container) ServicePorts() ([]PortSpec, error) { return ports, nil } -func (c *Container) ServiceSpec() ServiceSpec { - // TODO: migrate api.Container type to use ContainerJSON to make it possible to construct - // a ServiceSpec from a Container. - return ServiceSpec{} +// ServiceSpec constructs a service spec from the container's configuration. +func (c *Container) ServiceSpec() (ServiceSpec, error) { + ports, err := c.ServicePorts() + if err != nil { + return ServiceSpec{}, fmt.Errorf("get service ports: %w", err) + } + + return ServiceSpec{ + Container: ContainerSpec{ + Command: c.Config.Cmd, + Image: c.Config.Image, + Init: c.HostConfig.Init, + Volumes: c.HostConfig.Binds, + }, + Mode: c.ServiceMode(), + Name: c.ServiceName(), + Ports: ports, + }, nil } // Healthy determines if the container is running and healthy. diff --git a/internal/api/container_test.go b/internal/api/container_test.go index 7ad68985..b770979a 100644 --- a/internal/api/container_test.go +++ b/internal/api/container_test.go @@ -6,9 +6,55 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "net/netip" + "reflect" "testing" ) +func TestContainer_ServiceSpec(t *testing.T) { + t.Parallel() + + init := true + ctr := &Container{ContainerJSON: types.ContainerJSON{ + ContainerJSONBase: &types.ContainerJSONBase{ + HostConfig: &container.HostConfig{ + Binds: []string{"/host/path:/container/path"}, + Init: &init, + }, + }, + Config: &container.Config{ + Cmd: []string{"/app/server"}, + Image: "app:latest", + Labels: map[string]string{ + LabelServiceID: "test-service-id", + LabelServiceName: "test-service-name", + LabelServicePorts: "app.example.com:8000/https", + }, + }, + }} + + expectedSpec := ServiceSpec{ + Container: ContainerSpec{ + Command: []string{"/app/server"}, + Image: "app:latest", + Init: &init, + Volumes: []string{"/host/path:/container/path"}, + }, + Name: "test-service-name", + Ports: []PortSpec{ + { + Hostname: "app.example.com", + ContainerPort: 8000, + Protocol: ProtocolHTTPS, + Mode: PortModeIngress, + }, + }, + } + + spec, err := ctr.ServiceSpec() + require.NoError(t, err) + assert.True(t, reflect.DeepEqual(spec, expectedSpec)) +} + func TestContainer_Healthy(t *testing.T) { t.Parallel() diff --git a/internal/api/service.go b/internal/api/service.go index 6d36f849..a195dbfa 100644 --- a/internal/api/service.go +++ b/internal/api/service.go @@ -39,6 +39,7 @@ func (s *ServiceSpec) Validate() error { } func (s *ServiceSpec) Equals(spec ServiceSpec) bool { + // TODO: ignore order of ports. return reflect.DeepEqual(*s, spec) } diff --git a/internal/cli/client/container.go b/internal/cli/client/container.go index 3d488e90..18e719dd 100644 --- a/internal/cli/client/container.go +++ b/internal/cli/client/container.go @@ -47,8 +47,8 @@ func (cli *Client) CreateContainer( api.LabelManaged: "", }, } - if spec.Mode == api.ServiceModeGlobal { - config.Labels[api.LabelServiceMode] = api.ServiceModeGlobal + if spec.Mode != "" { + config.Labels[api.LabelServiceMode] = spec.Mode } if len(spec.Ports) > 0 { diff --git a/internal/cli/client/strategy.go b/internal/cli/client/strategy.go index 68471ae5..db3326d5 100644 --- a/internal/cli/client/strategy.go +++ b/internal/cli/client/strategy.go @@ -120,7 +120,10 @@ func reconcileGlobalContainer( continue } - svcSpec := c.Container.ServiceSpec() + svcSpec, err := c.Container.ServiceSpec() + if err != nil { + return nil, fmt.Errorf("get service spec: %w", err) + } if svcSpec.Equals(spec) { // The container is already running with the same spec. upToDate = true diff --git a/test/e2e/service_test.go b/test/e2e/service_test.go index 315007a7..13df2335 100644 --- a/test/e2e/service_test.go +++ b/test/e2e/service_test.go @@ -3,7 +3,6 @@ package e2e import ( "context" "errors" - "fmt" "github.com/docker/docker/api/types/container" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -37,13 +36,14 @@ func TestDeployment(t *testing.T) { require.ErrorIs(t, err, client.ErrNotFound) }) - deploy, err := cli.NewDeployment(api.ServiceSpec{ + spec := api.ServiceSpec{ Name: name, Mode: api.ServiceModeGlobal, Container: api.ContainerSpec{ Image: "portainer/pause:latest", }, - }, nil) + } + deploy, err := cli.NewDeployment(spec, nil) require.NoError(t, err) err = deploy.Validate(ctx) @@ -53,7 +53,6 @@ func TestDeployment(t *testing.T) { require.NoError(t, err) assert.IsType(t, &client.SequenceOperation{}, plan) assert.Len(t, plan.(*client.SequenceOperation).Operations, 3) // 3 run - fmt.Println("# First plan:", plan) err = deploy.Run(ctx) require.NoError(t, err) @@ -64,8 +63,12 @@ func TestDeployment(t *testing.T) { assert.Equal(t, api.ServiceModeGlobal, svc.Mode) assert.Len(t, svc.Containers, 3) + svcSpec, err := svc.Containers[0].Container.ServiceSpec() + require.NoError(t, err) + assert.True(t, svcSpec.Equals(spec)) + // Deploy a published port. - deploy, err = cli.NewDeployment(api.ServiceSpec{ + specWithPort := api.ServiceSpec{ Name: name, Mode: api.ServiceModeGlobal, Container: api.ContainerSpec{ @@ -79,14 +82,14 @@ func TestDeployment(t *testing.T) { Mode: api.PortModeHost, }, }, - }, nil) + } + deploy, err = cli.NewDeployment(specWithPort, nil) require.NoError(t, err) plan, err = deploy.Plan(ctx) require.NoError(t, err) assert.IsType(t, &client.SequenceOperation{}, plan) assert.Len(t, plan.(*client.SequenceOperation).Operations, 6) // 3 run + 3 remove - fmt.Println("# Second plan:", plan) err = deploy.Run(ctx) require.NoError(t, err) @@ -97,9 +100,13 @@ func TestDeployment(t *testing.T) { assert.Equal(t, api.ServiceModeGlobal, svc.Mode) assert.Len(t, svc.Containers, 3) + svcSpec, err = svc.Containers[0].Container.ServiceSpec() + require.NoError(t, err) + assert.True(t, svcSpec.Equals(specWithPort)) + // Deploy the same conflicting port but with container spec changes init := true - spec := api.ServiceSpec{ + specWithPortAndInit := api.ServiceSpec{ Name: name, Mode: api.ServiceModeGlobal, Container: api.ContainerSpec{ @@ -115,14 +122,13 @@ func TestDeployment(t *testing.T) { }, }, } - deploy, err = cli.NewDeployment(spec, nil) + deploy, err = cli.NewDeployment(specWithPortAndInit, nil) require.NoError(t, err) plan, err = deploy.Plan(ctx) require.NoError(t, err) assert.IsType(t, &client.SequenceOperation{}, plan) assert.Len(t, plan.(*client.SequenceOperation).Operations, 9) // 3 stop + 3 run + 3 remove - fmt.Println("# Third plan:", plan) err = deploy.Run(ctx) require.NoError(t, err) @@ -133,24 +139,27 @@ func TestDeployment(t *testing.T) { assert.Equal(t, api.ServiceModeGlobal, svc.Mode) assert.Len(t, svc.Containers, 3) + svcSpec, err = svc.Containers[0].Container.ServiceSpec() + require.NoError(t, err) + assert.True(t, svcSpec.Equals(specWithPortAndInit)) + // Deploying the same spec should be a no-op. - //deploy, err = cli.NewDeployment(spec, nil) - //require.NoError(t, err) - // - //plan, err = deploy.Plan(ctx) - //require.NoError(t, err) - //assert.IsType(t, &client.SequenceOperation{}, plan) - //assert.Len(t, plan.(*client.SequenceOperation).Operations, 0) // no-op - //fmt.Println("# Forth plan:", plan) - // - //err = deploy.Run(ctx) - //require.NoError(t, err) - // - //svc, err = cli.InspectService(ctx, name) - //require.NoError(t, err) - //assert.Equal(t, name, svc.Name) - //assert.Equal(t, api.ServiceModeGlobal, svc.Mode) - //assert.Len(t, svc.Containers, 3) + deploy, err = cli.NewDeployment(specWithPortAndInit, nil) + require.NoError(t, err) + + plan, err = deploy.Plan(ctx) + require.NoError(t, err) + assert.IsType(t, &client.SequenceOperation{}, plan) + assert.Len(t, plan.(*client.SequenceOperation).Operations, 0) // no-op + + err = deploy.Run(ctx) + require.NoError(t, err) + + svc, err = cli.InspectService(ctx, name) + require.NoError(t, err) + assert.Equal(t, name, svc.Name) + assert.Equal(t, api.ServiceModeGlobal, svc.Mode) + assert.Len(t, svc.Containers, 3) }) }