fix: OCPBUGS-86799: Add SpotTerminationHandlerConfigured condition for spot NodePools - #8646
Conversation
…t NodePools When a NodePool with marketType: Spot is created on a HostedCluster that does not have spec.platform.aws.terminationHandlerQueueURL configured, the Node Termination Handler (NTH) silently fails to deploy. This leaves spot instances without graceful interruption handling. Add a new NodePool condition SpotTerminationHandlerConfigured that: - Sets False with a descriptive message when terminationHandlerQueueURL is missing on the HostedCluster for spot-enabled NodePools - Sets True when the queue URL is properly configured - Is only present on spot-enabled NodePools (removed otherwise) - Is a warning only and does not block NodePool reconciliation Co-authored-by: Cursor <cursoragent@cursor.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/severity-moderate
|
@dpateriya: This pull request references Jira Issue OCPBUGS-86799, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
📝 WalkthroughWalkthroughThis PR adds a new condition to track whether graceful spot termination handler infrastructure is configured on the HostedCluster. The change introduces the 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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/api
openshift-ci-robot
added
the
jira/valid-bug
|
@dpateriya: This pull request references Jira Issue OCPBUGS-86799, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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-robot
removed
the
jira/invalid-bug
|
/jira refresh |
|
@dpateriya: This pull request references Jira Issue OCPBUGS-86799, which is valid. 3 validation(s) were run on this bug
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
hypershift-operator/controllers/nodepool/nodepool_controller.go (1)
286-303:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove this advisory condition before the short-circuiting checks.
signalConditionsexits on the first non-nil result, so placingr.spotTerminationHandlerConfiguredConditionafterreleaseImageCondition,ignitionEndpointAvailableCondition,validMachineConfigCondition, etc. means the warning is often never set while the NodePool is still reconciling. This condition only depends on spec, so it should run before those blockers.Proposed fix
signalConditions := []func(ctx context.Context, nodePool *hyperv1.NodePool, hcluster *hyperv1.HostedCluster) (*ctrl.Result, error){ r.autoscalerEnabledCondition, r.updateManagementEnabledCondition, + r.spotTerminationHandlerConfiguredCondition, r.releaseImageCondition, r.ignitionEndpointAvailableCondition, r.validArchPlatformCondition, @@ r.reachedIgnitionEndpointCondition, r.machineAndNodeConditions, r.validPlatformConfigCondition, - r.spotTerminationHandlerConfiguredCondition, // TODO(alberto): consider moving here:🤖 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 `@hypershift-operator/controllers/nodepool/nodepool_controller.go` around lines 286 - 303, The advisory condition r.spotTerminationHandlerConfiguredCondition is currently placed after several short-circuiting checks and thus may never run; move r.spotTerminationHandlerConfiguredCondition up into the early part of signalConditions (for example immediately after r.reconciliationActiveCondition or before r.releaseImageCondition) because it only depends on the NodePool spec and must run before blockers like r.releaseImageCondition, r.ignitionEndpointAvailableCondition, r.validMachineConfigCondition, r.validGeneratedPayloadCondition, r.reachedIgnitionEndpointCondition, r.machineAndNodeConditions and r.validPlatformConfigCondition so it can set its warning while reconciliation continues.
🧹 Nitpick comments (1)
hypershift-operator/controllers/nodepool/conditions_test.go (1)
530-545: ⚡ Quick winThe empty-URL case is not actually distinct from the nil-AWS case.
Line 602 only creates
hc.Spec.Platform.AWSwhen the URL is non-empty, so both"terminationHandlerQueueURL is empty"and"AWS platform is nil"hit the same branch. That leaves the non-nil AWS + empty string path untested.Proposed fix
tests := []struct { name string spotEnabled bool + hostedClusterAWSConfigured bool terminationHandlerQueueURL string existingCondition bool expectedConditionPresent bool @@ { name: "When spot is enabled and terminationHandlerQueueURL is set it should set condition to True", spotEnabled: true, + hostedClusterAWSConfigured: true, terminationHandlerQueueURL: "https://sqs.us-east-1.amazonaws.com/123456789012/my-queue", expectedConditionPresent: true, @@ { name: "When spot is enabled and terminationHandlerQueueURL is empty it should set condition to False", spotEnabled: true, + hostedClusterAWSConfigured: true, terminationHandlerQueueURL: "", expectedConditionPresent: true, @@ - if tc.terminationHandlerQueueURL != "" { + if tc.hostedClusterAWSConfigured { hc.Spec.Platform.AWS = &hyperv1.AWSPlatformSpec{ TerminationHandlerQueueURL: tc.terminationHandlerQueueURL, } }Also applies to: 602-606
🤖 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 `@hypershift-operator/controllers/nodepool/conditions_test.go` around lines 530 - 545, Test currently conflates the "nil AWS platform" and "non-nil AWS platform with empty terminationHandlerQueueURL" cases because hc.Spec.Platform.AWS is only created when terminationHandlerQueueURL is non-empty; update the test setup in conditions_test.go to explicitly construct a non-nil hc.Spec.Platform.AWS (e.g., set hc.Spec.Platform.AWS = &hyperv1.AWSPlatformSpec{}) for the case where you want AWS present but terminationHandlerQueueURL == "" so the branch in the reconciler that checks for a present AWS platform with an empty URL is exercised; ensure the test case named "When spot is enabled and terminationHandlerQueueURL is empty it should set condition to False" leaves terminationHandlerQueueURL == "" but forces a non-nil Platform.AWS to verify expectedConditionPresent/expectedStatus/expectedReason/expectedMessage.
🤖 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.
Outside diff comments:
In `@hypershift-operator/controllers/nodepool/nodepool_controller.go`:
- Around line 286-303: The advisory condition
r.spotTerminationHandlerConfiguredCondition is currently placed after several
short-circuiting checks and thus may never run; move
r.spotTerminationHandlerConfiguredCondition up into the early part of
signalConditions (for example immediately after r.reconciliationActiveCondition
or before r.releaseImageCondition) because it only depends on the NodePool spec
and must run before blockers like r.releaseImageCondition,
r.ignitionEndpointAvailableCondition, r.validMachineConfigCondition,
r.validGeneratedPayloadCondition, r.reachedIgnitionEndpointCondition,
r.machineAndNodeConditions and r.validPlatformConfigCondition so it can set its
warning while reconciliation continues.
---
Nitpick comments:
In `@hypershift-operator/controllers/nodepool/conditions_test.go`:
- Around line 530-545: Test currently conflates the "nil AWS platform" and
"non-nil AWS platform with empty terminationHandlerQueueURL" cases because
hc.Spec.Platform.AWS is only created when terminationHandlerQueueURL is
non-empty; update the test setup in conditions_test.go to explicitly construct a
non-nil hc.Spec.Platform.AWS (e.g., set hc.Spec.Platform.AWS =
&hyperv1.AWSPlatformSpec{}) for the case where you want AWS present but
terminationHandlerQueueURL == "" so the branch in the reconciler that checks for
a present AWS platform with an empty URL is exercised; ensure the test case
named "When spot is enabled and terminationHandlerQueueURL is empty it should
set condition to False" leaves terminationHandlerQueueURL == "" but forces a
non-nil Platform.AWS to verify
expectedConditionPresent/expectedStatus/expectedReason/expectedMessage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 5e82549b-8c47-44ee-99f7-01305d9dd00f
⛔ Files ignored due to path filters (1)
vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/nodepool_conditions.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (4)
api/hypershift/v1beta1/nodepool_conditions.gohypershift-operator/controllers/nodepool/conditions.gohypershift-operator/controllers/nodepool/conditions_test.gohypershift-operator/controllers/nodepool/nodepool_controller.go
dpateriya
changed the title
|
/approve for API |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dpateriya, JoelSpeed 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 |
|
Stale PRs are closed after 21d of inactivity. If this PR is still relevant, comment to refresh it or remove the stale label. If this PR is safe to close now please do so with /lifecycle stale |
openshift-ci
Bot
added
the
lifecycle/stale
|
The final agent confirmed the same finding — the All analysis is complete. The report above covers all three failures comprehensively. |
|
@csrwng , would you please review this Pull request? |
dpateriya
changed the title
|
/retest-required |
dpateriya
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8646 +/- ##
==========================================
+ Coverage 40.68% 40.69% +0.01%
==========================================
Files 755 755
Lines 93368 93390 +22
==========================================
+ Hits 37985 38007 +22
Misses 52649 52649
Partials 2734 2734
... and 15 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
@dpateriya: all tests passed! 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. |
|
Stale PRs rot after 14d of inactivity. Mark the PR as fresh by commenting If this PR is safe to close now please do so with /lifecycle rotten |
openshift-ci
Bot
added
lifecycle/rotten
|
Rotten PRs close after 7d of inactivity. Reopen the PR by commenting /close |
|
@openshift-ci[bot]: Closed this PR. 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. |
|
@dpateriya: This pull request references Jira Issue OCPBUGS-86799. The bug has been updated to no longer refer to the pull request using the external bug tracker. All external bug links have been closed. The bug has been moved to the NEW state. 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. |
3 participants
Summary
SpotTerminationHandlerConfiguredthat warns users when a spot-enabled NodePool exists but the HostedCluster lacksspec.platform.aws.terminationHandlerQueueURLFalsewith reasonTerminationHandlerQueueURLNotSetand an actionable message when the queue URL is missingJira: https://redhat.atlassian.net/browse/OCPBUGS-86799
Problem
When a NodePool with
marketType: Spotis created on a HostedCluster withoutterminationHandlerQueueURL, the Node Termination Handler (NTH) silently fails to deploy. Users unknowingly run spot instances without graceful interruption handling — spot interruptions result in abrupt instance termination with no cordon, drain, or PDB compliance.Solution
Surface a NodePool status condition that:
Test plan
conditions_test.gocovering:./hypershift-operator/controllers/nodepool/...test suite passesmake hypershift-apiregenerates CRDs successfullygofmtcleanMade with Cursor
Summary by CodeRabbit
Release Notes