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. |
WalkthroughThe PR adds HTTPS liveness/readiness probes to operator manifests and expands reconciliation status handling to include ChangesCondition status and health probes
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Controller
participant ReconcileResult
participant ReconcileError
participant StatusConditions
Controller->>ReconcileResult: HandleReconcileResult(error)
ReconcileResult->>ReconcileError: GetConditionReason(error)
ReconcileResult->>StatusConditions: Set Degraded, Ready, and Progressing
StatusConditions-->>Controller: updateConditionFn(result)
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 |
sebrandon1
force-pushed
the
add-operator-health-probes
branch
from
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
sebrandon1
force-pushed
the
add-operator-health-probes
branch
from
cc2910e to
d9d40bd
Compare
sebrandon1
force-pushed
the
add-operator-health-probes
branch
from
d9d40bd to
7285ee6
Compare
sebrandon1
force-pushed
the
add-operator-health-probes
branch
from
7285ee6 to
53508d0
Compare
sebrandon1
changed the title
sebrandon1
force-pushed
the
add-operator-health-probes
branch
2 times, most recently
from
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. |
sebrandon1
force-pushed
the
add-operator-health-probes
branch
2 times, most recently
from
d43d440 to
3cdd7f9
Compare
sebrandon1
force-pushed
the
add-operator-health-probes
branch
2 times, most recently
from
858bcea to
28e5593
Compare
sebrandon1
force-pushed
the
add-operator-health-probes
branch
from
28e5593 to
b550fd8
Compare
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
Add liveness and readiness probes to the operator deployment. Add a Progressing condition type and specific reason codes (Reconciling, WaitingForDependencies, ValidationFailed, MultipleInstancesFound) to IstioCSR and TrustManager CRs for richer diagnostic information without checking operator logs.
sebrandon1
force-pushed
the
add-operator-health-probes
branch
from
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
| func GetConditionReason(err error) string { | ||
| rerr := &ReconcileError{} | ||
| if errors.As(err, &rerr) { | ||
| return rerr.ConditionReason |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 '\bGetConditionReason\s*\(' --glob '*.go' || true
rg -n -C 4 '\(\*ReconcileError\)\(nil\)|var[[:space:]].*\*ReconcileError' --glob '*_test.go' || trueRepository: openshift/cert-manager-operator
Length of output: 169
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and inspect the surrounding code.
git ls-files 'pkg/controller/common/errors.go' 'pkg/controller/common/*test.go'
wc -l pkg/controller/common/errors.go
sed -n '1,240p' pkg/controller/common/errors.go
# Look for definitions/usages of ReconcileError and existing tests.
rg -n -C 3 'type ReconcileError|ConditionReason|GetConditionReason|errors\.As\(' pkg/controller/common --glob '*.go'Repository: openshift/cert-manager-operator
Length of output: 14082
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Narrowly inspect tests and any typed-nil construction patterns.
rg -n -C 3 'ReconcileError|GetConditionReason|typed nil|nil\)\)|\(\*ReconcileError\)\(nil\)' pkg --glob '*.go'Repository: openshift/cert-manager-operator
Length of output: 14380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat >/tmp/typed_nil_probe.go <<'GO'
package main
import (
"errors"
"fmt"
)
type ReconcileError struct {
ConditionReason string
}
func (e *ReconcileError) Error() string { return "x" }
func main() {
var inner *ReconcileError = nil
var err error = inner
var rerr *ReconcileError
ok := errors.As(err, &rerr)
fmt.Printf("errors.As=%v rerr==nil=%v\n", ok, rerr == nil)
if ok && rerr != nil {
fmt.Println(rerr.ConditionReason)
} else if ok {
fmt.Println("would panic if dereferenced")
}
}
GO
go run /tmp/typed_nil_probe.goRepository: openshift/cert-manager-operator
Length of output: 227
Guard GetConditionReason against typed-nil *ReconcileError values. errors.As can match an error interface holding a nil *ReconcileError, leaving rerr == nil and making rerr.ConditionReason panic. Check rerr != nil before dereferencing and add a regression test.
🤖 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 `@pkg/controller/common/errors.go` around lines 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.
| if err := r.validateTrustNamespace(trustNamespace); err != nil { | ||
| return common.NewIrrecoverableError(err, "trust namespace %q validation failed", trustNamespace) | ||
| return common.NewIrrecoverableError(err, "trust namespace %q validation failed", trustNamespace). | ||
| WithConditionReason(v1alpha1.ReasonWaitingForDependencies) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 10 '\b(namespaceExists|validateTrustNamespace)\b' --glob '*.go' || true
rg -n -C 5 'ReasonWaitingForDependencies|NewIrrecoverableError' --glob '*.go' || trueRepository: openshift/cert-manager-operator
Length of output: 169
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files of interest:\n'
git ls-files 'pkg/controller/trustmanager/*' 'pkg/controller/**/reconcile_result.go' 'pkg/**/reconcile_result.go' 'pkg/**/install_trustmanager.go' | sed -n '1,200p'
printf '\nSearch for relevant symbols:\n'
rg -n -C 3 'validateTrustNamespace|namespaceExists|ReasonWaitingForDependencies|NewIrrecoverableError|irrecoverable' pkg --glob '*.go' || true
printf '\nOutline candidate file(s):\n'
for f in $(git ls-files 'pkg/controller/trustmanager/install_trustmanager.go' 'pkg/controller/**/reconcile_result.go' 'pkg/**/reconcile_result.go' 2>/dev/null); do
echo "### $f"
ast-grep outline "$f" --view expanded 2>/dev/null || true
doneRepository: openshift/cert-manager-operator
Length of output: 29604
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '70,90p' pkg/controller/trustmanager/install_trustmanager.go | cat -n
printf '\n---\n'
sed -n '1,80p' pkg/controller/common/reconcile_result.go | cat -nRepository: openshift/cert-manager-operator
Length of output: 5136
Split trust-namespace lookup failures from not-found pkg/controller/trustmanager/install_trustmanager.go:21-23
validateTrustNamespace returns both “namespace does not exist” and “failed to check if namespace exists,” but both are wrapped as ReasonWaitingForDependencies and then treated as irrecoverable. That makes transient client/API failures skip the retry path; keep the waiting-for-dependencies reason only for the not-found case.
🤖 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 `@pkg/controller/trustmanager/install_trustmanager.go` around lines 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.
|
@sebrandon1: The following tests failed, say
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