OCPBUGS-86798: spot MHC not created when autoRepair=true and ignition endpoint not reached

[dpateriya]

Summary

• Fixes a deadlock where the spot-specific MachineHealthCheck is never created when autoRepair: true and ReachedIgnitionEndpoint is False
• Moves the spot MHC reconciliation before the autoRepair/ignition gate so it is always created when spot instances are enabled
• Adds dedicated test covering all combinations of spot/autoRepair/ignition state

Problem

The spot MHC block was placed after the autoRepair gate in CAPI.Reconcile(). When autoRepair: true and the ignition endpoint hasn't been reached yet, the function returns early via return nil at line 158, preventing the spot MHC from being created.

This creates a permanent deadlock when a spot instance fails to provision:

1. Spot instance fails → Machine stuck in Pending with InstanceProvisionFailed
2. No node joins → ReachedIgnitionEndpoint stays False forever
3. autoRepair: true + ignition not reached → return nil → spot MHC never created
4. Without spot MHC → no NodeStartupTimeout → no remediation → Machine stuck forever

Fix

Move the spot MHC reconciliation block before the autoRepair gate. The spot MHC is independent of autoRepair — it serves as a safety net for spot instance failures using maxUnhealthy: 100% and a 20-minute NodeStartupTimeout.

Reproduction

apiVersion: hypershift.openshift.io/v1beta1
kind: NodePool
spec:
  management:
    autoRepair: true
  platform:
    aws:
      placement:
        marketType: Spot

With spot capacity unavailable, the Machine stays Pending indefinitely and no <nodepool>-spot MHC is created.

Test plan

• New test TestSpotMHCCreatedIndependentlyOfAutoRepairIgnitionGate with 4 cases:
  • spot + autoRepair=true + ignition NOT reached → spot MHC exists, regular MHC does not
  • spot + autoRepair=true + ignition reached → both MHCs exist
  • spot + autoRepair=false → spot MHC exists, regular MHC does not
  • no spot + autoRepair=true + ignition NOT reached → no MHCs
• All existing TestCAPIReconcile tests pass (no regression)
• All TestReconcileMachineHealthCheck tests pass
• Full nodepool package tests pass
• go vet clean

Summary by CodeRabbit

• Bug Fixes

  • Spot instance health checks are now reconciled independently of auto-repair and ignition-gate state, ensuring spot-enabled node pools consistently have their own MHC created or removed as appropriate.

• Tests

  • Added table-driven tests covering spot MHC creation/removal and regular MHC behavior across combinations of spot, auto-repair, and ignition-endpoint conditions.

[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]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

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

Review profile: CHILL

Plan: Enterprise

Run ID: fc647cd4-e064-4801-bcc6-ffc4f7d41380

📥 Commits

Reviewing files that changed from the base of the PR and between a0f1b6d and 1a9eec3.

📒 Files selected for processing (2)

• hypershift-operator/controllers/nodepool/capi.go
• hypershift-operator/controllers/nodepool/capi_test.go

🚧 Files skipped from review as they are similar to previous changes (2)

• hypershift-operator/controllers/nodepool/capi.go
• hypershift-operator/controllers/nodepool/capi_test.go

---

📝 Walkthrough

Walkthrough

This PR decouples spot instance health checks from the auto-repair gating logic. The Reconcile method now creates or updates a spot-specific MachineHealthCheck early based solely on whether spot instances are enabled, and deletes it if spot is disabled. The previous conditional block tied to auto-repair and ignition-endpoint status was removed. A new table-driven test verifies spot MHC behavior across combinations of spot enablement, auto-repair setting, and ignition endpoint readiness.

Suggested reviewers

• muraee
• sdminonne

🚥 Pre-merge checks | ✅ 10 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Test Structure And Quality	⚠️ Warning	Test lacks assertion messages on 6 assertions (lines 3383, 3388, 3389, 3419, 3423, 3424) and has no cleanup code. Most assertions are missing meaningful failure diagnostic messages per requirement #4.	Add diagnostic messages to all assertions; add t.Cleanup() or proper cleanup for fake client resources per requirements #2 and #4.

