Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 42 additions & 6 deletions internal/usecase/container/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
preferredVolumes := namedVolumeMounts(existing)
if existing == nil {
preferredVolumes = snapshotRouteVolumeMounts(allContainers, route.Domain)
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
if err := s.cleanupOrphanedContainers(ctx, route.Domain, existingID, allContainers); err != nil {
log.WrapErr(err, "failed to cleanup orphaned containers")
}

Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
}
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 {
Expand Down
118 changes: 117 additions & 1 deletion internal/usecase/container/service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1880,6 +1880,122 @@ func TestService_SetupVolumes_RejectsUnverifiedLegacyVolumeWithoutStableReplacem
assert.Empty(t, volumes)
}

func TestService_PrepareDeployResources_ReusesExitedOwnerVolumes(t *testing.T) {
Comment thread
coderabbitai[bot] marked this conversation as resolved.
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)
Expand Down Expand Up @@ -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)
}
Expand Down
Loading