CM-1114: Add health probes and richer status conditions#417
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@sebrandon1: This pull request references CNF-23436 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (14)
🚧 Files skipped from review as they are similar to previous changes (11)
WalkthroughThe PR adds HTTPS liveness/readiness probes to operator manifests and expands reconciliation status handling to include ChangesReconciliation status propagation
Operator health probes
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Controller
participant HandleReconcileResult
participant ReconcileError
participant ConditionalStatus
Controller->>HandleReconcileResult: pass reconciliation result
HandleReconcileResult->>ReconcileError: extract condition reason
HandleReconcileResult->>ConditionalStatus: set Degraded, Ready, Progressing
ConditionalStatus-->>Controller: update conditions when changed
Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sebrandon1 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
e2f1df3 to
cc2910e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
config/manager/manager.yaml (1)
114-122: ⚡ Quick winTune readiness probe for faster drain on shutdown.
To better align with graceful termination, Line 120 and Line 122 are a bit slow (
10s * 3worst-case before NotReady). Consider faster readiness failure so endpoints stop routing sooner.Suggested tweak
readinessProbe: httpGet: path: /readyz port: https scheme: HTTPS initialDelaySeconds: 5 - periodSeconds: 10 + periodSeconds: 5 timeoutSeconds: 5 - failureThreshold: 3 + failureThreshold: 1🤖 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 `@config/manager/manager.yaml` around lines 114 - 122, The readinessProbe for the manager (httpGet path "/readyz", scheme HTTPS) is too slow to mark Pod NotReady during shutdown; adjust readinessProbe settings to fail faster by lowering periodSeconds (e.g., from 10 to 2–3), reducing failureThreshold (e.g., from 3 to 1–2) and/or decreasing timeoutSeconds to ensure the probe transitions to NotReady quickly so endpoints are drained sooner; update the readinessProbe block (httpGet path /readyz, initialDelaySeconds, periodSeconds, timeoutSeconds, failureThreshold) accordingly.
🤖 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 `@config/manager/manager.yaml`:
- Around line 114-122: The readinessProbe for the manager (httpGet path
"/readyz", scheme HTTPS) is too slow to mark Pod NotReady during shutdown;
adjust readinessProbe settings to fail faster by lowering periodSeconds (e.g.,
from 10 to 2–3), reducing failureThreshold (e.g., from 3 to 1–2) and/or
decreasing timeoutSeconds to ensure the probe transitions to NotReady quickly so
endpoints are drained sooner; update the readinessProbe block (httpGet path
/readyz, initialDelaySeconds, periodSeconds, timeoutSeconds, failureThreshold)
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f1db3899-53e4-40c3-a38c-c2fc93c4f11f
📒 Files selected for processing (2)
bundle/manifests/cert-manager-operator.clusterserviceversion.yamlconfig/manager/manager.yaml
cc2910e to
d9d40bd
Compare
d9d40bd to
7285ee6
Compare
7285ee6 to
53508d0
Compare
sebrandon1
changed the title
19e3212 to
4e2bb0d
Compare
|
/retest |
sebrandon1
changed the title
|
@sebrandon1: This pull request references CM-1114 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
d43d440 to
3cdd7f9
Compare
858bcea to
28e5593
Compare
28e5593 to
b550fd8
Compare
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
b550fd8 to
b484ca2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@pkg/controller/common/errors.go`:
- Around line 143-146: Update GetConditionReason to verify rerr is non-nil after
errors.As succeeds before accessing ConditionReason, returning the existing
fallback for a typed-nil *ReconcileError; add a regression test covering an
error interface containing a nil *ReconcileError.
In `@pkg/controller/trustmanager/install_trustmanager.go`:
- Around line 21-23: Update the error handling around validateTrustNamespace in
the trust-manager installation flow to distinguish a missing trust namespace
from lookup/API failures. Wrap only the not-found result with
ReasonWaitingForDependencies; propagate other validation errors without that
reason so transient failures remain retryable.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: bac91557-f9e0-4aaf-8ed7-9e5aedab1f3c
📒 Files selected for processing (13)
api/operator/v1alpha1/conditions.goapi/operator/v1alpha1/conditions_test.gobundle/manifests/cert-manager-operator.clusterserviceversion.yamlconfig/manager/manager.yamlpkg/controller/common/errors.gopkg/controller/common/errors_test.gopkg/controller/common/reconcile_result.gopkg/controller/istiocsr/controller_test.gopkg/controller/istiocsr/install_istiocsr.gopkg/controller/istiocsr/utils.gopkg/controller/trustmanager/controller_test.gopkg/controller/trustmanager/install_trustmanager.gotest/e2e/trustmanager_test.go
🚧 Files skipped from review as they are similar to previous changes (10)
- pkg/controller/istiocsr/install_istiocsr.go
- bundle/manifests/cert-manager-operator.clusterserviceversion.yaml
- config/manager/manager.yaml
- pkg/controller/istiocsr/utils.go
- pkg/controller/common/errors_test.go
- api/operator/v1alpha1/conditions.go
- pkg/controller/trustmanager/controller_test.go
- api/operator/v1alpha1/conditions_test.go
- test/e2e/trustmanager_test.go
- pkg/controller/istiocsr/controller_test.go
Add Progressing condition to operator status alongside existing Degraded and Ready conditions. Introduce WithConditionReason/GetConditionReason on ReconcileError for structured condition reason propagation through the error chain. Add liveness and readiness probes to the operator deployment manifest. Add HandleReconcileResult unit tests covering success, irrecoverable, and recoverable error paths including custom ConditionReason propagation, no-change skip optimization, and updateConditionFn error propagation. Apply typed-nil guard consistently to all errors.As call sites in the error classification functions.
37c05f4 to
a3a14a4
Compare
|
@sebrandon1: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Summary
Health Probes
The operator deployment currently has no health probes, so Kubernetes cannot detect if the operator process is stuck or not yet ready to serve. All cert-manager operands (controller, webhook, cainjector, trust-manager, istio-csr) already have probes configured — the operator itself is the only component missing them.
The library-go
controllercmdframework already serves/healthzand/readyzover HTTPS on port 8443 via its GenericAPIServer, so no Go code changes are needed./healthz(ping, log, post-start hooks)/readyz(same checks +shutdown, so the pod drains traffic during graceful termination)Richer Status Conditions
IstioCSR and TrustManager CRs currently report only two status conditions (
ReadyandDegraded) with generic reason constants (Failed,Ready,Progressing). Users can't tell why something is progressing or degraded without reading operator logs.This adds:
Progressingcondition type alongsideReadyandDegradedReconciling,WaitingForDependencies,ValidationFailed,MultipleInstancesFoundConditionReasonfield onReconcileErrorwithWithConditionReason()chainable setter so controllers can annotate errors with specific reasonsHandleReconcileResultto manage all three conditions and extract specific reasons from errorsTest plan
Health Probes
Status Conditions — Reconciling
IstioCSR CR during active reconciliation (retrying due to missing namespace):
[ { "lastTransitionTime": "2026-06-08T21:44:45Z", "message": "", "reason": "Ready", "status": "False", "type": "Degraded" }, { "lastTransitionTime": "2026-06-08T21:44:45Z", "message": "reconciliation failed, retrying: failed to create istio-system/cert-manager-istio-csr role resource: ...", "reason": "Progressing", "status": "False", "type": "Ready" }, { "lastTransitionTime": "2026-06-08T21:44:45Z", "message": "reconciliation in progress: failed to create istio-system/cert-manager-istio-csr role resource: ...", "reason": "Reconciling", "status": "True", "type": "Progressing" } ]Status Conditions — MultipleInstancesFound
Second IstioCSR CR rejected as a duplicate:
[ { "lastTransitionTime": "2026-06-08T21:45:06Z", "message": "", "reason": "MultipleInstancesFound", "status": "False", "type": "Degraded" }, { "lastTransitionTime": "2026-06-08T21:45:06Z", "message": "multiple instances of istiocsr exists, cert-manager-operator/default will not be processed", "reason": "MultipleInstancesFound", "status": "False", "type": "Ready" }, { "lastTransitionTime": "2026-06-08T21:45:06Z", "message": "multiple instances of istiocsr exists, cert-manager-operator/default will not be processed", "reason": "MultipleInstancesFound", "status": "False", "type": "Progressing" } ]Summary by CodeRabbit
New Features
Bug Fixes
Tests