✅ Passed checks (10 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main fix: moving spot MHC reconciliation before the autoRepair/ignition gate to resolve the deadlock condition.
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	The test uses standard Go table-driven tests (not Ginkgo). All test names are static, descriptive, and contain no dynamic information like pod names, timestamps, UUIDs, or node names.
Topology-Aware Scheduling Compatibility	✅ Passed	PR reconciles Cluster API MachineHealthCheck resources (infrastructure Machines, not Pods). No pod-level scheduling constraints introduced; changes are topology-agnostic.
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	The new test is a standard Go unit test using testing.T, not a Ginkgo e2e test, making this check inapplicable to the PR.
No-Weak-Crypto	✅ Passed	No weak cryptography, custom crypto implementations, or insecure secret comparisons found in the PR's code changes to spot MachineHealthCheck reconciliation logic.
Container-Privileges	✅ Passed	PR modifies only Go source code files (capi.go and capi_test.go) with no container/K8s manifests, privileged settings, or security context changes. Check is not applicable.
No-Sensitive-Data-In-Logs	✅ Passed	Spot MHC logging adds only enum results and K8s object keys (namespace/name), consistent with existing patterns; no passwords, tokens, PII, or customer data exposed.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ 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

• X
• Mastodon
• Reddit
• LinkedIn

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

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dpateriya
Once this PR has been reviewed and has the lgtm label, please assign cblecker 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:

• OWNERS

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

[openshift-ci-robot]

@dpateriya: This pull request references Jira Issue OCPBUGS-86798, 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

• Fixes a deadlock where the spot-specific MachineHealthCheck is never created when autoRepair: true and ReachedIgnitionEndpoint is False
• Moves the spot MHC reconciliation before the autoRepair/ignition gate so it is always created when spot instances are enabled
• Adds dedicated test covering all combinations of spot/autoRepair/ignition state

Problem

The spot MHC block was placed after the autoRepair gate in CAPI.Reconcile(). When autoRepair: true and the ignition endpoint hasn't been reached yet, the function returns early via return nil at line 158, preventing the spot MHC from being created.

This creates a permanent deadlock when a spot instance fails to provision:

1. Spot instance fails → Machine stuck in Pending with InstanceProvisionFailed
2. No node joins → ReachedIgnitionEndpoint stays False forever
3. autoRepair: true + ignition not reached → return nil → spot MHC never created
4. Without spot MHC → no NodeStartupTimeout → no remediation → Machine stuck forever

Fix

Move the spot MHC reconciliation block before the autoRepair gate. The spot MHC is independent of autoRepair — it serves as a safety net for spot instance failures using maxUnhealthy: 100% and a 20-minute NodeStartupTimeout.

Reproduction

apiVersion: hypershift.openshift.io/v1beta1
kind: NodePool
spec:
 management:
   autoRepair: true
 platform:
   aws:
     placement:
       marketType: Spot

With spot capacity unavailable, the Machine stays Pending indefinitely and no <nodepool>-spot MHC is created.

Test plan

• New test TestSpotMHCCreatedIndependentlyOfAutoRepairIgnitionGate with 4 cases:
• spot + autoRepair=true + ignition NOT reached → spot MHC exists, regular MHC does not
• spot + autoRepair=true + ignition reached → both MHCs exist
• spot + autoRepair=false → spot MHC exists, regular MHC does not
• no spot + autoRepair=true + ignition NOT reached → no MHCs
• All existing TestCAPIReconcile tests pass (no regression)
• All TestReconcileMachineHealthCheck tests pass
• Full nodepool package tests pass
• go vet clean

Summary by CodeRabbit

• Bug Fixes

• Spot instance monitoring now operates independently of auto-repair settings, ensuring consistent health checks for spot-enabled node pools.

• Tests

• Added comprehensive test coverage for spot instance health check behavior across different auto-repair configurations.

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.

[dpateriya]

/jira refresh

[openshift-ci-robot]

@dpateriya: This pull request references Jira Issue OCPBUGS-86798, 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.

[coderabbitai]

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/nodepool/capi_test.go`:
- Around line 3301-3334: The test case name strings in the table of cases (the
struct entries that include fields like name, autoRepair, ignitionReached,
spotEnabled, expectSpotMHC, expectRegularMHC, expectAutoRepairStatus) do not
follow the repo policy; update each name to the required format starting with
"When ..." and then "it should ..." (e.g. "When spot enabled and autoRepair true
and ignition NOT reached it should create spot MHC and not create regular MHC"),
preserving the rest of the case fields and semantics; change only the name
values in the test cases inside capi_test.go so they conform to the "When ... it
should ..." pattern.

🪄 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: f9c051ee-dacb-4f42-a86c-f7ad5906603b

📥 Commits

Reviewing files that changed from the base of the PR and between ab1e63b and be91e33.

📒 Files selected for processing (2)

• hypershift-operator/controllers/nodepool/capi.go
• hypershift-operator/controllers/nodepool/capi_test.go

[openshift-ci-robot]

@dpateriya: This pull request references Jira Issue OCPBUGS-86798, which is valid.

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 POST, which is one of the valid states (NEW, ASSIGNED, POST)

Details

In response to this:

Summary

• Fixes a deadlock where the spot-specific MachineHealthCheck is never created when autoRepair: true and ReachedIgnitionEndpoint is False
• Moves the spot MHC reconciliation before the autoRepair/ignition gate so it is always created when spot instances are enabled
• Adds dedicated test covering all combinations of spot/autoRepair/ignition state

Problem

The spot MHC block was placed after the autoRepair gate in CAPI.Reconcile(). When autoRepair: true and the ignition endpoint hasn't been reached yet, the function returns early via return nil at line 158, preventing the spot MHC from being created.

This creates a permanent deadlock when a spot instance fails to provision:

1. Spot instance fails → Machine stuck in Pending with InstanceProvisionFailed
2. No node joins → ReachedIgnitionEndpoint stays False forever
3. autoRepair: true + ignition not reached → return nil → spot MHC never created
4. Without spot MHC → no NodeStartupTimeout → no remediation → Machine stuck forever

Fix

Move the spot MHC reconciliation block before the autoRepair gate. The spot MHC is independent of autoRepair — it serves as a safety net for spot instance failures using maxUnhealthy: 100% and a 20-minute NodeStartupTimeout.

Reproduction

apiVersion: hypershift.openshift.io/v1beta1
kind: NodePool
spec:
 management:
   autoRepair: true
 platform:
   aws:
     placement:
       marketType: Spot

With spot capacity unavailable, the Machine stays Pending indefinitely and no <nodepool>-spot MHC is created.

Test plan

• New test TestSpotMHCCreatedIndependentlyOfAutoRepairIgnitionGate with 4 cases:
• spot + autoRepair=true + ignition NOT reached → spot MHC exists, regular MHC does not
• spot + autoRepair=true + ignition reached → both MHCs exist
• spot + autoRepair=false → spot MHC exists, regular MHC does not
• no spot + autoRepair=true + ignition NOT reached → no MHCs
• All existing TestCAPIReconcile tests pass (no regression)
• All TestReconcileMachineHealthCheck tests pass
• Full nodepool package tests pass
• go vet clean

Summary by CodeRabbit

• Bug Fixes

• Spot instance health checks are now reconciled independently of auto-repair or ignition-gate state, ensuring spot-enabled node pools consistently have their own MHC created or removed as appropriate.

• Tests

• New table-driven tests cover spot MHC creation and regular MHC behavior across combinations of spot, auto-repair, and ignition endpoint conditions.

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]

@dpateriya: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/images	1a9eec3	link	true	/test images
ci/prow/okd-scos-images	1a9eec3	link	true	/test okd-scos-images

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]

PR needs rebase.

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.

[hypershift-jira-solve-ci]

Now I have the complete picture. Let me compile the final report. Here's a summary of all failures found:

1. Tide: ERROR — "Not mergeable. PR has a merge conflict." This is the primary blocker the user asked about.
2. ci/prow/images: FAILURE — imagestreams.image.openshift.io "5.0" not found (transient CI infrastructure issue — resolved on newer PRs)
3. ci/prow/okd-scos-images: FAILURE — imagestreams.image.openshift.io "scos-4.21" not found (same transient CI infrastructure issue)
4. lint: FAILURE — Two lint errors in the PR code: (a) gci import ordering issue in capi_test.go:3467, (b) staticcheck SA1019 for using deprecated spotMHC.Spec.MaxUnhealthy field
5. verify: FAILURE — capi_test.go: needs update (file formatting not up to date, related to the gci issue)
6. gitlint: FAILURE — Commit message does not start with a conventional prefix (fix, feat, chore, etc.)

Test Failure Analysis Complete

Job Information

• Prow Job: tide (merge status check)
• Build ID: N/A (tide is a merge controller, not a build job)
• PR: #8645 — OCPBUGS-86798: spot MHC not created when autoRepair=true and ignition endpoint not reached
• Author: dpateriya
• Branch: fix/spot-mhc-autorepair-deadlock
• Failed Checks: tide (ERROR), ci/prow/images, ci/prow/okd-scos-images, lint, verify, gitlint

Test Failure Analysis

Error

tide: Not mergeable. PR has a merge conflict.
lint: hypershift-operator/controllers/nodepool/capi_test.go:3467:1: File is not properly formatted (gci)
lint: hypershift-operator/controllers/nodepool/capi_test.go:3486:14: SA1019: spotMHC.Spec.MaxUnhealthy is deprecated (staticcheck)
verify: hypershift-operator/controllers/nodepool/capi_test.go: needs update
gitlint: Title does not start with one of fix, feat, chore, docs, style, refactor, perf, test, revert, ci, build
ci/prow/images: could not resolve source imagestream ocp/5.0 for release latest
ci/prow/okd-scos-images: could not resolve source imagestream origin/scos-4.21 for release latest

Summary

PR #8645 has six independent failures across three categories. The primary blocker is a merge conflict — the PR's mergeStateStatus is DIRTY and mergeable is CONFLICTING, which causes tide to report an ERROR state and refuse to merge. Beyond the merge conflict, the PR has two lint errors introduced by the new test code: (1) a gci import ordering violation at line 3467 of capi_test.go, and (2) use of the deprecated Spec.MaxUnhealthy field on the CAPI MachineHealthCheck type at line 3486 (deprecated per cluster-api#10722). The verify check fails because the test file needs formatting. The gitlint check fails because the commit title "OCPBUGS-86798: spot MHC not created when autoRepair=true and ignition endpoint not reached" does not use a conventional-commit prefix. The two Prow image build failures (ocp/5.0 and origin/scos-4.21 imagestream not found) are transient CI infrastructure issues unrelated to this PR — other PRs submitted at the same time had the same error, and later runs on different PRs succeed.

Root Cause

There are three PR-authoring issues and two transient CI infrastructure issues:

1. Merge Conflict (tide ERROR — primary blocker)
The PR branch fix/spot-mhc-autorepair-deadlock has fallen behind main and has conflicting changes in the modified files (hypershift-operator/controllers/nodepool/capi.go and/or capi_test.go). GitHub reports mergeStateStatus: DIRTY and mergeable: CONFLICTING. Tide cannot merge the PR until the conflict is resolved.

2. Lint Failures (2 issues in PR code)

• gci import ordering (capi_test.go:3467): The new test function TestSpotMHCCreatedIndependentlyOfAutoRepairIgnitionGate has imports that don't follow the project's required grouping/ordering convention. The gci linter (Go Code Import) flags the file as not properly formatted.
• staticcheck SA1019 (capi_test.go:3486): The test assertion g.Expect(spotMHC.Spec.MaxUnhealthy.String()).To(Equal("100%")) uses the deprecated MaxUnhealthy field. The CAPI project deprecated this field and recommends using the replacement per cluster-api#10722. The production code in capi.go likely also sets this deprecated field (the test is asserting the value set by reconcileSpotMachineHealthCheck), so both the production code and test may need updating.

3. Gitlint Failure (commit message format)
The commit title "OCPBUGS-86798: spot MHC not created when autoRepair=true and ignition endpoint not reached" does not follow the project's conventional-commit convention. It must start with one of: fix, feat, chore, docs, style, refactor, perf, test, revert, ci, build. The correct format would be something like: fix(OCPBUGS-86798): spot MHC not created when autoRepair=true and ignition endpoint not reached.

4. CI Infrastructure Issues (transient, NOT caused by this PR)
Both ci/prow/images and ci/prow/okd-scos-images failed because CI could not resolve imagestreams ocp/5.0 and origin/scos-4.21 respectively. These are transient CI infrastructure issues — PR #8685 submitted around the same time has a successful images run with the same job configuration. A /retest would resolve these.

Recommendations

1. Rebase the branch onto latest main to resolve the merge conflict:

   git fetch upstream main
   git rebase upstream/main
   # Resolve conflicts in capi.go and/or capi_test.go
   git push --force-with-lease

2. Fix the gci import ordering in capi_test.go — run make verify locally to auto-format, or run:

   gci write hypershift-operator/controllers/nodepool/capi_test.go

3. Replace the deprecated Spec.MaxUnhealthy field usage:

   • Check the CAPI v1beta1 API for the replacement field (likely Spec.UnhealthyRange or a new mechanism per cluster-api#10722)
   • Update both the production code in reconcileSpotMachineHealthCheck() and the test assertion
   • Alternatively, add a //nolint:staticcheck directive if the deprecated field must be used for backward compatibility, with a comment explaining why

4. Fix the commit message to use conventional-commit format:

   git commit --amend -m "fix(OCPBUGS-86798): spot MHC not created when autoRepair=true and ignition endpoint not reached"

5. Retest the Prow image jobs after rebasing — the ocp/5.0 imagestream issue was transient and will resolve on retry:

   /retest
   

Evidence

Evidence	Detail
Tide error message	Not mergeable. PR has a merge conflict.
GitHub merge state	mergeStateStatus: DIRTY, mergeable: CONFLICTING
Lint error 1 (gci)	capi_test.go:3467:1: File is not properly formatted (gci)
Lint error 2 (staticcheck)	capi_test.go:3486:14: SA1019: spotMHC.Spec.MaxUnhealthy is deprecated
Verify error	hypershift-operator/controllers/nodepool/capi_test.go: needs update
Gitlint error	Title does not start with one of fix, feat, chore, docs, style, refactor, perf, test, revert, ci, build
Images job error	step [release-inputs:latest] failed: could not resolve source imagestream ocp/5.0 for release latest: imagestreams.image.openshift.io "5.0" not found
OKD images job error	step [release-inputs:latest] failed: could not resolve source imagestream origin/scos-4.21 for release latest: imagestreams.image.openshift.io "scos-4.21" not found
Infra issue is transient	PR #8685's images job (build 2062909253659660288) succeeded with status success — same job, different PR
Deprecated CAPI field	Spec.MaxUnhealthy deprecated per cluster-api#10722
Files changed	hypershift-operator/controllers/nodepool/capi.go, hypershift-operator/controllers/nodepool/capi_test.go

---