fix(container): skip redundant deploy when container already runs the same image#71
fix(container): skip redundant deploy when container already runs the same image#71
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdded image-ID propagation and lookup: Docker runtime exposes GetImageID, Container gained ImageID, service deploy checks image IDs and may skip creating a new container when an identical image is already running; mocks and tests updated accordingly. Changes
Sequence DiagramsequenceDiagram
participant User
participant Service
participant Docker as Docker Runtime
participant Existing as Existing Container
User->>Service: Deploy(targetImageRef)
Service->>Docker: GetImageID(targetImageRef)
Docker-->>Service: targetImageID / error
alt targetImageID obtained
Service->>Existing: Read ImageID & Status
Existing-->>Service: imageID, status
alt imageID == targetImageID and status == Running
Service-->>User: Return existing container (skip creation)
else
Service->>Service: Proceed with full create/start/ready flow
end
else inspection error
Service->>Service: Proceed with full create/start/ready flow
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR prevents a redundant second deploy during gordon push by detecting when the currently running container is already on the exact same Docker image ID and short-circuiting the deploy. To support that check, it extends the container domain model and runtime interface to surface image IDs, and adds tests for the new behavior.
Changes:
- Add
ImageIDtodomain.Containerand populate it from Docker in bothListContainersandInspectContainer. - Extend
ContainerRuntimewithGetImageID(imageRef)and implement it in the Docker runtime adapter (plus mocks). - Add deploy logic to skip a redundant redeploy when the tracked container is running the same image ID, with tests for success/fallback cases.
Reviewed changes
Copilot reviewed 3 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/usecase/container/service.go | Adds the redundant-deploy short-circuit logic based on Docker image ID. |
| internal/usecase/container/service_test.go | Adds new test coverage for skip/proceed scenarios; adjusts an existing deploy test. |
| internal/domain/container.go | Extends Container model with ImageID. |
| internal/boundaries/out/runtime.go | Adds GetImageID() to the runtime interface. |
| internal/boundaries/out/mocks/mock_container_runtime.go | Updates mocks to implement the new runtime method. |
| internal/adapters/out/docker/runtime.go | Populates ImageID for containers and implements GetImageID() via ImageInspect. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Skip redundant deploy: if the existing container is already running | ||
| // the exact same image (by Docker image ID), return it immediately. | ||
| // This prevents the double-deploy caused by the event-based deploy | ||
| // (triggered by image.pushed) racing with the explicit CLI deploy call. | ||
| if hasExisting && existing.ImageID != "" { | ||
| if skip, container := s.skipRedundantDeploy(ctx, existing, resources.actualImageRef); skip { | ||
| return container, nil | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The new redundancy short-circuit makes Deploy a no-op whenever a running tracked container has the same Docker image ID. This changes the behavior of manual/explicit deploys (the CLI help text says deploy/redeploy happens even if a container is already running) and can prevent legitimate redeploys intended to apply updated env/volume/attachment config without changing the image. Consider scoping the optimization to the specific double-deploy race (e.g., only skip when the previous successful deploy for this domain completed very recently and the image ID matches, or gate it behind a context/flag for event-triggered deploys) so explicit deploy remains a true redeploy when desired.
| // Skip redundant deploy: if the existing container is already running | |
| // the exact same image (by Docker image ID), return it immediately. | |
| // This prevents the double-deploy caused by the event-based deploy | |
| // (triggered by image.pushed) racing with the explicit CLI deploy call. | |
| if hasExisting && existing.ImageID != "" { | |
| if skip, container := s.skipRedundantDeploy(ctx, existing, resources.actualImageRef); skip { | |
| return container, nil | |
| } | |
| } |
| @@ -313,11 +313,64 @@ func TestService_Deploy_ReplacesExistingContainer(t *testing.T) { | |||
|
|
|||
| assert.NoError(t, err) | |||
| assert.Equal(t, "new-container", result.ID) | |||
There was a problem hiding this comment.
This test no longer verifies that the service updates its tracked container entry after replacing an existing container (the earlier svc.Get(...) assertions were removed). Since replace/zero-downtime deploy is a distinct path from the initial deploy, it would be good to restore an assertion that svc.Get("test.example.com") returns the new container ID after Deploy completes.
| assert.Equal(t, "new-container", result.ID) | |
| assert.Equal(t, "new-container", result.ID) | |
| // Ensure the service's tracked container entry now points to the new container. | |
| trackedContainer, ok := svc.Get("test.example.com") | |
| assert.True(t, ok) | |
| assert.Equal(t, "new-container", trackedContainer.ID) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/usecase/container/service.go`:
- Around line 242-250: The current short-circuit in service.go returns the
existing container based only on ImageID and running state, which ignores
configuration changes; update the logic so s.skipRedundantDeploy (and the call
sites around the other block at 274-304) also verifies that container
configuration matches the requested deploy (compare a persisted config
hash/label or individual fields such as env vars, volumes, labels, attachments,
networks) or restrict skipping to a safe recent-deploy window; modify
skipRedundantDeploy to accept the desired config (or compute/compare a config
hash) and only return skip=true when both image ID and config match, otherwise
proceed with redeploy.
| // Skip redundant deploy: if the existing container is already running | ||
| // the exact same image (by Docker image ID), return it immediately. | ||
| // This prevents the double-deploy caused by the event-based deploy | ||
| // (triggered by image.pushed) racing with the explicit CLI deploy call. | ||
| if hasExisting && existing.ImageID != "" { | ||
| if skip, container := s.skipRedundantDeploy(ctx, existing, resources.actualImageRef); skip { | ||
| return container, nil | ||
| } | ||
| } |
There was a problem hiding this comment.
Skip logic can ignore config/env changes when image ID is unchanged.
Deploy now short-circuits solely on image ID + running state, so explicit redeploys with the same image won’t apply updated env vars, volumes, labels, attachments, or network changes. That’s a behavior regression for users who redeploy to pick up configuration changes.
Consider gating the skip to only known redundant cases (e.g., a short “recent deploy” window) or comparing a persisted config hash/label so redeploys still happen when config changes.
Also applies to: 274-304
🤖 Prompt for AI Agents
In `@internal/usecase/container/service.go` around lines 242 - 250, The current
short-circuit in service.go returns the existing container based only on ImageID
and running state, which ignores configuration changes; update the logic so
s.skipRedundantDeploy (and the call sites around the other block at 274-304)
also verifies that container configuration matches the requested deploy (compare
a persisted config hash/label or individual fields such as env vars, volumes,
labels, attachments, networks) or restrict skipping to a safe recent-deploy
window; modify skipRedundantDeploy to accept the desired config (or
compute/compare a config hash) and only return skip=true when both image ID and
config match, otherwise proceed with redeploy.
…ingContainer test
Summary
gordon push: the registry'simage.pushedevent triggers a deploy, and the CLI's explicitPOST /deploytriggers a second identical one. The second deploy now detects the container is already running the same image (by Docker image ID) and short-circuits immediately.ImageIDfield toContainerdomain model andGetImageID()to the runtime interface, populated in bothListContainersandInspectContainerso the check works across restarts.GetImageIDerror (graceful degradation).Root Cause
When
docker pushcompletes, two things happen concurrently:EventImagePushed→ event handler callsDeploy()POST /deploy/<domain>→ also callsDeploy()The domain deploy lock serializes them, but the second deploy was fully redundant — pulling the same image, creating a new container, running readiness checks — doubling the total push time.
Fix
After
prepareDeployResources(image is pulled), compare the new image's Docker ID with the existing container'sImageID. If they match and the container is running, return the existing container immediately.Summary by CodeRabbit
New Features
Tests