CNTRLPLANE-3532: migrate HCCO/route status patches to statuspatching - #9385
CNTRLPLANE-3532: migrate HCCO/route status patches to statuspatching#9385vsolanki12 wants to merge 2 commits into
Conversation
…uspatching Replace the raw MergeFromWithOptimisticLock status patch in destroyCloudResources with statuspatching.PatchStatusCondition, adding retry-on-conflict. The condition is derived from a live guest-cluster check on each call, so replaying it on retry is safe even if CPO's timeout fallback landed on the object in between. Part of the broader CNTRLPLANE-3532 status-patching migration. Signed-off-by: Vimal Solanki <vsolanki@redhat.com>
…tching removeHCPIngressFromRoutes used a bare client.MergeFrom with no optimistic lock at all. Switch to statuspatching.PatchStatus and move the ingress filtering inside the mutate closure so it recomputes against whatever is freshly fetched on each retry, rather than replaying a value captured before the patch. Part of the broader CNTRLPLANE-3532 status-patching migration. Signed-off-by: Vimal Solanki <vsolanki@redhat.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
@vsolanki12: This pull request references CNTRLPLANE-3532 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.1.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 YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughRoute ingress cleanup now uses 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 |
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: vsolanki12 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9385 +/- ##
==========================================
- Coverage 46.13% 46.12% -0.01%
==========================================
Files 783 783
Lines 98377 98375 -2
==========================================
- Hits 45382 45380 -2
- Misses 49919 49920 +1
+ Partials 3076 3075 -1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
@vsolanki12: The following test 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. |
What this PR does / why we need it:
Completes 2 of the remaining CNTRLPLANE-3532 status-patching sites flagged during #8966's review:
resources.go:destroyCloudResources'sCloudResourcesDestroyedcondition patch — was rawMergeFromWithOptimisticLock(correct locking, no retry). Migrated tostatuspatching.PatchStatusCondition.hostedcontrolplane_controller.go:removeHCPIngressFromRoutes's route-ingress status patch — was bareclient.MergeFromwith no optimistic lock at all, the worst of the remaining sites. Migrated tostatuspatching.PatchStatus, with the ingress-filtering logic moved inside the mutate closure so it recomputes against whatever is freshly fetched on retry instead of replaying a value captured beforehand.Part of the broader CNTRLPLANE-3532 migration. Independent of #8966 — different functions/files, no overlap, no ordering dependency.
Which issue(s) this PR fixes:
Part of CNTRLPLANE-3532 — does not close it, see remaining work below.
Special notes for your reviewer:
resourcesDestroyedCondis computed synchronously from a live guest-cluster check (ensureCloudResourcesDestroyed) right before the patch — traced the full call chain, it depends only on Spec + live checks, never onhcp.Status. So replaying it onPatchStatusCondition's retry is safe even if CPO's fallback timeout condition landed on the object in between — HCCO is the authoritative writer for this condition when reachable, and this is intentionally a "last-live-write-wins" pattern, not a race.make lintintegration;AGENTS.mdguidance update;support/statuspatchingJSON Patch (RFC 6902) variant + nullable-field test;resources.go:1125(reconcileConfig's Infrastructure status update) — same "no retry-on-conflict" bug class, different resource/function, found during review of this PR but out of scope here.TestDestroyCloudResources*,TestRemoveHCPIngressFromRoutes) already exercise both call sites end-to-end against a real fake client and pass unchanged; the shared helper's retry-on-conflict mechanics are covered separately insupport/statuspatching/statuspatching_test.go.Checklist:
Summary by CodeRabbit