CNTRLPLANE-3532: migrate HO karpenter status patch to statuspatching helper - #8968
CNTRLPLANE-3532: migrate HO karpenter status patch to statuspatching helper#8968vsolanki12 wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@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.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. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughWhen Karpenter is disabled, the hostedcluster reconciler clears stale Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 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 |
openshift-ci
Bot
added
area/hypershift-operator
541d030 to
82ccf32
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8968 +/- ##
=======================================
Coverage 45.77% 45.77%
=======================================
Files 781 781
Lines 97864 97864
=======================================
+ Hits 44794 44797 +3
+ Misses 50003 50000 -3
Partials 3067 3067
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
The analysis is confirmed. Here is the final report: Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe Root CauseThe On the This PR replaces those 3 uncovered lines with 4 new lines (the The specific uncovered lines (57–61 in the new file) are: if err := statuspatching.PatchStatus(cpContext, cpContext.Client, cpContext.HCP, func() error {
cpContext.HCP.Status.AutoNode = hyperv1.AutoNodeStatus{}
return nil
}); err != nil {This is a pre-existing coverage gap exposed by the refactor, not a new regression introduced by the PR. Recommendations
Evidence
|
|
Dropped some comments. Thanks The "This change includes unit tests" checkbox is checked, but the PR doesn't add any test code. {
name: "When karpenter is disabled and AutoNode status is stale it should clear it",
// setup HCP with non-empty AutoNode status, karpenter disabled
// verify HCP.Status.AutoNode == AutoNodeStatus{} after reconcile
} |
| @@ -53,9 +54,10 @@ func (r *HostedClusterReconciler) reconcileKarpenterOperator(cpContext controlpl | |||
| // while it was running. Since the karpenter-operator only runs when enabled, it cannot | |||
| // clear this itself. | |||
| if cpContext.HCP.Status.AutoNode != (hyperv1.AutoNodeStatus{}) { | |||
There was a problem hiding this comment.
The pre-check cpContext.HCP.Status.AutoNode != (hyperv1.AutoNodeStatus{}) runs before PatchStatus, so it evaluates the potentially stale in-memory version. That's fine — if another controller cleared AutoNode between the check and the PatchStatus call, the helper's no-op detection (DeepEqual after re-fetch) catches it and skips the API call. Just confirming the race is handled correctly. 👍
There was a problem hiding this comment.
Correct — the pre-check is a cheap guard to skip the PatchStatus call entirely when there's nothing to clear. If another controller races and clears it between the check and the re-fetch inside PatchStatus, the DeepEqual no-op detection handles it gracefully.
Added a unit test (TestClearAutoNodeStatusViaPatchStatus) that verifies the AutoNode status is correctly cleared through the helper.
AI-assisted response via Claude Code
|
Thanks for the review @jparrill! Added |
82ccf32 to
fe313ad
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@hypershift-operator/controllers/hostedcluster/karpenter_test.go`:
- Line 464: Rename the test function TestClearAutoNodeStatusViaPatchStatus to
follow the required “When ... it should ...” naming format while preserving its
existing test behavior and coverage.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: ca7e73bf-a3f4-4307-944e-7f0d60039b24
📒 Files selected for processing (2)
hypershift-operator/controllers/hostedcluster/karpenter.gohypershift-operator/controllers/hostedcluster/karpenter_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- hypershift-operator/controllers/hostedcluster/karpenter.go
|
/retest |
|
/approve |
openshift-ci
Bot
added
the
approved
fe313ad to
a9a7d0f
Compare
|
/test security |
a9a7d0f to
a5212ca
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Scheduling tests matching the |
011d91c to
e373a25
Compare
|
Scheduling tests matching the |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
/retest-required |
e373a25 to
34a5853
Compare
openshift-ci
Bot
removed
lgtm
|
/approve |
|
@muraee: changing LGTM is restricted to collaborators 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. |
|
/lgtm |
|
@muraee: changing LGTM is restricted to collaborators 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. |
|
/lgtm |
|
Scheduling tests matching the |
bryan-cox
left a comment
bryan-cox
left a comment
There was a problem hiding this comment.
Late pass — migration looks right, PatchStatus is the correct target here. Three nits, none blocking.
Commit subject carries a Jira ID. DEVELOPMENT.md:127: "Do NOT put Jira IDs in commit messages — they belong only in PR titles."
Other two inline.
| } | ||
| } | ||
|
|
||
| func TestWhenKarpenterDisabledAndStaleAutoNodeStatusItShouldClearStatus(t *testing.T) { |
There was a problem hiding this comment.
TESTING.md:34 — test functions are named after the function under test, 1:1. This tests reconcileKarpenterOperator, so: TestReconcileKarpenterOperator.
Flagging that the earlier thread resolved this the wrong way: CodeRabbit cited the "When … it should …" rule, but that's TESTING.md:9, which governs table-case names (the name: field), not function names. The rename moved this away from the rule. @mehabhalodiya's TestReconcileKarpenterOperator_When... satisfied both.
Conforming shape is the scenario as a table case under TestReconcileKarpenterOperator:
name: "When karpenter is disabled and AutoNode status is stale, it should clear the status",There was a problem hiding this comment.
Done. Renamed to TestReconcileKarpenterOperator with the scenario as a table case: "When karpenter is disabled and AutoNode status is stale, it should clear the status".
| @@ -53,9 +54,10 @@ func (r *HostedClusterReconciler) reconcileKarpenterOperator(cpContext controlpl | |||
| // while it was running. Since the karpenter-operator only runs when enabled, it cannot | |||
| // clear this itself. | |||
| if cpContext.HCP.Status.AutoNode != (hyperv1.AutoNodeStatus{}) { | |||
There was a problem hiding this comment.
Following up on @jparrill's thread rather than opening a new one — I think the resolution covers only one direction.
Agreed reasoning: cached non-empty → live empty → PatchStatus re-fetches, DeepEqual skips. Correct.
Not covered: cached empty → live non-empty. The guard reads cpContext.HCP, populated by createOrUpdate at hostedcluster_controller.go:1600, many API calls earlier. If the karpenter-operator wrote AutoNode in that window, the if is false, we never enter PatchStatus, the re-fetch never happens, and stale status survives. The optimistic lock can't help — we never reach it.
Self-heals next reconcile, so low severity. Either drop the guard (the helper's DeepEqual already makes the no-op path cost one Get) or comment that it's best-effort and correctness relies on requeue.
There was a problem hiding this comment.
Good catch. Dropped the guard entirely — PatchStatus re-fetches and no-ops via DeepEqual when there's nothing to clear, so it wasn't saving real work, only introducing the staleness risk. Added a comment explaining why and a no-op test case.
…atching helper Replace the manual MergeFrom status patch in reconcileKarpenterOperator with statuspatching.PatchStatus, adding optimistic locking and retry-on-conflict for the AutoNode status clearing path. Signed-off-by: Vimal Solanki <vsolanki@redhat.com>
34a5853 to
a67a776
Compare
|
New changes are detected. LGTM label has been removed. |
|
@bryan-cox Done — reworded commit subject to |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mehabhalodiya, muraee, 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 |
openshift-ci
Bot
added
the
approved
|
@vsolanki12: 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. |
7 participants
What this PR does / why we need it:
Migrates the manual
MergeFromstatus patch inreconcileKarpenterOperatorto usestatuspatching.PatchStatus, adding optimistic locking and retry-on-conflict for the AutoNode status clearing path.This was a Tier 2 site — it used
MergeFromwithout optimistic lock and without retry. The migration adds both.Part of the broader CNTRLPLANE-3532 migration. Depends on PR #8782 (merged).
Which issue(s) this PR fixes:
Fixes CNTRLPLANE-3532
Special notes for your reviewer:
Checklist:
Summary by CodeRabbit
Bug Fixes
Tests