Skip to content

Commit

Permalink
registry: tweak how desired image is reconciled (#345)
Browse files Browse the repository at this point in the history 
the old logic wasn't doing the 3-way merge quite right

fixes #344

Signed-off-by: Nick Santos <[email protected]>
  • Loading branch information
@nicks
nicks authored Jun 18, 2024
1 parent 5857103 commit b9c0b12
Show file tree
Hide file tree
Showing 2 changed files with 60 additions and 43 deletions.
29 changes: 23 additions & 6 deletions pkg/registry/registry.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,6 @@ func FillDefaults(registry *api.Registry) {
if registry.Name == "" {
registry.Name = "ctlptl-registry"
}

if registry.Image == "" {
registry.Image = DefaultRegistryImageRef
}
}

type socatController interface {
Expand Down Expand Up @@ -212,7 +208,11 @@ func (c *Controller) Apply(ctx context.Context, desired *api.Registry) (*api.Reg
// If the port has changed, let's delete the registry and recreate it.
needsDelete = true
}
if !imagesRefsEqual(existing.Status.Image, desired.Image) {
// If the desired image is different
// from the existing image, we need
// to delete the registry and recreate it.
if existing.Status.Image != "" && desired.Image != "" &&
!imagesRefsEqual(existing.Status.Image, desired.Image) {
needsDelete = true
}
if existing.Status.State != containerStateRunning {
Expand Down Expand Up @@ -284,13 +284,15 @@ func (c *Controller) Apply(ctx context.Context, desired *api.Registry) (*api.Reg
return nil, err
}

image := c.imageConfig(existing, desired)

err = dctr.Run(
ctx,
c.dockerCLI,
desired.Name,
&container.Config{
Hostname: desired.Name,
Image: desired.Image,
Image: image,
ExposedPorts: exposedPorts,
Labels: c.labelConfigs(existing, desired),
Env: desired.Env,
Expand Down Expand Up @@ -377,6 +379,21 @@ func (c *Controller) labelConfigs(existing *api.Registry, desired *api.Registry)
return newLabels
}

// Compute the image to ContainerCreate() call
func (c *Controller) imageConfig(existing *api.Registry, desired *api.Registry) string {
// Desired image takes precedence.
if desired.Image != "" {
return desired.Image
}

// Preserve existing image when possible.
if existing.Status.Image != "" {
return existing.Status.Image
}

return DefaultRegistryImageRef
}

func (c *Controller) maybeCreateForwarder(ctx context.Context, port int) error {
if docker.IsLocalHost(c.dockerCLI.Client().DaemonHost()) {
return nil
Expand Down
74 changes: 37 additions & 37 deletions pkg/registry/registry_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ func kindRegistry() types.Container {
return types.Container{
ID: "a815c0ec15f1f7430bd402e3fffe65026dd692a1a99861a52b3e30ad6e253a08",
Names: []string{"/kind-registry"},
Image: "registry:2",
Image: DefaultRegistryImageRef,
ImageID: "sha256:2d4f4b5309b1e41b4f83ae59b44df6d673ef44433c734b14c1c103ebca82c116",
Command: "/entrypoint.sh /etc/docker/registry/config.yml",
Created: 1603483645,
Expand Down Expand Up @@ -56,7 +56,7 @@ func kindRegistryLoopback() types.Container {
return types.Container{
ID: "d62f2587ff7b03858f144d3cf83c789578a6d6403f8b82a459ab4e317917cd42",
Names: []string{"/kind-registry-loopback"},
Image: "registry:2",
Image: DefaultRegistryImageRef,
ImageID: "sha256:2d4f4b5309b1e41b4f83ae59b44df6d673ef44433c734b14c1c103ebca82c116",
Command: "/entrypoint.sh /etc/docker/registry/config.yml",
Created: 1603483646,
Expand Down Expand Up @@ -139,7 +139,7 @@ func TestListRegistries(t *testing.T) {
ContainerID: "a815c0ec15f1f7430bd402e3fffe65026dd692a1a99861a52b3e30ad6e253a08",
State: "running",
Labels: map[string]string{"dev.tilt.ctlptl.role": "registry"},
Image: "registry:2",
Image: DefaultRegistryImageRef,
Env: []string{"REGISTRY_STORAGE_DELETE_ENABLED=true", "PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"},
},
}, list.Items[0])
Expand Down Expand Up @@ -174,7 +174,7 @@ func TestListRegistries(t *testing.T) {
Networks: []string{"bridge", "kind"},
ContainerID: "d62f2587ff7b03858f144d3cf83c789578a6d6403f8b82a459ab4e317917cd42",
State: "running",
Image: "registry:2",
Image: DefaultRegistryImageRef,
Env: []string{"REGISTRY_STORAGE_DELETE_ENABLED=true", "PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"},
},
}, list.Items[2])
Expand Down Expand Up @@ -202,7 +202,7 @@ func TestGetRegistry(t *testing.T) {
ContainerID: "a815c0ec15f1f7430bd402e3fffe65026dd692a1a99861a52b3e30ad6e253a08",
State: "running",
Labels: map[string]string{"dev.tilt.ctlptl.role": "registry"},
Image: "registry:2",
Image: DefaultRegistryImageRef,
Env: []string{"REGISTRY_STORAGE_DELETE_ENABLED=true", "PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"},
},
}, registry)
Expand All @@ -216,10 +216,6 @@ func TestApplyDeadRegistry(t *testing.T) {
deadRegistry.State = "dead"
f.docker.containers = []types.Container{deadRegistry}

f.docker.onCreate = func() {
f.docker.containers = []types.Container{kindRegistry()}
}

registry, err := f.c.Apply(context.Background(), &api.Registry{
TypeMeta: typeMeta,
Name: "kind-registry",
Expand All @@ -239,10 +235,6 @@ func TestApplyLabels(t *testing.T) {
// because it doesn't have the labels we want.
f.docker.containers = []types.Container{kindRegistry()}

f.docker.onCreate = func() {
f.docker.containers = []types.Container{kindRegistry()}
}

registry, err := f.c.Apply(context.Background(), &api.Registry{
TypeMeta: typeMeta,
Name: "kind-registry",
Expand All @@ -258,7 +250,7 @@ func TestApplyLabels(t *testing.T) {
"dev.tilt.ctlptl.role": "registry",
}, config.Labels)
assert.Equal(t, "kind-registry", config.Hostname)
assert.Equal(t, "docker.io/library/registry:2", config.Image)
assert.Equal(t, DefaultRegistryImageRef, config.Image)
assert.Equal(t, []string{"REGISTRY_STORAGE_DELETE_ENABLED=true"}, config.Env)
}
}
Expand All @@ -272,11 +264,6 @@ func TestPreservePort(t *testing.T) {
existingRegistry.Ports[0].PublicPort = 5010
f.docker.containers = []types.Container{existingRegistry}

// Running a command makes the registry come alive!
f.docker.onCreate = func() {
f.docker.containers = []types.Container{kindRegistry()}
}

registry, err := f.c.Apply(context.Background(), &api.Registry{
TypeMeta: typeMeta,
Name: "kind-registry",
Expand All @@ -289,7 +276,7 @@ func TestPreservePort(t *testing.T) {
if assert.NotNil(t, config) {
assert.Equal(t, map[string]string{"dev.tilt.ctlptl.role": "registry"}, config.Labels)
assert.Equal(t, "kind-registry", config.Hostname)
assert.Equal(t, "docker.io/library/registry:2", config.Image)
assert.Equal(t, DefaultRegistryImageRef, config.Image)
}
}

Expand All @@ -301,15 +288,11 @@ func TestCustomImage(t *testing.T) {
// because it doesn't have the image we want.
f.docker.containers = []types.Container{kindRegistry()}

f.docker.onCreate = func() {
f.docker.containers = []types.Container{kindRegistry()}
}

// ensure stable w/o image change
_, err := f.c.Apply(context.Background(), &api.Registry{
TypeMeta: typeMeta,
Name: "kind-registry",
Image: "registry:2",
Image: DefaultRegistryImageRef,
})
if assert.NoError(t, err) {
assert.Nil(t, f.docker.lastCreateConfig, "Registry should not have been re-created")
Expand All @@ -330,6 +313,26 @@ func TestCustomImage(t *testing.T) {
assert.Equal(t, "kind-registry", config.Hostname)
assert.Equal(t, "fake.tilt.dev/different-registry-image:latest", config.Image)
}

// Apply a config with new labels,
// ensure image is not changed.
registry, err = f.c.Apply(context.Background(), &api.Registry{
TypeMeta: typeMeta,
Name: "kind-registry",
Labels: map[string]string{"extra-label": "ctlptl"},
})
if assert.NoError(t, err) {
assert.Equal(t, "running", registry.Status.State)
}
config = f.docker.lastCreateConfig
if assert.NotNil(t, config) {
assert.Equal(t, map[string]string{
"dev.tilt.ctlptl.role": "registry",
"extra-label": "ctlptl",
}, config.Labels)
assert.Equal(t, "kind-registry", config.Hostname)
assert.Equal(t, "fake.tilt.dev/different-registry-image:latest", config.Image)
}
}

func TestCustomEnv(t *testing.T) {
Expand All @@ -340,15 +343,11 @@ func TestCustomEnv(t *testing.T) {
// because it doesn't have the image we want.
f.docker.containers = []types.Container{kindRegistry()}

f.docker.onCreate = func() {
f.docker.containers = []types.Container{kindRegistry()}
}

// ensure stable w/o image change
_, err := f.c.Apply(context.Background(), &api.Registry{
TypeMeta: typeMeta,
Name: "kind-registry",
Image: "registry:2",
Image: DefaultRegistryImageRef,
})
if assert.NoError(t, err) {
assert.Nil(t, f.docker.lastCreateConfig, "Registry should not have been re-created")
Expand All @@ -358,7 +357,7 @@ func TestCustomEnv(t *testing.T) {
registry, err := f.c.Apply(context.Background(), &api.Registry{
TypeMeta: typeMeta,
Name: "kind-registry",
Image: "registry:2",
Image: DefaultRegistryImageRef,
Env: []string{"REGISTRY_STORAGE_DELETE_ENABLED=false"},
})
if assert.NoError(t, err) {
Expand All @@ -368,7 +367,7 @@ func TestCustomEnv(t *testing.T) {
if assert.NotNil(t, config) {
assert.Equal(t, map[string]string{"dev.tilt.ctlptl.role": "registry"}, config.Labels)
assert.Equal(t, "kind-registry", config.Hostname)
assert.Equal(t, "registry:2", config.Image)
assert.Equal(t, DefaultRegistryImageRef, config.Image)
assert.Equal(t, []string{"REGISTRY_STORAGE_DELETE_ENABLED=false"}, config.Env)
}
}
Expand All @@ -390,7 +389,6 @@ type fakeDocker struct {
lastRemovedContainer string
lastCreateConfig *container.Config
lastCreateHostConfig *container.HostConfig
onCreate func()
}

type objectNotFoundError struct {
Expand Down Expand Up @@ -432,7 +430,7 @@ func (d *fakeDocker) ContainerInspect(ctx context.Context, containerID string) (
Cmd: []string{"/etc/docker/registry/config.yml"},
Healthcheck: (*container.HealthConfig)(nil),
ArgsEscaped: false,
Image: "docker.io/library/registry:2",
Image: DefaultRegistryImageRef,
Volumes: map[string]struct{}{"/var/lib/registry": struct{}{}},
WorkingDir: "",
Entrypoint: []string{"/entrypoint.sh"},
Expand Down Expand Up @@ -483,9 +481,11 @@ func (d *fakeDocker) ContainerCreate(ctx context.Context, config *container.Conf
containerName string) (container.CreateResponse, error) {
d.lastCreateConfig = config
d.lastCreateHostConfig = hostConfig
if d.onCreate != nil {
d.onCreate()
}

c := kindRegistry()
c.Image = config.Image
d.containers = []types.Container{c}

return container.CreateResponse{}, nil
}
func (d *fakeDocker) ContainerStart(ctx context.Context, containerID string,
Expand Down

0 comments on commit b9c0b12

Please sign in to comment.