OCPBUGS-93462: Fix stale resourceVersion in HCCO patchHCPStatusCondition#8902
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci-robot
added
jira/severity-important
|
@vsolanki12: This pull request references Jira Issue OCPBUGS-93462, 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. |
openshift-ci-robot
added
the
jira/valid-bug
openshift-ci
Bot
added
do-not-merge/needs-area
area/control-plane-operator
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis change updates the hosted cluster config operator’s HostedControlPlane condition patching flow. The restored-cluster reconcile path now calls Sequence Diagram(s)sequenceDiagram
participant Reconciler
participant patchHCPStatusCondition
participant statuspatching.PatchStatusCondition
participant KubeAPIServer
Reconciler->>patchHCPStatusCondition: patchHCPStatusCondition(condition)
patchHCPStatusCondition->>statuspatching.PatchStatusCondition: delegate patching
statuspatching.PatchStatusCondition->>KubeAPIServer: read/patch HostedControlPlane status
KubeAPIServer-->>statuspatching.PatchStatusCondition: patch result
statuspatching.PatchStatusCondition-->>patchHCPStatusCondition: result
patchHCPStatusCondition-->>Reconciler: return result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 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 |
|
@vsolanki12: This pull request references Jira Issue OCPBUGS-93462, 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. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8902 +/- ##
==========================================
+ Coverage 43.28% 43.45% +0.17%
==========================================
Files 771 771
Lines 95503 95707 +204
==========================================
+ Hits 41335 41589 +254
+ Misses 51284 51232 -52
- Partials 2884 2886 +2
... and 14 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
I have tried to test on my test cluster: Before fix: After fix: |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
/lgtm |
|
Scheduling tests matching the |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
Test Resultse2e-aws
Failed TestsTotal failed tests: 35
... and 30 more failed tests e2e-aks
|
cblecker
left a comment
cblecker
left a comment
There was a problem hiding this comment.
reconcileClusterRecovery (around line 585) has the same stale-resourceVersion pattern this PR fixes — it deep-copies the potentially stale hcp, mutates the condition, and patches with MergeFromWithOptimisticLock without re-fetch or retry on conflict.
Since patchHCPStatusCondition now delegates to the shared helper and the import is already here, would it make sense to convert this too?
condition := metav1.Condition{
Type: string(hyperv1.HostedClusterRestoredFromBackup),
Status: metav1.ConditionTrue,
Reason: "Restored",
Message: "This hosted cluster has been restored from a backup.",
ObservedGeneration: hcp.Generation,
}
return ctrl.Result{}, r.patchHCPStatusCondition(ctx, hcp, &condition)| // It only performs the API call if the condition actually changed. | ||
| // Delegates to statuspatching.PatchStatusCondition which re-fetches the HCP on each attempt, | ||
| // avoiding stale resourceVersion conflicts when concurrent controllers update HCP status. | ||
| func (r *reconciler) patchHCPStatusCondition(ctx context.Context, hcp *hyperv1.HostedControlPlane, condition *metav1.Condition) error { |
There was a problem hiding this comment.
The old implementation wrapped errors with the condition type (fmt.Errorf("failed to patch HCP status with %s condition: %w", condition.Type, err)), which was helpful for debugging since this controller manages ~13 distinct conditions per reconcile cycle. The new one-liner returns the raw error without that context, and 12 of 13 call sites don't add their own wrapping.
Consider preserving it here:
func (r *reconciler) patchHCPStatusCondition(ctx context.Context, hcp *hyperv1.HostedControlPlane, condition *metav1.Condition) error {
if err := statuspatching.PatchStatusCondition(ctx, r.cpClient, hcp, &hcp.Status.Conditions, *condition); err != nil {
return fmt.Errorf("failed to patch HCP status with %s condition: %w", condition.Type, err)
}
return nil
}There was a problem hiding this comment.
Done. Preserved the error wrapping with condition type context around the helper call.
The HCCO patchHCPStatusCondition method operated on a stale HCP object fetched once at reconciliation start. When concurrent controllers bumped the resourceVersion between fetch and patch, the optimistic lock returned a 409 conflict that was not retried, silently dropping condition updates. Replace with statuspatching.PatchStatusCondition which re-fetches the HCP on each attempt and retries on conflict automatically. Signed-off-by: Vimal Solanki <vsolanki@redhat.com>
|
@cblecker Good catch on AI-assisted response via Claude Code |
vsolanki12
force-pushed
the
vsolanki/ocpbugs-93462-fix-stale-rv
branch
from
ac20bf8 to
9f72cbe
Compare
|
/lgtm |
|
Scheduling tests matching the |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
|
/retest |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
|
Confirmed: TestHAEtcdChaos and TestPullSecretUnavailable (LoadBalancer-based) passed. All Route-based tests failed. Now I have all the evidence needed. Let me produce the final report. Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryAll 17 test failures share a single root cause: the External DNS controller on the AKS management cluster failed to create DNS A-records in the Azure DNS zone Root CauseThe External DNS controller in the AKS management cluster is not creating DNS records for hosted clusters that use Route-based service publishing with custom hostnames under Failure chain:
Why PR #8902 is NOT the cause:
Recommendations
Evidence
|
|
/approve |
|
/retest |
csrwng
added
the
approved
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: clebs, vsolanki12 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 |
|
/verified by @vsolanki12 |
openshift-ci-robot
added
the
verified
|
@vsolanki12: 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. |
|
/label acknowledge-critical-fixes-only |
openshift-ci
Bot
added
the
acknowledge-critical-fixes-only
|
@vsolanki12: 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. |
openshift-merge-bot
Bot
merged commit bbc1bc5
into
openshift:main
|
@vsolanki12: Jira Issue Verification Checks: Jira Issue OCPBUGS-93462 Jira Issue OCPBUGS-93462 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. |
6 participants
What does this PR do?
Replaces the inline
patchHCPStatusConditionimplementation in the HCCO resources controller with the sharedstatuspatching.PatchStatusConditionhelper introduced in PR #8782.The old implementation operated on a stale HCP object fetched once at reconciliation start. When concurrent controllers (reencryption, hcpstatus) bumped the resourceVersion between fetch and patch, the optimistic lock returned a 409 conflict that was not retried, silently dropping condition updates like
ConfigOperatorReconciliationSucceeded.The new helper re-fetches the HCP on each attempt and retries on conflict automatically, fixing the root cause.
Jira
OCPBUGS-93462
Summary by CodeRabbit