fix: prevent ghost pods.#1619
Open
AlexSCorey wants to merge 1 commit into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUnresponsive activation handling now persists ChangesActivation cleanup and failure handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ActivationManager
participant KubernetesEngine
participant KubernetesAPI
participant ActivationState
participant RestartScheduler
ActivationManager->>KubernetesEngine: Run cleanup
KubernetesEngine->>KubernetesAPI: Get resource_version and watch pod deletion
KubernetesAPI-->>KubernetesEngine: DELETED event or watch termination
KubernetesEngine-->>ActivationManager: Cleanup success or ContainerCleanupError
ActivationManager->>ActivationState: Persist FAILED status
ActivationManager->>RestartScheduler: Schedule restart when cleanup succeeds and policy allows
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/aap_eda/services/activation/engine/kubernetes.py (1)
592-612: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftAvoid starting the deletion watch after the delete request
The pod can disappear before this watch is established, so the
DELETEDevent is missed and cleanup falls through toContainerCleanupError. Seed the watch from a pre-deletelist_namespaced_podresourceVersion(or use another list-then-watch handoff) so deletion cannot be skipped.🤖 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 `@src/aap_eda/services/activation/engine/kubernetes.py` around lines 592 - 612, The pod deletion watch must be established from a pre-delete snapshot so a fast deletion cannot be missed. Update the cleanup flow around watcher.stream and list_namespaced_pod to list the target pod before issuing the delete request, capture its resourceVersion, and start the watch from that version using the Kubernetes list-then-watch handoff; preserve the existing pod_deleted logging and successful return behavior.Source: Path instructions
🤖 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 `@src/aap_eda/services/activation/activation_manager.py`:
- Around line 393-397: Update _cleanup to support a propagating mode that
re-raises ContainerCleanupError, while preserving best-effort behavior for
existing callers. Have _handle_unresponsive_activation invoke this mode so
cleanup failures remain observable and prevent
_fail_instance/_unresponsive_policy from scheduling a restart. Update the
relevant tests to make container_engine.cleanup fail through the real _cleanup
implementation rather than mocking around it.
---
Outside diff comments:
In `@src/aap_eda/services/activation/engine/kubernetes.py`:
- Around line 592-612: The pod deletion watch must be established from a
pre-delete snapshot so a fast deletion cannot be missed. Update the cleanup flow
around watcher.stream and list_namespaced_pod to list the target pod before
issuing the delete request, capture its resourceVersion, and start the watch
from that version using the Kubernetes list-then-watch handoff; preserve the
existing pod_deleted logging and successful return behavior.
🪄 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: Enterprise
Run ID: 0996f425-1602-4e22-bebc-a91aa848a9d7
📒 Files selected for processing (4)
src/aap_eda/services/activation/activation_manager.pysrc/aap_eda/services/activation/engine/kubernetes.pytests/integration/services/activation/engine/test_kubernetes.pytests/integration/services/activation/test_manager.py
AlexSCorey
force-pushed
the
81201_ghost_activations
branch
3 times, most recently
from
July 16, 2026 19:35
f912295 to
c99cdb4
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #1619 +/- ##
==========================================
+ Coverage 92.75% 92.89% +0.13%
==========================================
Files 244 244
Lines 11346 11368 +22
==========================================
+ Hits 10524 10560 +36
+ Misses 822 808 -14
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
AlexSCorey
force-pushed
the
81201_ghost_activations
branch
from
July 16, 2026 20:41
c99cdb4 to
0baa2c3
Compare
Member
Author
|
/run-atf-tests |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/aap_eda/services/activation/engine/kubernetes.py (1)
641-641: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse conversion flag instead of
str()in f-string.Ruff RUF010: replace
f"Error during cleanup: {str(exc)}"withf"Error during cleanup: {exc!s}".🤖 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 `@src/aap_eda/services/activation/engine/kubernetes.py` at line 641, Update the cleanup error message to use the f-string conversion flag !s for exc instead of calling str(exc) explicitly, preserving the existing “Error during cleanup” message and behavior.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In `@src/aap_eda/services/activation/engine/kubernetes.py`:
- Line 641: Update the cleanup error message to use the f-string conversion flag
!s for exc instead of calling str(exc) explicitly, preserving the existing
“Error during cleanup” message and behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: c323c4fe-50c1-4f1d-b719-3a5b19ec734a
📒 Files selected for processing (4)
src/aap_eda/services/activation/activation_manager.pysrc/aap_eda/services/activation/engine/kubernetes.pytests/integration/services/activation/engine/test_kubernetes.pytests/integration/services/activation/test_manager.py
🚧 Files skipped from review as they are similar to previous changes (3)
- src/aap_eda/services/activation/activation_manager.py
- tests/integration/services/activation/engine/test_kubernetes.py
- tests/integration/services/activation/test_manager.py
❌ Test Results - FAILEDSummary
Pass Rate: 74.2% ❌ Failed Tests
|
…tart
When the monitor detects an unresponsive activation and triggers a
restart, _watch_pod_deletion falsely reports success if the watch
stream times out without receiving a DELETED event. This allows
_unresponsive_policy to restart the activation while the original
pod is still running, producing ghost pods that compete for shared
resources like Kafka consumer groups.
Two fixes:
1. _watch_pod_deletion now tracks whether a DELETED event was actually
received. If the watch times out without one, it retries instead of
reporting success. After all retries, it raises ContainerCleanupError.
2. _unresponsive_policy now calls _cleanup explicitly via a new
_handle_unresponsive_activation helper. If cleanup fails, the
restart is skipped to avoid creating a duplicate pod. The activation
is still marked FAILED regardless of cleanup outcome.
3. adds a list_then_watch flow for deleting a pod so that we don't loose track
of the pods we are trying to delete
Fixes: AAP-81201
AlexSCorey
force-pushed
the
81201_ghost_activations
branch
from
July 17, 2026 12:52
0baa2c3 to
1a58669
Compare
|
Member
Author
|
/run-atf-tests |
❌ Test Results - FAILEDSummary
Pass Rate: 50.0% ❌ Failed Tests
|
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
3 participants
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.
When the monitor detects an unresponsive activation and triggers a
restart, _watch_pod_deletion falsely reports success if the watch
stream times out without receiving a DELETED event. This allows
_unresponsive_policy to restart the activation while the original
pod is still running, producing ghost pods that compete for shared
resources like Kafka consumer groups.
Two fixes:
_watch_pod_deletion now tracks whether a DELETED event was actually
received. If the watch times out without one, it retries instead of
reporting success. After all retries, it raises ContainerCleanupError.
_unresponsive_policy now calls _cleanup explicitly via a new
_handle_unresponsive_activation helper. If cleanup fails, the
restart is skipped to avoid creating a duplicate pod. The activation
is still marked FAILED regardless of cleanup outcome.
Fixes: AAP-83199
Summary by CodeRabbit