Skip to content

CNTRLPLANE-3532: migrate HO karpenter status patch to statuspatching helper#8968

Open
vsolanki12 wants to merge 1 commit into
openshift:mainfrom
vsolanki12:vsolanki/cntrlplane-3532-ho-statuspatching
Open

CNTRLPLANE-3532: migrate HO karpenter status patch to statuspatching helper#8968
vsolanki12 wants to merge 1 commit into
openshift:mainfrom
vsolanki12:vsolanki/cntrlplane-3532-ho-statuspatching

Conversation

@vsolanki12

@vsolanki12 vsolanki12 commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

What this PR does / why we need it:

Migrates the manual MergeFrom status patch in reconcileKarpenterOperator to use statuspatching.PatchStatus, adding optimistic locking and retry-on-conflict for the AutoNode status clearing path.

This was a Tier 2 site — it used MergeFrom without 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:

  • This is a single-site migration in the HO. Route status patches will be added in a follow-up.

Checklist:

  • Subject and description added to both, commit and PR.
  • Relevant issues have been referenced.
  • This change includes docs.
  • This change includes unit tests.

Summary by CodeRabbit

  • Bug Fixes
    • Improved handling of cluster autoscaling status when Karpenter is disabled, ensuring the node autoscaling state is cleared reliably.
    • Made status updates more resilient so failures are surfaced cleanly if the update cannot be applied.

@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Pipeline controller notification
This repo is configured to use the pipeline controller. Second-stage tests will be triggered either automatically or after lgtm label is added, depending on the repository configuration. The pipeline controller will automatically detect which contexts are required and will utilize /test Prow commands to trigger the second stage.

For optional jobs, comment /test ? to see a list of all defined jobs. To trigger manually all jobs from second stage use /pipeline required command.

This repository is configured in: LGTM mode

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jul 9, 2026
@openshift-ci-robot

openshift-ci-robot commented Jul 9, 2026

Copy link
Copy Markdown

@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.

Details

In response to this:

What this PR does / why we need it:

Migrates the manual MergeFrom status patch in reconcileKarpenterOperator to use statuspatching.PatchStatus, adding optimistic locking and retry-on-conflict for the AutoNode status clearing path.

This was a Tier 2 site — it used MergeFrom without 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:

  • This is a single-site migration in the HO. Route status patches will be added in a follow-up.

Checklist:

  • Subject and description added to both, commit and PR.
  • Relevant issues have been referenced.
  • This change includes docs.
  • This change includes unit tests.

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 openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 9, 2026
@openshift-ci

openshift-ci Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@coderabbitai

coderabbitai Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

The hostedcluster Karpenter reconciler now resets HCP.Status.AutoNode through statuspatching.PatchStatus when Karpenter is disabled. The code removes the manual deep copy and direct Client.Status().Patch(...) flow, and performs the status update inside the helper callback while returning any patch error.

🚥 Pre-merge checks | ✅ 4 | ❌ 7

❌ Failed checks (7 inconclusive)

Check name Status Explanation Resolution
Stable And Deterministic Test Names ❓ Inconclusive Repository clone failed, so this custom check could not run with code access. Retry the review run. If this persists, inspect pre-merge custom-check logs for infrastructure or agent runtime failures.
Test Structure And Quality ❓ Inconclusive Repository clone failed, so this custom check could not run with code access. Retry the review run. If this persists, inspect pre-merge custom-check logs for infrastructure or agent runtime failures.
Topology-Aware Scheduling Compatibility ❓ Inconclusive Repository clone failed, so this custom check could not run with code access. Retry the review run. If this persists, inspect pre-merge custom-check logs for infrastructure or agent runtime failures.
Ipv6 And Disconnected Network Test Compatibility ❓ Inconclusive Repository clone failed, so this custom check could not run with code access. Retry the review run. If this persists, inspect pre-merge custom-check logs for infrastructure or agent runtime failures.
No-Weak-Crypto ❓ Inconclusive Repository clone failed, so this custom check could not run with code access. Retry the review run. If this persists, inspect pre-merge custom-check logs for infrastructure or agent runtime failures.
Container-Privileges ❓ Inconclusive Repository clone failed, so this custom check could not run with code access. Retry the review run. If this persists, inspect pre-merge custom-check logs for infrastructure or agent runtime failures.
No-Sensitive-Data-In-Logs ❓ Inconclusive Repository clone failed, so this custom check could not run with code access. Retry the review run. If this persists, inspect pre-merge custom-check logs for infrastructure or agent runtime failures.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: migrating the HO Karpenter status patch to the statuspatching helper.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@openshift-ci

openshift-ci Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: vsolanki12
Once this PR has been reviewed and has the lgtm label, please assign csrwng for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added area/hypershift-operator Indicates the PR includes changes for the hypershift operator and API - outside an OCP release and removed do-not-merge/needs-area labels Jul 9, 2026
…spatching 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>
@vsolanki12 vsolanki12 force-pushed the vsolanki/cntrlplane-3532-ho-statuspatching branch from 541d030 to 82ccf32 Compare July 9, 2026 13:57
@codecov

codecov Bot commented Jul 9, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 43.53%. Comparing base (1f2811f) to head (82ccf32).
⚠️ Report is 35 commits behind head on main.

