Use broader exception to ensure things are properly deleted#48
Open
brucetony wants to merge 2 commits into
Open
Use broader exception to ensure things are properly deleted#48brucetony wants to merge 2 commits into
brucetony wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughAdds bounded pod-IP waits and a shared _IP_WAIT_TIMEOUT, centralizes Kubernetes Deployment deletion and standardizes resource delete error handling, makes Analysis.start persist STARTING and mark FAILED on deployment errors, and adds a STARTING timeout with reconciler logic to detect stuck analyses. ChangesOrchestration lifecycle updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![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/k8s/kubernetes.py (1)
358-366:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winPod IP wait loops in
_create_nginx_config_mapcheck pod existence instead of IP availability.Both the message broker wait loop (Lines 358-366) and the pod orchestration wait loop (Lines 380-392) check
while pod is None:when they should checkwhile pod_ip is None:. The pod object can exist withstatus.pod_ip = Nonebefore the pod is scheduled or started. This causes both loops to exit before the IP is available, leading toNonebeing used in the nginx configuration template (Lines 517-518), which breaks the configuration. The shared root cause is the incorrect loop condition in both waits; each must loop until the IP is available, not just until the pod object exists.🤖 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/k8s/kubernetes.py` around lines 358 - 366, In _create_nginx_config_map the wait loops currently test the pod object existence (e.g., while message_broker_pod is None / while pod is None) but must instead wait for the pod IP to be populated; change the loops to check the IP variable (e.g., while message_broker_ip is None and while pod_ip is None), call core_client.read_namespaced_pod each iteration to refresh status, set message_broker_ip = message_broker_pod.status.pod_ip (and pod_ip = pod.status.pod_ip) after the read, and keep the same timeout logic so the loop only exits when a non-None pod IP is available for use in the nginx template.
🧹 Nitpick comments (2)
src/k8s/kubernetes.py (1)
385-389: ⚡ Quick winReplace bare
except:with specific exception handling.The bare
except:clause catches all exceptions includingSystemExitandKeyboardInterrupt, and masks the original exception details. The error message assumes a "not found" scenario, but other failures (network issues, API errors) would also be caught here with a misleading message.♻️ Proposed fix
try: pod_orchestration_pod = core_client.read_namespaced_pod(name=pod_orchestration_name, namespace=namespace) - except: + except client.exceptions.ApiException as e: + if e.reason == 'Not Found': - raise ValueError(f"Could not find pod orchestration pod with name {pod_orchestration_name} in namespace {namespace}. ") + raise ValueError(f"Could not find pod orchestration pod with name {pod_orchestration_name} in namespace {namespace}") + logger.error(f"API error reading pod orchestration pod: {repr(e)}") + raise + except Exception as e: + logger.error(f"Unexpected error reading pod orchestration pod: {repr(e)}") + raise🤖 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/k8s/kubernetes.py` around lines 385 - 389, Replace the bare except in the read_namespaced_pod call with specific exception handling: catch the Kubernetes API exception (e.g., kubernetes.client.exceptions.ApiException) to handle 404 "not found" separately from other errors, include the original exception details in the raised ValueError or re-raise non-404 exceptions; reference core_client.read_namespaced_pod, pod_orchestration_pod, pod_orchestration_name and namespace so you can return a clear "not found" message for status code 404 and propagate or log other ApiException/connection errors with the original exception info.src/resources/analysis/entity.py (1)
72-79: ⚡ Quick winConsider logging the exception before re-raising.
The exception handler correctly marks the deployment as FAILED, but doesn't log the exception details. Adding a log statement would improve observability by capturing the failure at the point of detection.
📝 Proposed enhancement
try: self.pod_ids = create_analysis_deployment(name=self.deployment_name, image=self.image_url, env=self.analysis_config, namespace=namespace) - except Exception: + except Exception as e: + logger.error(f"Failed to create analysis deployment {self.deployment_name}: {repr(e)}") database.update_deployment_status(self.deployment_name, AnalysisStatus.FAILED.value) raise🤖 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/resources/analysis/entity.py` around lines 72 - 79, Add an exception log before re-raising in the create_analysis_deployment block: when create_analysis_deployment(...) fails, call a logger (preferably self.logger.exception(...) if the class has one, otherwise logging.exception(...)) to record the exception and context (include self.deployment_name, image_url, namespace) immediately before calling database.update_deployment_status(self.deployment_name, AnalysisStatus.FAILED.value) and re-raising the exception; this keeps the existing status update and re-raise behavior but captures the stacktrace and useful identifiers for debugging.
🤖 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/status/status.py`:
- Around line 213-221: When handling AnalysisStatus.STARTING in
src/status/status.py, explicitly check if analysis.time_created is None before
computing elapsed; if None, log a warning including analysis_id about missing
time_created (data integrity), then treat it as stuck by setting db_status =
AnalysisStatus.STARTED.value and int_status = AnalysisStatus.FAILED.value (same
as the timeout branch); otherwise compute elapsed = time.time() -
analysis.time_created and keep the existing _STARTING_TIMEOUT logic.
---
Outside diff comments:
In `@src/k8s/kubernetes.py`:
- Around line 358-366: In _create_nginx_config_map the wait loops currently test
the pod object existence (e.g., while message_broker_pod is None / while pod is
None) but must instead wait for the pod IP to be populated; change the loops to
check the IP variable (e.g., while message_broker_ip is None and while pod_ip is
None), call core_client.read_namespaced_pod each iteration to refresh status,
set message_broker_ip = message_broker_pod.status.pod_ip (and pod_ip =
pod.status.pod_ip) after the read, and keep the same timeout logic so the loop
only exits when a non-None pod IP is available for use in the nginx template.
---
Nitpick comments:
In `@src/k8s/kubernetes.py`:
- Around line 385-389: Replace the bare except in the read_namespaced_pod call
with specific exception handling: catch the Kubernetes API exception (e.g.,
kubernetes.client.exceptions.ApiException) to handle 404 "not found" separately
from other errors, include the original exception details in the raised
ValueError or re-raise non-404 exceptions; reference
core_client.read_namespaced_pod, pod_orchestration_pod, pod_orchestration_name
and namespace so you can return a clear "not found" message for status code 404
and propagate or log other ApiException/connection errors with the original
exception info.
In `@src/resources/analysis/entity.py`:
- Around line 72-79: Add an exception log before re-raising in the
create_analysis_deployment block: when create_analysis_deployment(...) fails,
call a logger (preferably self.logger.exception(...) if the class has one,
otherwise logging.exception(...)) to record the exception and context (include
self.deployment_name, image_url, namespace) immediately before calling
database.update_deployment_status(self.deployment_name,
AnalysisStatus.FAILED.value) and re-raising the exception; this keeps the
existing status update and re-raise behavior but captures the stacktrace and
useful identifiers for debugging.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ec21e33e-bbf3-4726-a1c5-870b1adff11a
📒 Files selected for processing (4)
src/k8s/kubernetes.pysrc/resources/analysis/entity.pysrc/status/constants.pysrc/status/status.py
✅ Files skipped from review due to trivial changes (1)
- src/status/constants.py
Comment on lines
+213
to
+221
| if db_status == AnalysisStatus.STARTING.value: | ||
| elapsed = time.time() - (analysis.time_created or 0) | ||
| if elapsed < _STARTING_TIMEOUT: # 360s / 6min | ||
| int_status = AnalysisStatus.STARTING.value | ||
| else: | ||
| # Safety net: creation took too long treat as STARTED+FAILED to trigger unstuck | ||
| logger.warning(f"Analysis {analysis_id} stuck in STARTING for {elapsed:.0f}s — triggering unstuck") | ||
| db_status = AnalysisStatus.STARTED.value | ||
| int_status = AnalysisStatus.FAILED.value |
There was a problem hiding this comment.
Handle None value for time_created more defensively.
The expression analysis.time_created or 0 uses 0 as a fallback if time_created is None. This would make elapsed = time.time() - 0 evaluate to the current Unix timestamp (~1.7 billion seconds), immediately triggering the timeout.
While the database creation code sets time_created=time.time(), so this should not happen in normal operation, a more defensive approach would explicitly check for None and either log a warning or treat it as a data integrity error.
🛡️ Proposed fix
db_status = analysis.status
if db_status == AnalysisStatus.STARTING.value:
- elapsed = time.time() - (analysis.time_created or 0)
+ if analysis.time_created is None:
+ logger.error(f"Analysis {analysis_id} missing time_created — treating as timed out")
+ elapsed = _STARTING_TIMEOUT + 1 # Force timeout path
+ else:
+ elapsed = time.time() - analysis.time_created
if elapsed < _STARTING_TIMEOUT: # 360s / 6min
int_status = AnalysisStatus.STARTING.value
else:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if db_status == AnalysisStatus.STARTING.value: | |
| elapsed = time.time() - (analysis.time_created or 0) | |
| if elapsed < _STARTING_TIMEOUT: # 360s / 6min | |
| int_status = AnalysisStatus.STARTING.value | |
| else: | |
| # Safety net: creation took too long treat as STARTED+FAILED to trigger unstuck | |
| logger.warning(f"Analysis {analysis_id} stuck in STARTING for {elapsed:.0f}s — triggering unstuck") | |
| db_status = AnalysisStatus.STARTED.value | |
| int_status = AnalysisStatus.FAILED.value | |
| if db_status == AnalysisStatus.STARTING.value: | |
| if analysis.time_created is None: | |
| logger.error(f"Analysis {analysis_id} missing time_created — treating as timed out") | |
| elapsed = _STARTING_TIMEOUT + 1 # Force timeout path | |
| else: | |
| elapsed = time.time() - analysis.time_created | |
| if elapsed < _STARTING_TIMEOUT: # 360s / 6min | |
| int_status = AnalysisStatus.STARTING.value | |
| else: | |
| # Safety net: creation took too long treat as STARTED+FAILED to trigger unstuck | |
| logger.warning(f"Analysis {analysis_id} stuck in STARTING for {elapsed:.0f}s — triggering unstuck") | |
| db_status = AnalysisStatus.STARTED.value | |
| int_status = AnalysisStatus.FAILED.value |
🤖 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/status/status.py` around lines 213 - 221, When handling
AnalysisStatus.STARTING in src/status/status.py, explicitly check if
analysis.time_created is None before computing elapsed; if None, log a warning
including analysis_id about missing time_created (data integrity), then treat it
as stuck by setting db_status = AnalysisStatus.STARTED.value and int_status =
AnalysisStatus.FAILED.value (same as the timeout branch); otherwise compute
elapsed = time.time() - analysis.time_created and keep the existing
_STARTING_TIMEOUT logic.
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.
If there is an unhandled error (i.e. an error other than ApiException) during
delete_deployment()then the rest of the resources fail to get deletedSummary by CodeRabbit
Bug Fixes
New Features