OCPBUGS-89689: (karpenter) use completed release image for unpinned NodeClaims during CP upgrade

[judexzhu]

Summary

• During a CP upgrade, hcp.Spec.ReleaseImage flips to the desired version immediately. The KarpenterIgnition controller was reading this for unpinned NodeClaims, causing premature drift detection and worker replacement before CP upgrade completed.
• Replace currentClusterVersion() with currentClusterReleaseImage() that returns both release image and version from the most recently completed history entry.
• Unpinned NodeClaims now wait for CP upgrade to complete before detecting drift. Pinned NodeClaims (with spec.version) are unaffected.

Test plan

• Unit test: TestCurrentClusterReleaseImage — 8 cases including upgrade-in-progress scenario (Partial + Completed history entries)
• Unit test: existing TestReconcile, TestReconcileVersionResolution, TestResolveVersion, TestReconcileKubeletConfigMap — all pass with updated fixtures
• go vet clean
• E2E: initiate CP upgrade on cluster with unpinned OpenshiftEC2NodeClass, verify NodeClaims do not drift until CP completes

Fixes: OCPBUGS-89689

🤖 Generated with Claude Code

Summary by CodeRabbit

Summary

• Bug Fixes

  • Improved reconciliation version tracking by deriving both release image and version from the most recently completed history entry.
  • Standardized fallback behavior to use desired release image/version when no completed history entries are available.

• Tests

  • Updated controller test fixtures to include image for completed history entries and added coverage for release-image+version resolution.
  • Refined control-plane upgrade e2e drift checks to confirm no drift during rollout, then validate drift detection after upgrade (via a new “not drifted” assertion helper).

[openshift-merge-bot]

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

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

The controller now derives both release image and version from the most recent completed HostedCluster history entry, with updated fallback handling when no completed entry exists or status version is missing. Reconcile uses that combined result instead of mixing spec release image with a separate version lookup. Tests were updated to include image values in completed history fixtures, replace the version-only helper test with image-and-version assertions, and sequence the e2e upgrade drift check around image rollout completion.

🚥 Pre-merge checks | ✅ 9 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Test Structure And Quality	⚠️ Warning	assertNodeClaimsNotDrifted uses a 10s ticker, ignores GET/condition errors, and waits on noDriftDone without a timeout, so it can miss early drift or hang.	Replace it with a Consistently/Eventually-style helper with an initial scan, explicit timeout, and hard-fail on fetch/condition errors.
Ipv6 And Disconnected Network Test Compatibility	⚠️ Warning	TestKarpenterUpgradeControlPlane calls testWorkload(), which hardcodes quay.io/openshift/origin-pod:4.22.0, requiring a public registry pull.	Use an internal/mirrored image or make the workload image configurable from a mirrored source; add [Skipped:Disconnected] if external pull is unavoidable.

