fix: prevent ghost pods.#1619
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
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:
|
c99cdb4 to
0baa2c3
Compare
|
/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
0baa2c3 to
1a58669
Compare
|
|
/run-atf-tests |
❌ Test Results - FAILEDSummary
Pass Rate: 50.0% ❌ Failed Tests
|
|
/run-atf-tests |
❌ Test Results - FAILEDSummary
Pass Rate: 50.0% ❌ Failed Tests
|
|
/run-atf-tests |
| after the activation is marked FAILED. | ||
| """ | ||
| try: | ||
| self._cleanup() |
There was a problem hiding this comment.
[Critical] _cleanup() (line 294) catches ContainerCleanupError internally and returns normally — it never lets the exception escape. So this try block will never see a ContainerCleanupError, the except in _unresponsive_policy (line 382) will never trigger, and system_restart_activation will always be called when the restart policy allows it — even when the pod is still running.
Consider either calling self.container_engine.cleanup() directly here (bypassing _cleanup's exception swallowing), or adding a raise_on_error parameter to _cleanup() so this caller can opt in to propagation while preserving best-effort behavior for existing callers.
| LOGGER.info(log_message) | ||
|
|
||
| container_logger.write(user_msg, flush=True) | ||
| self._fail_instance(msg=user_msg) |
There was a problem hiding this comment.
[Major] _fail_instance() calls _cleanup() again internally (line 651). Since _handle_unresponsive_activation already called _cleanup() at line 403, this results in two cleanup round-trips to the Kubernetes API per unresponsive event. If the first cleanup succeeded, the second one hits the API for already-deleted resources.
| if cleanup_calls == 1: | ||
| raise engine_exceptions.ContainerCleanupError("cleanup failed") | ||
|
|
||
| with patch.object( |
There was a problem hiding this comment.
[Major] All new manager tests mock _cleanup at the ActivationManager level, replacing it with a side_effect that raises ContainerCleanupError. But the real _cleanup() catches that exception internally and never raises it (line 294 of activation_manager.py). These tests verify the orchestration logic in isolation but don't exercise the production path — they give false confidence that the ghost pod scenario is prevented.
| user_msg += " Restart policy is not applicable." | ||
| else: | ||
| log_message += " Restart policy is applied." | ||
| user_msg += " Activation is going to be restarted." |
There was a problem hiding this comment.
[Minor] This message says "Activation is going to be restarted" but it's in the finally block, which runs regardless of whether cleanup succeeded. If the _cleanup propagation issue is fixed so that _unresponsive_policy can skip the restart, this message will still promise a restart that won't happen.
| return False | ||
|
|
||
| def _wait_for_pod_to_start(self, _log_handler: LogHandler) -> None: | ||
| def _wait_for_pod_to_start(self, log_handler: LogHandler) -> None: |
There was a problem hiding this comment.
[Minor] The parameter was renamed from _log_handler to log_handler, removing the "unused" underscore convention, but it's still not used in the method body. This signals to readers that the parameter is now used when it isn't.
| K8S_API_RETRIES = 3 | ||
| K8S_API_RETRY_BACKOFF = 1.0 | ||
| K8S_API_TRANSIENT_STATUS_CODES = {401, 403, 500, 502, 503, 504} | ||
| K8S_API_TRANSIENT_STATUS_CODES = {401, 403, 410, 500, 502, 503, 504} |
There was a problem hiding this comment.
[Nit] Adding 410 (Gone) is correct for the watch use case (stale resourceVersion), but K8S_API_TRANSIENT_STATUS_CODES is also used by _call_k8s_api which wraps all K8s API calls — not just watches. A 410 from a non-watch endpoint would now be silently retried. Low risk in practice since 410 is rare outside watch contexts, but worth noting the broader retry surface.
| self._log_and_backoff(exc, attempt, desc) | ||
| finally: | ||
| watcher.stop() | ||
| raise ContainerCleanupError( |
There was a problem hiding this comment.
[Minor] When all retries are exhausted due to pure timeouts (the watch stream ends without a DELETED event, no ApiException raised), last_exc is still None. This produces the message "watch pod deletion test-job failed after 3 retries: None". Consider a clearer message distinguishing timeout exhaustion from API errors, e.g. "timed out waiting for pod deletion after {K8S_API_RETRIES} attempts".
4 participants
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