Files with missing lines Patch % Lines
...ft-operator/controllers/hostedcluster/karpenter.go 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8968      +/-   ##
==========================================
- Coverage   43.53%   43.53%   -0.01%     
==========================================
  Files         771      771              
  Lines       95798    95799       +1     
==========================================
  Hits        41707    41707              
- Misses      51192    51193       +1     
  Partials     2899     2899              
Files with missing lines Coverage Δ
...ft-operator/controllers/hostedcluster/karpenter.go 74.66% <0.00%> (-0.51%) ⬇️
Flag Coverage Δ
cmd-support 37.22% <ø> (ø)
cpo-hostedcontrolplane 45.51% <ø> (ø)
cpo-other 45.10% <ø> (ø)
hypershift-operator 53.65% <0.00%> (-0.01%) ⬇️
other 32.08% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@vsolanki12 vsolanki12 marked this pull request as ready for review July 13, 2026 09:27
@openshift-ci openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 13, 2026
@openshift-ci openshift-ci Bot requested review from jparrill and muraee July 13, 2026 09:28
@openshift-ci

openshift-ci Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

@vsolanki12: all tests passed!

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

@hypershift-jira-solve-ci

hypershift-jira-solve-ci Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

The analysis is confirmed. Here is the final report:

Test Failure Analysis Complete

Job Information

  • Prow Job: codecov/patch
  • Build ID: 86784934108 (GitHub check run)
  • PR: #8968CNTRLPLANE-3532: migrate HO karpenter status patch to statuspatching helper
  • Target File: hypershift-operator/controllers/hostedcluster/karpenter.go

Test Failure Analysis

Error

codecov/patch: 0.00% of diff hit (target 43.53%)
Patch coverage is 0% with 4 lines in your changes missing coverage.

Summary

The codecov/patch check failed because none of the 4 new/modified lines introduced by the PR are exercised by any unit test. The PR refactors reconcileKarpenterOperator in karpenter.go to replace a direct client.MergeFrom + Status().Patch() call with the statuspatching.PatchStatus() helper. The old code at those same lines was already uncovered on main, but Codecov treats rewritten lines as new diff and now flags them. No test exists for reconcileKarpenterOperator — only TestResolveKarpenterFinalizer is covered in karpenter_test.go. The codecov/project check passed (43.53%), so only the stricter patch-level gate failed.

Root Cause

The reconcileKarpenterOperator function has zero unit test coverage — neither on main nor in this PR. The existing test file karpenter_test.go only covers TestResolveKarpenterFinalizer, not reconcileKarpenterOperator.

On the main branch, lines 56–58 (the MergeFrom/Status().Patch() block) were already uncovered, tracked as existing misses in the project baseline (51,192 misses). Because they weren't part of any recent diff, they never triggered the patch coverage check.

This PR replaces those 3 uncovered lines with 4 new lines (the statuspatching.PatchStatus() call with its closure). Codecov treats all modified/added lines as "new diff" and requires them to be covered by tests. Since reconcileKarpenterOperator is never called in any unit test, all 4 diff lines show 0% coverage, failing the patch coverage check.

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
  1. Add a unit test for reconcileKarpenterOperator in karpenter_test.go that covers the "karpenter disabled + stale AutoNode status" code path. The test should:

    • Create a fake ControlPlaneContext with a non-empty HCP.Status.AutoNode
    • Ensure karpenterutil.IsKarpenterEnabled() returns false
    • Call reconcileKarpenterOperator and verify AutoNode status is cleared via the statuspatching.PatchStatus() helper
  2. Alternatively, if the team considers this a trivial mechanical refactor not warranting new tests, a maintainer can override the codecov/patch check — it is typically advisory in the hypershift repo (codecov/project already passed at 43.53%). However, adding the test is preferred since it closes a pre-existing gap.

  3. Long-term: Consider adding broader test coverage for reconcileKarpenterOperator, since the function contains multiple untested branches (karpenter enabled/disabled, finalizer resolution, deployment reconciliation) — the file is at 74.66% overall but 0% on this specific reconciliation path.

Evidence
Evidence Detail
Codecov patch result 0.00% patch coverage — 4 lines in diff, 0 lines hit
Codecov project result 43.53% (−0.01%) — passed (project check is green)
File coverage delta karpenter.go: 74.66% → 74.66% (−0.51%)
Changed file hypershift-operator/controllers/hostedcluster/karpenter.go (+5 −3), lines 57–61
Diff content Replaced client.MergeFrom + Status().Patch() with statuspatching.PatchStatus() helper
Test file karpenter_test.go — contains only TestResolveKarpenterFinalizer, no test for reconcileKarpenterOperator
Old code coverage Lines 56–58 on main were already uncovered (part of 51,192 project misses)
Coverage diff Files: 771 (unchanged), Lines: 95,798 → 95,799 (+1), Misses: 51,192 → 51,193 (+1)
Codecov flag impacted hypershift-operator: 53.65% (−0.01%)
Base commit 1f2811f
Head commit 82ccf32

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/hypershift-operator Indicates the PR includes changes for the hypershift operator and API - outside an OCP release jira/valid-reference Indicates that this PR references a valid Jira ticket of any type.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants