From 0791ce26e75ee61a1da7d28da3856d3459dd31b4 Mon Sep 17 00:00:00 2001 From: brice Date: Wed, 9 Sep 2026 14:58:06 +0200 Subject: [PATCH 1/5] fix(deploy): reuse exited owner volume mounts on reboot redeploy prepareDeployResources now snapshots the canonical container's named volume mounts from the same listing the orphan cleanup consumes, and passes them as preferred volumes to setupVolumes. After a reboot the owner is exited and cleanup deletes it before volume resolution runs, which previously destroyed the only proof of volume ownership and made the deploy fail closed (or boot on a fresh empty volume pre-#236). Listing is shared between snapshot and cleanup so no extra ListContainers call is added to the deploy path; callers with existing mock budgets are unaffected. Closes #277 --- internal/usecase/container/service.go | 48 +++++++-- internal/usecase/container/service_test.go | 118 ++++++++++++++++++++- 2 files changed, 159 insertions(+), 7 deletions(-) diff --git a/internal/usecase/container/service.go b/internal/usecase/container/service.go index 14816bd54..c0dead352 100644 --- a/internal/usecase/container/service.go +++ b/internal/usecase/container/service.go @@ -568,7 +568,23 @@ func (s *Service) prepareDeployResources(ctx context.Context, route domain.Route if existing != nil { existingID = existing.ID } - if err := s.cleanupOrphanedContainers(ctx, route.Domain, existingID); err != nil { + // Snapshot volume identity before orphan cleanup removes the owner. + // After a reboot the owner is exited (invisible to resolveExistingContainer) + // and cleanup deletes it, destroying the only proof of which volume holds + // the data. Carry its mounts as preferred volumes (same pattern as + // attachment redeploys) so setupVolumes reuses them instead of creating + // a fresh empty volume or failing closed on unverified legacy ownership. + // Read the snapshot from the same listing the cleanup pass consumes, so no + // extra ListContainers call is added to the deploy path. + allContainers, err := s.runtime.ListContainers(ctx, true) + if err != nil { + log.WrapErr(err, "failed to list containers for deploy, proceeding without volume snapshot") + } + preferredVolumes := namedVolumeMounts(existing) + if existing == nil { + preferredVolumes = snapshotRouteVolumeMounts(allContainers, route.Domain) + } + if err := s.cleanupOrphanedContainers(ctx, route.Domain, existingID, allContainers); err != nil { log.WrapErr(err, "failed to cleanup orphaned containers") } @@ -607,7 +623,7 @@ func (s *Service) prepareDeployResources(ctx context.Context, route domain.Route } envHash := hashEnvironment(envVars) - volumes, err := s.setupVolumes(ctx, route.Domain, actualImageRef, nil) + volumes, err := s.setupVolumes(ctx, route.Domain, actualImageRef, preferredVolumes) if err != nil { return nil, err } @@ -2668,15 +2684,35 @@ func (s *Service) createNetworkIfNeeded(ctx context.Context, networkName string) return nil } -func (s *Service) cleanupOrphanedContainers(ctx context.Context, domainName string, skipContainerID string) error { +// snapshotRouteVolumeMounts returns the named volume mounts of the route's +// canonical container (usually exited after a reboot) from an already-fetched +// container listing. Only containers owned by this route contribute, so a +// foreign workload squatting the same name can never inject its volumes. +func snapshotRouteVolumeMounts(allContainers []*domain.Container, domainName string) map[string]namedVolumeMount { + for _, c := range allContainers { + if c.Name != managedContainerName(domainName) { + continue + } + if !isManagedRouteContainerForDomain(c, domainName) { + return nil + } + return namedVolumeMounts(c) + } + return nil +} + +func (s *Service) cleanupOrphanedContainers(ctx context.Context, domainName string, skipContainerID string, allContainers []*domain.Container) error { log := zerowrap.FromCtx(ctx) expectedName := managedContainerName(domainName) expectedNewName := expectedName + "-new" expectedNextName := expectedName + "-next" - allContainers, err := s.runtime.ListContainers(ctx, true) - if err != nil { - return err + if allContainers == nil { + var err error + allContainers, err = s.runtime.ListContainers(ctx, true) + if err != nil { + return err + } } for _, c := range allContainers { diff --git a/internal/usecase/container/service_test.go b/internal/usecase/container/service_test.go index e7a1fb48c..c2bc04b90 100644 --- a/internal/usecase/container/service_test.go +++ b/internal/usecase/container/service_test.go @@ -1880,6 +1880,122 @@ func TestService_SetupVolumes_RejectsUnverifiedLegacyVolumeWithoutStableReplacem assert.Empty(t, volumes) } +func TestService_PrepareDeployResources_ReusesExitedOwnerVolumes(t *testing.T) { + runtime := mocks.NewMockContainerRuntime(t) + envLoader := mocks.NewMockEnvLoader(t) + svc := NewService(runtime, envLoader, nil, nil, Config{ + AllowedRegistries: []string{"docker.io"}, + VolumeAutoCreate: true, + VolumePrefix: "gordon", + }, nil) + legacyName := legacyVolumeName("gordon", "app.example.com", "/data") + stableName := generateVolumeName("gordon", "app.example.com", "/data") + + // Reboot scenario: the owning container is exited, so it is invisible to + // resolveExistingContainer (existing == nil) and orphan cleanup removes it + // before volume resolution runs. + exitedOwner := &domain.Container{ + ID: "exited-owner", + Name: "gordon-app.example.com", + Status: "exited", + Labels: map[string]string{ + domain.LabelManaged: "true", + domain.LabelRoute: "app.example.com", + domain.LabelDomain: "app.example.com", + }, + VolumeMounts: []domain.ContainerVolumeMount{ + {Name: legacyName, Type: "volume", Destination: "/data"}, + }, + } + + // One shared listing serves both the mount snapshot and the cleanup pass. + // It reflects removals: once orphan cleanup deletes the exited owner, + // later calls (e.g. the legacy ownership probe) no longer see it — exactly + // like Docker after a reboot-style Deploy. Without the fix, the probe runs + // after the removal and the deploy fails closed with + // ErrVolumeOwnershipUnverified instead of reusing the legacy volume. + listed := []*domain.Container{exitedOwner} + runtime.EXPECT().ListContainers(mock.Anything, true).RunAndReturn( + func(context.Context, bool) ([]*domain.Container, error) { + return listed, nil + }, + ).Once() + runtime.EXPECT().StopContainer(mock.Anything, "exited-owner").Return(nil).Once() + runtime.EXPECT().RemoveContainer(mock.Anything, "exited-owner", true).Run( + func(context.Context, string, bool) { listed = nil }, + ).Return(nil).Once() + + runtime.EXPECT().ListImages(mock.Anything).Return([]string{"myapp:latest"}, nil) + runtime.EXPECT().GetImageExposedPorts(mock.Anything, "myapp:latest").Return([]int{8080}, nil) + runtime.EXPECT().GetImageLabels(mock.Anything, "myapp:latest").Return(nil, nil) + envLoader.EXPECT().LoadEnv(mock.Anything, "app.example.com").Return([]string{}, nil) + runtime.EXPECT().InspectImageEnv(mock.Anything, "myapp:latest").Return([]string{}, nil) + + // Preferred volume inherited from the removed owner: existence is verified + // and the legacy ownership probe is skipped entirely. The stable lookup is + // only reached without the fix, when the probe finds no owner. + runtime.EXPECT().VolumeExists(mock.Anything, legacyName).Return(true, nil).Once() + runtime.EXPECT().VolumeExists(mock.Anything, stableName).Return(false, nil).Maybe() + runtime.EXPECT().InspectImageVolumes(mock.Anything, "myapp:latest").Return([]string{"/data"}, nil).Once() + + resources, err := svc.prepareDeployResources(testContext(), domain.Route{ + Domain: "app.example.com", + Image: "myapp:latest", + }, nil) + + require.NoError(t, err) + assert.Equal(t, map[string]string{"/data": legacyName}, resources.volumes) + runtime.AssertNotCalled(t, "CreateVolume", mock.Anything, mock.Anything) +} + +func TestService_PrepareDeployResources_PrefersExistingContainerMounts(t *testing.T) { + runtime := mocks.NewMockContainerRuntime(t) + envLoader := mocks.NewMockEnvLoader(t) + svc := NewService(runtime, envLoader, nil, nil, Config{ + AllowedRegistries: []string{"docker.io"}, + VolumeAutoCreate: true, + VolumePrefix: "gordon", + }, nil) + legacyName := legacyVolumeName("gordon", "app.example.com", "/data") + + existing := &domain.Container{ + ID: "running-container", + Name: "gordon-app.example.com", + Status: "running", + Labels: map[string]string{ + domain.LabelManaged: "true", + domain.LabelRoute: "app.example.com", + domain.LabelDomain: "app.example.com", + }, + VolumeMounts: []domain.ContainerVolumeMount{ + {Name: legacyName, Type: "volume", Destination: "/data"}, + }, + } + + // The running container stays listed (cleanup skips it via skipContainerID), + // so the legacy ownership probe would find it even without the fix. + // No exited-owner lookup happens when the existing container is known. + runtime.EXPECT().ListContainers(mock.Anything, true).Return([]*domain.Container{existing}, nil).Once() + + runtime.EXPECT().ListImages(mock.Anything).Return([]string{"myapp:latest"}, nil) + runtime.EXPECT().GetImageExposedPorts(mock.Anything, "myapp:latest").Return([]int{8080}, nil) + runtime.EXPECT().GetImageLabels(mock.Anything, "myapp:latest").Return(nil, nil) + envLoader.EXPECT().LoadEnv(mock.Anything, "app.example.com").Return([]string{}, nil) + runtime.EXPECT().InspectImageEnv(mock.Anything, "myapp:latest").Return([]string{}, nil) + + runtime.EXPECT().VolumeExists(mock.Anything, legacyName).Return(true, nil).Once() + runtime.EXPECT().InspectImageVolumes(mock.Anything, "myapp:latest").Return([]string{"/data"}, nil).Once() + + resources, err := svc.prepareDeployResources(testContext(), domain.Route{ + Domain: "app.example.com", + Image: "myapp:latest", + }, existing) + + require.NoError(t, err) + assert.Equal(t, map[string]string{"/data": legacyName}, resources.volumes) + runtime.AssertNotCalled(t, "CreateVolume", mock.Anything, mock.Anything) +} + func TestService_SetupVolumes_RejectsLegacyVolumeSharedWithForeignWorkload(t *testing.T) { runtime := mocks.NewMockContainerRuntime(t) svc := NewService(runtime, nil, nil, nil, Config{VolumeAutoCreate: true, VolumePrefix: "gordon"}, nil) @@ -3778,7 +3894,7 @@ func TestService_CleanupOrphanedContainers_SkipsRestartingContainer(t *testing.T }, }, nil) - err := svc.cleanupOrphanedContainers(ctx, "test.example.com", "") + err := svc.cleanupOrphanedContainers(ctx, "test.example.com", "", nil) assert.NoError(t, err) } From ca420d31ba0598d4b486201da2bfb2ef02caf741 Mon Sep 17 00:00:00 2001 From: brice Date: Mon, 14 Sep 2026 18:22:33 +0200 Subject: [PATCH 2/5] fix(deploy): harden exited-owner volume reuse Address review findings on the reboot redeploy volume reuse: - Skip orphan cleanup when the deploy listing fails instead of letting a retried listing delete the only remaining evidence of volume ownership. - Run cleanup after volume resolution, so a failed preparation step can no longer leave the route without the owner a retry needs to prove legacy volume ownership. - Snapshot mounts from canonical, -new and -next leftovers that carry the route's ownership labels, with canonical-first precedence; a foreign container squatting a gordon name never contributes mounts. - Treat snapshotted mounts as a hint: a volume deleted while its container was stopped holds no data, so the path falls back to normal resolution instead of failing the deploy with ErrVolumeNotFound. Live mounts from an existing container and from attachments still fail closed. - Re-confirm a leftover is still stopped before removing it, since the listing is taken before the image pull. - Pass the deploy listing into cleanup instead of re-listing it. Refs #278 --- internal/boundaries/out/runtime.go | 4 + internal/usecase/container/service.go | 201 ++++++++--- internal/usecase/container/service_test.go | 389 +++++++++++++++++++-- 3 files changed, 510 insertions(+), 84 deletions(-) diff --git a/internal/boundaries/out/runtime.go b/internal/boundaries/out/runtime.go index e424ce5d7..1a4a2effe 100644 --- a/internal/boundaries/out/runtime.go +++ b/internal/boundaries/out/runtime.go @@ -51,6 +51,10 @@ type ContainerRuntime interface { // Volume management InspectImageVolumes(ctx context.Context, imageRef string) ([]string, error) + // VolumeExists reports whether the named volume exists. Callers use it to + // decide whether preserved data is gone, so an adapter must return false only + // when the volume is definitively absent, and an error when the check itself + // failed. VolumeExists(ctx context.Context, volumeName string) (bool, error) CreateVolume(ctx context.Context, volumeName string) error RemoveVolume(ctx context.Context, volumeName string, force bool) error diff --git a/internal/usecase/container/service.go b/internal/usecase/container/service.go index c0dead352..881a4fa65 100644 --- a/internal/usecase/container/service.go +++ b/internal/usecase/container/service.go @@ -568,24 +568,27 @@ func (s *Service) prepareDeployResources(ctx context.Context, route domain.Route if existing != nil { existingID = existing.ID } - // Snapshot volume identity before orphan cleanup removes the owner. - // After a reboot the owner is exited (invisible to resolveExistingContainer) - // and cleanup deletes it, destroying the only proof of which volume holds - // the data. Carry its mounts as preferred volumes (same pattern as - // attachment redeploys) so setupVolumes reuses them instead of creating - // a fresh empty volume or failing closed on unverified legacy ownership. - // Read the snapshot from the same listing the cleanup pass consumes, so no - // extra ListContainers call is added to the deploy path. - allContainers, err := s.runtime.ListContainers(ctx, true) - if err != nil { - log.WrapErr(err, "failed to list containers for deploy, proceeding without volume snapshot") - } - preferredVolumes := namedVolumeMounts(existing) - if existing == nil { - preferredVolumes = snapshotRouteVolumeMounts(allContainers, route.Domain) - } - if err := s.cleanupOrphanedContainers(ctx, route.Domain, existingID, allContainers); err != nil { - log.WrapErr(err, "failed to cleanup orphaned containers") + // After a reboot the volume owner is exited, so it is invisible to + // resolveExistingContainer and orphan cleanup deletes it. Snapshot its + // mounts first so setupVolumes reuses the existing volumes instead of + // creating empty replacements or failing closed on unverified legacy + // ownership. The snapshot reads the same listing cleanup consumes, so the + // deploy path needs no extra runtime call. + allContainers, listErr := s.runtime.ListContainers(ctx, true) + if listErr != nil { + // A failed listing leaves no snapshot, and removing the route's + // containers without one would delete the last evidence of where the + // data lives: skip cleanup rather than retrying it blind. A leftover + // container can then block container creation with a name conflict, which + // the operator sees as a failed deploy and can retry. + log.WrapErr(listErr, "failed to list containers for deploy; skipping volume snapshot and orphan cleanup") + } + preferredVolumes := volumePreference{mounts: namedVolumeMounts(existing)} + if existing == nil && listErr == nil { + preferredVolumes = volumePreference{ + mounts: snapshotRouteVolumeMounts(allContainers, route.Domain), + fromExitedContainer: true, + } } imageRef, err := s.buildValidatedImageRef(ctx, route.Image) @@ -628,6 +631,17 @@ func (s *Service) prepareDeployResources(ctx context.Context, route domain.Route return nil, err } + // Cleanup runs last because it deletes the exited owner whose mounts were + // snapshotted above: every step that may still need that evidence (image + // resolution, legacy volume ownership probes) must have completed. The new + // container is created only after this function returns, so the names are + // free by then. + if listErr == nil { + if err := s.cleanupOrphanedContainers(ctx, route.Domain, existingID, allContainers); err != nil { + log.WrapErr(err, "failed to cleanup orphaned containers") + } + } + return &deployResources{ networkName: networkName, actualImageRef: actualImageRef, @@ -2529,13 +2543,24 @@ func (s *Service) loadEnvironment(ctx context.Context, preResolved []string, dom return mergeEnvironmentVariables(dockerfileEnvVars, userEnvVars), nil } -func (s *Service) setupVolumes(ctx context.Context, domainName, imageRef string, preferredVolumes map[string]namedVolumeMount) (map[string]string, error) { +// volumePreference is the set of named-volume mounts a deploy wants to reuse. +type volumePreference struct { + mounts map[string]namedVolumeMount + // fromExitedContainer marks mounts snapshotted from an exited container. + // Those mounts are evidence of where the data lives, not a promise that it + // still does: a volume deleted while its container was stopped holds no data + // to preserve, so such a path falls back to normal volume resolution + // instead of failing the deploy with domain.ErrVolumeNotFound. + fromExitedContainer bool +} + +func (s *Service) setupVolumes(ctx context.Context, domainName, imageRef string, preferred volumePreference) (map[string]string, error) { s.mu.RLock() cfg := s.config s.mu.RUnlock() log := zerowrap.FromCtx(ctx) - volumes, err := s.validatePreferredVolumes(ctx, preferredVolumes) + volumes, err := s.validatePreferredVolumes(ctx, preferred) if err != nil { return nil, err } @@ -2598,20 +2623,28 @@ func (s *Service) setupVolumes(ctx context.Context, domainName, imageRef string, return volumes, nil } -func (s *Service) validatePreferredVolumes(ctx context.Context, preferredVolumes map[string]namedVolumeMount) (map[string]string, error) { +func (s *Service) validatePreferredVolumes(ctx context.Context, preferred volumePreference) (map[string]string, error) { + log := zerowrap.FromCtx(ctx) volumes := make(map[string]string) - for path, preferred := range preferredVolumes { - if preferred.Name == "" { + for path, mount := range preferred.mounts { + if mount.Name == "" { continue } - exists, err := s.runtime.VolumeExists(ctx, preferred.Name) + exists, err := s.runtime.VolumeExists(ctx, mount.Name) if err != nil { - return nil, fmt.Errorf("check previously mounted volume %q: %w", preferred.Name, err) + return nil, fmt.Errorf("check previously mounted volume %q: %w", mount.Name, err) } if !exists { - return nil, fmt.Errorf("previously mounted volume %q no longer exists: %w", preferred.Name, domain.ErrVolumeNotFound) + if preferred.fromExitedContainer { + log.Info(). + Str("volume", mount.Name). + Str(zerowrap.FieldPath, path). + Msg("snapshotted volume no longer exists, resolving volume through the normal path") + continue + } + return nil, fmt.Errorf("previously mounted volume %q no longer exists: %w", mount.Name, domain.ErrVolumeNotFound) } - volumes[path] = preferred.Name + volumes[path] = mount.Name } return volumes, nil } @@ -2685,43 +2718,64 @@ func (s *Service) createNetworkIfNeeded(ctx context.Context, networkName string) } // snapshotRouteVolumeMounts returns the named volume mounts of the route's -// canonical container (usually exited after a reboot) from an already-fetched -// container listing. Only containers owned by this route contribute, so a -// foreign workload squatting the same name can never inject its volumes. +// exited containers from an already-fetched container listing. Canonical, -new +// and -next are all valid names left behind by an interrupted deploy, checked +// in that order so the canonical container wins when several exist. Only +// containers whose labels prove they belong to this route contribute: matching +// on the name alone would let a foreign workload squatting a gordon name inject +// its volumes into this route. func snapshotRouteVolumeMounts(allContainers []*domain.Container, domainName string) map[string]namedVolumeMount { - for _, c := range allContainers { - if c.Name != managedContainerName(domainName) { - continue - } - if !isManagedRouteContainerForDomain(c, domainName) { - return nil + canonicalName := managedContainerName(domainName) + for _, name := range []string{canonicalName, canonicalName + "-new", canonicalName + "-next"} { + for _, c := range allContainers { + if c == nil || c.Name != name || !hasRouteOwnershipLabels(c, domainName) { + continue + } + return namedVolumeMounts(c) } - return namedVolumeMounts(c) } return nil } +// hasRouteOwnershipLabels reports whether the container labels prove the +// container belongs to the route. isManagedRouteContainerForDomain cannot answer +// that for the names matched by snapshotRouteVolumeMounts, because its name +// clause holds by definition once the name matches. Only leftover containers +// reach this check: a running container holding the route's canonical name is +// already resolved as the previous deploy by resolveExistingContainer. +func hasRouteOwnershipLabels(c *domain.Container, domainName string) bool { + if c.Labels == nil || c.Labels[domain.LabelManaged] != "true" { + return false + } + if c.Labels[domain.LabelAttachment] == "true" { + return false + } + return c.Labels[domain.LabelRoute] == domainName || c.Labels[domain.LabelDomain] == domainName +} + +// cleanupOrphanedContainers removes the route's canonical, -new and -next +// containers left behind by a previous or interrupted deploy. allContainers is +// the listing the caller already fetched: cleanup must decide on the same +// snapshot the caller used, so it never re-lists on its own. func (s *Service) cleanupOrphanedContainers(ctx context.Context, domainName string, skipContainerID string, allContainers []*domain.Container) error { log := zerowrap.FromCtx(ctx) expectedName := managedContainerName(domainName) expectedNewName := expectedName + "-new" expectedNextName := expectedName + "-next" - if allContainers == nil { - var err error - allContainers, err = s.runtime.ListContainers(ctx, true) - if err != nil { - return err + for _, c := range allContainers { + if c == nil || c.ID == skipContainerID { + continue + } + if c.Name != expectedName && c.Name != expectedNewName && c.Name != expectedNextName { + continue } - } - for _, c := range allContainers { - if (c.Name == expectedName || c.Name == expectedNewName || c.Name == expectedNextName) && c.ID != skipContainerID { - // Without a selected active container, preserve anything that may still - // be serving traffic. When an active container is selected, other active - // temp-name containers are stale candidates left by interrupted deploys. - isActive := c.Status == "running" || c.Status == "restarting" - if isActive && (skipContainerID == "" || c.Name == expectedName) { + // Without a selected active container, preserve anything that may still + // be serving traffic. When an active container is selected, other active + // temp-name containers are stale candidates left by interrupted deploys. + if c.Status == "running" || c.Status == "restarting" { + if skipContainerID == "" || c.Name == expectedName { log.Debug(). Str(zerowrap.FieldEntityID, c.ID). Str("container_name", c.Name). @@ -2729,22 +2783,53 @@ func (s *Service) cleanupOrphanedContainers(ctx context.Context, domainName stri Msg("skipping active container during orphan cleanup") continue } + } else if s.startedSinceListing(ctx, log, c.ID) { + continue + } - log.Info().Str(zerowrap.FieldEntityID, c.ID).Str("container_name", c.Name).Str(zerowrap.FieldStatus, c.Status).Msg("found orphaned container, removing") + log.Info().Str(zerowrap.FieldEntityID, c.ID).Str("container_name", c.Name).Str(zerowrap.FieldStatus, c.Status).Msg("found orphaned container, removing") - if err := s.runtime.StopContainer(ctx, c.ID); err != nil { - log.WrapErrWithFields(err, "failed to stop orphaned container", map[string]any{zerowrap.FieldEntityID: c.ID}) - } + if err := s.runtime.StopContainer(ctx, c.ID); err != nil { + log.WrapErrWithFields(err, "failed to stop orphaned container", map[string]any{zerowrap.FieldEntityID: c.ID}) + } - if err := s.runtime.RemoveContainer(ctx, c.ID, true); err != nil { - log.WrapErrWithFields(err, "failed to remove orphaned container", map[string]any{zerowrap.FieldEntityID: c.ID}) - } + if err := s.runtime.RemoveContainer(ctx, c.ID, true); err != nil { + log.WrapErrWithFields(err, "failed to remove orphaned container", map[string]any{zerowrap.FieldEntityID: c.ID}) } } return nil } +// startedSinceListing reports whether a container the listing reported as +// inactive is running now. Callers may take that listing well before removal +// runs (the volume snapshot reads it before the image pull), so an operator or +// external supervisor can restart a leftover container in between; cleanup must +// not stop a container that is serving traffic again. An unreadable status is +// treated as running: skipping a removable leftover is recoverable, killing a +// live container is not. +func (s *Service) startedSinceListing(ctx context.Context, log zerowrap.Logger, containerID string) bool { + current, err := s.runtime.InspectContainer(ctx, containerID) + if err != nil { + log.Warn(). + Err(err). + Str(zerowrap.FieldEntityID, containerID). + Msg("could not confirm leftover container status, skipping removal") + return true + } + if current == nil { + return false + } + if current.Status != "running" && current.Status != "restarting" { + return false + } + log.Debug(). + Str(zerowrap.FieldEntityID, containerID). + Str(zerowrap.FieldStatus, current.Status). + Msg("skipping container that started during deploy") + return true +} + func (s *Service) cleanupVolumesForDomain(_ context.Context, _ string) error { return nil } @@ -3105,7 +3190,7 @@ func (s *Service) attachmentExposedPorts(ctx context.Context, imageRef string) [ } func (s *Service) attachmentVolumes(ctx context.Context, containerName, imageRef string, preferredVolumes map[string]namedVolumeMount) (map[string]string, map[string]string, error) { - volumes, err := s.setupVolumes(ctx, containerName, imageRef, preferredVolumes) + volumes, err := s.setupVolumes(ctx, containerName, imageRef, volumePreference{mounts: preferredVolumes}) if err != nil { return nil, nil, fmt.Errorf("failed to setup volumes for attachment %s with image %s: %w", containerName, imageRef, err) } diff --git a/internal/usecase/container/service_test.go b/internal/usecase/container/service_test.go index c2bc04b90..e4efde171 100644 --- a/internal/usecase/container/service_test.go +++ b/internal/usecase/container/service_test.go @@ -1799,7 +1799,7 @@ func TestService_SetupVolumes_ReusesLegacyVolume(t *testing.T) { VolumeMounts: []domain.ContainerVolumeMount{{Name: legacyName}}, }}, nil) - volumes, err := svc.setupVolumes(testContext(), "app.example.com", "myapp:latest", nil) + volumes, err := svc.setupVolumes(testContext(), "app.example.com", "myapp:latest", volumePreference{}) require.NoError(t, err) assert.Equal(t, map[string]string{"/data": legacyName}, volumes) @@ -1820,7 +1820,7 @@ func TestService_SetupVolumes_DoesNotReuseCollidingLegacyVolume(t *testing.T) { }}, nil) runtime.EXPECT().VolumeExists(mock.Anything, stableName).Return(true, nil) - volumes, err := svc.setupVolumes(testContext(), "app.example.com", "myapp:latest", nil) + volumes, err := svc.setupVolumes(testContext(), "app.example.com", "myapp:latest", volumePreference{}) require.NoError(t, err) assert.Equal(t, map[string]string{"/data": stableName}, volumes) @@ -1841,7 +1841,7 @@ func TestService_SetupVolumes_CreatesStableVolumeWhenLegacyVolumeIsForeign(t *te runtime.EXPECT().VolumeExists(mock.Anything, stableName).Return(false, nil).Once() runtime.EXPECT().CreateVolume(mock.Anything, stableName).Return(nil).Once() - volumes, err := svc.setupVolumes(testContext(), "app.example.com", "myapp:latest", nil) + volumes, err := svc.setupVolumes(testContext(), "app.example.com", "myapp:latest", volumePreference{}) require.NoError(t, err) assert.Equal(t, map[string]string{"/data": stableName}, volumes) @@ -1856,7 +1856,7 @@ func TestService_SetupVolumes_PreservesPreferredVolumesWhenAutoCreateDisabled(t runtime.EXPECT().VolumeExists(mock.Anything, "existing-data").Return(true, nil).Once() - volumes, err := svc.setupVolumes(testContext(), "app.example.com", "myapp:latest", preferred) + volumes, err := svc.setupVolumes(testContext(), "app.example.com", "myapp:latest", volumePreference{mounts: preferred}) require.NoError(t, err) assert.Equal(t, map[string]string{"/data": "existing-data"}, volumes) @@ -1873,7 +1873,7 @@ func TestService_SetupVolumes_RejectsUnverifiedLegacyVolumeWithoutStableReplacem runtime.EXPECT().ListContainers(mock.Anything, true).Return([]*domain.Container{}, nil).Once() runtime.EXPECT().VolumeExists(mock.Anything, stableName).Return(false, nil).Once() - volumes, err := svc.setupVolumes(testContext(), "app.example.com", "myapp:latest", nil) + volumes, err := svc.setupVolumes(testContext(), "app.example.com", "myapp:latest", volumePreference{}) require.ErrorContains(t, err, "refusing to create a replacement") require.ErrorIs(t, err, domain.ErrVolumeOwnershipUnverified) @@ -1892,8 +1892,8 @@ func TestService_PrepareDeployResources_ReusesExitedOwnerVolumes(t *testing.T) { stableName := generateVolumeName("gordon", "app.example.com", "/data") // Reboot scenario: the owning container is exited, so it is invisible to - // resolveExistingContainer (existing == nil) and orphan cleanup removes it - // before volume resolution runs. + // resolveExistingContainer (existing == nil) and orphan cleanup would + // otherwise remove it, losing the only proof of which volume holds the data. exitedOwner := &domain.Container{ ID: "exited-owner", Name: "gordon-app.example.com", @@ -1908,18 +1908,78 @@ func TestService_PrepareDeployResources_ReusesExitedOwnerVolumes(t *testing.T) { }, } - // One shared listing serves both the mount snapshot and the cleanup pass. - // It reflects removals: once orphan cleanup deletes the exited owner, - // later calls (e.g. the legacy ownership probe) no longer see it — exactly - // like Docker after a reboot-style Deploy. Without the fix, the probe runs - // after the removal and the deploy fails closed with - // ErrVolumeOwnershipUnverified instead of reusing the legacy volume. + // One listing serves both the mount snapshot and the cleanup pass. Cleanup + // runs only after volume resolution finished, so the exited owner is still + // listed while the snapshot is validated; without the snapshot the owner is + // removed first and the legacy ownership probe finds no owner, failing the + // deploy closed with ErrVolumeOwnershipUnverified. + runtime.EXPECT().ListContainers(mock.Anything, true). + Return([]*domain.Container{exitedOwner}, nil).Once() + runtime.EXPECT().InspectContainer(mock.Anything, "exited-owner").Return(exitedOwner, nil).Once() + runtime.EXPECT().StopContainer(mock.Anything, "exited-owner").Return(nil).Once() + runtime.EXPECT().RemoveContainer(mock.Anything, "exited-owner", true).Return(nil).Once() + + runtime.EXPECT().ListImages(mock.Anything).Return([]string{"myapp:latest"}, nil) + runtime.EXPECT().GetImageExposedPorts(mock.Anything, "myapp:latest").Return([]int{8080}, nil) + runtime.EXPECT().GetImageLabels(mock.Anything, "myapp:latest").Return(nil, nil) + envLoader.EXPECT().LoadEnv(mock.Anything, "app.example.com").Return([]string{}, nil) + runtime.EXPECT().InspectImageEnv(mock.Anything, "myapp:latest").Return([]string{}, nil) + + // The snapshotted volume is reused: only its existence is verified, so the + // legacy ownership probe and the stable-name lookup never run. + runtime.EXPECT().VolumeExists(mock.Anything, legacyName).Return(true, nil).Once() + runtime.EXPECT().InspectImageVolumes(mock.Anything, "myapp:latest").Return([]string{"/data"}, nil).Once() + + resources, err := svc.prepareDeployResources(testContext(), domain.Route{ + Domain: "app.example.com", + Image: "myapp:latest", + }, nil) + + require.NoError(t, err) + assert.Equal(t, map[string]string{"/data": legacyName}, resources.volumes) + runtime.AssertNotCalled(t, "VolumeExists", mock.Anything, stableName) + runtime.AssertNotCalled(t, "CreateVolume", mock.Anything, mock.Anything) +} + +func TestService_PrepareDeployResources_ResolvesOwnershipBeforeRemovingOwner(t *testing.T) { + runtime := mocks.NewMockContainerRuntime(t) + envLoader := mocks.NewMockEnvLoader(t) + svc := NewService(runtime, envLoader, nil, nil, Config{ + AllowedRegistries: []string{"docker.io"}, + VolumeAutoCreate: true, + VolumePrefix: "gordon", + }, nil) + legacyName := legacyVolumeName("gordon", "app.example.com", "/data") + stableName := generateVolumeName("gordon", "app.example.com", "/data") + + // The exited owner mounts the volume at a destination the new image no + // longer declares, so the snapshotted mount does not cover the image path + // and the legacy ownership probe has to find the owner container itself. + exitedOwner := &domain.Container{ + ID: "exited-owner", + Name: "gordon-app.example.com", + Status: "exited", + Labels: map[string]string{ + domain.LabelManaged: "true", + domain.LabelRoute: "app.example.com", + domain.LabelDomain: "app.example.com", + }, + VolumeMounts: []domain.ContainerVolumeMount{ + {Name: legacyName, Type: "volume", Destination: "/var/lib/legacy"}, + }, + } + + // The listing reflects removals like Docker does: once orphan cleanup + // deletes the owner, the ownership probe can no longer see it. Removing the + // owner before volume resolution would therefore fail the deploy closed with + // ErrVolumeOwnershipUnverified. listed := []*domain.Container{exitedOwner} runtime.EXPECT().ListContainers(mock.Anything, true).RunAndReturn( func(context.Context, bool) ([]*domain.Container, error) { return listed, nil }, - ).Once() + ).Times(2) + runtime.EXPECT().InspectContainer(mock.Anything, "exited-owner").Return(exitedOwner, nil).Once() runtime.EXPECT().StopContainer(mock.Anything, "exited-owner").Return(nil).Once() runtime.EXPECT().RemoveContainer(mock.Anything, "exited-owner", true).Run( func(context.Context, string, bool) { listed = nil }, @@ -1931,11 +1991,9 @@ func TestService_PrepareDeployResources_ReusesExitedOwnerVolumes(t *testing.T) { envLoader.EXPECT().LoadEnv(mock.Anything, "app.example.com").Return([]string{}, nil) runtime.EXPECT().InspectImageEnv(mock.Anything, "myapp:latest").Return([]string{}, nil) - // Preferred volume inherited from the removed owner: existence is verified - // and the legacy ownership probe is skipped entirely. The stable lookup is - // only reached without the fix, when the probe finds no owner. - runtime.EXPECT().VolumeExists(mock.Anything, legacyName).Return(true, nil).Once() - runtime.EXPECT().VolumeExists(mock.Anything, stableName).Return(false, nil).Maybe() + // Existence is checked once while validating the snapshot and once while + // probing the image path, where the owner is still listed. + runtime.EXPECT().VolumeExists(mock.Anything, legacyName).Return(true, nil).Times(2) runtime.EXPECT().InspectImageVolumes(mock.Anything, "myapp:latest").Return([]string{"/data"}, nil).Once() resources, err := svc.prepareDeployResources(testContext(), domain.Route{ @@ -1944,8 +2002,8 @@ func TestService_PrepareDeployResources_ReusesExitedOwnerVolumes(t *testing.T) { }, nil) require.NoError(t, err) - assert.Equal(t, map[string]string{"/data": legacyName}, resources.volumes) - runtime.AssertNotCalled(t, "CreateVolume", mock.Anything, mock.Anything) + assert.Equal(t, map[string]string{"/data": legacyName, "/var/lib/legacy": legacyName}, resources.volumes) + runtime.AssertNotCalled(t, "VolumeExists", mock.Anything, stableName) } func TestService_PrepareDeployResources_PrefersExistingContainerMounts(t *testing.T) { @@ -1996,6 +2054,233 @@ func TestService_PrepareDeployResources_PrefersExistingContainerMounts(t *testin runtime.AssertNotCalled(t, "CreateVolume", mock.Anything, mock.Anything) } +func TestService_PrepareDeployResources_ReusesExitedTempNameOwnerVolumes(t *testing.T) { + runtime := mocks.NewMockContainerRuntime(t) + envLoader := mocks.NewMockEnvLoader(t) + svc := NewService(runtime, envLoader, nil, nil, Config{ + AllowedRegistries: []string{"docker.io"}, + VolumeAutoCreate: true, + VolumePrefix: "gordon", + }, nil) + legacyName := legacyVolumeName("gordon", "app.example.com", "/data") + stableName := generateVolumeName("gordon", "app.example.com", "/data") + + // A deploy interrupted after the replacement container was created leaves + // the volume owner under a temp name; after a reboot that exited container + // is the only remaining proof of which volume holds the route's data. + exitedTempOwner := &domain.Container{ + ID: "exited-new-owner", + Name: "gordon-app.example.com-new", + Status: "exited", + Labels: map[string]string{ + domain.LabelManaged: "true", + domain.LabelRoute: "app.example.com", + domain.LabelDomain: "app.example.com", + }, + VolumeMounts: []domain.ContainerVolumeMount{ + {Name: legacyName, Type: "volume", Destination: "/data"}, + }, + } + + runtime.EXPECT().ListContainers(mock.Anything, true). + Return([]*domain.Container{exitedTempOwner}, nil).Once() + runtime.EXPECT().InspectContainer(mock.Anything, "exited-new-owner").Return(exitedTempOwner, nil).Once() + runtime.EXPECT().StopContainer(mock.Anything, "exited-new-owner").Return(nil).Once() + runtime.EXPECT().RemoveContainer(mock.Anything, "exited-new-owner", true).Return(nil).Once() + + runtime.EXPECT().ListImages(mock.Anything).Return([]string{"myapp:latest"}, nil) + runtime.EXPECT().GetImageExposedPorts(mock.Anything, "myapp:latest").Return([]int{8080}, nil) + runtime.EXPECT().GetImageLabels(mock.Anything, "myapp:latest").Return(nil, nil) + envLoader.EXPECT().LoadEnv(mock.Anything, "app.example.com").Return([]string{}, nil) + runtime.EXPECT().InspectImageEnv(mock.Anything, "myapp:latest").Return([]string{}, nil) + + runtime.EXPECT().VolumeExists(mock.Anything, legacyName).Return(true, nil).Once() + runtime.EXPECT().InspectImageVolumes(mock.Anything, "myapp:latest").Return([]string{"/data"}, nil).Once() + + resources, err := svc.prepareDeployResources(testContext(), domain.Route{ + Domain: "app.example.com", + Image: "myapp:latest", + }, nil) + + require.NoError(t, err) + assert.Equal(t, map[string]string{"/data": legacyName}, resources.volumes) + runtime.AssertNotCalled(t, "VolumeExists", mock.Anything, stableName) + runtime.AssertNotCalled(t, "CreateVolume", mock.Anything, mock.Anything) +} + +func TestService_PrepareDeployResources_IgnoresForeignContainerOnCanonicalName(t *testing.T) { + runtime := mocks.NewMockContainerRuntime(t) + envLoader := mocks.NewMockEnvLoader(t) + svc := NewService(runtime, envLoader, nil, nil, Config{ + AllowedRegistries: []string{"docker.io"}, + VolumeAutoCreate: true, + VolumePrefix: "gordon", + }, nil) + legacyName := legacyVolumeName("gordon", "app.example.com", "/data") + stableName := generateVolumeName("gordon", "app.example.com", "/data") + foreignVolume := "other-route-data" + + // A container squatting the canonical name but labelled for another route + // stays a leftover rather than becoming this route's previous version, so + // the deploy must never adopt its mounts. Cleanup still removes it as a name + // leftover, which is pre-existing cleanup behaviour; the guarantee under + // test only covers its volumes. + foreign := &domain.Container{ + ID: "foreign-container", + Name: "gordon-app.example.com", + Status: "exited", + Labels: map[string]string{ + domain.LabelManaged: "true", + domain.LabelRoute: "other.example.com", + domain.LabelDomain: "other.example.com", + }, + VolumeMounts: []domain.ContainerVolumeMount{ + {Name: foreignVolume, Type: "volume", Destination: "/data"}, + }, + } + + runtime.EXPECT().ListContainers(mock.Anything, true).Return([]*domain.Container{foreign}, nil).Once() + runtime.EXPECT().InspectContainer(mock.Anything, "foreign-container").Return(foreign, nil).Once() + runtime.EXPECT().StopContainer(mock.Anything, "foreign-container").Return(nil).Once() + runtime.EXPECT().RemoveContainer(mock.Anything, "foreign-container", true).Return(nil).Once() + + runtime.EXPECT().ListImages(mock.Anything).Return([]string{"myapp:latest"}, nil) + runtime.EXPECT().GetImageExposedPorts(mock.Anything, "myapp:latest").Return([]int{8080}, nil) + runtime.EXPECT().GetImageLabels(mock.Anything, "myapp:latest").Return(nil, nil) + envLoader.EXPECT().LoadEnv(mock.Anything, "app.example.com").Return([]string{}, nil) + runtime.EXPECT().InspectImageEnv(mock.Anything, "myapp:latest").Return([]string{}, nil) + + // Volume resolution takes the normal path: the legacy volume does not + // exist, so a stable replacement volume is created instead. + runtime.EXPECT().VolumeExists(mock.Anything, legacyName).Return(false, nil).Once() + runtime.EXPECT().VolumeExists(mock.Anything, stableName).Return(false, nil).Once() + runtime.EXPECT().CreateVolume(mock.Anything, stableName).Return(nil).Once() + runtime.EXPECT().InspectImageVolumes(mock.Anything, "myapp:latest").Return([]string{"/data"}, nil).Once() + + resources, err := svc.prepareDeployResources(testContext(), domain.Route{ + Domain: "app.example.com", + Image: "myapp:latest", + }, nil) + + require.NoError(t, err) + assert.Equal(t, map[string]string{"/data": stableName}, resources.volumes) + runtime.AssertNotCalled(t, "VolumeExists", mock.Anything, foreignVolume) +} + +func TestService_PrepareDeployResources_IgnoresDeletedSnapshottedVolume(t *testing.T) { + runtime := mocks.NewMockContainerRuntime(t) + envLoader := mocks.NewMockEnvLoader(t) + svc := NewService(runtime, envLoader, nil, nil, Config{ + AllowedRegistries: []string{"docker.io"}, + VolumeAutoCreate: true, + VolumePrefix: "gordon", + }, nil) + legacyName := legacyVolumeName("gordon", "app.example.com", "/data") + stableName := generateVolumeName("gordon", "app.example.com", "/data") + + // The exited owner still references a volume that was deleted after it + // stopped: there is no data left to preserve, so the path must fall back to + // normal resolution instead of failing with domain.ErrVolumeNotFound. + exitedOwner := &domain.Container{ + ID: "exited-owner", + Name: "gordon-app.example.com", + Status: "exited", + Labels: map[string]string{ + domain.LabelManaged: "true", + domain.LabelRoute: "app.example.com", + domain.LabelDomain: "app.example.com", + }, + VolumeMounts: []domain.ContainerVolumeMount{ + {Name: "deleted-data", Type: "volume", Destination: "/data"}, + }, + } + + runtime.EXPECT().ListContainers(mock.Anything, true). + Return([]*domain.Container{exitedOwner}, nil).Once() + runtime.EXPECT().InspectContainer(mock.Anything, "exited-owner").Return(exitedOwner, nil).Once() + runtime.EXPECT().StopContainer(mock.Anything, "exited-owner").Return(nil).Once() + runtime.EXPECT().RemoveContainer(mock.Anything, "exited-owner", true).Return(nil).Once() + + runtime.EXPECT().ListImages(mock.Anything).Return([]string{"myapp:latest"}, nil) + runtime.EXPECT().GetImageExposedPorts(mock.Anything, "myapp:latest").Return([]int{8080}, nil) + runtime.EXPECT().GetImageLabels(mock.Anything, "myapp:latest").Return(nil, nil) + envLoader.EXPECT().LoadEnv(mock.Anything, "app.example.com").Return([]string{}, nil) + runtime.EXPECT().InspectImageEnv(mock.Anything, "myapp:latest").Return([]string{}, nil) + + runtime.EXPECT().VolumeExists(mock.Anything, "deleted-data").Return(false, nil).Once() + runtime.EXPECT().VolumeExists(mock.Anything, legacyName).Return(false, nil).Once() + runtime.EXPECT().VolumeExists(mock.Anything, stableName).Return(false, nil).Once() + runtime.EXPECT().CreateVolume(mock.Anything, stableName).Return(nil).Once() + runtime.EXPECT().InspectImageVolumes(mock.Anything, "myapp:latest").Return([]string{"/data"}, nil).Once() + + resources, err := svc.prepareDeployResources(testContext(), domain.Route{ + Domain: "app.example.com", + Image: "myapp:latest", + }, nil) + + require.NoError(t, err) + assert.Equal(t, map[string]string{"/data": stableName}, resources.volumes) +} + +func TestService_PrepareDeployResources_SkipsCleanupWhenListingFails(t *testing.T) { + runtime := mocks.NewMockContainerRuntime(t) + envLoader := mocks.NewMockEnvLoader(t) + svc := NewService(runtime, envLoader, nil, nil, Config{ + AllowedRegistries: []string{"docker.io"}, + VolumeAutoCreate: true, + VolumePrefix: "gordon", + }, nil) + legacyName := legacyVolumeName("gordon", "app.example.com", "/data") + + exitedOwner := &domain.Container{ + ID: "exited-owner", + Name: "gordon-app.example.com", + Status: "exited", + Labels: map[string]string{ + domain.LabelManaged: "true", + domain.LabelRoute: "app.example.com", + domain.LabelDomain: "app.example.com", + }, + VolumeMounts: []domain.ContainerVolumeMount{ + {Name: legacyName, Type: "volume", Destination: "/data"}, + }, + } + + // The deploy listing fails, so no snapshot exists. Removing the route's + // containers afterwards would delete the only evidence of where the volume + // data lives, so cleanup must be skipped entirely. The legacy ownership + // probe re-lists later and still sees the owner. + listCalls := 0 + runtime.EXPECT().ListContainers(mock.Anything, true).RunAndReturn( + func(context.Context, bool) ([]*domain.Container, error) { + listCalls++ + if listCalls == 1 { + return nil, errors.New("container listing unavailable") + } + return []*domain.Container{exitedOwner}, nil + }, + ).Times(2) + + runtime.EXPECT().ListImages(mock.Anything).Return([]string{"myapp:latest"}, nil) + runtime.EXPECT().GetImageExposedPorts(mock.Anything, "myapp:latest").Return([]int{8080}, nil) + runtime.EXPECT().GetImageLabels(mock.Anything, "myapp:latest").Return(nil, nil) + envLoader.EXPECT().LoadEnv(mock.Anything, "app.example.com").Return([]string{}, nil) + runtime.EXPECT().InspectImageEnv(mock.Anything, "myapp:latest").Return([]string{}, nil) + + runtime.EXPECT().VolumeExists(mock.Anything, legacyName).Return(true, nil).Once() + runtime.EXPECT().InspectImageVolumes(mock.Anything, "myapp:latest").Return([]string{"/data"}, nil).Once() + + resources, err := svc.prepareDeployResources(testContext(), domain.Route{ + Domain: "app.example.com", + Image: "myapp:latest", + }, nil) + + require.NoError(t, err) + assert.Equal(t, map[string]string{"/data": legacyName}, resources.volumes) + runtime.AssertNotCalled(t, "StopContainer", mock.Anything, "exited-owner") + runtime.AssertNotCalled(t, "RemoveContainer", mock.Anything, "exited-owner", true) +} + func TestService_SetupVolumes_RejectsLegacyVolumeSharedWithForeignWorkload(t *testing.T) { runtime := mocks.NewMockContainerRuntime(t) svc := NewService(runtime, nil, nil, nil, Config{VolumeAutoCreate: true, VolumePrefix: "gordon"}, nil) @@ -2017,7 +2302,7 @@ func TestService_SetupVolumes_RejectsLegacyVolumeSharedWithForeignWorkload(t *te }, nil).Once() runtime.EXPECT().VolumeExists(mock.Anything, stableName).Return(false, nil).Once() - volumes, err := svc.setupVolumes(testContext(), "app.example.com", "myapp:latest", nil) + volumes, err := svc.setupVolumes(testContext(), "app.example.com", "myapp:latest", volumePreference{}) require.ErrorIs(t, err, domain.ErrVolumeOwnershipUnverified) assert.Empty(t, volumes) @@ -2732,6 +3017,10 @@ func TestService_Deploy_OrphanCleanupRemovesTrueOrphans(t *testing.T) { }, nil) // Stopped orphan should be stopped and removed BEFORE we proceed + runtime.EXPECT().InspectContainer(mock.Anything, "orphan-container").Return(&domain.Container{ + ID: "orphan-container", + Status: "exited", + }, nil) runtime.EXPECT().StopContainer(mock.Anything, "orphan-container").Return(nil) runtime.EXPECT().RemoveContainer(mock.Anything, "orphan-container", true).Return(nil) @@ -3824,6 +4113,10 @@ func TestService_Deploy_OrphanCleanup_NeverKillsRunningCanonical(t *testing.T) { }, nil) // Only the stale -new container should be stopped and removed during orphan cleanup. + runtime.EXPECT().InspectContainer(mock.Anything, "stale-leftover").Return(&domain.Container{ + ID: "stale-leftover", + Status: "exited", + }, nil).Once() runtime.EXPECT().StopContainer(mock.Anything, "stale-leftover").Return(nil).Once() runtime.EXPECT().RemoveContainer(mock.Anything, "stale-leftover", true).Return(nil).Once() @@ -3886,19 +4179,63 @@ func TestService_CleanupOrphanedContainers_SkipsRestartingContainer(t *testing.T svc := NewService(runtime, envLoader, eventBus, nil, Config{}, nil) ctx := testContext() - runtime.EXPECT().ListContainers(mock.Anything, true).Return([]*domain.Container{ + err := svc.cleanupOrphanedContainers(ctx, "test.example.com", "", []*domain.Container{ { ID: "restarting-container", Name: "gordon-test.example.com", Status: "restarting", }, - }, nil) - - err := svc.cleanupOrphanedContainers(ctx, "test.example.com", "", nil) + }) assert.NoError(t, err) } +func TestService_CleanupOrphanedContainers_SkipsContainerStartedSinceListing(t *testing.T) { + runtime := mocks.NewMockContainerRuntime(t) + svc := NewService(runtime, nil, nil, nil, Config{}, nil) + + // The listing was taken before the image pull and reported the container as + // exited, but it serves traffic again by the time cleanup runs. + runtime.EXPECT().InspectContainer(mock.Anything, "revived-container").Return(&domain.Container{ + ID: "revived-container", + Name: "gordon-test.example.com", + Status: "running", + }, nil).Once() + + err := svc.cleanupOrphanedContainers(testContext(), "test.example.com", "", []*domain.Container{ + { + ID: "revived-container", + Name: "gordon-test.example.com", + Status: "exited", + }, + }) + + require.NoError(t, err) + runtime.AssertNotCalled(t, "StopContainer", mock.Anything, "revived-container") + runtime.AssertNotCalled(t, "RemoveContainer", mock.Anything, "revived-container", true) +} + +func TestService_CleanupOrphanedContainers_SkipsRemovalWhenStatusUnreadable(t *testing.T) { + runtime := mocks.NewMockContainerRuntime(t) + svc := NewService(runtime, nil, nil, nil, Config{}, nil) + + // An unreadable status is treated as running: skipping a removable leftover + // is recoverable, killing a live container is not. + runtime.EXPECT().InspectContainer(mock.Anything, "unknown-container").Return(nil, errors.New("inspect failed")).Once() + + err := svc.cleanupOrphanedContainers(testContext(), "test.example.com", "", []*domain.Container{ + { + ID: "unknown-container", + Name: "gordon-test.example.com", + Status: "exited", + }, + }) + + require.NoError(t, err) + runtime.AssertNotCalled(t, "StopContainer", mock.Anything, "unknown-container") + runtime.AssertNotCalled(t, "RemoveContainer", mock.Anything, "unknown-container", true) +} + // TestService_Deploy_RollbackOnPostSwitchCrash verifies that if the new container crashes // during the post-switch stabilization window, traffic is automatically rolled back to the // old container: the old container is restored as tracked, the proxy cache is re-invalidated, From e06c5b77ad0e7a2a3855ed40048f62e480906750 Mon Sep 17 00:00:00 2001 From: brice Date: Tue, 15 Sep 2026 18:23:29 +0200 Subject: [PATCH 3/5] fix(deploy): keep exited owner until the replacement mounts its volumes The reboot redeploy path snapshotted an exited owner's volume mounts and then removed that container before the replacement existed. A failed create, start or readiness check left the route with no container proving which volumes belong to it, so the next deploy could fail closed with ErrVolumeOwnershipUnverified or create empty replacements. The exited owner is now retained as ownership evidence: the replacement is created under a temporary name while the owner keeps its own, and the owner is removed only after the replacement runs. Removal reuses the post-switch finalize path, which stops the previous container and renames the replacement to the canonical name. The owner is kept when the replacement does not mount every preserved volume, so the last proof of ownership is never dropped silently. Cleanup skips it through an explicit keep parameter, leaving the rule for running leftovers unchanged, and stabilization now runs only against a running predecessor, since an exited owner is not a rollback target. Refs #278 --- internal/usecase/container/service.go | 142 ++++++++--- internal/usecase/container/service_test.go | 278 ++++++++++++++++++--- 2 files changed, 357 insertions(+), 63 deletions(-) diff --git a/internal/usecase/container/service.go b/internal/usecase/container/service.go index 881a4fa65..9cf000e14 100644 --- a/internal/usecase/container/service.go +++ b/internal/usecase/container/service.go @@ -353,14 +353,22 @@ func (s *Service) Deploy(ctx context.Context, route domain.Route) (*domain.Conta } } - newContainer, err := s.createStartedContainer(ctx, route, existing, resources) + // The container this deploy replaces. The exited owner is treated as the + // predecessor, so the replacement runs under a temporary name and the owner + // survives until the switch completes. + previous := previousContainerForDeploy(existing, resources.retainedOwner) + + newContainer, err := s.createStartedContainer(ctx, route, previous, resources) if err != nil { + // The retained owner is still there: a retry snapshots its mounts again. return nil, err } invalidated := s.activateDeployedContainer(ctx, route.Domain, newContainer) - // Post-switch stabilization: verify new container stays running + // Post-switch stabilization: verify new container stays running. Only a + // running predecessor can be rolled back to, so a retained exited owner is + // not stabilized against. if hasExisting { stable, stabilizeErr := s.stabilizeNewContainer(ctx, route.Domain, newContainer, existing) if stabilizeErr != nil { @@ -375,6 +383,10 @@ func (s *Service) Deploy(ctx context.Context, route domain.Route) (*domain.Conta } } + if !mayFinalizeRetainedOwner(ctx, resources, newContainer) { + previous = nil + } + // Finalize old container in the background — the new container is already // serving traffic, so there's no reason to block the deploy response while // waiting for the old container to stop (which can take 20s if the app @@ -382,7 +394,7 @@ func (s *Service) Deploy(ctx context.Context, route domain.Route) (*domain.Conta s.cleanupWg.Add(1) go func() { defer s.cleanupWg.Done() - s.finalizePreviousContainer(context.WithoutCancel(ctx), route.Domain, existing, hasExisting, invalidated, newContainer.ID) + s.finalizePreviousContainer(context.WithoutCancel(ctx), route.Domain, previous, previous != nil, invalidated, newContainer.ID) }() // Start container log collection (non-blocking, errors don't fail deployment) @@ -495,6 +507,10 @@ type deployResources struct { envVars []string envHash string volumes map[string]string + // retainedOwner is the route's exited volume owner. It holds the only proof + // that the preserved volumes belong to this route, so Deploy replaces it + // under a temporary name and removes it only once the replacement runs. + retainedOwner *domain.Container } // resolveExistingContainer returns the currently running container for a domain. @@ -561,6 +577,56 @@ func (s *Service) resolveExistingContainer(ctx context.Context, domainName strin return nil, false } +// previousContainerForDeploy returns the container a deploy replaces: the tracked +// one when there is one, otherwise the exited owner retained as ownership +// evidence. Naming the replacement after it leaves that owner on disk until the +// switch completes. +func previousContainerForDeploy(existing, retainedOwner *domain.Container) *domain.Container { + if existing != nil { + return existing + } + return retainedOwner +} + +// mayFinalizeRetainedOwner reports whether the deploy may remove the exited owner +// it retained as ownership evidence. The owner is kept unless the replacement +// mounts every volume it preserved: it is the last container proving which +// volumes belong to the route, so dropping it while the data is unaccounted for +// would leave the route unable to resolve its own volumes. +func mayFinalizeRetainedOwner(ctx context.Context, resources *deployResources, replacement *domain.Container) bool { + if resources.retainedOwner == nil || containerOwnsEveryVolume(replacement, resources.volumes) { + return true + } + + log := zerowrap.FromCtx(ctx) + log.Warn(). + Str(zerowrap.FieldEntityID, resources.retainedOwner.ID). + Msg("replacement container does not mount every preserved volume; keeping the exited owner as ownership evidence") + + return false +} + +// deployVolumePreference returns the volumes a deploy should reuse and the exited +// owner to retain as ownership evidence for them. An existing container is +// authoritative on its own, and a failed listing has no snapshot to offer. +func deployVolumePreference(existing *domain.Container, allContainers []*domain.Container, listed bool, domainName string) (volumePreference, *domain.Container) { + if existing != nil || !listed { + return volumePreference{mounts: namedVolumeMounts(existing)}, nil + } + + owner := snapshotRouteVolumeOwner(allContainers, domainName) + mounts := namedVolumeMounts(owner) + if len(mounts) == 0 { + return volumePreference{}, nil + } + + // The owner is kept as ownership evidence until the replacement mounts + // these volumes: while it is the only container mounting them, deleting it + // first would leave a failed deploy without any way to prove that a legacy + // volume belongs to this route. + return volumePreference{mounts: mounts, fromExitedContainer: true}, owner +} + func (s *Service) prepareDeployResources(ctx context.Context, route domain.Route, existing *domain.Container) (*deployResources, error) { log := zerowrap.FromCtx(ctx) @@ -583,13 +649,7 @@ func (s *Service) prepareDeployResources(ctx context.Context, route domain.Route // the operator sees as a failed deploy and can retry. log.WrapErr(listErr, "failed to list containers for deploy; skipping volume snapshot and orphan cleanup") } - preferredVolumes := volumePreference{mounts: namedVolumeMounts(existing)} - if existing == nil && listErr == nil { - preferredVolumes = volumePreference{ - mounts: snapshotRouteVolumeMounts(allContainers, route.Domain), - fromExitedContainer: true, - } - } + preferredVolumes, retainedOwner := deployVolumePreference(existing, allContainers, listErr == nil, route.Domain) imageRef, err := s.buildValidatedImageRef(ctx, route.Image) if err != nil { @@ -631,13 +691,16 @@ func (s *Service) prepareDeployResources(ctx context.Context, route domain.Route return nil, err } - // Cleanup runs last because it deletes the exited owner whose mounts were - // snapshotted above: every step that may still need that evidence (image - // resolution, legacy volume ownership probes) must have completed. The new - // container is created only after this function returns, so the names are - // free by then. + // Cleanup runs last because it removes route containers whose mounts were + // needed above: every step that may still need that evidence (image + // resolution, legacy volume ownership probes) must have completed. The + // retained owner is not removed here, only after the replacement runs. if listErr == nil { - if err := s.cleanupOrphanedContainers(ctx, route.Domain, existingID, allContainers); err != nil { + retainedID := "" + if retainedOwner != nil { + retainedID = retainedOwner.ID + } + if err := s.cleanupOrphanedContainers(ctx, route.Domain, existingID, retainedID, allContainers); err != nil { log.WrapErr(err, "failed to cleanup orphaned containers") } } @@ -650,6 +713,7 @@ func (s *Service) prepareDeployResources(ctx context.Context, route domain.Route envVars: envVars, envHash: envHash, volumes: volumes, + retainedOwner: retainedOwner, }, nil } @@ -2717,21 +2781,21 @@ func (s *Service) createNetworkIfNeeded(ctx context.Context, networkName string) return nil } -// snapshotRouteVolumeMounts returns the named volume mounts of the route's -// exited containers from an already-fetched container listing. Canonical, -new -// and -next are all valid names left behind by an interrupted deploy, checked -// in that order so the canonical container wins when several exist. Only -// containers whose labels prove they belong to this route contribute: matching -// on the name alone would let a foreign workload squatting a gordon name inject -// its volumes into this route. -func snapshotRouteVolumeMounts(allContainers []*domain.Container, domainName string) map[string]namedVolumeMount { +// snapshotRouteVolumeOwner returns the route's leftover volume owner from an +// already-fetched container listing. Canonical, -new and -next are all valid +// names left behind by an interrupted deploy, checked in that order so the +// canonical container wins when several exist. Only containers whose labels +// prove they belong to this route are returned: matching on the name alone +// would let a foreign workload squatting a gordon name inject its volumes into +// this route. +func snapshotRouteVolumeOwner(allContainers []*domain.Container, domainName string) *domain.Container { canonicalName := managedContainerName(domainName) for _, name := range []string{canonicalName, canonicalName + "-new", canonicalName + "-next"} { for _, c := range allContainers { if c == nil || c.Name != name || !hasRouteOwnershipLabels(c, domainName) { continue } - return namedVolumeMounts(c) + return c } } return nil @@ -2739,7 +2803,7 @@ func snapshotRouteVolumeMounts(allContainers []*domain.Container, domainName str // hasRouteOwnershipLabels reports whether the container labels prove the // container belongs to the route. isManagedRouteContainerForDomain cannot answer -// that for the names matched by snapshotRouteVolumeMounts, because its name +// that for the names matched by snapshotRouteVolumeOwner, because its name // clause holds by definition once the name matches. Only leftover containers // reach this check: a running container holding the route's canonical name is // already resolved as the previous deploy by resolveExistingContainer. @@ -2756,15 +2820,17 @@ func hasRouteOwnershipLabels(c *domain.Container, domainName string) bool { // cleanupOrphanedContainers removes the route's canonical, -new and -next // containers left behind by a previous or interrupted deploy. allContainers is // the listing the caller already fetched: cleanup must decide on the same -// snapshot the caller used, so it never re-lists on its own. -func (s *Service) cleanupOrphanedContainers(ctx context.Context, domainName string, skipContainerID string, allContainers []*domain.Container) error { +// snapshot the caller used, so it never re-lists on its own. skipContainerID is +// the container the deploy is replacing; keepContainerID, when set, is an exited +// owner retained as ownership evidence, and is never removed here. +func (s *Service) cleanupOrphanedContainers(ctx context.Context, domainName, skipContainerID, keepContainerID string, allContainers []*domain.Container) error { log := zerowrap.FromCtx(ctx) expectedName := managedContainerName(domainName) expectedNewName := expectedName + "-new" expectedNextName := expectedName + "-next" for _, c := range allContainers { - if c == nil || c.ID == skipContainerID { + if c == nil || c.ID == skipContainerID || c.ID == keepContainerID { continue } if c.Name != expectedName && c.Name != expectedNewName && c.Name != expectedNextName { @@ -3438,6 +3504,24 @@ func containerMountsVolume(container *domain.Container, volumeName string) bool return false } +// containerOwnsEveryVolume reports whether the container mounts every volume in +// volumes. Callers use it to confirm a replacement took over preserved data +// before they drop the container that proved where that data lives. +func containerOwnsEveryVolume(container *domain.Container, volumes map[string]string) bool { + if len(volumes) == 0 { + return true + } + if container == nil { + return false + } + for _, name := range volumes { + if !containerMountsVolume(container, name) { + return false + } + } + return true +} + func mergeEnvironmentVariables(dockerfileEnv, userEnv []string) []string { envMap := make(map[string]string) diff --git a/internal/usecase/container/service_test.go b/internal/usecase/container/service_test.go index e4efde171..130e08d6f 100644 --- a/internal/usecase/container/service_test.go +++ b/internal/usecase/container/service_test.go @@ -1908,16 +1908,11 @@ func TestService_PrepareDeployResources_ReusesExitedOwnerVolumes(t *testing.T) { }, } - // One listing serves both the mount snapshot and the cleanup pass. Cleanup - // runs only after volume resolution finished, so the exited owner is still - // listed while the snapshot is validated; without the snapshot the owner is - // removed first and the legacy ownership probe finds no owner, failing the - // deploy closed with ErrVolumeOwnershipUnverified. + // One listing serves both the mount snapshot and the cleanup pass. The owner + // is not removed here: the snapshot stays the last proof of which volume + // holds the data until the replacement container mounts it. runtime.EXPECT().ListContainers(mock.Anything, true). Return([]*domain.Container{exitedOwner}, nil).Once() - runtime.EXPECT().InspectContainer(mock.Anything, "exited-owner").Return(exitedOwner, nil).Once() - runtime.EXPECT().StopContainer(mock.Anything, "exited-owner").Return(nil).Once() - runtime.EXPECT().RemoveContainer(mock.Anything, "exited-owner", true).Return(nil).Once() runtime.EXPECT().ListImages(mock.Anything).Return([]string{"myapp:latest"}, nil) runtime.EXPECT().GetImageExposedPorts(mock.Anything, "myapp:latest").Return([]int{8080}, nil) @@ -1937,11 +1932,14 @@ func TestService_PrepareDeployResources_ReusesExitedOwnerVolumes(t *testing.T) { require.NoError(t, err) assert.Equal(t, map[string]string{"/data": legacyName}, resources.volumes) + assert.Same(t, exitedOwner, resources.retainedOwner) runtime.AssertNotCalled(t, "VolumeExists", mock.Anything, stableName) runtime.AssertNotCalled(t, "CreateVolume", mock.Anything, mock.Anything) + runtime.AssertNotCalled(t, "StopContainer", mock.Anything, "exited-owner") + runtime.AssertNotCalled(t, "RemoveContainer", mock.Anything, "exited-owner", true) } -func TestService_PrepareDeployResources_ResolvesOwnershipBeforeRemovingOwner(t *testing.T) { +func TestService_PrepareDeployResources_KeepsExitedOwnerForLegacyOwnershipProbe(t *testing.T) { runtime := mocks.NewMockContainerRuntime(t) envLoader := mocks.NewMockEnvLoader(t) svc := NewService(runtime, envLoader, nil, nil, Config{ @@ -1969,21 +1967,11 @@ func TestService_PrepareDeployResources_ResolvesOwnershipBeforeRemovingOwner(t * }, } - // The listing reflects removals like Docker does: once orphan cleanup - // deletes the owner, the ownership probe can no longer see it. Removing the - // owner before volume resolution would therefore fail the deploy closed with - // ErrVolumeOwnershipUnverified. - listed := []*domain.Container{exitedOwner} - runtime.EXPECT().ListContainers(mock.Anything, true).RunAndReturn( - func(context.Context, bool) ([]*domain.Container, error) { - return listed, nil - }, - ).Times(2) - runtime.EXPECT().InspectContainer(mock.Anything, "exited-owner").Return(exitedOwner, nil).Once() - runtime.EXPECT().StopContainer(mock.Anything, "exited-owner").Return(nil).Once() - runtime.EXPECT().RemoveContainer(mock.Anything, "exited-owner", true).Run( - func(context.Context, string, bool) { listed = nil }, - ).Return(nil).Once() + // The owner outlives the preparation pass, so the ownership probe that runs + // while resolving the image's own volume path still sees it. Removing the + // owner first would fail the deploy closed with ErrVolumeOwnershipUnverified. + runtime.EXPECT().ListContainers(mock.Anything, true). + Return([]*domain.Container{exitedOwner}, nil).Times(2) runtime.EXPECT().ListImages(mock.Anything).Return([]string{"myapp:latest"}, nil) runtime.EXPECT().GetImageExposedPorts(mock.Anything, "myapp:latest").Return([]int{8080}, nil) @@ -2003,7 +1991,10 @@ func TestService_PrepareDeployResources_ResolvesOwnershipBeforeRemovingOwner(t * require.NoError(t, err) assert.Equal(t, map[string]string{"/data": legacyName, "/var/lib/legacy": legacyName}, resources.volumes) + assert.Same(t, exitedOwner, resources.retainedOwner) runtime.AssertNotCalled(t, "VolumeExists", mock.Anything, stableName) + runtime.AssertNotCalled(t, "StopContainer", mock.Anything, "exited-owner") + runtime.AssertNotCalled(t, "RemoveContainer", mock.Anything, "exited-owner", true) } func TestService_PrepareDeployResources_PrefersExistingContainerMounts(t *testing.T) { @@ -2084,9 +2075,6 @@ func TestService_PrepareDeployResources_ReusesExitedTempNameOwnerVolumes(t *test runtime.EXPECT().ListContainers(mock.Anything, true). Return([]*domain.Container{exitedTempOwner}, nil).Once() - runtime.EXPECT().InspectContainer(mock.Anything, "exited-new-owner").Return(exitedTempOwner, nil).Once() - runtime.EXPECT().StopContainer(mock.Anything, "exited-new-owner").Return(nil).Once() - runtime.EXPECT().RemoveContainer(mock.Anything, "exited-new-owner", true).Return(nil).Once() runtime.EXPECT().ListImages(mock.Anything).Return([]string{"myapp:latest"}, nil) runtime.EXPECT().GetImageExposedPorts(mock.Anything, "myapp:latest").Return([]int{8080}, nil) @@ -2104,8 +2092,10 @@ func TestService_PrepareDeployResources_ReusesExitedTempNameOwnerVolumes(t *test require.NoError(t, err) assert.Equal(t, map[string]string{"/data": legacyName}, resources.volumes) + assert.Same(t, exitedTempOwner, resources.retainedOwner) runtime.AssertNotCalled(t, "VolumeExists", mock.Anything, stableName) runtime.AssertNotCalled(t, "CreateVolume", mock.Anything, mock.Anything) + runtime.AssertNotCalled(t, "RemoveContainer", mock.Anything, "exited-new-owner", true) } func TestService_PrepareDeployResources_IgnoresForeignContainerOnCanonicalName(t *testing.T) { @@ -2143,7 +2133,6 @@ func TestService_PrepareDeployResources_IgnoresForeignContainerOnCanonicalName(t runtime.EXPECT().InspectContainer(mock.Anything, "foreign-container").Return(foreign, nil).Once() runtime.EXPECT().StopContainer(mock.Anything, "foreign-container").Return(nil).Once() runtime.EXPECT().RemoveContainer(mock.Anything, "foreign-container", true).Return(nil).Once() - runtime.EXPECT().ListImages(mock.Anything).Return([]string{"myapp:latest"}, nil) runtime.EXPECT().GetImageExposedPorts(mock.Anything, "myapp:latest").Return([]int{8080}, nil) runtime.EXPECT().GetImageLabels(mock.Anything, "myapp:latest").Return(nil, nil) @@ -2164,6 +2153,7 @@ func TestService_PrepareDeployResources_IgnoresForeignContainerOnCanonicalName(t require.NoError(t, err) assert.Equal(t, map[string]string{"/data": stableName}, resources.volumes) + assert.Nil(t, resources.retainedOwner) runtime.AssertNotCalled(t, "VolumeExists", mock.Anything, foreignVolume) } @@ -2197,9 +2187,6 @@ func TestService_PrepareDeployResources_IgnoresDeletedSnapshottedVolume(t *testi runtime.EXPECT().ListContainers(mock.Anything, true). Return([]*domain.Container{exitedOwner}, nil).Once() - runtime.EXPECT().InspectContainer(mock.Anything, "exited-owner").Return(exitedOwner, nil).Once() - runtime.EXPECT().StopContainer(mock.Anything, "exited-owner").Return(nil).Once() - runtime.EXPECT().RemoveContainer(mock.Anything, "exited-owner", true).Return(nil).Once() runtime.EXPECT().ListImages(mock.Anything).Return([]string{"myapp:latest"}, nil) runtime.EXPECT().GetImageExposedPorts(mock.Anything, "myapp:latest").Return([]int{8080}, nil) @@ -2220,6 +2207,8 @@ func TestService_PrepareDeployResources_IgnoresDeletedSnapshottedVolume(t *testi require.NoError(t, err) assert.Equal(t, map[string]string{"/data": stableName}, resources.volumes) + assert.Same(t, exitedOwner, resources.retainedOwner) + runtime.AssertNotCalled(t, "RemoveContainer", mock.Anything, "exited-owner", true) } func TestService_PrepareDeployResources_SkipsCleanupWhenListingFails(t *testing.T) { @@ -2281,6 +2270,227 @@ func TestService_PrepareDeployResources_SkipsCleanupWhenListingFails(t *testing. runtime.AssertNotCalled(t, "RemoveContainer", mock.Anything, "exited-owner", true) } +// exitedOwnerDeployFixture wires a Deploy for a route whose only container is +// an exited volume owner left by a reboot: resolveExistingContainer cannot see +// it, so the deploy knows it only through the container listing. +type exitedOwnerDeployFixture struct { + svc *Service + runtime *mocks.MockContainerRuntime + eventBus *mocks.MockEventPublisher + route domain.Route + owner *domain.Container + legacyName string +} + +func newExitedOwnerDeployFixture(t *testing.T, ownerName string) *exitedOwnerDeployFixture { + t.Helper() + + runtime := mocks.NewMockContainerRuntime(t) + envLoader := mocks.NewMockEnvLoader(t) + eventBus := mocks.NewMockEventPublisher(t) + svc := NewService(runtime, envLoader, eventBus, nil, Config{ + AllowedRegistries: []string{"docker.io"}, + VolumeAutoCreate: true, + VolumePrefix: "gordon", + ReadinessDelay: time.Millisecond, + StabilizationDelay: time.Millisecond, + }, nil) + + legacyName := legacyVolumeName("gordon", "app.example.com", "/data") + owner := &domain.Container{ + ID: "exited-owner", + Name: ownerName, + Status: "exited", + Labels: map[string]string{ + domain.LabelManaged: "true", + domain.LabelRoute: "app.example.com", + domain.LabelDomain: "app.example.com", + }, + VolumeMounts: []domain.ContainerVolumeMount{ + {Name: legacyName, Type: "volume", Destination: "/data"}, + }, + } + + runtime.EXPECT().ListContainers(mock.Anything, false).Return([]*domain.Container{}, nil).Once() + runtime.EXPECT().ListContainers(mock.Anything, true). + Return([]*domain.Container{owner}, nil).Once() + + runtime.EXPECT().ListImages(mock.Anything).Return([]string{"myapp:latest"}, nil) + runtime.EXPECT().GetImageExposedPorts(mock.Anything, "myapp:latest").Return([]int{8080}, nil) + runtime.EXPECT().GetImageLabels(mock.Anything, "myapp:latest").Return(nil, nil) + envLoader.EXPECT().LoadEnv(mock.Anything, "app.example.com").Return([]string{}, nil) + runtime.EXPECT().InspectImageEnv(mock.Anything, "myapp:latest").Return([]string{}, nil) + + // The snapshot resolves the image path to the volume the owner mounts, so + // resolution only has to confirm that volume still exists. + runtime.EXPECT().VolumeExists(mock.Anything, legacyName).Return(true, nil).Once() + runtime.EXPECT().InspectImageVolumes(mock.Anything, "myapp:latest").Return([]string{"/data"}, nil).Once() + + return &exitedOwnerDeployFixture{ + svc: svc, + runtime: runtime, + eventBus: eventBus, + route: domain.Route{Domain: "app.example.com", Image: "myapp:latest"}, + owner: owner, + legacyName: legacyName, + } +} + +func TestService_Deploy_KeepsExitedOwnerUntilReplacementRuns(t *testing.T) { + f := newExitedOwnerDeployFixture(t, "gordon-app.example.com") + + // The exited owner still holds the canonical name, so the replacement runs + // under a temporary name and takes over the preserved volume. + newContainer := &domain.Container{ID: "new-container", Name: "gordon-app.example.com-new", Status: "created"} + f.runtime.EXPECT().CreateContainer(mock.Anything, mock.MatchedBy(func(cfg *domain.ContainerConfig) bool { + return cfg.Name == "gordon-app.example.com-new" && cfg.Volumes["/data"] == f.legacyName + })).Return(newContainer, nil) + f.runtime.EXPECT().StartContainer(mock.Anything, "new-container").Return(nil) + f.runtime.EXPECT().IsContainerRunning(mock.Anything, "new-container").Return(true, nil).Times(2) + inspectCall := f.runtime.EXPECT().InspectContainer(mock.Anything, "new-container").Return(&domain.Container{ + ID: "new-container", + Name: "gordon-app.example.com-new", + Status: "running", + Ports: []int{8080}, + VolumeMounts: []domain.ContainerVolumeMount{{Name: f.legacyName, Type: "volume", Destination: "/data"}}, + }, nil) + f.eventBus.EXPECT().Publish(domain.EventContainerDeployed, mock.AnythingOfType("*domain.ContainerEventPayload")).Return(nil) + + // The owner is dropped and the replacement promoted only once that + // replacement was created, started, ready and inspected with the volume. + f.runtime.EXPECT().StopContainer(mock.Anything, "exited-owner").Return(nil).NotBefore(inspectCall.Call) + f.runtime.EXPECT().RemoveContainer(mock.Anything, "exited-owner", true).Return(nil).NotBefore(inspectCall.Call) + f.runtime.EXPECT().RenameContainer(mock.Anything, "new-container", "gordon-app.example.com").Return(nil).NotBefore(inspectCall.Call) + + result, err := f.svc.Deploy(testContext(), f.route) + + require.NoError(t, err) + require.NotNil(t, result) + assert.Equal(t, "new-container", result.ID) + f.svc.WaitForCleanup() +} + +func TestService_Deploy_KeepsExitedOwnerUntilReplacementRuns_AfterTempNameOwner(t *testing.T) { + f := newExitedOwnerDeployFixture(t, "gordon-app.example.com-new") + + // The owner already holds a temporary name, so the replacement must take the + // other one instead of colliding with it. + newContainer := &domain.Container{ID: "new-container", Name: "gordon-app.example.com-next", Status: "created"} + f.runtime.EXPECT().CreateContainer(mock.Anything, mock.MatchedBy(func(cfg *domain.ContainerConfig) bool { + return cfg.Name == "gordon-app.example.com-next" + })).Return(newContainer, nil) + f.runtime.EXPECT().StartContainer(mock.Anything, "new-container").Return(nil) + f.runtime.EXPECT().IsContainerRunning(mock.Anything, "new-container").Return(true, nil).Times(2) + inspectCall := f.runtime.EXPECT().InspectContainer(mock.Anything, "new-container").Return(&domain.Container{ + ID: "new-container", + Name: "gordon-app.example.com-next", + Status: "running", + Ports: []int{8080}, + VolumeMounts: []domain.ContainerVolumeMount{{Name: f.legacyName, Type: "volume", Destination: "/data"}}, + }, nil) + f.eventBus.EXPECT().Publish(domain.EventContainerDeployed, mock.AnythingOfType("*domain.ContainerEventPayload")).Return(nil) + + f.runtime.EXPECT().StopContainer(mock.Anything, "exited-owner").Return(nil).NotBefore(inspectCall.Call) + f.runtime.EXPECT().RemoveContainer(mock.Anything, "exited-owner", true).Return(nil).NotBefore(inspectCall.Call) + f.runtime.EXPECT().RenameContainer(mock.Anything, "new-container", "gordon-app.example.com").Return(nil).NotBefore(inspectCall.Call) + + result, err := f.svc.Deploy(testContext(), f.route) + + require.NoError(t, err) + require.NotNil(t, result) + assert.Equal(t, "new-container", result.ID) + f.svc.WaitForCleanup() +} + +func TestService_Deploy_KeepsExitedOwnerWhenReplacementCreationFails(t *testing.T) { + f := newExitedOwnerDeployFixture(t, "gordon-app.example.com") + + f.runtime.EXPECT().CreateContainer(mock.Anything, mock.AnythingOfType("*domain.ContainerConfig")). + Return(nil, errors.New("create failed")) + + result, err := f.svc.Deploy(testContext(), f.route) + + require.Error(t, err) + assert.Nil(t, result) + + // The owner survives the failed deploy, so a retry snapshots its mounts + // instead of losing the only proof of where the volumes live. + f.runtime.AssertNotCalled(t, "StopContainer", mock.Anything, "exited-owner") + f.runtime.AssertNotCalled(t, "RemoveContainer", mock.Anything, "exited-owner", true) +} + +func TestService_Deploy_KeepsExitedOwnerWhenReplacementFailsToStart(t *testing.T) { + f := newExitedOwnerDeployFixture(t, "gordon-app.example.com") + + newContainer := &domain.Container{ID: "new-container", Name: "gordon-app.example.com-new", Status: "created"} + f.runtime.EXPECT().CreateContainer(mock.Anything, mock.AnythingOfType("*domain.ContainerConfig")).Return(newContainer, nil) + f.runtime.EXPECT().StartContainer(mock.Anything, "new-container").Return(errors.New("start failed")) + // The failed candidate is cleaned up; the exited owner is not. + f.runtime.EXPECT().RemoveContainer(mock.Anything, "new-container", true).Return(nil) + + result, err := f.svc.Deploy(testContext(), f.route) + + require.Error(t, err) + assert.Nil(t, result) + f.runtime.AssertNotCalled(t, "StopContainer", mock.Anything, "exited-owner") + f.runtime.AssertNotCalled(t, "RemoveContainer", mock.Anything, "exited-owner", true) +} + +func TestService_Deploy_KeepsExitedOwnerWhenReplacementFailsReadiness(t *testing.T) { + f := newExitedOwnerDeployFixture(t, "gordon-app.example.com") + + newContainer := &domain.Container{ID: "new-container", Name: "gordon-app.example.com-new", Status: "created"} + f.runtime.EXPECT().CreateContainer(mock.Anything, mock.AnythingOfType("*domain.ContainerConfig")).Return(newContainer, nil) + f.runtime.EXPECT().StartContainer(mock.Anything, "new-container").Return(nil) + // Fail readiness through a missing healthcheck: the delay mode would wait + // out its recovery window before giving up. + f.svc.mu.Lock() + f.svc.config.ReadinessMode = "docker-health" + f.svc.mu.Unlock() + f.runtime.EXPECT().IsContainerRunning(mock.Anything, "new-container").Return(true, nil).Once() + f.runtime.EXPECT().GetContainerHealthStatus(mock.Anything, "new-container").Return("", false, nil).Once() + f.runtime.EXPECT().GetContainerLogs(mock.Anything, "new-container", false).Return(dockerLogFrames(1, "boom\n"), nil) + f.runtime.EXPECT().StopContainer(mock.Anything, "new-container").Return(nil) + f.runtime.EXPECT().RemoveContainer(mock.Anything, "new-container", true).Return(nil) + + result, err := f.svc.Deploy(testContext(), f.route) + + var deployErr *domain.DeployFailureError + require.ErrorAs(t, err, &deployErr) + assert.Nil(t, result) + f.runtime.AssertNotCalled(t, "StopContainer", mock.Anything, "exited-owner") + f.runtime.AssertNotCalled(t, "RemoveContainer", mock.Anything, "exited-owner", true) +} + +func TestService_Deploy_KeepsExitedOwnerWhenReplacementMissesVolume(t *testing.T) { + f := newExitedOwnerDeployFixture(t, "gordon-app.example.com") + + newContainer := &domain.Container{ID: "new-container", Name: "gordon-app.example.com-new", Status: "created"} + f.runtime.EXPECT().CreateContainer(mock.Anything, mock.AnythingOfType("*domain.ContainerConfig")).Return(newContainer, nil) + f.runtime.EXPECT().StartContainer(mock.Anything, "new-container").Return(nil) + f.runtime.EXPECT().IsContainerRunning(mock.Anything, "new-container").Return(true, nil).Times(2) + // The inspected replacement mounts nothing, so it never took over the data. + f.runtime.EXPECT().InspectContainer(mock.Anything, "new-container").Return(&domain.Container{ + ID: "new-container", + Name: "gordon-app.example.com-new", + Status: "running", + Ports: []int{8080}, + }, nil) + f.eventBus.EXPECT().Publish(domain.EventContainerDeployed, mock.AnythingOfType("*domain.ContainerEventPayload")).Return(nil) + + result, err := f.svc.Deploy(testContext(), f.route) + + require.NoError(t, err) + require.NotNil(t, result) + f.svc.WaitForCleanup() + + // Lifeless ownership evidence beats none: the owner is kept, and the + // replacement keeps its temporary name because the canonical one is taken. + f.runtime.AssertNotCalled(t, "StopContainer", mock.Anything, "exited-owner") + f.runtime.AssertNotCalled(t, "RemoveContainer", mock.Anything, "exited-owner", true) + f.runtime.AssertNotCalled(t, "RenameContainer", mock.Anything, "new-container", "gordon-app.example.com") +} + func TestService_SetupVolumes_RejectsLegacyVolumeSharedWithForeignWorkload(t *testing.T) { runtime := mocks.NewMockContainerRuntime(t) svc := NewService(runtime, nil, nil, nil, Config{VolumeAutoCreate: true, VolumePrefix: "gordon"}, nil) @@ -4179,7 +4389,7 @@ func TestService_CleanupOrphanedContainers_SkipsRestartingContainer(t *testing.T svc := NewService(runtime, envLoader, eventBus, nil, Config{}, nil) ctx := testContext() - err := svc.cleanupOrphanedContainers(ctx, "test.example.com", "", []*domain.Container{ + err := svc.cleanupOrphanedContainers(ctx, "test.example.com", "", "", []*domain.Container{ { ID: "restarting-container", Name: "gordon-test.example.com", @@ -4202,7 +4412,7 @@ func TestService_CleanupOrphanedContainers_SkipsContainerStartedSinceListing(t * Status: "running", }, nil).Once() - err := svc.cleanupOrphanedContainers(testContext(), "test.example.com", "", []*domain.Container{ + err := svc.cleanupOrphanedContainers(testContext(), "test.example.com", "", "", []*domain.Container{ { ID: "revived-container", Name: "gordon-test.example.com", @@ -4223,7 +4433,7 @@ func TestService_CleanupOrphanedContainers_SkipsRemovalWhenStatusUnreadable(t *t // is recoverable, killing a live container is not. runtime.EXPECT().InspectContainer(mock.Anything, "unknown-container").Return(nil, errors.New("inspect failed")).Once() - err := svc.cleanupOrphanedContainers(testContext(), "test.example.com", "", []*domain.Container{ + err := svc.cleanupOrphanedContainers(testContext(), "test.example.com", "", "", []*domain.Container{ { ID: "unknown-container", Name: "gordon-test.example.com", From be68285e3b6536d1dbbc588693483d3b7ae158e7 Mon Sep 17 00:00:00 2001 From: brice Date: Tue, 15 Sep 2026 21:00:48 +0200 Subject: [PATCH 4/5] fix(deploy): reuse mounts only the exited owner carries A deploy interrupted before its traffic switch can leave the canonical container exited while the temporary replacement keeps running, and resolveExistingContainer then selects the temporary one. Its mounts were the only preference the deploy reused, and cleanup removed the exited owner right after, so a mount that only that container carried was dropped: the path lost its data volume and the last container proving where that volume lived was gone. Mounts of the route's exited containers are now merged into the preference set by destination, with a container that is still there staying authoritative, and the merged paths are recorded per path. Missing-volume validation is relaxed only for paths that came from the exited owner, since a volume deleted while its container was stopped holds no data to preserve; a mount of a container that is still there still fails closed with ErrVolumeNotFound. Refs #278 --- internal/usecase/container/service.go | 85 ++++++++-- internal/usecase/container/service_test.go | 182 +++++++++++++++++++++ 2 files changed, 257 insertions(+), 10 deletions(-) diff --git a/internal/usecase/container/service.go b/internal/usecase/container/service.go index 9cf000e14..64dd4056b 100644 --- a/internal/usecase/container/service.go +++ b/internal/usecase/container/service.go @@ -607,11 +607,24 @@ func mayFinalizeRetainedOwner(ctx context.Context, resources *deployResources, r } // deployVolumePreference returns the volumes a deploy should reuse and the exited -// owner to retain as ownership evidence for them. An existing container is -// authoritative on its own, and a failed listing has no snapshot to offer. +// owner to retain as ownership evidence for them. A container that is still there +// is authoritative on its own mounts, and a failed listing offers no snapshot. func deployVolumePreference(existing *domain.Container, allContainers []*domain.Container, listed bool, domainName string) (volumePreference, *domain.Container) { - if existing != nil || !listed { - return volumePreference{mounts: namedVolumeMounts(existing)}, nil + if existing != nil { + mounts := namedVolumeMounts(existing) + if !listed { + return volumePreference{mounts: mounts}, nil + } + + // An interrupted deploy can leave a temporary container running while the + // canonical one is exited, and resolveExistingContainer then selects the + // temporary one. The mounts only the exited container carries are the last + // record of where that data lives, so the replacement has to take them + // over before cleanup removes their only owner. + return mergeExitedOwnerMounts(mounts, exitedOwnerMounts(allContainers, domainName, existing.ID)), nil + } + if !listed { + return volumePreference{}, nil } owner := snapshotRouteVolumeOwner(allContainers, domainName) @@ -624,7 +637,58 @@ func deployVolumePreference(existing *domain.Container, allContainers []*domain. // these volumes: while it is the only container mounting them, deleting it // first would leave a failed deploy without any way to prove that a legacy // volume belongs to this route. - return volumePreference{mounts: mounts, fromExitedContainer: true}, owner + fromExitedOwner := make(map[string]bool, len(mounts)) + for path := range mounts { + fromExitedOwner[path] = true + } + + return volumePreference{mounts: mounts, fromExitedOwner: fromExitedOwner}, owner +} + +// exitedOwnerMounts returns the named volume mounts of the route's exited +// containers from an already-fetched listing, canonical first so the container +// that outlived the others wins a destination. skipContainerID is the container +// the deploy replaces, which contributes its own mounts separately. +func exitedOwnerMounts(allContainers []*domain.Container, domainName, skipContainerID string) map[string]namedVolumeMount { + mounts := make(map[string]namedVolumeMount) + canonicalName := managedContainerName(domainName) + for _, name := range []string{canonicalName, canonicalName + "-new", canonicalName + "-next"} { + for _, c := range allContainers { + if c == nil || c.ID == skipContainerID || c.Name != name || !hasRouteOwnershipLabels(c, domainName) { + continue + } + if c.Status == "running" || c.Status == "restarting" { + continue + } + for path, mount := range namedVolumeMounts(c) { + if _, taken := mounts[path]; !taken { + mounts[path] = mount + } + } + } + } + return mounts +} + +// mergeExitedOwnerMounts fills the destinations only the exited owner has, and +// records them in fromExitedOwner. A container that is still there stays +// authoritative for the destinations it already mounts. +func mergeExitedOwnerMounts(mounts, exitedOwner map[string]namedVolumeMount) volumePreference { + merged := make(map[string]namedVolumeMount, len(mounts)+len(exitedOwner)) + for path, mount := range mounts { + merged[path] = mount + } + + fromExitedOwner := make(map[string]bool, len(exitedOwner)) + for path, mount := range exitedOwner { + if _, taken := merged[path]; taken { + continue + } + merged[path] = mount + fromExitedOwner[path] = true + } + + return volumePreference{mounts: merged, fromExitedOwner: fromExitedOwner} } func (s *Service) prepareDeployResources(ctx context.Context, route domain.Route, existing *domain.Container) (*deployResources, error) { @@ -2610,12 +2674,13 @@ func (s *Service) loadEnvironment(ctx context.Context, preResolved []string, dom // volumePreference is the set of named-volume mounts a deploy wants to reuse. type volumePreference struct { mounts map[string]namedVolumeMount - // fromExitedContainer marks mounts snapshotted from an exited container. + // fromExitedOwner marks the mount paths that come from an exited container. // Those mounts are evidence of where the data lives, not a promise that it // still does: a volume deleted while its container was stopped holds no data - // to preserve, so such a path falls back to normal volume resolution - // instead of failing the deploy with domain.ErrVolumeNotFound. - fromExitedContainer bool + // to preserve, so such a path falls back to normal volume resolution instead + // of failing the deploy with domain.ErrVolumeNotFound. Mounts of a container + // that is still there stay authoritative and fail closed. + fromExitedOwner map[string]bool } func (s *Service) setupVolumes(ctx context.Context, domainName, imageRef string, preferred volumePreference) (map[string]string, error) { @@ -2699,7 +2764,7 @@ func (s *Service) validatePreferredVolumes(ctx context.Context, preferred volume return nil, fmt.Errorf("check previously mounted volume %q: %w", mount.Name, err) } if !exists { - if preferred.fromExitedContainer { + if preferred.fromExitedOwner[path] { log.Info(). Str("volume", mount.Name). Str(zerowrap.FieldPath, path). diff --git a/internal/usecase/container/service_test.go b/internal/usecase/container/service_test.go index 130e08d6f..fd253b555 100644 --- a/internal/usecase/container/service_test.go +++ b/internal/usecase/container/service_test.go @@ -2270,6 +2270,188 @@ func TestService_PrepareDeployResources_SkipsCleanupWhenListingFails(t *testing. runtime.AssertNotCalled(t, "RemoveContainer", mock.Anything, "exited-owner", true) } +func TestService_PrepareDeployResources_MergesExitedOwnerMountsIntoRunningTempContainer(t *testing.T) { + runtime := mocks.NewMockContainerRuntime(t) + envLoader := mocks.NewMockEnvLoader(t) + svc := NewService(runtime, envLoader, nil, nil, Config{ + AllowedRegistries: []string{"docker.io"}, + VolumeAutoCreate: true, + VolumePrefix: "gordon", + }, nil) + stableName := generateVolumeName("gordon", "app.example.com", "/data") + legacyName := legacyVolumeName("gordon", "app.example.com", "/data") + ownerOnlyMount := legacyVolumeName("gordon", "app.example.com", "/var/lib/legacy") + + // A deploy interrupted before its traffic switch leaves the canonical + // container exited while the temporary replacement still runs, so the + // temporary one is what resolveExistingContainer selects and passes in here. + running := &domain.Container{ + ID: "running-temp", + Name: "gordon-app.example.com-new", + Status: "running", + Labels: map[string]string{ + domain.LabelManaged: "true", + domain.LabelRoute: "app.example.com", + domain.LabelDomain: "app.example.com", + }, + VolumeMounts: []domain.ContainerVolumeMount{ + {Name: stableName, Type: "volume", Destination: "/data"}, + }, + } + exitedOwner := &domain.Container{ + ID: "exited-owner", + Name: "gordon-app.example.com", + Status: "exited", + Labels: map[string]string{ + domain.LabelManaged: "true", + domain.LabelRoute: "app.example.com", + domain.LabelDomain: "app.example.com", + }, + VolumeMounts: []domain.ContainerVolumeMount{ + // Same destination as the running container, which stays authoritative. + {Name: legacyName, Type: "volume", Destination: "/data"}, + {Name: ownerOnlyMount, Type: "volume", Destination: "/var/lib/legacy"}, + }, + } + + runtime.EXPECT().ListContainers(mock.Anything, true). + Return([]*domain.Container{running, exitedOwner}, nil).Once() + + runtime.EXPECT().ListImages(mock.Anything).Return([]string{"myapp:latest"}, nil) + runtime.EXPECT().GetImageExposedPorts(mock.Anything, "myapp:latest").Return([]int{8080}, nil) + runtime.EXPECT().GetImageLabels(mock.Anything, "myapp:latest").Return(nil, nil) + envLoader.EXPECT().LoadEnv(mock.Anything, "app.example.com").Return([]string{}, nil) + runtime.EXPECT().InspectImageEnv(mock.Anything, "myapp:latest").Return([]string{}, nil) + + runtime.EXPECT().VolumeExists(mock.Anything, stableName).Return(true, nil).Once() + runtime.EXPECT().VolumeExists(mock.Anything, ownerOnlyMount).Return(true, nil).Once() + // The image declares /data, which the running container already covers. + runtime.EXPECT().InspectImageVolumes(mock.Anything, "myapp:latest").Return([]string{"/data"}, nil).Once() + + // Cleanup still removes the exited owner: the replacement takes over the + // mounts it preserved before that removal runs. + runtime.EXPECT().InspectContainer(mock.Anything, "exited-owner").Return(exitedOwner, nil).Once() + runtime.EXPECT().StopContainer(mock.Anything, "exited-owner").Return(nil).Once() + runtime.EXPECT().RemoveContainer(mock.Anything, "exited-owner", true).Return(nil).Once() + + resources, err := svc.prepareDeployResources(testContext(), domain.Route{ + Domain: "app.example.com", + Image: "myapp:latest", + }, running) + + require.NoError(t, err) + assert.Equal(t, map[string]string{"/data": stableName, "/var/lib/legacy": ownerOnlyMount}, resources.volumes) + runtime.AssertNotCalled(t, "VolumeExists", mock.Anything, legacyName) +} + +func TestService_PrepareDeployResources_DropsMissingMergedExitedOwnerMount(t *testing.T) { + runtime := mocks.NewMockContainerRuntime(t) + envLoader := mocks.NewMockEnvLoader(t) + svc := NewService(runtime, envLoader, nil, nil, Config{ + AllowedRegistries: []string{"docker.io"}, + VolumeAutoCreate: true, + VolumePrefix: "gordon", + }, nil) + stableName := generateVolumeName("gordon", "app.example.com", "/data") + deletedMount := "deleted-owner-data" + + running := &domain.Container{ + ID: "running-temp", + Name: "gordon-app.example.com-new", + Status: "running", + Labels: map[string]string{ + domain.LabelManaged: "true", + domain.LabelRoute: "app.example.com", + domain.LabelDomain: "app.example.com", + }, + VolumeMounts: []domain.ContainerVolumeMount{ + {Name: stableName, Type: "volume", Destination: "/data"}, + }, + } + exitedOwner := &domain.Container{ + ID: "exited-owner", + Name: "gordon-app.example.com", + Status: "exited", + Labels: map[string]string{ + domain.LabelManaged: "true", + domain.LabelRoute: "app.example.com", + domain.LabelDomain: "app.example.com", + }, + VolumeMounts: []domain.ContainerVolumeMount{ + {Name: deletedMount, Type: "volume", Destination: "/var/lib/legacy"}, + }, + } + + runtime.EXPECT().ListContainers(mock.Anything, true). + Return([]*domain.Container{running, exitedOwner}, nil).Once() + + runtime.EXPECT().ListImages(mock.Anything).Return([]string{"myapp:latest"}, nil) + runtime.EXPECT().GetImageExposedPorts(mock.Anything, "myapp:latest").Return([]int{8080}, nil) + runtime.EXPECT().GetImageLabels(mock.Anything, "myapp:latest").Return(nil, nil) + envLoader.EXPECT().LoadEnv(mock.Anything, "app.example.com").Return([]string{}, nil) + runtime.EXPECT().InspectImageEnv(mock.Anything, "myapp:latest").Return([]string{}, nil) + + runtime.EXPECT().VolumeExists(mock.Anything, stableName).Return(true, nil).Once() + // The volume the exited owner pointed at is gone, which holds no data to + // preserve: the path is dropped instead of failing the deploy closed. + runtime.EXPECT().VolumeExists(mock.Anything, deletedMount).Return(false, nil).Once() + runtime.EXPECT().InspectImageVolumes(mock.Anything, "myapp:latest").Return([]string{"/data"}, nil).Once() + runtime.EXPECT().InspectContainer(mock.Anything, "exited-owner").Return(exitedOwner, nil).Once() + runtime.EXPECT().StopContainer(mock.Anything, "exited-owner").Return(nil).Once() + runtime.EXPECT().RemoveContainer(mock.Anything, "exited-owner", true).Return(nil).Once() + + resources, err := svc.prepareDeployResources(testContext(), domain.Route{ + Domain: "app.example.com", + Image: "myapp:latest", + }, running) + + require.NoError(t, err) + assert.Equal(t, map[string]string{"/data": stableName}, resources.volumes) +} + +func TestService_PrepareDeployResources_RejectsMissingMountOfRunningContainer(t *testing.T) { + runtime := mocks.NewMockContainerRuntime(t) + envLoader := mocks.NewMockEnvLoader(t) + svc := NewService(runtime, envLoader, nil, nil, Config{ + AllowedRegistries: []string{"docker.io"}, + VolumeAutoCreate: true, + VolumePrefix: "gordon", + }, nil) + goneMount := "running-container-data" + + // Mounts of a container that is still there stay authoritative: a volume that + // disappeared under a running container is an error, not a hint. + running := &domain.Container{ + ID: "running-temp", + Name: "gordon-app.example.com-new", + Status: "running", + Labels: map[string]string{ + domain.LabelManaged: "true", + domain.LabelRoute: "app.example.com", + domain.LabelDomain: "app.example.com", + }, + VolumeMounts: []domain.ContainerVolumeMount{ + {Name: goneMount, Type: "volume", Destination: "/data"}, + }, + } + + runtime.EXPECT().ListContainers(mock.Anything, true).Return([]*domain.Container{running}, nil).Once() + runtime.EXPECT().ListImages(mock.Anything).Return([]string{"myapp:latest"}, nil) + runtime.EXPECT().GetImageExposedPorts(mock.Anything, "myapp:latest").Return([]int{8080}, nil) + runtime.EXPECT().GetImageLabels(mock.Anything, "myapp:latest").Return(nil, nil) + envLoader.EXPECT().LoadEnv(mock.Anything, "app.example.com").Return([]string{}, nil) + runtime.EXPECT().InspectImageEnv(mock.Anything, "myapp:latest").Return([]string{}, nil) + runtime.EXPECT().VolumeExists(mock.Anything, goneMount).Return(false, nil).Once() + + resources, err := svc.prepareDeployResources(testContext(), domain.Route{ + Domain: "app.example.com", + Image: "myapp:latest", + }, running) + + require.ErrorIs(t, err, domain.ErrVolumeNotFound) + assert.Nil(t, resources) +} + // exitedOwnerDeployFixture wires a Deploy for a route whose only container is // an exited volume owner left by a reboot: resolveExistingContainer cannot see // it, so the deploy knows it only through the container listing. From d3f52222a6eb2438a473bdb3d16c81392dcc2217 Mon Sep 17 00:00:00 2001 From: brice Date: Tue, 15 Sep 2026 21:07:52 +0200 Subject: [PATCH 5/5] fix(deploy): keep merged exited owners until the replacement runs When a temporary replacement is left running while the canonical container is exited, its mounts were merged into the deploy preference, but cleanup still removed the exited owner inside prepareDeployResources, before the replacement existed. A create, start or readiness failure then left the merged paths with no container mounting them and no container proving where they lived. Retained owners are a list now. In that case the canonical owner is kept, and dropped synchronously once the replacement is confirmed to mount every volume it preserved, before the finalize step renames the replacement into the canonical name the owner still holds. Exited temporary leftovers that contribute mounts stay out of that list on purpose: keeping one could collide with the alternate temporary name the replacement has to take. Also routes both volume-preference branches through the same merge, and covers the deploy path end-to-end plus the exited-owner selection rules. Refs #278 --- internal/usecase/container/service.go | 192 +++++++++++-------- internal/usecase/container/service_test.go | 204 +++++++++++++++++++-- 2 files changed, 305 insertions(+), 91 deletions(-) diff --git a/internal/usecase/container/service.go b/internal/usecase/container/service.go index 64dd4056b..4362c1893 100644 --- a/internal/usecase/container/service.go +++ b/internal/usecase/container/service.go @@ -353,10 +353,10 @@ func (s *Service) Deploy(ctx context.Context, route domain.Route) (*domain.Conta } } - // The container this deploy replaces. The exited owner is treated as the - // predecessor, so the replacement runs under a temporary name and the owner - // survives until the switch completes. - previous := previousContainerForDeploy(existing, resources.retainedOwner) + // The container this deploy replaces. An exited owner is treated as the + // predecessor only when nothing else is there, so the replacement runs under a + // temporary name and the owner survives until the switch completes. + previous := previousContainerForDeploy(existing, resources.retainedOwners) newContainer, err := s.createStartedContainer(ctx, route, previous, resources) if err != nil { @@ -383,9 +383,7 @@ func (s *Service) Deploy(ctx context.Context, route domain.Route) (*domain.Conta } } - if !mayFinalizeRetainedOwner(ctx, resources, newContainer) { - previous = nil - } + previous = s.finalizeRetainedOwners(ctx, resources, newContainer, previous) // Finalize old container in the background — the new container is already // serving traffic, so there's no reason to block the deploy response while @@ -507,10 +505,10 @@ type deployResources struct { envVars []string envHash string volumes map[string]string - // retainedOwner is the route's exited volume owner. It holds the only proof - // that the preserved volumes belong to this route, so Deploy replaces it - // under a temporary name and removes it only once the replacement runs. - retainedOwner *domain.Container + // retainedOwners are the route's exited volume owners. They hold the only + // proof that the preserved volumes belong to this route, so Deploy removes + // them only once the replacement mounts what they preserved. + retainedOwners []*domain.Container } // resolveExistingContainer returns the currently running container for a domain. @@ -581,11 +579,14 @@ func (s *Service) resolveExistingContainer(ctx context.Context, domainName strin // one when there is one, otherwise the exited owner retained as ownership // evidence. Naming the replacement after it leaves that owner on disk until the // switch completes. -func previousContainerForDeploy(existing, retainedOwner *domain.Container) *domain.Container { +func previousContainerForDeploy(existing *domain.Container, retainedOwners []*domain.Container) *domain.Container { if existing != nil { return existing } - return retainedOwner + if len(retainedOwners) == 0 { + return nil + } + return retainedOwners[0] } // mayFinalizeRetainedOwner reports whether the deploy may remove the exited owner @@ -593,23 +594,46 @@ func previousContainerForDeploy(existing, retainedOwner *domain.Container) *doma // mounts every volume it preserved: it is the last container proving which // volumes belong to the route, so dropping it while the data is unaccounted for // would leave the route unable to resolve its own volumes. -func mayFinalizeRetainedOwner(ctx context.Context, resources *deployResources, replacement *domain.Container) bool { - if resources.retainedOwner == nil || containerOwnsEveryVolume(replacement, resources.volumes) { - return true +// finalizeRetainedOwners drops the exited owners the deploy kept as ownership +// evidence and returns the container to finalize. Owners are kept unless the +// replacement mounts every volume they preserved: they are the last containers +// proving which volumes belong to the route, so dropping them while the data is +// unaccounted for would leave the route unable to resolve its own volumes. An +// owner that is not the container being replaced goes first, because a canonical +// one holds the name the replacement is renamed into. +func (s *Service) finalizeRetainedOwners(ctx context.Context, resources *deployResources, replacement, previous *domain.Container) *domain.Container { + if len(resources.retainedOwners) == 0 { + return previous } log := zerowrap.FromCtx(ctx) - log.Warn(). - Str(zerowrap.FieldEntityID, resources.retainedOwner.ID). - Msg("replacement container does not mount every preserved volume; keeping the exited owner as ownership evidence") + if !containerOwnsEveryVolume(replacement, resources.volumes) { + log.Warn(). + Str(zerowrap.FieldEntityID, resources.retainedOwners[0].ID). + Msg("replacement container does not mount every preserved volume; keeping the exited owners as ownership evidence") + return nil + } - return false + for _, owner := range resources.retainedOwners { + if owner == previous { + // Finalizing the container being replaced stops and removes it. + continue + } + if err := s.runtime.StopContainer(ctx, owner.ID); err != nil { + log.Warn().Err(err).Str(zerowrap.FieldEntityID, owner.ID).Msg("failed to stop retained owner") + } + if err := s.runtime.RemoveContainer(ctx, owner.ID, true); err != nil { + log.Warn().Err(err).Str(zerowrap.FieldEntityID, owner.ID).Msg("failed to remove retained owner") + } + } + + return previous } // deployVolumePreference returns the volumes a deploy should reuse and the exited -// owner to retain as ownership evidence for them. A container that is still there +// owners to retain as ownership evidence for them. A container that is still there // is authoritative on its own mounts, and a failed listing offers no snapshot. -func deployVolumePreference(existing *domain.Container, allContainers []*domain.Container, listed bool, domainName string) (volumePreference, *domain.Container) { +func deployVolumePreference(existing *domain.Container, allContainers []*domain.Container, listed bool, domainName string) (volumePreference, []*domain.Container) { if existing != nil { mounts := namedVolumeMounts(existing) if !listed { @@ -618,39 +642,36 @@ func deployVolumePreference(existing *domain.Container, allContainers []*domain. // An interrupted deploy can leave a temporary container running while the // canonical one is exited, and resolveExistingContainer then selects the - // temporary one. The mounts only the exited container carries are the last - // record of where that data lives, so the replacement has to take them - // over before cleanup removes their only owner. - return mergeExitedOwnerMounts(mounts, exitedOwnerMounts(allContainers, domainName, existing.ID)), nil + // temporary one. The mounts only the exited containers carry are the last + // record of where that data lives, so the replacement has to take them over + // before their owners are gone. + preference, owners := mergeExitedOwnerMounts(mounts, exitedRouteOwners(allContainers, domainName, existing.ID)) + return preference, retainableExitedOwners(owners, domainName) } if !listed { return volumePreference{}, nil } - owner := snapshotRouteVolumeOwner(allContainers, domainName) - mounts := namedVolumeMounts(owner) - if len(mounts) == 0 { + // After a reboot the volume owner is exited, so it is invisible to + // resolveExistingContainer and orphan cleanup would delete it. Its mounts are + // the only record of which volumes hold the route's data: the owner is kept as + // ownership evidence until the replacement mounts them, since deleting it + // first would leave a failed deploy with no way to prove that a legacy volume + // belongs to this route. + preference, owners := mergeExitedOwnerMounts(nil, []*domain.Container{snapshotRouteVolumeOwner(allContainers, domainName)}) + if len(preference.mounts) == 0 { return volumePreference{}, nil } - // The owner is kept as ownership evidence until the replacement mounts - // these volumes: while it is the only container mounting them, deleting it - // first would leave a failed deploy without any way to prove that a legacy - // volume belongs to this route. - fromExitedOwner := make(map[string]bool, len(mounts)) - for path := range mounts { - fromExitedOwner[path] = true - } - - return volumePreference{mounts: mounts, fromExitedOwner: fromExitedOwner}, owner + return preference, owners } -// exitedOwnerMounts returns the named volume mounts of the route's exited -// containers from an already-fetched listing, canonical first so the container -// that outlived the others wins a destination. skipContainerID is the container -// the deploy replaces, which contributes its own mounts separately. -func exitedOwnerMounts(allContainers []*domain.Container, domainName, skipContainerID string) map[string]namedVolumeMount { - mounts := make(map[string]namedVolumeMount) +// exitedRouteOwners returns the route's exited containers from an already-fetched +// listing, canonical first so the containers that outlived the others win a +// destination. skipContainerID is the container the deploy replaces, which +// contributes its own mounts separately. +func exitedRouteOwners(allContainers []*domain.Container, domainName, skipContainerID string) []*domain.Container { + var owners []*domain.Container canonicalName := managedContainerName(domainName) for _, name := range []string{canonicalName, canonicalName + "-new", canonicalName + "-next"} { for _, c := range allContainers { @@ -660,35 +681,56 @@ func exitedOwnerMounts(allContainers []*domain.Container, domainName, skipContai if c.Status == "running" || c.Status == "restarting" { continue } - for path, mount := range namedVolumeMounts(c) { - if _, taken := mounts[path]; !taken { - mounts[path] = mount - } - } + owners = append(owners, c) } } - return mounts + return owners } -// mergeExitedOwnerMounts fills the destinations only the exited owner has, and -// records them in fromExitedOwner. A container that is still there stays -// authoritative for the destinations it already mounts. -func mergeExitedOwnerMounts(mounts, exitedOwner map[string]namedVolumeMount) volumePreference { - merged := make(map[string]namedVolumeMount, len(mounts)+len(exitedOwner)) +// retainableExitedOwners drops the exited owners whose name the replacement may +// still need: keeping those would block container creation instead of protecting +// data. +func retainableExitedOwners(owners []*domain.Container, domainName string) []*domain.Container { + canonicalName := managedContainerName(domainName) + retained := make([]*domain.Container, 0, len(owners)) + for _, owner := range owners { + if owner.Name == canonicalName { + retained = append(retained, owner) + } + } + return retained +} + +// mergeExitedOwnerMounts returns the mounts the deploy reuses: those of the +// containers that are still there, plus the destinations only the route's exited +// owners carry. Those extra destinations are marked in fromExitedOwner, because a +// volume deleted while its container was stopped holds no data to preserve. It +// also reports which owners contributed a mount, so the caller can keep them as +// ownership evidence. +func mergeExitedOwnerMounts(mounts map[string]namedVolumeMount, owners []*domain.Container) (volumePreference, []*domain.Container) { + merged := make(map[string]namedVolumeMount, len(mounts)) for path, mount := range mounts { merged[path] = mount } - fromExitedOwner := make(map[string]bool, len(exitedOwner)) - for path, mount := range exitedOwner { - if _, taken := merged[path]; taken { - continue + fromExitedOwner := make(map[string]bool) + var contributors []*domain.Container + for _, owner := range owners { + contributed := false + for path, mount := range namedVolumeMounts(owner) { + if _, taken := merged[path]; taken { + continue + } + merged[path] = mount + fromExitedOwner[path] = true + contributed = true + } + if contributed { + contributors = append(contributors, owner) } - merged[path] = mount - fromExitedOwner[path] = true } - return volumePreference{mounts: merged, fromExitedOwner: fromExitedOwner} + return volumePreference{mounts: merged, fromExitedOwner: fromExitedOwner}, contributors } func (s *Service) prepareDeployResources(ctx context.Context, route domain.Route, existing *domain.Container) (*deployResources, error) { @@ -713,7 +755,7 @@ func (s *Service) prepareDeployResources(ctx context.Context, route domain.Route // the operator sees as a failed deploy and can retry. log.WrapErr(listErr, "failed to list containers for deploy; skipping volume snapshot and orphan cleanup") } - preferredVolumes, retainedOwner := deployVolumePreference(existing, allContainers, listErr == nil, route.Domain) + preferredVolumes, retainedOwners := deployVolumePreference(existing, allContainers, listErr == nil, route.Domain) imageRef, err := s.buildValidatedImageRef(ctx, route.Image) if err != nil { @@ -757,14 +799,14 @@ func (s *Service) prepareDeployResources(ctx context.Context, route domain.Route // Cleanup runs last because it removes route containers whose mounts were // needed above: every step that may still need that evidence (image - // resolution, legacy volume ownership probes) must have completed. The - // retained owner is not removed here, only after the replacement runs. + // resolution, legacy volume ownership probes) must have completed. Retained + // owners are not removed here, only after the replacement runs. if listErr == nil { - retainedID := "" - if retainedOwner != nil { - retainedID = retainedOwner.ID + retainedIDs := make([]string, 0, len(retainedOwners)) + for _, owner := range retainedOwners { + retainedIDs = append(retainedIDs, owner.ID) } - if err := s.cleanupOrphanedContainers(ctx, route.Domain, existingID, retainedID, allContainers); err != nil { + if err := s.cleanupOrphanedContainers(ctx, route.Domain, existingID, retainedIDs, allContainers); err != nil { log.WrapErr(err, "failed to cleanup orphaned containers") } } @@ -777,7 +819,7 @@ func (s *Service) prepareDeployResources(ctx context.Context, route domain.Route envVars: envVars, envHash: envHash, volumes: volumes, - retainedOwner: retainedOwner, + retainedOwners: retainedOwners, }, nil } @@ -2886,16 +2928,16 @@ func hasRouteOwnershipLabels(c *domain.Container, domainName string) bool { // containers left behind by a previous or interrupted deploy. allContainers is // the listing the caller already fetched: cleanup must decide on the same // snapshot the caller used, so it never re-lists on its own. skipContainerID is -// the container the deploy is replacing; keepContainerID, when set, is an exited -// owner retained as ownership evidence, and is never removed here. -func (s *Service) cleanupOrphanedContainers(ctx context.Context, domainName, skipContainerID, keepContainerID string, allContainers []*domain.Container) error { +// the container the deploy is replacing; keepContainerIDs are exited owners +// retained as ownership evidence, and are never removed here. +func (s *Service) cleanupOrphanedContainers(ctx context.Context, domainName, skipContainerID string, keepContainerIDs []string, allContainers []*domain.Container) error { log := zerowrap.FromCtx(ctx) expectedName := managedContainerName(domainName) expectedNewName := expectedName + "-new" expectedNextName := expectedName + "-next" for _, c := range allContainers { - if c == nil || c.ID == skipContainerID || c.ID == keepContainerID { + if c == nil || c.ID == skipContainerID || slices.Contains(keepContainerIDs, c.ID) { continue } if c.Name != expectedName && c.Name != expectedNewName && c.Name != expectedNextName { diff --git a/internal/usecase/container/service_test.go b/internal/usecase/container/service_test.go index fd253b555..66d4c8408 100644 --- a/internal/usecase/container/service_test.go +++ b/internal/usecase/container/service_test.go @@ -1932,7 +1932,7 @@ func TestService_PrepareDeployResources_ReusesExitedOwnerVolumes(t *testing.T) { require.NoError(t, err) assert.Equal(t, map[string]string{"/data": legacyName}, resources.volumes) - assert.Same(t, exitedOwner, resources.retainedOwner) + assert.Equal(t, []*domain.Container{exitedOwner}, resources.retainedOwners) runtime.AssertNotCalled(t, "VolumeExists", mock.Anything, stableName) runtime.AssertNotCalled(t, "CreateVolume", mock.Anything, mock.Anything) runtime.AssertNotCalled(t, "StopContainer", mock.Anything, "exited-owner") @@ -1991,7 +1991,7 @@ func TestService_PrepareDeployResources_KeepsExitedOwnerForLegacyOwnershipProbe( require.NoError(t, err) assert.Equal(t, map[string]string{"/data": legacyName, "/var/lib/legacy": legacyName}, resources.volumes) - assert.Same(t, exitedOwner, resources.retainedOwner) + assert.Equal(t, []*domain.Container{exitedOwner}, resources.retainedOwners) runtime.AssertNotCalled(t, "VolumeExists", mock.Anything, stableName) runtime.AssertNotCalled(t, "StopContainer", mock.Anything, "exited-owner") runtime.AssertNotCalled(t, "RemoveContainer", mock.Anything, "exited-owner", true) @@ -2092,7 +2092,7 @@ func TestService_PrepareDeployResources_ReusesExitedTempNameOwnerVolumes(t *test require.NoError(t, err) assert.Equal(t, map[string]string{"/data": legacyName}, resources.volumes) - assert.Same(t, exitedTempOwner, resources.retainedOwner) + assert.Equal(t, []*domain.Container{exitedTempOwner}, resources.retainedOwners) runtime.AssertNotCalled(t, "VolumeExists", mock.Anything, stableName) runtime.AssertNotCalled(t, "CreateVolume", mock.Anything, mock.Anything) runtime.AssertNotCalled(t, "RemoveContainer", mock.Anything, "exited-new-owner", true) @@ -2153,7 +2153,7 @@ func TestService_PrepareDeployResources_IgnoresForeignContainerOnCanonicalName(t require.NoError(t, err) assert.Equal(t, map[string]string{"/data": stableName}, resources.volumes) - assert.Nil(t, resources.retainedOwner) + assert.Empty(t, resources.retainedOwners) runtime.AssertNotCalled(t, "VolumeExists", mock.Anything, foreignVolume) } @@ -2207,7 +2207,7 @@ func TestService_PrepareDeployResources_IgnoresDeletedSnapshottedVolume(t *testi require.NoError(t, err) assert.Equal(t, map[string]string{"/data": stableName}, resources.volumes) - assert.Same(t, exitedOwner, resources.retainedOwner) + assert.Equal(t, []*domain.Container{exitedOwner}, resources.retainedOwners) runtime.AssertNotCalled(t, "RemoveContainer", mock.Anything, "exited-owner", true) } @@ -2328,11 +2328,8 @@ func TestService_PrepareDeployResources_MergesExitedOwnerMountsIntoRunningTempCo // The image declares /data, which the running container already covers. runtime.EXPECT().InspectImageVolumes(mock.Anything, "myapp:latest").Return([]string{"/data"}, nil).Once() - // Cleanup still removes the exited owner: the replacement takes over the - // mounts it preserved before that removal runs. - runtime.EXPECT().InspectContainer(mock.Anything, "exited-owner").Return(exitedOwner, nil).Once() - runtime.EXPECT().StopContainer(mock.Anything, "exited-owner").Return(nil).Once() - runtime.EXPECT().RemoveContainer(mock.Anything, "exited-owner", true).Return(nil).Once() + // The owner is retained, so cleanup leaves it in place: it is the container + // that proves where the merged mount lives. resources, err := svc.prepareDeployResources(testContext(), domain.Route{ Domain: "app.example.com", @@ -2341,7 +2338,10 @@ func TestService_PrepareDeployResources_MergesExitedOwnerMountsIntoRunningTempCo require.NoError(t, err) assert.Equal(t, map[string]string{"/data": stableName, "/var/lib/legacy": ownerOnlyMount}, resources.volumes) + assert.Equal(t, []*domain.Container{exitedOwner}, resources.retainedOwners) runtime.AssertNotCalled(t, "VolumeExists", mock.Anything, legacyName) + runtime.AssertNotCalled(t, "StopContainer", mock.Anything, "exited-owner") + runtime.AssertNotCalled(t, "RemoveContainer", mock.Anything, "exited-owner", true) } func TestService_PrepareDeployResources_DropsMissingMergedExitedOwnerMount(t *testing.T) { @@ -2396,9 +2396,6 @@ func TestService_PrepareDeployResources_DropsMissingMergedExitedOwnerMount(t *te // preserve: the path is dropped instead of failing the deploy closed. runtime.EXPECT().VolumeExists(mock.Anything, deletedMount).Return(false, nil).Once() runtime.EXPECT().InspectImageVolumes(mock.Anything, "myapp:latest").Return([]string{"/data"}, nil).Once() - runtime.EXPECT().InspectContainer(mock.Anything, "exited-owner").Return(exitedOwner, nil).Once() - runtime.EXPECT().StopContainer(mock.Anything, "exited-owner").Return(nil).Once() - runtime.EXPECT().RemoveContainer(mock.Anything, "exited-owner", true).Return(nil).Once() resources, err := svc.prepareDeployResources(testContext(), domain.Route{ Domain: "app.example.com", @@ -2407,6 +2404,8 @@ func TestService_PrepareDeployResources_DropsMissingMergedExitedOwnerMount(t *te require.NoError(t, err) assert.Equal(t, map[string]string{"/data": stableName}, resources.volumes) + assert.Equal(t, []*domain.Container{exitedOwner}, resources.retainedOwners) + runtime.AssertNotCalled(t, "RemoveContainer", mock.Anything, "exited-owner", true) } func TestService_PrepareDeployResources_RejectsMissingMountOfRunningContainer(t *testing.T) { @@ -2452,6 +2451,86 @@ func TestService_PrepareDeployResources_RejectsMissingMountOfRunningContainer(t assert.Nil(t, resources) } +func TestExitedRouteOwners(t *testing.T) { + canonicalName := "gordon-app.example.com" + owned := func(id, name, status string) *domain.Container { + return &domain.Container{ + ID: id, + Name: name, + Status: status, + Labels: map[string]string{ + domain.LabelManaged: "true", + domain.LabelRoute: "app.example.com", + domain.LabelDomain: "app.example.com", + }, + } + } + + tests := []struct { + name string + listed []*domain.Container + skipID string + want []string + }{ + { + name: "prefers the canonical container over temp leftovers", + listed: []*domain.Container{owned("temp", canonicalName+"-new", "exited"), owned("canonical", canonicalName, "exited")}, + want: []string{"canonical", "temp"}, + }, + { + name: "skips running containers", + listed: []*domain.Container{owned("running", canonicalName, "running"), owned("exited", canonicalName+"-next", "exited")}, + want: []string{"exited"}, + }, + { + name: "skips restarting containers", + listed: []*domain.Container{owned("restarting", canonicalName, "restarting")}, + }, + { + name: "skips the container the deploy replaces", + listed: []*domain.Container{owned("selected", canonicalName, "exited"), owned("other", canonicalName+"-new", "exited")}, + skipID: "selected", + want: []string{"other"}, + }, + { + name: "skips foreign and attachment containers", + listed: []*domain.Container{ + {ID: "foreign", Name: canonicalName, Status: "exited", Labels: map[string]string{domain.LabelManaged: "true", domain.LabelRoute: "other.example.com"}}, + {ID: "attachment", Name: canonicalName, Status: "exited", Labels: map[string]string{domain.LabelManaged: "true", domain.LabelRoute: "app.example.com", domain.LabelAttachment: "true"}}, + {ID: "unmanaged", Name: canonicalName, Status: "exited"}, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + owners := exitedRouteOwners(tt.listed, "app.example.com", tt.skipID) + var ids []string + for _, owner := range owners { + ids = append(ids, owner.ID) + } + assert.Equal(t, tt.want, ids) + }) + } +} + +func TestDeployVolumePreference_WithoutListingKeepsOnlyTheContainerMounts(t *testing.T) { + existing := &domain.Container{ + ID: "running-temp", + Name: "gordon-app.example.com-new", + Status: "running", + VolumeMounts: []domain.ContainerVolumeMount{{Name: "keep", Type: "volume", Destination: "/data"}}, + } + + // Without a listing there is no exited owner to merge in, and nothing to + // retain: removal is the cleanup pass's business, and it was skipped. + preference, retained := deployVolumePreference(existing, nil, false, "app.example.com") + + assert.Equal(t, map[string]namedVolumeMount{"/data": {Name: "keep"}}, preference.mounts) + assert.Empty(t, preference.fromExitedOwner) + assert.Empty(t, retained) +} + // exitedOwnerDeployFixture wires a Deploy for a route whose only container is // an exited volume owner left by a reboot: resolveExistingContainer cannot see // it, so the deploy knows it only through the container listing. @@ -2644,6 +2723,99 @@ func TestService_Deploy_KeepsExitedOwnerWhenReplacementFailsReadiness(t *testing f.runtime.AssertNotCalled(t, "RemoveContainer", mock.Anything, "exited-owner", true) } +func TestService_Deploy_KeepsMergedExitedOwnerUntilReplacementRuns(t *testing.T) { + runtime := mocks.NewMockContainerRuntime(t) + envLoader := mocks.NewMockEnvLoader(t) + eventBus := mocks.NewMockEventPublisher(t) + svc := NewService(runtime, envLoader, eventBus, nil, Config{ + AllowedRegistries: []string{"docker.io"}, + VolumeAutoCreate: true, + VolumePrefix: "gordon", + ReadinessDelay: time.Millisecond, + StabilizationDelay: time.Millisecond, + }, nil) + stableName := generateVolumeName("gordon", "app.example.com", "/data") + ownerMount := legacyVolumeName("gordon", "app.example.com", "/var/lib/legacy") + + // An interrupted deploy left its temporary replacement running while the + // canonical container it was meant to replace is exited. + running := &domain.Container{ + ID: "running-temp", + Name: "gordon-app.example.com-new", + Status: "running", + Labels: map[string]string{ + domain.LabelManaged: "true", + domain.LabelRoute: "app.example.com", + domain.LabelDomain: "app.example.com", + }, + VolumeMounts: []domain.ContainerVolumeMount{ + {Name: stableName, Type: "volume", Destination: "/data"}, + }, + } + exitedOwner := &domain.Container{ + ID: "exited-owner", + Name: "gordon-app.example.com", + Status: "exited", + Labels: map[string]string{ + domain.LabelManaged: "true", + domain.LabelRoute: "app.example.com", + domain.LabelDomain: "app.example.com", + }, + VolumeMounts: []domain.ContainerVolumeMount{ + {Name: ownerMount, Type: "volume", Destination: "/var/lib/legacy"}, + }, + } + + runtime.EXPECT().ListContainers(mock.Anything, false).Return([]*domain.Container{running}, nil).Once() + runtime.EXPECT().ListContainers(mock.Anything, true). + Return([]*domain.Container{running, exitedOwner}, nil).Once() + + runtime.EXPECT().ListImages(mock.Anything).Return([]string{"myapp:latest"}, nil) + runtime.EXPECT().GetImageExposedPorts(mock.Anything, "myapp:latest").Return([]int{8080}, nil) + runtime.EXPECT().GetImageLabels(mock.Anything, "myapp:latest").Return(nil, nil) + envLoader.EXPECT().LoadEnv(mock.Anything, "app.example.com").Return([]string{}, nil) + runtime.EXPECT().InspectImageEnv(mock.Anything, "myapp:latest").Return([]string{}, nil) + runtime.EXPECT().VolumeExists(mock.Anything, stableName).Return(true, nil).Once() + runtime.EXPECT().VolumeExists(mock.Anything, ownerMount).Return(true, nil).Once() + runtime.EXPECT().InspectImageVolumes(mock.Anything, "myapp:latest").Return([]string{"/data"}, nil).Once() + + // The replacement takes the other temporary name and mounts both volumes. + newContainer := &domain.Container{ID: "new-container", Name: "gordon-app.example.com-next", Status: "created"} + runtime.EXPECT().CreateContainer(mock.Anything, mock.MatchedBy(func(cfg *domain.ContainerConfig) bool { + return cfg.Name == "gordon-app.example.com-next" && + cfg.Volumes["/data"] == stableName && cfg.Volumes["/var/lib/legacy"] == ownerMount + })).Return(newContainer, nil) + runtime.EXPECT().StartContainer(mock.Anything, "new-container").Return(nil) + runtime.EXPECT().IsContainerRunning(mock.Anything, "new-container").Return(true, nil).Times(3) + inspectCall := runtime.EXPECT().InspectContainer(mock.Anything, "new-container").Return(&domain.Container{ + ID: "new-container", + Name: "gordon-app.example.com-next", + Status: "running", + Ports: []int{8080}, + VolumeMounts: []domain.ContainerVolumeMount{ + {Name: stableName, Type: "volume", Destination: "/data"}, + {Name: ownerMount, Type: "volume", Destination: "/var/lib/legacy"}, + }, + }, nil) + eventBus.EXPECT().Publish(domain.EventContainerDeployed, mock.AnythingOfType("*domain.ContainerEventPayload")).Return(nil) + + // The retained owner is dropped once the replacement mounts its volumes, and + // before the replacement is renamed into the canonical name it still holds. + runtime.EXPECT().StopContainer(mock.Anything, "exited-owner").Return(nil).NotBefore(inspectCall.Call) + ownerRemoved := runtime.EXPECT().RemoveContainer(mock.Anything, "exited-owner", true).Return(nil).NotBefore(inspectCall.Call) + runtime.EXPECT().RenameContainer(mock.Anything, "new-container", "gordon-app.example.com"). + Return(nil).NotBefore(ownerRemoved) + runtime.EXPECT().StopContainer(mock.Anything, "running-temp").Return(nil) + runtime.EXPECT().RemoveContainer(mock.Anything, "running-temp", true).Return(nil) + + result, err := svc.Deploy(testContext(), domain.Route{Domain: "app.example.com", Image: "myapp:latest"}) + + require.NoError(t, err) + require.NotNil(t, result) + assert.Equal(t, "new-container", result.ID) + svc.WaitForCleanup() +} + func TestService_Deploy_KeepsExitedOwnerWhenReplacementMissesVolume(t *testing.T) { f := newExitedOwnerDeployFixture(t, "gordon-app.example.com") @@ -4571,7 +4743,7 @@ func TestService_CleanupOrphanedContainers_SkipsRestartingContainer(t *testing.T svc := NewService(runtime, envLoader, eventBus, nil, Config{}, nil) ctx := testContext() - err := svc.cleanupOrphanedContainers(ctx, "test.example.com", "", "", []*domain.Container{ + err := svc.cleanupOrphanedContainers(ctx, "test.example.com", "", nil, []*domain.Container{ { ID: "restarting-container", Name: "gordon-test.example.com", @@ -4594,7 +4766,7 @@ func TestService_CleanupOrphanedContainers_SkipsContainerStartedSinceListing(t * Status: "running", }, nil).Once() - err := svc.cleanupOrphanedContainers(testContext(), "test.example.com", "", "", []*domain.Container{ + err := svc.cleanupOrphanedContainers(testContext(), "test.example.com", "", nil, []*domain.Container{ { ID: "revived-container", Name: "gordon-test.example.com", @@ -4615,7 +4787,7 @@ func TestService_CleanupOrphanedContainers_SkipsRemovalWhenStatusUnreadable(t *t // is recoverable, killing a live container is not. runtime.EXPECT().InspectContainer(mock.Anything, "unknown-container").Return(nil, errors.New("inspect failed")).Once() - err := svc.cleanupOrphanedContainers(testContext(), "test.example.com", "", "", []*domain.Container{ + err := svc.cleanupOrphanedContainers(testContext(), "test.example.com", "", nil, []*domain.Container{ { ID: "unknown-container", Name: "gordon-test.example.com",