OCPBUGS-98461: requeue CRR on transiently unavailable resources - #8997
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
@jparrill: GitHub didn't allow me to request PR reviews from the following users: cewong. Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs. 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 kubernetes-sigs/prow repository. |
📝 WalkthroughWalkthroughThe certificate revocation controller now returns Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
area/control-plane-pki-operator
jparrill
changed the title
openshift-ci-robot
added
jira/valid-reference
|
@jparrill: This pull request references Jira Issue OCPBUGS-98461, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
/test aws-e2e |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8997 +/- ##
==========================================
+ Coverage 44.11% 44.12% +0.01%
==========================================
Files 772 772
Lines 96226 96226
==========================================
+ Hits 42448 42463 +15
+ Misses 50832 50822 -10
+ Partials 2946 2941 -5
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller_test.go (1)
2029-2042: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the remaining changed requeue branches.
This test covers only the missing trust bundle during old-signer revocation. Verify that tests also cover missing signer data and missing trust bundles during new-signer propagation, plus a missing current signer during revocation; add focused subtests if they are not already covered.
As per coding guidelines, unit test any code changes and additions.
🤖 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 `@control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller_test.go` around lines 2029 - 2042, Extend the certificate revocation controller tests around processCertificateRevocationRequest to cover each remaining requeue branch: missing signer data, missing trust bundle during new-signer propagation, and missing current signer during revocation. Add focused subtests using the existing revoked CRR, secret, controller, and expectation helpers, asserting no error and requeue=true for each transiently unavailable dependency.Source: Coding guidelines
🤖 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
`@control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller_test.go`:
- Around line 2029-2042: Extend the certificate revocation controller tests
around processCertificateRevocationRequest to cover each remaining requeue
branch: missing signer data, missing trust bundle during new-signer propagation,
and missing current signer during revocation. Add focused subtests using the
existing revoked CRR, secret, controller, and expectation helpers, asserting no
error and requeue=true for each transiently unavailable dependency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: d2b5c30b-21e6-49dd-9aaf-78481e22e58b
📒 Files selected for processing (2)
control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller.gocontrol-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller_test.go
|
@jparrill: This pull request references Jira Issue OCPBUGS-98461, which is valid. 3 validation(s) were run on this bug
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. |
|
/lgtm |
|
Scheduling tests matching the |
Test Resultse2e-aws
e2e-aks
|
|
Both analyses are complete. Now I have the full picture for both jobs. Let me compile the final report. Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryBoth job failures are caused by transient infrastructure/timing flakes during hosted cluster bring-up — none are related to the PR's code changes. The e2e-aws job hit two issues: (a) the management cluster API server was overloaded with 14+ concurrent hosted cluster creations, causing a 13-second ValidatingAdmissionPolicy timeout on namespace creation, and (b) control-plane-operator pods crashed during startup because the hosted cluster kube-apiserver was not yet serving TLS, then recovered after 5 restarts and ran normally. The e2e-azure job had a single cluster-network-operator container crash 3 seconds after first start during the cluster's Root CauseNeither job failure is caused by PR #8997. The PR only modifies e2e-aws: Two Distinct Failure PatternsPattern 1 — ValidatingAdmissionPolicy timeout (TestCreateCluster) Pattern 2 — control-plane-operator CrashLoopBackOff (TestAutoscaling, TestNodePool, TestCreateClusterProxy)
These crashes occurred at e2e-azure: cluster-network-operator Startup CrashTimeline:
The CNO crash occurred while the hosted cluster was in Recommendations
Evidence
|
Three return paths in ensureOldSignerCertificateRevoked and ensureNewSignerCertificatePropagated returned requeueSoon=false when a resource was transiently nil (signer secret, trust bundle ConfigMap). Without a synthetic requeue, the controller waited for a watch event that might never come — the same class of bug fixed in openshift#8563 for the stale trust bundle cache case. Change requeueSoon from false to true for these transient nil returns so the controller retries instead of stalling. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller.go (1)
686-699: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRequeuing forever on documented "should never happen" branches, with no logging. Both
ensureNewSignerCertificatePropagatedandensureOldSignerCertificateRevokedhave asecretForSignerClass!okbranch explicitly commented as unreachable (signer class is validated/immutable), yet both were switched fromrequeue=falsetorequeue=trueas part of this change — the same blanket edit that correctly fixed the transient signer-secret/trust-bundle nil checks elsewhere. Unlike those transient cases, retrying an immutable-field mismatch can never converge, so if this branch is ever hit it will requeue indefinitely (with controller-runtime backoff) with zero error, event, or log to alert on the underlying misconfiguration.
control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller.go#L686-L699: add a warning log (or return an error) in the!okbranch ofensureNewSignerCertificatePropagatedbefore requeuing, so a real occurrence isn't silently retried forever.control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller.go#L1008-L1019: apply the same warning log (or error) to the equivalent!okbranch inensureOldSignerCertificateRevoked.🤖 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 `@control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller.go` around lines 686 - 699, Handle the impossible signer-class branches in both ensureNewSignerCertificatePropagated and ensureOldSignerCertificateRevoked by emitting a warning log or returning an error before requeuing, so the mismatch is visible and does not silently retry forever. Apply this change at control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller.go lines 686-699 and 1008-1019.
🤖 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
`@control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller.go`:
- Around line 686-699: Handle the impossible signer-class branches in both
ensureNewSignerCertificatePropagated and ensureOldSignerCertificateRevoked by
emitting a warning log or returning an error before requeuing, so the mismatch
is visible and does not silently retry forever. Apply this change at
control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller.go
lines 686-699 and 1008-1019.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: f7de3749-054e-432e-85d2-e534b00d8b32
📒 Files selected for processing (2)
control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller.gocontrol-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller_test.go
|
/acknowledge-critical-fixes-only |
csrwng
left a comment
csrwng
left a comment
There was a problem hiding this comment.
LGTM — clean, minimal fix. All 6 return paths correctly flip requeueSoon from false to true for transiently nil resources. Requeue goes through AddRateLimited (exponential backoff starting at 5ms, capped at 1000s), so no risk of tight loops or API server pressure. The controller only reads secrets/ConfigMaps from the local informer cache at these points, so there's no contention with the control plane operator. Tests are well-structured and cover the exercisable paths.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: csrwng, jparrill The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
csrwng
added
the
acknowledge-critical-fixes-only
|
Scheduling tests matching the |
|
/retest-required |
|
/verified by e2e |
openshift-ci-robot
added
the
verified
|
@jparrill: This PR has been marked as verified by 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. |
|
Failures are infrastructure-related, unrelated to this PR's CRR requeue fix:
|
|
/retest-required Infra issue... |
|
@jparrill: 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. |
|
@jparrill: Jira Issue Verification Checks: Jira Issue OCPBUGS-98461 Jira Issue OCPBUGS-98461 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 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. |
|
Fix included in release 5.0.0-0.nightly-2026-07-16-201519 |
6 participants
Summary
requeueSoonfromfalsetotruein 6 return paths acrossensureOldSignerCertificateRevokedandensureNewSignerCertificatePropagatedwhere a resource is transiently nilFixes
Root Cause
CertificateRevocationRequest operations stall at
PreviousCertificatesRevoked=Falsefor 10+ minutes (3/8 runs, Jul 8-10, most frequent CI failure). #8563 fixed one instance of this class of bug (stale trust bundle cache returningrequeueSoon=false), but three other return paths in each function have the same issue::690,:1011secretForSignerClassnot foundrequeueSoon=falserequeueSoon=true:698,:1018requeueSoon=falserequeueSoon=true:717,:1029requeueSoon=falserequeueSoon=trueWhen any of these resources is transiently unavailable (being re-created, not yet propagated), the controller returned
needsWork=true, requeueSoon=false— waiting for a watch event that might never come. WithrequeueSoon=true, it retries on the next synthetic requeue.Frequency
3/8 runs in Jul 8-10 window. Most frequent issue in
e2e-aws-ovn. One CRR completed in 2min while a parallel one in the same run stalled for 10min+ — consistent with a transient resource availability race.Test plan
go test ./control-plane-pki-operator/certificaterevocationcontroller/— all passmake lint— 0 issuesmake verify— clean (except expected uncommitted files)/cc @csrwng
🤖 Generated with Claude Code
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
Tests