✅ Passed checks (9 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.
Stable And Deterministic Test Names	✅ Passed	PASS: The edited test titles are static strings; no It/Describe/Context/When names use dynamic values. Dynamic fmt.Sprintf text appears only in assertion labels, not titles.
Topology-Aware Scheduling Compatibility	✅ Passed	Changes only adjust release-image/version resolution and e2e drift timing; no new topology-sensitive affinity, nodeSelector, spread, or PDB logic was added.
No-Weak-Crypto	✅ Passed	No MD5/SHA1/DES/RC4/etc., custom crypto, or non-constant-time secret/token compares appear in the changed files.
Container-Privileges	✅ Passed	PASS: The diff only changes release-image logic and e2e drift flow; no privileged/securityContext fields, hostPID/Network/IPC, or allowPrivilegeEscalation additions were introduced.
No-Sensitive-Data-In-Logs	✅ Passed	New log lines only mention upgrade state and NodeClaim names; no passwords, tokens, PII, or internal hostnames were added.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the bug fix and the use of the completed release image for unpinned NodeClaims during control-plane upgrades.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

[coderabbitai]

🧹 Nitpick comments (1)

karpenter-operator/controllers/karpenterignition/karpenterignition_controller.go (1)

343-376: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

Nil CompletionTime tie-break could silently pick a stale entry.

If a CompletedUpdate entry with a nil CompletionTime is followed by another completed entry with a non-nil one, the entry.CompletionTime != nil && latest.CompletionTime != nil guard skips promotion, so the entry actually carrying a timestamp never replaces the nil one. In practice this is unlikely to occur since the cluster-version-operator always backfills CompletionTime before marking an entry Completed, but it's a low-cost hardening given this exact code path is what prevents premature drift detection.

🛡️ Optional hardening

 		if latest == nil {
 			latest = entry
 			continue
 		}
-		if entry.CompletionTime != nil && latest.CompletionTime != nil && entry.CompletionTime.After(latest.CompletionTime.Time) {
-			latest = entry
-		}
+		switch {
+		case entry.CompletionTime == nil:
+			// keep latest; nothing to compare against
+		case latest.CompletionTime == nil:
+			latest = entry
+		case entry.CompletionTime.After(latest.CompletionTime.Time):
+			latest = entry
+		}

🤖 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
`@karpenter-operator/controllers/karpenterignition/karpenterignition_controller.go`
around lines 343 - 376, The current `currentClusterReleaseImage` selection logic
can keep a stale `CompletedUpdate` when `latest.CompletionTime` is nil and a
later completed entry has a timestamp, because the comparison only promotes when
both times are non-nil. Update the `currentClusterReleaseImage` loop to treat a
non-nil `CompletionTime` as newer than a nil one, while still selecting the most
recent completed history entry in `hostedCluster.Status.Version.History` and
preserving the existing fallback to `hostedCluster.Spec.Release.Image` and
`Desired.Version`.

🤖 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
`@karpenter-operator/controllers/karpenterignition/karpenterignition_controller.go`:
- Around line 343-376: The current `currentClusterReleaseImage` selection logic
can keep a stale `CompletedUpdate` when `latest.CompletionTime` is nil and a
later completed entry has a timestamp, because the comparison only promotes when
both times are non-nil. Update the `currentClusterReleaseImage` loop to treat a
non-nil `CompletionTime` as newer than a nil one, while still selecting the most
recent completed history entry in `hostedCluster.Status.Version.History` and
preserving the existing fallback to `hostedCluster.Spec.Release.Image` and
`Desired.Version`.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: b770e5aa-9ce8-4bdb-bc27-8ff0e439ef63

📥 Commits

Reviewing files that changed from the base of the PR and between 1aca754 and d7ff09a.

📒 Files selected for processing (2)

• karpenter-operator/controllers/karpenterignition/karpenterignition_controller.go
• karpenter-operator/controllers/karpenterignition/karpenterignition_controller_test.go

[codecov]

Codecov Report

❌ Patch coverage is 73.68421% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 44.77%. Comparing base (29e8d5d) to head (3357cf4).
⚠️ Report is 39 commits behind head on main.

Files with missing lines	Patch %	Lines
.../karpenterignition/karpenterignition_controller.go	73.68%	4 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8957      +/-   ##
==========================================
- Coverage   44.77%   44.77%   -0.01%     
==========================================
  Files         775      775              
  Lines       97210    97214       +4     
==========================================
- Hits        43528    43527       -1     
- Misses      50681    50685       +4     
- Partials     3001     3002       +1     

Files with missing lines	Coverage Δ
.../karpenterignition/karpenterignition_controller.go	64.22% <73.68%> (-0.72%)	⬇️

Flag	Coverage Δ
cmd-support	38.42% <ø> (ø)
cpo-hostedcontrolplane	47.25% <ø> (ø)
cpo-other	45.45% <ø> (ø)
hypershift-operator	54.63% <ø> (ø)
other	34.29% <73.68%> (-0.03%)	⬇️

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.

[maxcao13]

/test e2e-aws-autonode

[maxcao13]

I think we are going to need to change how the e2e works. Right now, the e2e assumes the NodeClaim will Drift immediately after upgrading the HC release image version.. We need to only check after the CP upgrade completes since that's what the PR does now.

[coderabbitai]

🧹 Nitpick comments (1)

test/e2e/karpenter_control_plane_upgrade_test.go (1)

96-98: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

Sequential drift wait could add up for multiple NodeClaims.

Each waitForNodeClaimDrifted call has its own timeout (5 min, per the helper), and the loop now runs sequentially instead of the previous goroutine-based concurrent wait. With replicas := 1 today this is a single iteration, but if this test is later scaled to more replicas, total wait time grows linearly. Consider parallelizing with an errgroup if NodeClaim counts increase.
[optional_refactor]

🤖 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 `@test/e2e/karpenter_control_plane_upgrade_test.go` around lines 96 - 98, The
drift wait in the NodeClaim loop is now sequential, which will scale timeout
linearly if more than one NodeClaim is present. Update the waiting logic around
waitForNodeClaimDrifted to run concurrently again, ideally using an errgroup or
similar pattern, so each NodeClaim can be awaited in parallel while preserving
error handling in the test.

🤖 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 `@test/e2e/karpenter_control_plane_upgrade_test.go`:
- Around line 96-98: The drift wait in the NodeClaim loop is now sequential,
which will scale timeout linearly if more than one NodeClaim is present. Update
the waiting logic around waitForNodeClaimDrifted to run concurrently again,
ideally using an errgroup or similar pattern, so each NodeClaim can be awaited
in parallel while preserving error handling in the test.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: 43c0d2fa-8256-4406-af0c-80fee723685d

📥 Commits

Reviewing files that changed from the base of the PR and between d7ff09a and e5d3974.

📒 Files selected for processing (1)

• test/e2e/karpenter_control_plane_upgrade_test.go

[judexzhu]

/test e2e-aws-autonode

[maxcao13]

thanks!

/lgtm

/assign @enxebre

[openshift-merge-bot]

Scheduling tests matching the pipeline_run_if_changed or not excluded by pipeline_skip_if_only_changed parameters:
/test e2e-aks-4-22
/test e2e-aws-4-22
/test e2e-aks
/test e2e-aws
/test e2e-aws-upgrade-hypershift-operator
/test e2e-azure-v2-self-managed
/test e2e-kubevirt-aws-ovn-reduced
/test e2e-v2-aws
/test e2e-v2-gke

[cwbotbot]

Test Results

e2e-aws

• Status: ❌ FAIL
• Started: 2026-07-12T21:41:17Z
• View Job
• View Job History

Failed Tests

Total failed tests: 9

• TestKarpenterUpgradeControlPlane
• TestKarpenterUpgradeControlPlane/Main
• TestNodePool
• TestNodePool/HostedCluster0
• TestNodePool/HostedCluster0/Main

... and 4 more failed tests

e2e-aks

• Status: ✅ PASS
• Started: 2026-07-09T02:02:59Z
• View Job
• View Job History

[judexzhu]

/test e2e-aks-4-22
/test e2e-aws

[maxcao13]

nit: can we put all this logic in it's own function?

[maxcao13]

/pipeline-required
/test e2e-aws-autonode

[openshift-ci-robot]

@judexzhu: This pull request references Jira Issue OCPBUGS-89689, which is invalid:

• expected the bug to target the "5.0.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

Summary

• During a CP upgrade, hcp.Spec.ReleaseImage flips to the desired version immediately. The KarpenterIgnition controller was reading this for unpinned NodeClaims, causing premature drift detection and worker replacement before CP upgrade completed.
• Replace currentClusterVersion() with currentClusterReleaseImage() that returns both release image and version from the most recently completed history entry.
• Unpinned NodeClaims now wait for CP upgrade to complete before detecting drift. Pinned NodeClaims (with spec.version) are unaffected.

Test plan

• Unit test: TestCurrentClusterReleaseImage — 8 cases including upgrade-in-progress scenario (Partial + Completed history entries)
• Unit test: existing TestReconcile, TestReconcileVersionResolution, TestResolveVersion, TestReconcileKubeletConfigMap — all pass with updated fixtures
• go vet clean
• E2E: initiate CP upgrade on cluster with unpinned OpenshiftEC2NodeClass, verify NodeClaims do not drift until CP completes

Fixes: OCPBUGS-89689

🤖 Generated with Claude Code

Summary by CodeRabbit

Summary

• Bug Fixes

• Improved reconciliation version tracking by deriving both release image and version from the most recently completed history entry.

• Standardized fallback behavior to use desired release image/version when no completed history entries are available.

• Tests

• Updated controller test fixtures to include image for completed history entries and added coverage for release-image+version resolution.

• Refined control-plane upgrade e2e drift checks to confirm no drift during rollout, then validate drift detection after upgrade (via a new “not drifted” assertion helper).

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.

[bryan-cox]

/jira refresh

[openshift-ci-robot]

@bryan-cox: This pull request references Jira Issue OCPBUGS-89689, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

• bug is open, matching expected state (open)
• bug target version (5.0.0) matches configured target version for branch (5.0.0)
• bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

Details

In response to this:

/jira refresh

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]

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

[openshift-ci-robot]

@judexzhu: Jira Issue Verification Checks: Jira Issue OCPBUGS-89689
✔️ This pull request was pre-merge verified.
✔️ All associated pull requests have merged.
✔️ All associated, merged pull requests were pre-merge verified.

Jira Issue OCPBUGS-89689 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓

Details

In response to this:

Summary

• During a CP upgrade, hcp.Spec.ReleaseImage flips to the desired version immediately. The KarpenterIgnition controller was reading this for unpinned NodeClaims, causing premature drift detection and worker replacement before CP upgrade completed.
• Replace currentClusterVersion() with currentClusterReleaseImage() that returns both release image and version from the most recently completed history entry.
• Unpinned NodeClaims now wait for CP upgrade to complete before detecting drift. Pinned NodeClaims (with spec.version) are unaffected.

Test plan

• Unit test: TestCurrentClusterReleaseImage — 8 cases including upgrade-in-progress scenario (Partial + Completed history entries)
• Unit test: existing TestReconcile, TestReconcileVersionResolution, TestResolveVersion, TestReconcileKubeletConfigMap — all pass with updated fixtures
• go vet clean
• E2E: initiate CP upgrade on cluster with unpinned OpenshiftEC2NodeClass, verify NodeClaims do not drift until CP completes

Fixes: OCPBUGS-89689

🤖 Generated with Claude Code

Summary by CodeRabbit

Summary

• Bug Fixes

• Improved reconciliation version tracking by deriving both release image and version from the most recently completed history entry.

• Standardized fallback behavior to use desired release image/version when no completed history entries are available.

• Tests

• Updated controller test fixtures to include image for completed history entries and added coverage for release-image+version resolution.

• Refined control-plane upgrade e2e drift checks to confirm no drift during rollout, then validate drift detection after upgrade (via a new “not drifted” assertion helper).

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-merge-robot]

Fix included in release 5.0.0-0.nightly-2026-08-04-172547