Conversation
When a user runs `kubectl apply -f` with a modified spec, Kubernetes increments metadata.generation. The operator now tracks status.observedGeneration and re-executes the health check whenever generation != observedGeneration, giving one-shot-per-apply semantics. Changes: - Add observedGeneration field to CRD status schema and Pydantic model - Extract shared _execute_healthcheck() from create/update handlers - on.create sets observedGeneration after execution - on.update re-runs when generation != observedGeneration (spec change) - Preserve existing annotation-based rerun as fallback for same-spec reruns - Add tests for generation-based trigger, skip, and annotation fallback https://claude.ai/code/session_01QY6zsEHPW9CfSKJBtE6WCd Signed-off-by: Claude <noreply@anthropic.com>
After the operator processes a rerun annotation, it now patches the resource to remove the annotation (set to null). This lets users simply re-add the annotation to trigger another run, without needing to manually remove and re-add it. https://claude.ai/code/session_01QY6zsEHPW9CfSKJBtE6WCd Signed-off-by: Claude <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
@claude review |
WalkthroughAdded observedGeneration to HealthCheck status and wired generation- and annotation-based re-execution: CRD, data model, status utilities, handlers updated; create/update handlers now delegate execution to a shared _execute_healthcheck and clear the rerun annotation after use. Changes
Sequence DiagramsequenceDiagram
participant K8s as Kubernetes API
participant Handler as HealthCheck Handler
participant Holmes as Holmes API
participant StatusAPI as Status Patch API
K8s->>Handler: create/update event (body, metadata.generation)
Handler->>Handler: read status.observedGeneration\ncheck holmesgpt.dev/rerun annotation
alt generation changed OR rerun annotation present
Handler->>Handler: call _execute_healthcheck(spec, name, ns, uid, generation, body)
Handler->>Holmes: invoke Holmes API (check execution)
Holmes-->>Handler: result / error
Handler->>StatusAPI: patch status (include observedGeneration = generation)
StatusAPI->>K8s: apply status patch
alt rerun annotation was present
Handler->>K8s: patch to clear holmesgpt.dev/rerun annotation
K8s->>Handler: patch result
end
else no action
Handler-->>K8s: skip execution
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
✅ Deploy Preview for holmes-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📂 Previous Runs📜 #4 · Run @ __fb882c1__ (#23814824032) — Mar 31, 19:27 UTC✅ Results of HolmesGPT evalsAutomatically triggered by commit fb882c1 on branch Results of HolmesGPT evals
Benchmark Comparison DetailsBaseline: latest ci-benchmark experiment on master Status: Success - 27 test/model combinations loaded Benchmark experiment:
No benchmark data available for comparison. Benchmark has no cost, total tokens, cached tokens data. Will appear after the next weekly benchmark run. Comparison indicators:
📜 #3 · Run @ __5803c3f__ (#23814426380) — Mar 31, 19:17 UTC✅ Results of HolmesGPT evalsAutomatically triggered by commit 5803c3f on branch Results of HolmesGPT evals
Benchmark Comparison DetailsBaseline: latest ci-benchmark experiment on master Status: Success - 114 test/model combinations loaded Benchmark experiment:
No benchmark data available for comparison. Benchmark has no cost, total tokens, cached tokens data. Will appear after the next weekly benchmark run. Comparison indicators:
📜 #2 · Run @ __ddf309c__ (#23814230173) — Mar 31, 19:02 UTC✅ Results of HolmesGPT evalsAutomatically triggered by commit ddf309c on branch Results of HolmesGPT evals
Benchmark Comparison DetailsBaseline: latest ci-benchmark experiment on master Status: Success - 114 test/model combinations loaded Benchmark experiment:
No benchmark data available for comparison. Benchmark has no cost, total tokens, cached tokens data. Will appear after the next weekly benchmark run. Comparison indicators:
📜 #1 · Run @ __29546b1__ (#23812193213) — Mar 31, 18:12 UTC✅ Results of HolmesGPT evalsAutomatically triggered by commit 29546b1 on branch Results of HolmesGPT evals
Benchmark Comparison DetailsBaseline: latest ci-benchmark experiment on master Status: Success - 114 test/model combinations loaded Benchmark experiment:
No benchmark data available for comparison. Benchmark has no cost, total tokens, cached tokens data. Will appear after the next weekly benchmark run. Comparison indicators:
✅ Results of HolmesGPT evalsAutomatically triggered by commit ee321a6 on branch Results of HolmesGPT evals
Benchmark Comparison DetailsBaseline: latest ci-benchmark experiment on master Status: Success - 44 test/model combinations loaded Benchmark experiment:
No benchmark data available for comparison. Benchmark has no cost, total tokens, cached tokens data. Will appear after the next weekly benchmark run. Comparison indicators:
📖 Legend
🔄 Re-run evals manually
Option 1: Comment on this PR with Or with more options (one per line): Run evals on a different branch (e.g., master) for comparison:
Quick re-run: Use Option 2: Trigger via GitHub Actions UI → "Run workflow" Option 3: Add PR labels to include extra evals (applies to both automatic runs and
Examples: 🏷️ Valid tags
🤖 Valid models
Commands: CLI: |
|
✅ Docker images ready for
Use these tags to pull the images for testing. 📋 Copy commandsgcloud auth configure-docker us-central1-docker.pkg.dev
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:b6594ed2
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:b6594ed2 me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:b6594ed2
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:b6594ed2
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:b6594ed2
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:b6594ed2 me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:b6594ed2
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:b6594ed2Patch Helm values in one line (choose the chart you use): HolmesGPT chart: helm upgrade --install holmesgpt ./helm/holmes \
--set registry=me-west1-docker.pkg.dev/robusta-development/development \
--set image=holmes-dev:b6594ed2 \
--set operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set operator.image=holmes-operator-dev:b6594ed2Robusta wrapper chart: helm upgrade --install robusta robusta/robusta \
--reuse-values \
--set holmes.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.image=holmes-dev:b6594ed2 \
--set holmes.operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.operator.image=holmes-operator-dev:b6594ed2 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/holmes_operator/test_healthcheck_component.py (1)
81-95: Consider adding type hints to the helper function.The helper function lacks type hints which are required per coding guidelines.
📝 Suggested fix
-def _make_body(name, namespace, uid, spec, generation=1): +def _make_body( + name: str, namespace: str, uid: str, spec: dict, generation: int = 1 +) -> dict: """Helper to build a HealthCheck resource body with metadata.generation."""As per coding guidelines:
**/*.py: Type hints are required throughout the codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/holmes_operator/test_healthcheck_component.py` around lines 81 - 95, The helper function _make_body lacks type hints; update its signature to add parameter and return type annotations (e.g., name: str, namespace: str, uid: str, spec: Dict[str, Any] or Mapping[str, Any], generation: int = 1) and annotate the return as Dict[str, Any]; also add any required typing imports (from typing import Any, Dict, Mapping) at the top of the module so the function and its returned dictionary conform to the project's type-hinting guidelines.holmes_operator/utils.py (1)
34-53: Consider updating the docstring to document the new parameter.The
observed_generationparameter is added but not documented in the Args section of the docstring.📝 Suggested docstring update
Args: api: Kubernetes CustomObjectsApi instance name: Name of the HealthCheck resource namespace: Namespace of the HealthCheck resource phase: Execution phase (Pending, Running, Completed, Failed) result: Check result (pass, fail, error) message: Human-readable summary rationale: LLM explanation duration: Execution duration in seconds error: Error details model_used: Model that was used notifications: List of notification statuses start_time: ISO format start time completion_time: ISO format completion time + observed_generation: Generation to record as processed """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes_operator/utils.py` around lines 34 - 53, The docstring for the function that updates HealthCheck status (update_healthcheck_status) is missing documentation for the new observed_generation parameter; update the Args section to add an entry for observed_generation (type Optional[int]) describing it as the resource's observedGeneration value used to indicate the controller's observed revision of the HealthCheck, and note that it should be set when reporting status to help reconcile loops detect changes.holmes_operator/handlers/healthcheck.py (1)
176-183: Consider using f-string conversion flag.Static analysis suggests using explicit conversion flag instead of
str(e).📝 Suggested fix
- message=f"Operator error: {str(e)}", - error=str(e), + message=f"Operator error: {e!s}", + error=f"{e!s}",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes_operator/handlers/healthcheck.py` around lines 176 - 183, Replace explicit str(e) calls with f-string conversion flags to make formatting clearer: update the f"Operator error: {str(e)}" to use f"Operator error: {e!s}" and change error=str(e) to error=f"{e!s}" in the set_healthcheck_failed call so the exception is converted via the f-string conversion flag; this touches the set_healthcheck_failed invocation in the healthcheck handler where variables name, namespace, generation and context.k8s_api are passed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@holmes_operator/handlers/healthcheck.py`:
- Around line 176-183: Replace explicit str(e) calls with f-string conversion
flags to make formatting clearer: update the f"Operator error: {str(e)}" to use
f"Operator error: {e!s}" and change error=str(e) to error=f"{e!s}" in the
set_healthcheck_failed call so the exception is converted via the f-string
conversion flag; this touches the set_healthcheck_failed invocation in the
healthcheck handler where variables name, namespace, generation and
context.k8s_api are passed.
In `@holmes_operator/utils.py`:
- Around line 34-53: The docstring for the function that updates HealthCheck
status (update_healthcheck_status) is missing documentation for the new
observed_generation parameter; update the Args section to add an entry for
observed_generation (type Optional[int]) describing it as the resource's
observedGeneration value used to indicate the controller's observed revision of
the HealthCheck, and note that it should be set when reporting status to help
reconcile loops detect changes.
In `@tests/holmes_operator/test_healthcheck_component.py`:
- Around line 81-95: The helper function _make_body lacks type hints; update its
signature to add parameter and return type annotations (e.g., name: str,
namespace: str, uid: str, spec: Dict[str, Any] or Mapping[str, Any], generation:
int = 1) and annotate the return as Dict[str, Any]; also add any required typing
imports (from typing import Any, Dict, Mapping) at the top of the module so the
function and its returned dictionary conform to the project's type-hinting
guidelines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ace34798-353e-4e34-a44c-01e033d8269a
📒 Files selected for processing (5)
helm/holmes/crds/healthcheck.yamlholmes_operator/handlers/healthcheck.pyholmes_operator/models.pyholmes_operator/utils.pytests/holmes_operator/test_healthcheck_component.py
- deployment-verification.md: Change example to use fixed check name with version in query (not name), explain auto re-run on spec change, update tips section - health-checks.md: Expand "Re-running Checks" section to document both spec-change trigger and annotation trigger, add observedGeneration to status fields reference https://claude.ai/code/session_01QY6zsEHPW9CfSKJBtE6WCd Signed-off-by: Claude <noreply@anthropic.com>
Kept remote's open-ended query style but added version reference (v2.4.1) so the query naturally changes between deploys, triggering the generation-based re-execution. Updated CI/CD gating script to use fixed check name. https://claude.ai/code/session_01QY6zsEHPW9CfSKJBtE6WCd Signed-off-by: Claude <noreply@anthropic.com>
Instead of requiring users to template version strings into queries or use run-id annotations, the operator now clears holmesgpt.dev/rerun on create too (not just update). This enables a simple toggle pattern: 1. Manifest includes holmesgpt.dev/rerun: "true" 2. Operator runs check, clears annotation 3. Next kubectl apply restores annotation from manifest → re-run Works with Helm, ArgoCD, or plain manifests with zero templating. Reverted the unused lastRunId/run-id approach. Updated docs with Helm and ArgoCD examples. https://claude.ai/code/session_01QY6zsEHPW9CfSKJBtE6WCd Signed-off-by: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/operator/deployment-verification.md (1)
81-87: Helpful tips section with clear guidance.The updated tips correctly guide users toward the fixed-name pattern with query updates for most cases, while noting that versioned names remain an option for audit trails. The annotation-based manual re-run is accurately documented as an alternative.
Optional style refinement: The phrase "exact same" on line 87 could be simplified to "same" for conciseness, but the current wording is clear.
Minor wording refinement
-- **Force re-run without spec changes:** If you need to re-run the exact same check, use `kubectl annotate hc/checkout-api-deploy-check holmesgpt.dev/rerun=true`. The annotation is cleared automatically after execution. +- **Force re-run without spec changes:** If you need to re-run the same check, use `kubectl annotate hc/checkout-api-deploy-check holmesgpt.dev/rerun=true`. The annotation is cleared automatically after execution.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/operator/deployment-verification.md` around lines 81 - 87, In the "Force re-run without spec changes:" tip, simplify the phrase "re-run the exact same check" to "re-run the same check" for conciseness; update the sentence that follows the header (the one that references the annotation example `kubectl annotate hc/checkout-api-deploy-check holmesgpt.dev/rerun=true`) to use "same" instead of "exact same" and keep the rest of the line and example unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/operator/deployment-verification.md`:
- Around line 81-87: In the "Force re-run without spec changes:" tip, simplify
the phrase "re-run the exact same check" to "re-run the same check" for
conciseness; update the sentence that follows the header (the one that
references the annotation example `kubectl annotate hc/checkout-api-deploy-check
holmesgpt.dev/rerun=true`) to use "same" instead of "exact same" and keep the
rest of the line and example unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c41c2dcf-14f6-4fb5-b2fd-c0705f576a51
📒 Files selected for processing (2)
docs/operator/deployment-verification.mddocs/operator/health-checks.md
- Use mychart.fullname helper instead of .Release.Name for the Deployment name reference (release name != app Deployment name) - Add namespace to Helm and ArgoCD examples - Add labels block to Helm template - Minor wording improvements https://claude.ai/code/session_01QY6zsEHPW9CfSKJBtE6WCd Signed-off-by: Claude <noreply@anthropic.com>
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)
docs/operator/deployment-verification.md (1)
107-123:⚠️ Potential issue | 🟠 MajorWait for the new generation before trusting
.status.result.This example now reuses a fixed
HealthCheckname, so the first poll afterkubectl applycan still read the previous run'spass/failbefore the operator has started the new execution. Gate onstatus.observedGeneration == metadata.generation(or a changed completion timestamp) before acting onstatus.result.🧪 Suggested update
+# Capture the generation created by this apply +TARGET_GEN=$(kubectl get hc checkout-api-deploy-check -n production -o jsonpath='{.metadata.generation}') + # Wait for the check to complete, then read the result for i in $(seq 1 30); do + OBSERVED_GEN=$(kubectl get hc checkout-api-deploy-check -n production -o jsonpath='{.status.observedGeneration}' 2>/dev/null) RESULT=$(kubectl get hc checkout-api-deploy-check -n production -o jsonpath='{.status.result}' 2>/dev/null) - if [ "$RESULT" = "pass" ]; then + if [ "$OBSERVED_GEN" = "$TARGET_GEN" ] && [ "$RESULT" = "pass" ]; then echo "Deploy verified healthy" exit 0 - elif [ "$RESULT" = "fail" ] || [ "$RESULT" = "error" ]; then + elif [ "$OBSERVED_GEN" = "$TARGET_GEN" ] && { [ "$RESULT" = "fail" ] || [ "$RESULT" = "error" ]; }; then echo "Deploy check failed:" kubectl get hc checkout-api-deploy-check -n production -o jsonpath='{.status.message}' exit 1 fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/operator/deployment-verification.md` around lines 107 - 123, The poll loop can read a previous HealthCheck run's result; modify the logic that queries the HealthCheck (named checkout-api-deploy-check) to first fetch and compare status.observedGeneration with metadata.generation (or alternatively wait for a changed status.completedAt) and only then consider status.result; in practice, update the loop that reads kubectl get hc ... -o jsonpath='{.status.result}' to first retrieve both metadata.generation and status.observedGeneration and continue sleeping until they match (or until completedAt advances) before evaluating status.result for "pass"/"fail"/"error".
🧹 Nitpick comments (1)
docs/operator/deployment-verification.md (1)
3-14: Document the ArgoCD drift caveat for this pattern.Because the operator removes
holmesgpt.dev/rerunfrom the live object, GitOps controllers will see this resource as drifted after every run. In ArgoCD withselfHealenabled, that can turn into a sync/rerun loop instead of “once per deploy”, so this section should mention either ignoring diffs for this annotation or limiting the pattern to manual/commit-driven syncs.Also applies to: 84-103
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/operator/deployment-verification.md` around lines 3 - 14, Update the paragraph about the holmesgpt.dev/rerun annotation to add an ArgoCD caveat: note that because the operator clears the holmesgpt.dev/rerun annotation from the live object, GitOps controllers (ArgoCD) will detect drift and—if selfHeal is enabled—may enter a sync loop; instruct users to either add an ArgoCD ignoreDifferences rule for the holmesgpt.dev/rerun annotation (resource.customizations or argocd-cm diff/ignore settings) or restrict this pattern to manual/commit-driven syncs (disable selfHeal for the resource) to avoid repeated re-runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@holmes_operator/handlers/healthcheck.py`:
- Around line 236-250: The post-execution cleanup that clears the
holmesgpt.dev/rerun annotation is skipped if _execute_healthcheck raises or if
the generation-mismatch early return occurs; wrap the call to
_execute_healthcheck (and any surrounding logic) in a try/finally so
_clear_rerun_annotation(name=..., namespace=..., logger=...) is always awaited
in the finally block, and modify the generation-mismatch branch inside the same
handler (the update handler around lines handling generation and returning
early) to call _clear_rerun_annotation before returning so the annotation is
removed in all paths.
---
Outside diff comments:
In `@docs/operator/deployment-verification.md`:
- Around line 107-123: The poll loop can read a previous HealthCheck run's
result; modify the logic that queries the HealthCheck (named
checkout-api-deploy-check) to first fetch and compare status.observedGeneration
with metadata.generation (or alternatively wait for a changed
status.completedAt) and only then consider status.result; in practice, update
the loop that reads kubectl get hc ... -o jsonpath='{.status.result}' to first
retrieve both metadata.generation and status.observedGeneration and continue
sleeping until they match (or until completedAt advances) before evaluating
status.result for "pass"/"fail"/"error".
---
Nitpick comments:
In `@docs/operator/deployment-verification.md`:
- Around line 3-14: Update the paragraph about the holmesgpt.dev/rerun
annotation to add an ArgoCD caveat: note that because the operator clears the
holmesgpt.dev/rerun annotation from the live object, GitOps controllers (ArgoCD)
will detect drift and—if selfHeal is enabled—may enter a sync loop; instruct
users to either add an ArgoCD ignoreDifferences rule for the holmesgpt.dev/rerun
annotation (resource.customizations or argocd-cm diff/ignore settings) or
restrict this pattern to manual/commit-driven syncs (disable selfHeal for the
resource) to avoid repeated re-runs.
🪄 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: f4610c6b-6ec8-4302-91fd-4ef39e771c0f
📒 Files selected for processing (4)
docs/operator/deployment-verification.mddocs/operator/health-checks.mdholmes_operator/handlers/healthcheck.pytests/holmes_operator/test_healthcheck_component.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/holmes_operator/test_healthcheck_component.py
| await _execute_healthcheck( | ||
| spec=spec, | ||
| name=name, | ||
| namespace=namespace, | ||
| uid=uid, | ||
| generation=generation, | ||
| logger=logger, | ||
| body=body, | ||
| ) | ||
|
|
||
| # Clear rerun annotation if present, so that the next kubectl apply | ||
| # (which restores it from the manifest) triggers a re-run | ||
| annotations = body.get("metadata", {}).get("annotations", {}) | ||
| if annotations.get("holmesgpt.dev/rerun") == "true": | ||
| await _clear_rerun_annotation(name=name, namespace=namespace, logger=logger) |
There was a problem hiding this comment.
Always clear holmesgpt.dev/rerun when an execution consumed it.
Two paths still leave the flag stuck at "true": the generation-mismatch branch returns before cleanup, and any _execute_healthcheck() exception skips the post-call cleanup entirely. After that, later kubectl apply / kubectl annotate ... rerun=true calls stop creating a fresh transition, so the rerun mechanism quietly stops working for that object.
♻️ Suggested direction
- await _execute_healthcheck(
- spec=spec,
- name=name,
- namespace=namespace,
- uid=uid,
- generation=generation,
- logger=logger,
- body=body,
- )
-
- # Clear rerun annotation if present, so that the next kubectl apply
- # (which restores it from the manifest) triggers a re-run
- annotations = body.get("metadata", {}).get("annotations", {})
- if annotations.get("holmesgpt.dev/rerun") == "true":
- await _clear_rerun_annotation(name=name, namespace=namespace, logger=logger)
+ annotations = body.get("metadata", {}).get("annotations", {})
+ should_clear_rerun = annotations.get("holmesgpt.dev/rerun") == "true"
+ try:
+ await _execute_healthcheck(
+ spec=spec,
+ name=name,
+ namespace=namespace,
+ uid=uid,
+ generation=generation,
+ logger=logger,
+ body=body,
+ )
+ finally:
+ if should_clear_rerun:
+ await _clear_rerun_annotation(name=name, namespace=namespace, logger=logger)Apply the same try/finally pattern to the update handler, and make sure the generation-mismatch branch clears the annotation before returning.
Also applies to: 277-313
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@holmes_operator/handlers/healthcheck.py` around lines 236 - 250, The
post-execution cleanup that clears the holmesgpt.dev/rerun annotation is skipped
if _execute_healthcheck raises or if the generation-mismatch early return
occurs; wrap the call to _execute_healthcheck (and any surrounding logic) in a
try/finally so _clear_rerun_annotation(name=..., namespace=..., logger=...) is
always awaited in the finally block, and modify the generation-mismatch branch
inside the same handler (the update handler around lines handling generation and
returning early) to call _clear_rerun_annotation before returning so the
annotation is removed in all paths.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/operator/deployment-verification.md (1)
3-14: Document generation-based re-execution alongside annotation-based mechanism.The documentation focuses exclusively on the
holmesgpt.dev/rerunannotation but doesn't mention that the operator also automatically re-executes when the HealthCheck spec changes (metadata.generation != status.observedGeneration). This could confuse users who see re-execution happen without the annotation, or lead them to believe the annotation is always required.Consider adding a note explaining both mechanisms:
- Generation-based (automatic): Triggers when spec fields change (query, timeout, mode, etc.)
- Annotation-based (manual): Triggers re-execution even when spec is unchanged — useful for deploying the same HealthCheck definition repeatedly
For this "Deployment Verification" use case where the HealthCheck spec is typically static, the annotation approach is correct, but documenting both mechanisms would improve clarity.
Based on context snippets from
holmes_operator/handlers/healthcheck.py:274-292showing the generation check as the first trigger, and fromhelm/holmes/crds/healthcheck.yaml:69-72documenting theobservedGenerationfield.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/operator/deployment-verification.md` around lines 3 - 14, Docs only describe the holmesgpt.dev/rerun annotation but omit the automatic generation-based re-execution; update deployment-verification.md to mention both triggers: explain that re-execution also happens automatically when metadata.generation != status.observedGeneration (i.e., spec changes such as query, timeout, mode) and that the holmesgpt.dev/rerun: "true" annotation forces re-run even when spec is unchanged; reference the operator check in holmes_operator/handlers/healthcheck.py (the generation vs observedGeneration logic) and the observedGeneration field documented in helm/holmes/crds/healthcheck.yaml so readers understand when to use the annotation vs relying on spec changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/operator/deployment-verification.md`:
- Around line 3-14: Docs only describe the holmesgpt.dev/rerun annotation but
omit the automatic generation-based re-execution; update
deployment-verification.md to mention both triggers: explain that re-execution
also happens automatically when metadata.generation != status.observedGeneration
(i.e., spec changes such as query, timeout, mode) and that the
holmesgpt.dev/rerun: "true" annotation forces re-run even when spec is
unchanged; reference the operator check in
holmes_operator/handlers/healthcheck.py (the generation vs observedGeneration
logic) and the observedGeneration field documented in
helm/holmes/crds/healthcheck.yaml so readers understand when to use the
annotation vs relying on spec changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 52f7f010-ce8e-4b39-8e09-f0153dd88c55
📒 Files selected for processing (1)
docs/operator/deployment-verification.md
| body = new | ||
| metadata = body.get("metadata", {}) | ||
| status = body.get("status", {}) | ||
| generation = metadata.get("generation") | ||
| observed_generation = status.get("observedGeneration") | ||
|
|
||
| # Trigger 1: Generation changed (spec was modified via kubectl apply) | ||
| if generation is not None and generation != observed_generation: | ||
| logger.info( | ||
| f"Re-running HealthCheck {namespace}/{name}: " | ||
| f"generation={generation} != observedGeneration={observed_generation}" | ||
| ) | ||
| await _execute_healthcheck( | ||
| spec=new.get("spec", {}), | ||
| name=name, |
There was a problem hiding this comment.
🔴 When Trigger 1 (generation mismatch) fires in on_healthcheck_update, it executes the check and does an early return without checking or clearing the holmesgpt.dev/rerun annotation, leaving it permanently as true in K8s. This breaks the documented Helm/ArgoCD CI/CD toggle pattern: on the next deploy with the same spec, kubectl apply sees no annotation diff and fires no update event, so no re-run ever happens again. Fix by calling _clear_rerun_annotation before the return in Trigger 1 if the annotation is present.
Extended reasoning...
Bug: Trigger 1 early return skips annotation clearing
What the bug is
In on_healthcheck_update (lines 271-285 of holmes_operator/handlers/healthcheck.py), the handler checks two triggers in order. Trigger 1 (generation mismatch) executes _execute_healthcheck and then does return, completely bypassing the Trigger 2 block that checks for and clears the holmesgpt.dev/rerun annotation.
The specific code path
Trigger 1 code:
Trigger 2 (never reached when Trigger 1 fires):
Why existing code does not prevent it
The annotation-clearing logic lives exclusively in the Trigger 2 block. Neither the _execute_healthcheck helper nor the Trigger 1 code path has any awareness of the annotation. Any code path that returns before reaching Trigger 2 silently skips the clearing.
Impact
This breaks the key Helm/ArgoCD rerun toggle pattern advertised in the PR documentation. The docs explicitly tell users to add holmesgpt.dev/rerun: "true" permanently to their manifest so that every helm upgrade or ArgoCD sync triggers a fresh health check. Once a user does a spec-changing deploy (bumping generation), this pattern permanently stops working.
Step-by-step proof
- User has holmesgpt.dev/rerun: "true" permanently in Helm chart template.
- Deploy 1 (initial): generation=1, observedGeneration=null. Trigger 1 fires (1 != null), check runs, observedGeneration set to 1, return executed — annotation stays true in K8s, never cleared.
- Deploy 2: user changes the spec query. Generation bumps to 2. Trigger 1 fires (2 != 1), check runs, observedGeneration set to 2, return — annotation remains true in K8s, never cleared.
- Deploy 3: same spec as Deploy 2, annotation still true in manifest. kubectl apply computes the diff: K8s annotation = true, manifest annotation = true — no change detected, no PATCH sent, no update event fires at all. Health check is silently skipped.
- Even if an update event fires for another reason: old_annotations["holmesgpt.dev/rerun"] = "true" (never cleared), new_annotations["holmesgpt.dev/rerun"] = "true" so Trigger 2 condition old != "true" is False and still no re-run happens.
Fix
Before the return in Trigger 1, check for and clear the rerun annotation if present:
| f"Re-running HealthCheck {namespace}/{name}: " | ||
| f"generation={generation} != observedGeneration={observed_generation}" | ||
| ) | ||
| await _execute_healthcheck( | ||
| spec=new.get("spec", {}), | ||
| name=name, | ||
| namespace=namespace, | ||
| uid=metadata.get("uid", ""), | ||
| generation=generation, | ||
| logger=logger, | ||
| body=body, | ||
| ) | ||
| return | ||
|
|
||
| # Trigger 2: Rerun annotation (for re-running without spec changes) | ||
| annotations = metadata.get("annotations", {}) | ||
| old_annotations = old.get("metadata", {}).get("annotations", {}) | ||
| if ( | ||
| annotations.get("holmesgpt.dev/rerun") == "true" | ||
| and old_annotations.get("holmesgpt.dev/rerun") != "true" | ||
| ): | ||
| logger.info(f"Re-running HealthCheck: {namespace}/{name}") | ||
|
|
||
| # Trigger re-execution by calling create handler | ||
| await on_healthcheck_create( | ||
| logger.info(f"Re-running HealthCheck via annotation: {namespace}/{name}") | ||
| await _execute_healthcheck( | ||
| spec=new.get("spec", {}), | ||
| name=name, | ||
| namespace=namespace, | ||
| uid=new.get("metadata", {}).get("uid", ""), | ||
| uid=metadata.get("uid", ""), |
There was a problem hiding this comment.
🔴 The on_healthcheck_update handler has two annotation-clearing gaps that can permanently break the Helm/ArgoCD rerun toggle pattern. In Trigger 1 (generation mismatch), an early return skips the annotation-clearing code entirely; and in Trigger 2 (annotation-based rerun), _clear_rerun_annotation is called after _execute_healthcheck which re-raises on failure, leaving the annotation stuck as true after any operator error. Both paths should clear the annotation (or use try/finally) before returning/raising.
Extended reasoning...
Bug 1 - Trigger 1 early return skips annotation clearing (lines 280-285)
When metadata.generation != status.observedGeneration, the handler calls _execute_healthcheck and immediately does return, bypassing the Trigger 2 block that is responsible for clearing the holmesgpt.dev/rerun annotation. This means any resource that has holmesgpt.dev/rerun: "true" in its manifest AND receives a spec change at the same time will exit Trigger 1 with the annotation still set to true in the live Kubernetes object.
Concrete proof (Bug 1):
- User has
holmesgpt.dev/rerun: "true"in their Helm/ArgoCD manifest (the toggle pattern documented in this PR). - User also changes
spec.query- generation bumps from 1 to 2. - Update event fires:
generation=2,observedGeneration=1-> Trigger 1 fires, check runs,set_healthcheck_completedsetsobservedGeneration=2, handler doesreturn. Annotation remains"true"in K8s. - Next
helm upgradeor ArgoCD sync with no spec change:kubectl applycomputes the diff - manifest saysrerun=true, K8s already hasrerun=true-> no diff, no PATCH, no update event. The check never re-runs. The toggle is permanently broken until someone manually removes the annotation.
Bug 2 - Trigger 2 annotation not cleared on _execute_healthcheck exception (lines 295-306)
In Trigger 2, _clear_rerun_annotation is called sequentially after _execute_healthcheck. But _execute_healthcheck re-raises any exception after setting observedGeneration via set_healthcheck_failed. The exception propagates before _clear_rerun_annotation is ever called.
Concrete proof (Bug 2):
- User sets
kubectl annotate hc my-check holmesgpt.dev/rerun=true. Old annotation is absent, new annotation is"true"-> Trigger 2 fires. - Holmes API returns 500.
_execute_healthcheckcallsset_healthcheck_failed(..., observed_generation=generation), then re-raises the exception. _clear_rerun_annotationis never reached. Nowstatus.observedGeneration == generationAND annotation =="true"in K8s.- On the next
kubectl applyor retry: Trigger 1 does not fire (generation == observedGeneration). Trigger 2 does not fire (old_annotations.get("holmesgpt.dev/rerun") != "true"evaluatesFalsesince the old state in kopf's cache also has"true"). The user is stuck and must manually delete then re-add the annotation to retry.
Why existing code does not prevent this:
The annotation-clearing logic in Trigger 2 assumes _execute_healthcheck always succeeds (or that exceptions propagate without side effects). The early return in Trigger 1 was probably intentional to avoid double-execution, but it accidentally skips annotation cleanup. There is no try/finally guard in either path.
Impact:
Both bugs break the primary CI/CD workflow advertised in this PR's documentation (the Helm/ArgoCD rerun toggle pattern). Bug 1 triggers reliably any time a user combines a spec change with the persistent annotation pattern. Bug 2 makes failure recovery impossible via annotation - users must perform a two-step manual kubectl annotate delete+restore.
Suggested fix:
- Trigger 1: Before
return, check if the annotation is present and call_clear_rerun_annotationif so. - Trigger 2: Wrap in
try/finallyto ensure_clear_rerun_annotationis always called regardless of whether_execute_healthcheckraises.
| echo "Deploy verified healthy" | ||
| exit 0 | ||
| elif [ "$RESULT" = "fail" ] || [ "$RESULT" = "error" ]; then | ||
| echo "Deploy check failed:" | ||
| kubectl get hc checkout-api-deploy-v2-4-1 -n production -o jsonpath='{.status.message}' | ||
| kubectl get hc checkout-api-deploy-check -n production -o jsonpath='{.status.message}' | ||
| exit 1 | ||
| fi | ||
| sleep 10 |
There was a problem hiding this comment.
🔴 The CI/CD polling script in deployment-verification.md checks only status.result without verifying status.phase == Completed, causing it to report false success during a re-run. Because set_healthcheck_pending only patches phase=Pending without clearing the old result field, the stale result=pass from a prior run is visible immediately when a new check starts — the script exits 0 before the new check completes. Fix by updating the polling script to also assert phase=Completed before trusting the result, or by having set_healthcheck_pending explicitly null out the result/message/etc. fields.
Extended reasoning...
What the bug is and how it manifests
set_healthcheck_pending in utils.py only patches phase=Pending and startTime. Kubernetes strategic merge PATCH preserves every field not explicitly set, so the old result, message, rationale, duration, completionTime, and modelUsed remain in the live resource status from the previous run. The CI/CD polling script in deployment-verification.md (lines 120-127) polls status.result directly, with no check that status.phase == Completed before trusting the result.
The specific code path that triggers it
When a re-run fires (either via generation mismatch in on_healthcheck_update Trigger 1, or via the holmesgpt.dev/rerun annotation in Trigger 2), _execute_healthcheck is called which immediately invokes set_healthcheck_pending. That function calls update_healthcheck_status with only phase=Pending and start_time. The status subresource PATCH does not include result, message, or completionTime, so Kubernetes preserves those fields at their previous values. The polling script immediately queries status.result which returns the stale value and exits 0.
Why existing code does not prevent it
Neither set_healthcheck_pending nor _execute_healthcheck explicitly nulls out the stale result fields. The polling script has no guard checking phase — it will immediately see the stale result=pass on the very first poll iteration, which may happen within milliseconds of kubectl apply before the operator has even processed the update event, let alone completed the check.
Impact
The documented CI/CD gating use case is silently broken for repeat deploys. A pipeline will declare "Deploy verified healthy" before the new health check completes, bypassing the entire safety gate. The previous docs used versioned resource names (e.g., checkout-api-deploy-v2-4-1) meaning each deploy started with a fresh resource and no prior result. This PR explicitly changes the guidance to a persistent name with the rerun toggle, making the stale-result window the default for every deploy after the first.
Step-by-step proof
- Deploy 1: HealthCheck checkout-api-deploy-check runs and passes. Status: {phase: Completed, result: pass, message: "All healthy"}
- Deploy 2: CI runs kubectl apply with holmesgpt.dev/rerun: "true" restored by Helm/ArgoCD. Operator fires on_healthcheck_update -> _execute_healthcheck -> set_healthcheck_pending patches only phase=Pending, startTime=now. Status is now: {phase: Pending, result: pass (STALE), ...}
- CI polling script starts its loop immediately after kubectl apply. On the first iteration it queries status.result -> sees "pass" -> prints "Deploy verified healthy" -> exit 0.
- The new check has not yet completed or even started running. The CI gate reports success on a stale result.
How to fix
Either: (1) Update the polling script to check phase=Completed before trusting result — add a PHASE check alongside the RESULT check so the script only exits when both phase=Completed AND result=pass. Or: (2) have set_healthcheck_pending explicitly null out result, message, rationale, completionTime, duration, modelUsed so there is no stale data window at all.
This branch has not been deployed
2 participants
Summary
This PR implements generation-based re-execution for HealthCheck resources, allowing checks to automatically re-run when the spec is modified. It also adds support for manual re-execution via a
holmesgpt.dev/rerunannotation, and tracks the last processed generation in the status viaobservedGeneration.Key Changes
Generation-based re-execution: When
metadata.generationdiffers fromstatus.observedGeneration, the check automatically re-executes. This leverages Kubernetes' built-in generation counter that increments whenever the spec changes.Rerun annotation support: Users can set the
holmesgpt.dev/rerun=trueannotation to manually trigger re-execution even without spec changes. The annotation is automatically cleared after processing.Refactored execution logic: Extracted common execution logic into
_execute_healthcheck()shared by both create and update handlers, reducing code duplication.Status tracking: Added
observedGenerationfield to HealthCheckStatus to track which generation was last processed, preventing infinite retry loops on failures.Update handler enhancement: The
on_healthcheck_updatehandler now implements two re-execution triggers (in order):CRD schema update: Added
observedGenerationfield definition to the HealthCheck CRD schema with documentation.Comprehensive test coverage: Added test cases for:
Implementation Details
_execute_healthcheck()function now accepts an optionalgenerationparameter that gets stored asobservedGenerationin the status, even on failure._clear_rerun_annotation()helper that patches the resource metadata.observedGenerationto prevent re-execution loops._make_body()helper to construct proper HealthCheck resource bodies with metadata.https://claude.ai/code/session_01QY6zsEHPW9CfSKJBtE6WCd
Summary by CodeRabbit
New Features
Documentation
Tests