CNTRLPLANE-1388: Spot with termination handler - #7567
Conversation
|
Skipping CI for Draft Pull Request. |
|
@enxebre: This pull request references CNTRLPLANE-1388 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 epic to target the "4.22.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-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
do-not-merge/work-in-progress
WalkthroughAdds an AWS Node Termination Handler: new v2 control-plane component, deployment and secret assets, SQS IAM and client wiring, HostedCluster/HostedControlPlane annotation propagation and disable flag, NodePool spot support with spot-specific MachineHealthChecks, unit and e2e tests, and multiple test fixture updates. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes ✨ Finishing touches
Comment |
openshift-ci
Bot
added
area/api
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: enxebre 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
213ba6a to
1e859ca
Compare
|
/test e2e-aws |
1 similar comment
|
/test e2e-aws |
|
/test e2e e2e-aws |
1e859ca to
38bf314
Compare
|
/test e2e-aws |
| grep certificate-authority-data /etc/kubernetes/kubeconfig | awk '{print $2}' | base64 -d > /var/run/secrets/kubernetes.io/serviceaccount/ca.crt | ||
|
|
||
| /usr/bin/control-plane-operator token-minter \ | ||
| --service-account-namespace=openshift-cluster-version \ |
There was a problem hiding this comment.
Does the openshift-cluster-version SA have the right rbac to be able to drain nodes?
There was a problem hiding this comment.
yeh it's admin. Note the comment in the script though. Hopefully we can get rid of this in future
# we use openshift-cluster-version/default service account to avoid the need to create a new one.
# We need to authenticate with a SA because the terminantion handler has hardcoded in cluster client credentials.
# https://github.com/aws/aws-node-termination-handler/pull/1230
|
|
||
| // AnnotationEnableSpot enables spot instance creation for this NodePool. | ||
| // When set to "true", the NodePool will create spot instances instead of on-demand instances. | ||
| AnnotationEnableSpot = "hypershift.openshift.io/enable-spot" |
There was a problem hiding this comment.
Should this be defined in the api module?
There was a problem hiding this comment.
I'd rather keep them here since this is just to hide this feature while having e2e coverage. The intent is to have a follow up to drop the annotations and expose in the API.
|
|
||
| // AnnotationTerminationHandlerQueueURL specifies the SQS queue URL for spot interruption events. | ||
| // This annotation is required on the HostedCluster to enable the termination handler component. | ||
| AnnotationTerminationHandlerQueueURL = "hypershift.openshift.io/aws-termination-handler-queue-url" |
There was a problem hiding this comment.
should this be defined in the api module?
There was a problem hiding this comment.
I'd rather keep them here since this is just to hide this feature while having e2e coverage. The intent is to have a follow up to drop the annotations and expose in the API.
| t.Run("SpotTerminationHandlerTest", func(t *testing.T) { | ||
| controlPlaneNamespace := manifests.HostedControlPlaneNamespace(s.hostedCluster.Namespace, s.hostedCluster.Name) | ||
|
|
||
| // Step 0: Add SQS permissions to the CPO role so the termination handler can access the queue |
There was a problem hiding this comment.
should the SQS permissions go in the nodepool role? (they're added there in the cli's iam.go)
There was a problem hiding this comment.
they are added as well already. I added in the test as well since I believe we had some e2e setup to run with the published managed policies therefore the inline append is needed
There was a problem hiding this comment.
yep cpo role wasn't intended. Updated.
2f63b2a to
f6dfa1c
Compare
f6dfa1c to
5234288
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: 5
🤖 Fix all issues with AI agents
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/awsnodeterminationhandler/secret.go`:
- Around line 11-29: adaptCredentialsSecret can panic if HCP AWS spec or
RolesRef.NodePoolManagementARN is nil/empty; add a defensive guard at the start
of adaptCredentialsSecret to validate hcp.Spec.Platform != nil,
hcp.Spec.Platform.AWS != nil and that
hcp.Spec.Platform.AWS.RolesRef.NodePoolManagementARN is non-empty, and return a
clear error (not panic) if the ARN is missing; update usage in
adaptCredentialsSecret (referencing hcp and roleARN) to only proceed building
the awsCredentialsTemplate when the checks pass.
In `@hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go`:
- Around line 4317-4338: The code in isAWSNodeTerminationHandlerNeeded currently
treats any presence of the annotation "hypershift.openshift.io/enable-spot" as
enabling spot handling; change the check to validate the annotation value (e.g.,
use strconv.ParseBool or compare to "true") so only truthy values enable the
handler. In the loop over nodePools in isAWSNodeTerminationHandlerNeeded, fetch
nodePool.Annotations["hypershift.openshift.io/enable-spot"], parse it with
strconv.ParseBool (or normalize and compare to "true"), and only return true
when parsing succeeds and yields true; keep using listNodePools and preserve
existing error handling. Ensure nil annotations are handled safely.
In `@hypershift-operator/controllers/nodepool/capi_test.go`:
- Around line 1740-1744: The test contains a redundant conditional where
expectedTemplateName is initialized to awsMachineTemplateName and then set to
the same value inside the if block using isSpotEnabled(tt.nodePool); remove the
redundant branch or implement the intended alternative value for spot instances.
Update the test by deleting the if block that assigns expectedTemplateName =
awsMachineTemplateName (or replace it with the correct template name for spot
instances), keeping the final assertion
g.Expect(templateList.Items[0].GetName()).To(Equal(expectedTemplateName))
unchanged; reference symbols: expectedTemplateName, awsMachineTemplateName,
isSpotEnabled, tt.nodePool, templateList.Items.
In `@test/e2e/nodepool_spot_termination_handler_test.go`:
- Around line 114-126: The SQS IAM policy constructed in the sqsPolicy string
currently uses a wildcard ARN ("arn:aws:sqs:%s:*:*") granting
ReceiveMessage/DeleteMessage to all queues in the region; change this to target
only the test queue by building the exact queue ARN (or URL-derived ARN) instead
of using the wildcard: obtain the queue name or URL (e.g., via the code that
creates/looks up the queue) and combine it with the AWS account ID and
s.clusterOpts.AWSPlatform.Region to produce the specific ARN, then replace the
Resource value in sqsPolicy with that specific ARN so only the intended queue is
allowed.
- Around line 286-293: The HostedCluster annotation removal must be registered
with defer so it always runs even if the test fails; move the
e2eutil.UpdateObject call that deletes AnnotationTerminationHandlerQueueURL
earlier (immediately after you set or assert the annotation exists) and wrap it
in a defer closure that calls e2eutil.UpdateObject(t, s.ctx, s.mgmtClient,
s.hostedCluster, func(obj *hyperv1.HostedCluster) { delete(obj.Annotations,
AnnotationTerminationHandlerQueueURL) }); then remove the explicit cleanup block
currently in Step 7 so you don't run the same removal twice.
🧹 Nitpick comments (2)
test/e2e/nodepool_test.go (1)
215-219: Consider making the SharedRole requirement more explicit.The
if i == 0condition couples the SharedRole configuration to the test's position in the slice. If tests are reordered or the SpotTerminationHandler test moves to a different HostedCluster group, this configuration won't apply correctly.A more robust approach might be to check if the test group contains tests requiring real roles, or add a field to
HostedClusterNodePoolTestCasesto indicate role requirements.test/e2e/nodepool_spot_termination_handler_test.go (1)
40-41: Hardcoded queue name reduces test portability.While the SQS queue URL is now discovered dynamically (addressing the previous account ID exposure concern), the queue name
agarcial-nth-queueis still hardcoded. This appears to be a personal/dev queue name and makes the test non-portable across different AWS accounts or environments.Consider parameterizing via environment variable:
🔧 Suggested fix
- // testSQSQueueName is the SQS queue name used for testing. - testSQSQueueName = "agarcial-nth-queue" +) + +var ( + // testSQSQueueName is read from environment for testing. + testSQSQueueName = os.Getenv("HYPERSHIFT_TEST_SQS_QUEUE_NAME")Then validate in
Setup:if testSQSQueueName == "" { t.Skip("HYPERSHIFT_TEST_SQS_QUEUE_NAME environment variable not set") }
| func adaptCredentialsSecret(cpContext component.WorkloadContext, secret *corev1.Secret) error { | ||
| hcp := cpContext.HCP | ||
|
|
||
| // Get the NodePoolManagementARN from the HCP spec. | ||
| // The predicate ensures this is set before the component is reconciled. | ||
| roleARN := hcp.Spec.Platform.AWS.RolesRef.NodePoolManagementARN | ||
|
|
||
| // Create AWS credentials file content using web identity token | ||
| // This follows the same pattern as karpenter operator | ||
| awsCredentialsTemplate := `[default] | ||
| role_arn = %s | ||
| web_identity_token_file = /var/run/secrets/openshift/serviceaccount/token | ||
| sts_regional_endpoints = regional | ||
| ` | ||
| credentials := fmt.Sprintf(awsCredentialsTemplate, roleARN) | ||
|
|
||
| // Set the credentials in the secret | ||
| secret.Data = map[string][]byte{"credentials": []byte(credentials)} | ||
| secret.Type = corev1.SecretTypeOpaque |
There was a problem hiding this comment.
Add a defensive guard for missing AWS spec/role ARN to avoid panic.
This keeps tests or future callers safe if the predicate is bypassed.
🛠️ Suggested fix
func adaptCredentialsSecret(cpContext component.WorkloadContext, secret *corev1.Secret) error {
hcp := cpContext.HCP
+ if hcp == nil || hcp.Spec.Platform.AWS == nil || hcp.Spec.Platform.AWS.RolesRef.NodePoolManagementARN == "" {
+ return fmt.Errorf("nodePoolManagementARN is required to build aws credentials")
+ }
// Get the NodePoolManagementARN from the HCP spec.
// The predicate ensures this is set before the component is reconciled.
roleARN := hcp.Spec.Platform.AWS.RolesRef.NodePoolManagementARN📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func adaptCredentialsSecret(cpContext component.WorkloadContext, secret *corev1.Secret) error { | |
| hcp := cpContext.HCP | |
| // Get the NodePoolManagementARN from the HCP spec. | |
| // The predicate ensures this is set before the component is reconciled. | |
| roleARN := hcp.Spec.Platform.AWS.RolesRef.NodePoolManagementARN | |
| // Create AWS credentials file content using web identity token | |
| // This follows the same pattern as karpenter operator | |
| awsCredentialsTemplate := `[default] | |
| role_arn = %s | |
| web_identity_token_file = /var/run/secrets/openshift/serviceaccount/token | |
| sts_regional_endpoints = regional | |
| ` | |
| credentials := fmt.Sprintf(awsCredentialsTemplate, roleARN) | |
| // Set the credentials in the secret | |
| secret.Data = map[string][]byte{"credentials": []byte(credentials)} | |
| secret.Type = corev1.SecretTypeOpaque | |
| func adaptCredentialsSecret(cpContext component.WorkloadContext, secret *corev1.Secret) error { | |
| hcp := cpContext.HCP | |
| if hcp == nil || hcp.Spec.Platform.AWS == nil || hcp.Spec.Platform.AWS.RolesRef.NodePoolManagementARN == "" { | |
| return fmt.Errorf("nodePoolManagementARN is required to build aws credentials") | |
| } | |
| // Get the NodePoolManagementARN from the HCP spec. | |
| // The predicate ensures this is set before the component is reconciled. | |
| roleARN := hcp.Spec.Platform.AWS.RolesRef.NodePoolManagementARN | |
| // Create AWS credentials file content using web identity token | |
| // This follows the same pattern as karpenter operator | |
| awsCredentialsTemplate := `[default] | |
| role_arn = %s | |
| web_identity_token_file = /var/run/secrets/openshift/serviceaccount/token | |
| sts_regional_endpoints = regional | |
| ` | |
| credentials := fmt.Sprintf(awsCredentialsTemplate, roleARN) | |
| // Set the credentials in the secret | |
| secret.Data = map[string][]byte{"credentials": []byte(credentials)} | |
| secret.Type = corev1.SecretTypeOpaque |
🤖 Prompt for AI Agents
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/awsnodeterminationhandler/secret.go`
around lines 11 - 29, adaptCredentialsSecret can panic if HCP AWS spec or
RolesRef.NodePoolManagementARN is nil/empty; add a defensive guard at the start
of adaptCredentialsSecret to validate hcp.Spec.Platform != nil,
hcp.Spec.Platform.AWS != nil and that
hcp.Spec.Platform.AWS.RolesRef.NodePoolManagementARN is non-empty, and return a
clear error (not panic) if the ARN is missing; update usage in
adaptCredentialsSecret (referencing hcp and roleARN) to only proceed building
the awsCredentialsTemplate when the checks pass.
| sqsPolicy := fmt.Sprintf(`{ | ||
| "Version": "2012-10-17", | ||
| "Statement": [ | ||
| { | ||
| "Effect": "Allow", | ||
| "Action": [ | ||
| "sqs:ReceiveMessage", | ||
| "sqs:DeleteMessage" | ||
| ], | ||
| "Resource": "arn:aws:sqs:%s:*:*" | ||
| } | ||
| ] | ||
| }`, s.clusterOpts.AWSPlatform.Region) |
There was a problem hiding this comment.
Overly permissive SQS IAM policy grants access to all queues in the region.
The policy uses a wildcard resource arn:aws:sqs:%s:*:* which grants ReceiveMessage and DeleteMessage permissions to all SQS queues in the region, not just the test queue. This violates the principle of least privilege.
Scope the policy to the specific queue:
🔒 Suggested fix
sqsPolicy := fmt.Sprintf(`{
"Version": "2012-10-17",
"Statement": [
{
"Effect": "Allow",
"Action": [
"sqs:ReceiveMessage",
"sqs:DeleteMessage"
],
- "Resource": "arn:aws:sqs:%s:*:*"
+ "Resource": "%s"
}
]
- }`, s.clusterOpts.AWSPlatform.Region)
+ }`, getQueueARNFromURL(sqsQueueURL))You'll need to discover the queue URL before creating the policy, or construct the ARN from the queue name and account ID.
🤖 Prompt for AI Agents
In `@test/e2e/nodepool_spot_termination_handler_test.go` around lines 114 - 126,
The SQS IAM policy constructed in the sqsPolicy string currently uses a wildcard
ARN ("arn:aws:sqs:%s:*:*") granting ReceiveMessage/DeleteMessage to all queues
in the region; change this to target only the test queue by building the exact
queue ARN (or URL-derived ARN) instead of using the wildcard: obtain the queue
name or URL (e.g., via the code that creates/looks up the queue) and combine it
with the AWS account ID and s.clusterOpts.AWSPlatform.Region to produce the
specific ARN, then replace the Resource value in sqsPolicy with that specific
ARN so only the intended queue is allowed.
|
/test e2e-aws |
Implement control plane operator v2 component for AWS node termination handler with MachineHealthCheck integration. Components added: - AWS node termination handler Deployment for spot interruption handling - AWS credentials secret with NodePoolManagement role ARN - MachineHealthCheck for spot instances with interruptible-instance label - Component predicate checking for AWS platform and SQS queue URL annotation - interruptible-instance label propagation to MachineDeployment templates and existing Machines The component is deployed when the HostedCluster has: - AWS platform type - Annotation: hypershift.openshift.io/aws-termination-handler-queue-url Features: - SQS queue monitoring for spot interruption events - Automatic node draining on spot interruption warnings - Machine health checking for interrupted instances - Web identity token authentication using NodePoolManagement ARN TODOs: - Replace aws-node-termination-handler image with mirror image or payload once available - Remove SQS queue URL annotation once the input is in the HostedCluster AWS API - Replace hardcoded constants with API based configuration Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…and spot Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
5234288 to
85008d9
Compare
|
/verified by e2e |
openshift-ci-robot
added
the
verified
|
@enxebre: This PR has been marked as verified by 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. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@control-plane-operator/controllers/hostedcontrolplane/testdata/aws-node-termination-handler/zz_fixture_TestControlPlaneComponents_aws_node_termination_handler_deployment.yaml`:
- Around line 84-86: The fixture is missing the AWS_REGION env var value causing
SQS client MissingRegion; update the deployment fixture so the AWS_REGION
environment variable is populated with the AWS region consistent with QUEUE_URL
(us-east-1). Edit the test YAML where the env list contains the AWS_REGION entry
(in the aws-node-termination-handler deployment fixture) and set its value to
"us-east-1" to mirror how awsnodeterminationhandler/deployment.go pulls
hcp.Spec.Platform.AWS.Region, ensuring the rendered deployment matches the
controller logic.
In
`@hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go`:
- Around line 5327-5328: Replace the hardcoded annotation key
"hypershift.openshift.io/enable-spot" in the test Annotations maps with the
existing constant nodepool.AnnotationEnableSpot; add the appropriate import for
the nodepool package (use an import alias if needed) and update all test
occurrences where that string is used so they reference
nodepool.AnnotationEnableSpot instead.
🧹 Nitpick comments (1)
control-plane-operator/controllers/hostedcontrolplane/v2/awsnodeterminationhandler/deployment_test.go (1)
100-116: Missing test coverage forissuerURL/ token-audience.The test sets
tc.issuerURLin the HCP spec but never verifies it was correctly applied to thetoken-minter-kubecontainer's args. This leaves the token-audience string replacement logic untested.♻️ Proposed addition to verify token-audience
g.Expect(regionValue).To(Equal(tc.expectedRegion)) g.Expect(queueURLValue).To(Equal(tc.expectedQueueURL)) + + // Verify token-audience was set correctly in token-minter-kube container + if tc.issuerURL != "" { + for _, container := range deployment.Spec.Template.Spec.Containers { + if container.Name == "token-minter-kube" { + found := false + for _, arg := range container.Args { + if strings.Contains(arg, "--token-audience="+tc.issuerURL) { + found = true + break + } + } + g.Expect(found).To(BeTrue(), "expected token-audience to contain issuerURL") + } + } + }Also add test cases with
issuerURLvalues to exercise this code path.
| - name: AWS_REGION | ||
| - name: QUEUE_URL | ||
| value: https://sqs.us-east-1.amazonaws.com/123456789012/test-queue |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the current state of the fixture file
cat control-plane-operator/controllers/hostedcontrolplane/testdata/aws-node-termination-handler/zz_fixture_TestControlPlaneComponents_aws_node_termination_handler_deployment.yamlRepository: openshift/hypershift
Length of output: 7410
🏁 Script executed:
#!/bin/bash
# Search for AWS_REGION environment variable setup in controller code
rg -n "AWS_REGION" control-plane-operator/controllers/hostedcontrolplane -g '*.go' -A 3 -B 1Repository: openshift/hypershift
Length of output: 4408
🏁 Script executed:
#!/bin/bash
# Find the controller file that manages aws-node-termination-handler
fd -e go -path '*hostedcontrolplane*' | xargs rg -l "aws.node.termination.handler|aws-node-termination-handler" | head -5Repository: openshift/hypershift
Length of output: 672
🏁 Script executed:
#!/bin/bash
# Examine the deployment.go file to understand how awsRegion is used
sed -n '30,70p' control-plane-operator/controllers/hostedcontrolplane/v2/awsnodeterminationhandler/deployment.goRepository: openshift/hypershift
Length of output: 1474
🏁 Script executed:
#!/bin/bash
# Check function signature and how awsRegion/queueURL parameters are passed
rg -n "func.*awsRegion|func.*queueURL" control-plane-operator/controllers/hostedcontrolplane/v2/awsnodeterminationhandler/Repository: openshift/hypershift
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Find where awsRegion variable is defined and initialized
sed -n '1,50p' control-plane-operator/controllers/hostedcontrolplane/v2/awsnodeterminationhandler/deployment.goRepository: openshift/hypershift
Length of output: 1506
🏁 Script executed:
#!/bin/bash
# Search for the function definition that contains this code
rg -n "func.*ReconcileAWSNodeTerminationHandler|func.*Build.*Deployment" control-plane-operator/controllers/hostedcontrolplane/v2/awsnodeterminationhandler/deployment.go -A 30Repository: openshift/hypershift
Length of output: 46
AWS_REGION must be populated in the rendered deployment.
The controller code in awsnodeterminationhandler/deployment.go correctly sets AWS_REGION from hcp.Spec.Platform.AWS.Region, but the fixture shows it as empty despite QUEUE_URL being set to a us-east-1 queue. Without AWS_REGION, the SQS client initialization will fail with a MissingRegion error. Update the fixture to populate AWS_REGION consistently with the queue region.
Fixture update
- - name: AWS_REGION
+ - name: AWS_REGION
+ value: us-east-1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: AWS_REGION | |
| - name: QUEUE_URL | |
| value: https://sqs.us-east-1.amazonaws.com/123456789012/test-queue | |
| - name: AWS_REGION | |
| value: us-east-1 | |
| - name: QUEUE_URL | |
| value: https://sqs.us-east-1.amazonaws.com/123456789012/test-queue |
🤖 Prompt for AI Agents
In
`@control-plane-operator/controllers/hostedcontrolplane/testdata/aws-node-termination-handler/zz_fixture_TestControlPlaneComponents_aws_node_termination_handler_deployment.yaml`
around lines 84 - 86, The fixture is missing the AWS_REGION env var value
causing SQS client MissingRegion; update the deployment fixture so the
AWS_REGION environment variable is populated with the AWS region consistent with
QUEUE_URL (us-east-1). Edit the test YAML where the env list contains the
AWS_REGION entry (in the aws-node-termination-handler deployment fixture) and
set its value to "us-east-1" to mirror how
awsnodeterminationhandler/deployment.go pulls hcp.Spec.Platform.AWS.Region,
ensuring the rendered deployment matches the controller logic.
| Annotations: map[string]string{ | ||
| "hypershift.openshift.io/enable-spot": "true", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for enable-spot annotation constant definitions
rg -n "enable-spot|EnableSpot" --type=go -g '!*_test.go' -C2Repository: openshift/hypershift
Length of output: 1598
🏁 Script executed:
#!/bin/bash
# Search for annotation constants in general to understand naming patterns
rg -n "Annotation.*=.*\"hypershift" --type=go -g '!*_test.go' | head -20Repository: openshift/hypershift
Length of output: 3610
🏁 Script executed:
#!/bin/bash
# Look at the test file to see where the annotation is used
rg -n "enable-spot" --type=go -C3 | grep -A3 -B3 "hostedcluster_controller_test.go"Repository: openshift/hypershift
Length of output: 2865
🏁 Script executed:
#!/bin/bash
# Check for constant definitions around line 5327-5328 context in test file
fd "hostedcluster_controller_test.go" --type f | xargs wc -lRepository: openshift/hypershift
Length of output: 147
Use the AnnotationEnableSpot constant instead of hardcoding the annotation key.
The annotation "hypershift.openshift.io/enable-spot" is hardcoded in test cases at lines 5328, 5366, and 5395. A constant AnnotationEnableSpot already exists in the nodepool package (controllers/nodepool/aws.go:31) and should be imported and used here for consistency and maintainability.
🤖 Prompt for AI Agents
In
`@hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go`
around lines 5327 - 5328, Replace the hardcoded annotation key
"hypershift.openshift.io/enable-spot" in the test Annotations maps with the
existing constant nodepool.AnnotationEnableSpot; add the appropriate import for
the nodepool package (use an import alias if needed) and update all test
occurrences where that string is used so they reference
nodepool.AnnotationEnableSpot instead.
|
/test e2e-aws |
Add e2e test to validate: - AWS Node Termination Handler deployment is created when SQS annotation is set - Spot MachineHealthCheck is created with correct interruptible-instance label selector Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
85008d9 to
ec91275
Compare
openshift-ci-robot
removed
the
verified
|
/verified by e2e |
openshift-ci-robot
added
the
verified
|
@enxebre: This PR has been marked as verified by 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. |
|
@enxebre: 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. |
|
/lgtm |
|
/override "Red Hat Konflux / hypershift-operator-main-enterprise-contract / hypershift-operator-main" |
|
@enxebre: Overrode contexts on behalf of enxebre: Red Hat Konflux / hypershift-operator-enterprise-contract / hypershift-operator-main, Red Hat Konflux / hypershift-operator-main-enterprise-contract / hypershift-operator-main 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. |
cf9ea7c
into
openshift:main
3 participants
What this PR does / why we need it:
Implement control plane operator v2 component for AWS node termination
handler with MachineHealthCheck integration.
Components added:
The component is deployed when the HostedCluster has:
Features:
TODOs:
Which issue(s) this PR fixes:
Fixes
Special notes for your reviewer:
Checklist: