fix: prevent orphaned old Watchtower containers from persisting - #1743
Merged
Conversation
- Add ExcludeOldNamedWatchtowerFilter to reject watchtower-old-* containers - Chain the filter as a base filter in BuildFilter to exclude old instances - Add CleanupOldWatchtowerContainers for per-update-cycle old container cleanup - Derive scope from current container to avoid crossing scope boundaries - Skip cleanup when scope is unknown to prevent accidental cross-scope removal - Add shouldUpdateContainer guard to skip old-named Watchtower containers - Move WatchtowerOldPrefix to types package as single source of truth - Update filters, container_id, and update to use shared constant
- Add ExcludeOldNamedWatchtowerFilter to reject watchtower-old-* containers - Chain filter as outermost wrapper in BuildFilter for early short-circuit - Add IsOldNamedWatchtower positive predicate for readable guard clauses - Add CleanupOldWatchtowerContainers for per-update-cycle old container cleanup - Pass current container to removeExcessContainers to enable image collection - Derive scope from current container to avoid crossing scope boundaries - Skip cleanup when current container not found in list - Add ShouldExitDueToInvalidRestart name-based check for old-named containers - Update restart policy to "no" before exit to prevent Docker restart - Add pre-update self-check that detects and stops old-named current container - Move WatchtowerOldPrefix to types package as single source of truth - Update misleading comments and log messages for accuracy
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds centralized old-name prefix and filters, detects old watchtower containers at startup (attempts to set RestartPolicy to "no" and exit), excludes old containers from updates, adds scoped per-cycle cleanup action, updates mocks and tests, and adjusts notification wording. ChangesOld-Named Watchtower Container Lifecycle
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
3 tasks
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Complexity | 2 medium |
🟢 Metrics 15 complexity · 136 duplication
Metric Results Complexity 15 Duplication 136
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #1743 +/- ##
========================================
Coverage 74.49% 74.49%
========================================
Files 61 61
Lines 10223 10345 +122
========================================
+ Hits 7616 7707 +91
- Misses 2328 2356 +28
- Partials 279 282 +3
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pkg/filters/filters.go (1)
68-93: ⚡ Quick winUnify old-named detection behind a single predicate.
ExcludeOldNamedWatchtowerFilterandIsOldNamedWatchtowercurrently encode the same matching logic separately. ReusingIsOldNamedWatchtowerinside the exclusion filter would reduce drift risk.♻️ Proposed refactor
func ExcludeOldNamedWatchtowerFilter(c types.FilterableContainer) bool { - if !c.IsWatchtower() { - return true - } - - if strings.HasPrefix(strings.TrimLeft(c.Name(), "/"), types.WatchtowerOldPrefix) { + if IsOldNamedWatchtower(c) { logrus.WithField("container", c.Name()). Debug("Excluding old-named Watchtower container from update cycle") return false } return true }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/filters/filters.go` around lines 68 - 93, ExcludeOldNamedWatchtowerFilter duplicates the old-name detection logic; replace its manual check with a call to IsOldNamedWatchtower to centralize the predicate. Update ExcludeOldNamedWatchtowerFilter so it first returns true when !c.IsWatchtower() (unchanged), then calls IsOldNamedWatchtower(c) to decide exclusion, logging with logrus.WithField("container", c.Name()).Debug(...) when IsOldNamedWatchtower(c) is true and returning false; otherwise return true. Ensure IsOldNamedWatchtower remains the single source of truth for the strings.HasPrefix(strings.TrimLeft(c.Name(), "/"), types.WatchtowerOldPrefix) check.pkg/filters/filters_test.go (1)
454-478: ⚡ Quick winAdd one BuildFilter integration assertion for old-named Watchtower exclusion.
The new predicate tests are good, but a direct
BuildFilter(...)case withIsWatchtower=trueandName=/watchtower-old-...would better guard filter-chain wiring regressions.Also applies to: 680-849
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/filters/filters_test.go` around lines 454 - 478, Add an integration assertion to the TestBuildFilterNoneScope test that exercises BuildFilter for the legacy Watchtower name: create a mock FilterableContainer where IsWatchtower returns true and Name returns a string like "/watchtower-old-..." (and other required mocks same as existing scoped/unscoped), then call the existing filter and assert it returns false (excluded); this ensures BuildFilter's filter chain properly excludes old-named Watchtower containers alongside the new predicates.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/actions/update_context_test.go`:
- Around line 214-330: The ginkgo.When block titled "the current container is an
old-named Watchtower instance" is declared inside the
TestUpdateAction_MidOperationCancellationCheck test function, which breaks
Ginkgo node registration; move that entire ginkgo.When { ... } block out of the
TestUpdateAction_MidOperationCancellationCheck function and place it at
top-level test scope (under an existing or new ginkgo.Describe/ginkgo.When for
the Update action), keeping all inner ginkgo.It specs intact and ensuring
references to Update, mockActions.CreateMockContainerWithConfig, and the
CurrentContainerID test cases remain the same.
In `@internal/actions/update.go`:
- Around line 1925-1947: The deriveScopeFromCurrentContainer function
incorrectly uses the sentinel "none" for unscoped containers which collides with
an explicit scope value "none"; change deriveScopeFromCurrentContainer (the
function that iterates allContainers and checks c.Scope()) to return the empty
string "" for unscoped containers (i.e., when containerHasScope is false or
containerScope == "") so unscoped and unknown remain distinct, and ensure
cleanup normalization in cleanup.go continues to treat unscoped as "" so
explicit "none" scope stays valid.
---
Nitpick comments:
In `@pkg/filters/filters_test.go`:
- Around line 454-478: Add an integration assertion to the
TestBuildFilterNoneScope test that exercises BuildFilter for the legacy
Watchtower name: create a mock FilterableContainer where IsWatchtower returns
true and Name returns a string like "/watchtower-old-..." (and other required
mocks same as existing scoped/unscoped), then call the existing filter and
assert it returns false (excluded); this ensures BuildFilter's filter chain
properly excludes old-named Watchtower containers alongside the new predicates.
In `@pkg/filters/filters.go`:
- Around line 68-93: ExcludeOldNamedWatchtowerFilter duplicates the old-name
detection logic; replace its manual check with a call to IsOldNamedWatchtower to
centralize the predicate. Update ExcludeOldNamedWatchtowerFilter so it first
returns true when !c.IsWatchtower() (unchanged), then calls
IsOldNamedWatchtower(c) to decide exclusion, logging with
logrus.WithField("container", c.Name()).Debug(...) when IsOldNamedWatchtower(c)
is true and returning false; otherwise return true. Ensure IsOldNamedWatchtower
remains the single source of truth for the
strings.HasPrefix(strings.TrimLeft(c.Name(), "/"), types.WatchtowerOldPrefix)
check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 56215aea-f7a8-4ab5-938b-62542808e753
📒 Files selected for processing (12)
cmd/root.gointernal/actions/cleanup.gointernal/actions/errors.gointernal/actions/mocks/client.gointernal/actions/update.gointernal/actions/update_context_test.gointernal/scheduling/scheduling.gointernal/scheduling/scheduling_test.gopkg/container/container_id.gopkg/filters/filters.gopkg/filters/filters_test.gopkg/types/container.go
- Move ginkgo When block to top-level Describe scope from inside a standard Go test function to fix Ginkgo node registration - Change deriveScopeFromCurrentContainer to return found bool and "" for unscoped so unscoped and not-found remain distinct - Normalize "" to "none" at the caller before passing to cleanup - Add BuildFilter integration test asserting old-named Watchtower containers are excluded by the composed filter chain - Refactor ExcludeOldNamedWatchtowerFilter to delegate to IsOldNamedWatchtower for single-source-of-truth predicate
…old" terminology - Rename `IsOldNamedContainer` to `IsOldContainer` in container package - Rename `ExcludeOldNamedWatchtowerFilter` to `ExcludeOldWatchtowerFilter` and related functions - Rename `IsOldNamedWatchtower` to `IsOldWatchtower` in filters package - Rename `ExcludeOldNamedWatchtowerFilterChain` to `ExcludeOldWatchtowerFilterChain` - Rename `errOldNamedSelfDetected` to `errOldSelfDetected` in errors package - Update all call sites across actions, scheduling, and container packages - Update test names, log messages, and notification templates to match new terminology - Update doc.go function reference from `CheckForMultipleWatchtowerInstances` to `CheckForMultipleWatchtowerContainers`
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/actions/cleanup.go (1)
128-179:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScope parameter should be normalized to avoid missing unscoped containers.
The
containerScopeis normalized from empty string to"none"at line 168, but thescopeparameter is not normalized. If the caller passesscope=""(meaning unscoped), the comparison at line 171 will fail to match containers that have no scope label (which normalize to"none").Consider normalizing the scope parameter at the start of the function:
Proposed fix
func CleanupOldWatchtowerContainers( ctx context.Context, client container.Client, cleanupImages bool, scope string, currentContainerID types.ContainerID, removeImageInfos *[]types.RemovedImageInfo, ) (int, error) { + // Normalize empty scope to "none" to match container scope normalization + if scope == "" { + scope = "none" + } + logrus.WithFields(logrus.Fields{🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/actions/cleanup.go` around lines 128 - 179, The function CleanupOldWatchtowerContainers normalizes containerScope to "none" when a container has no scope label but does not normalize the incoming scope parameter, so a caller passing scope=="" won't match unscoped containers; fix by normalizing the scope parameter at the start of CleanupOldWatchtowerContainers (e.g., if scope == "" set scope = "none") before any scope comparisons (so the later check comparing containerScope to scope uses the same normalization).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/actions/cleanup.go`:
- Around line 128-179: The function CleanupOldWatchtowerContainers normalizes
containerScope to "none" when a container has no scope label but does not
normalize the incoming scope parameter, so a caller passing scope=="" won't
match unscoped containers; fix by normalizing the scope parameter at the start
of CleanupOldWatchtowerContainers (e.g., if scope == "" set scope = "none")
before any scope comparisons (so the later check comparing containerScope to
scope uses the same normalization).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 263e92e9-761f-47e4-98fa-2a156b965e2a
📒 Files selected for processing (15)
cmd/root.gointernal/actions/cleanup.gointernal/actions/cleanup_test.gointernal/actions/doc.gointernal/actions/errors.gointernal/actions/update.gointernal/actions/update_context_test.gointernal/scheduling/scheduling.gointernal/scheduling/scheduling_test.gopkg/container/container_id.gopkg/container/container_id_test.gopkg/filters/filters.gopkg/filters/filters_test.gopkg/notifications/common_templates.gopkg/notifications/preview/data/preview_strings.go
✅ Files skipped from review due to trivial changes (3)
- pkg/notifications/preview/data/preview_strings.go
- internal/actions/doc.go
- internal/actions/cleanup_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/actions/errors.go
- internal/scheduling/scheduling_test.go
- internal/actions/update_context_test.go
- internal/scheduling/scheduling.go
- internal/actions/update.go
- Add scope normalization in CleanupOldWatchtowerContainers to handle empty scope values
- Add tests covering no-container, non-Watchtower, and current-only scenarios - Include scope normalization tests for empty scope matching unscoped old containers - Add scope filtering tests to verify old containers in different scopes are skipped - Verify container removal behavior with mock client expectations
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR addresses a persistent issue where orphaned
watchtower-old-*containers survive cleanup, resist other safeguards, and continue update and self-update cycles. It adds multiple layers of defense to detect and stop old-named Watchtower instances.Problems
Solutions
Changes
Summary by CodeRabbit
Bug Fixes
New Features
Tests
Documentation