Skip to content

OCPSTRAT-1677: feat: allow for aws spot market options on node pools #6707

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
gdbranco wants to merge 1 commit into from
Closed

OCPSTRAT-1677: feat: allow for aws spot market options on node pools #6707

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions api/hypershift/v1beta1/aws.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,21 @@ type AWSNodePoolPlatform struct {
Placement *PlacementOptions `json:"placement,omitempty"`
}

// AWSSpotMarketOptions defines configuration for AWS Spot instances
type AWSSpotMarketOptions struct {
// maxPrice defines the maximum price (USD per instance‑hour) you are willing to pay for a Spot instance.
// If omitted, the On‑Demand price is used as the ceiling.

@enxebre enxebre Sep 8, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// If omitted, the On‑Demand price is used as the ceiling.

How can I express intent for this with this API?

@enxebre enxebre Sep 8, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nvm, I was thinking of omitzero. I guess this is a legit case to use omitempty and not omitzero cc @JoelSpeed

@JoelSpeed JoelSpeed Sep 8, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Historically, spot markets could exceed the on-demand price, is that still the case?

What is the use case for 0 for the maximum price? Does AWS allow a 0 max price?

@gdbranco gdbranco Sep 18, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"If you specify a maximum price, it must be more than USD $0.001. Specifying a value below USD $0.001 will result in an InvalidParameterValue error message."
https://docs.aws.amazon.com/AWSEC2/latest/APIReference/API_SpotMarketOptions.html

@JoelSpeed JoelSpeed Sep 18, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your pattern presently allows 0 as a valid choice, please update to remove that

// Example: "0.0739"
// Format: up to 10 integer digits and up to 6 fractional digits; no leading zeros unless the value is "0"; scientific notation is not allowed.
// +optional
// +kubebuilder:validation:Pattern=`^(0|[1-9][0-9]{0,9})(\.[0-9]{1,6})?$`
// +kubebuilder:validation:MaxLength=17
MaxPrice *string `json:"maxPrice,omitempty"`
}

// PlacementOptions specifies the placement options for the EC2 instances.
// +kubebuilder:validation:XValidation:rule="has(self.tenancy) && self.tenancy == 'host' ? !has(self.capacityReservation) : true", message="AWS Capacity Reservations cannot be used with Dedicated Hosts (tenancy 'host')"
// +kubebuilder:validation:XValidation:rule="has(self.spotMarketOptions) ? (!has(self.capacityReservation) && (!has(self.tenancy) || self.tenancy == 'default')) : true", message="spotMarketOptions is incompatible with capacityReservation and requires tenancy to be 'default' or unset (not 'dedicated' or 'host')"
type PlacementOptions struct {
// tenancy indicates if instance should run on shared or single-tenant hardware.
//
Expand All @@ -83,6 +96,12 @@ type PlacementOptions struct {
//
// +optional
CapacityReservation *CapacityReservationOptions `json:"capacityReservation,omitempty"`

// spotMarketOptions specifies options for using AWS Spot instances.
// When specified, instances will be launched as Spot instances with the given configuration.
// Mutually exclusive with capacityReservation, and tenancy must be unset or set to "default".
// +optional
SpotMarketOptions *AWSSpotMarketOptions `json:"spotMarketOptions,omitempty"`
Comment on lines +100 to +104

@JoelSpeed JoelSpeed Sep 8, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it's copying a very old API which I suspect wouldn't be the way we would recommend implementing this API if we were to go through the API review process today

In particular, it's awkward to use spotMarketOptions: {} as a way to enable a spot instance.

Really you need a discriminated union, which would include the capacity reservation options as well.

If you look at the way MAPI implements this, we have one of the MarketType options as Spot

The ideal here would have been

placement:
  market:
    type: OnDemand | Spot | CapacityBlocks
    capacityBlocks: // Valid only when type is CapacityBlocks
      id: ...
    spot: // Valid only when type is Spot
      maxPrice: ...

I'm not sure why MarketType ended up inside the CapacityReservationOptions here, we really should have looked at how this looked in MAPI and predicted this need 🤔

@enxebre enxebre Sep 9, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can either keep it as in the PR or mark CapacityReservation.MarketType as deprecated and expose a new one within PlacementOptions.

@JoelSpeed JoelSpeed Sep 10, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're happy with a deprecated field, I think it would make more sense to restructure this slightly. We should be able to validate at admission time that the two fields are not different if the existing CapacityReservation.MarketType is present

@gdbranco gdbranco Sep 18, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know what is the direction to follow on this, I agree having the market type directly on placement makes more sense as well

}

// MarketType describes the market type of the CapacityReservation for an Instance.
Expand Down
25 changes: 25 additions & 0 deletions api/hypershift/v1beta1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

22 changes: 22 additions & 0 deletions ...z_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/AAA_ungated.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -484,6 +484,22 @@ spec:
requires a Capacity Reservation ID
rule: 'has(self.marketType) && self.marketType == ''CapacityBlocks''
? has(self.id) : true'
spotMarketOptions:
description: |-
spotMarketOptions specifies options for using AWS Spot instances.
When specified, instances will be launched as Spot instances with the given configuration.
Mutually exclusive with capacityReservation, and tenancy must be unset or set to "default".
properties:
maxPrice:
description: |-
maxPrice defines the maximum price (USD per instance‑hour) you are willing to pay for a Spot instance.
If omitted, the On‑Demand price is used as the ceiling.
Example: "0.0739"
Format: up to 10 integer digits and up to 6 fractional digits; no leading zeros unless the value is "0"; scientific notation is not allowed.
maxLength: 17
pattern: ^(0|[1-9][0-9]{0,9})(\.[0-9]{1,6})?$
type: string
type: object
tenancy:
description: |-
tenancy indicates if instance should run on shared or single-tenant hardware.
Expand All @@ -506,6 +522,12 @@ spec:
Hosts (tenancy 'host')
rule: 'has(self.tenancy) && self.tenancy == ''host'' ? !has(self.capacityReservation)
: true'
- message: spotMarketOptions is incompatible with capacityReservation
and requires tenancy to be 'default' or unset (not 'dedicated'
or 'host')
rule: 'has(self.spotMarketOptions) ? (!has(self.capacityReservation)
&& (!has(self.tenancy) || self.tenancy == ''default''))
: true'
Comment thread
coderabbitai[bot] marked this conversation as resolved.
resourceTags:
description: |-
resourceTags is an optional list of additional tags to apply to AWS node
Expand Down
22 changes: 22 additions & 0 deletions .../zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/OpenStack.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -484,6 +484,22 @@ spec:
requires a Capacity Reservation ID
rule: 'has(self.marketType) && self.marketType == ''CapacityBlocks''
? has(self.id) : true'
spotMarketOptions:
description: |-
spotMarketOptions specifies options for using AWS Spot instances.
When specified, instances will be launched as Spot instances with the given configuration.
Mutually exclusive with capacityReservation, and tenancy must be unset or set to "default".
properties:
maxPrice:
description: |-
maxPrice defines the maximum price (USD per instance‑hour) you are willing to pay for a Spot instance.
If omitted, the On‑Demand price is used as the ceiling.
Example: "0.0739"
Format: up to 10 integer digits and up to 6 fractional digits; no leading zeros unless the value is "0"; scientific notation is not allowed.
maxLength: 17
pattern: ^(0|[1-9][0-9]{0,9})(\.[0-9]{1,6})?$
type: string
type: object
tenancy:
description: |-
tenancy indicates if instance should run on shared or single-tenant hardware.
Expand All @@ -506,6 +522,12 @@ spec:
Hosts (tenancy 'host')
rule: 'has(self.tenancy) && self.tenancy == ''host'' ? !has(self.capacityReservation)
: true'
- message: spotMarketOptions is incompatible with capacityReservation
and requires tenancy to be 'default' or unset (not 'dedicated'
or 'host')
rule: 'has(self.spotMarketOptions) ? (!has(self.capacityReservation)
&& (!has(self.tenancy) || self.tenancy == ''default''))
: true'
resourceTags:
description: |-
resourceTags is an optional list of additional tags to apply to AWS node
Expand Down
38 changes: 38 additions & 0 deletions client/applyconfiguration/hypershift/v1beta1/awsspotmarketoptions.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 9 additions & 0 deletions client/applyconfiguration/hypershift/v1beta1/placementoptions.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions client/applyconfiguration/utils.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

22 changes: 22 additions & 0 deletions .../assets/hypershift-operator/zz_generated.crd-manifests/nodepools-CustomNoUpgrade.crd.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -487,6 +487,22 @@ spec:
requires a Capacity Reservation ID
rule: 'has(self.marketType) && self.marketType == ''CapacityBlocks''
? has(self.id) : true'
spotMarketOptions:
description: |-
spotMarketOptions specifies options for using AWS Spot instances.
When specified, instances will be launched as Spot instances with the given configuration.
Mutually exclusive with capacityReservation, and tenancy must be unset or set to "default".
properties:
maxPrice:
description: |-
maxPrice defines the maximum price (USD per instance‑hour) you are willing to pay for a Spot instance.
If omitted, the On‑Demand price is used as the ceiling.
Example: "0.0739"
Format: up to 10 integer digits and up to 6 fractional digits; no leading zeros unless the value is "0"; scientific notation is not allowed.
maxLength: 17
pattern: ^(0|[1-9][0-9]{0,9})(\.[0-9]{1,6})?$
type: string
type: object
tenancy:
description: |-
tenancy indicates if instance should run on shared or single-tenant hardware.
Expand All @@ -509,6 +525,12 @@ spec:
Hosts (tenancy 'host')
rule: 'has(self.tenancy) && self.tenancy == ''host'' ? !has(self.capacityReservation)
: true'
- message: spotMarketOptions is incompatible with capacityReservation
and requires tenancy to be 'default' or unset (not 'dedicated'
or 'host')
rule: 'has(self.spotMarketOptions) ? (!has(self.capacityReservation)
&& (!has(self.tenancy) || self.tenancy == ''default''))
: true'
resourceTags:
description: |-
resourceTags is an optional list of additional tags to apply to AWS node
Expand Down
22 changes: 22 additions & 0 deletions cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/nodepools-Default.crd.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -487,6 +487,22 @@ spec:
requires a Capacity Reservation ID
rule: 'has(self.marketType) && self.marketType == ''CapacityBlocks''
? has(self.id) : true'
spotMarketOptions:
description: |-
spotMarketOptions specifies options for using AWS Spot instances.
When specified, instances will be launched as Spot instances with the given configuration.
Mutually exclusive with capacityReservation, and tenancy must be unset or set to "default".
properties:
maxPrice:
description: |-
maxPrice defines the maximum price (USD per instance‑hour) you are willing to pay for a Spot instance.
If omitted, the On‑Demand price is used as the ceiling.
Example: "0.0739"
Format: up to 10 integer digits and up to 6 fractional digits; no leading zeros unless the value is "0"; scientific notation is not allowed.
maxLength: 17
pattern: ^(0|[1-9][0-9]{0,9})(\.[0-9]{1,6})?$
type: string
type: object
Comment on lines +490 to +505

@coderabbitai coderabbitai Bot Sep 4, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Non-ASCII hyphens in descriptions (“instance‑hour”, “On‑Demand”).

Replace U+2011/U+2013 style hyphens with ASCII "-" in the Go API comments (api/hypershift/v1beta1/aws.go), then regenerate CRDs to avoid encoding issues in tooling.

Apply in api/hypershift/v1beta1/aws.go:

-// maxPrice defines the maximum price (USD per instance‑hour) you are willing to pay for a Spot instance.
-// If omitted, the On‑Demand price is used as the ceiling.
+// maxPrice defines the maximum price (USD per instance-hour) you are willing to pay for a Spot instance.
+// If omitted, the On-Demand price is used as the ceiling.

Then run codegen and verify no non-ASCII hyphens remain:

🏁 Script executed:

#!/bin/bash
rg -nU "[\u2010-\u2015]" cmd/install/assets/hypershift-operator/zz_generated.crd-manifests || true

Length of output: 1397

Fix non-ASCII hyphens and regenerate CRDs

The following generated CRDs still contain non-ASCII hyphens at lines 498–499:

  • cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/nodepools-TechPreviewNoUpgrade.crd.yaml
  • cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/nodepools-CustomNoUpgrade.crd.yaml
  • cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/nodepools-Default.crd.yaml

Apply the diff in api/hypershift/v1beta1/aws.go, rerun codegen, then verify no non-ASCII hyphens remain:

rg -nU "[\u2010-\u2015]" cmd/install/assets/hypershift-operator/zz_generated.crd-manifests || true
🤖 Prompt for AI Agents 
In
cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/nodepools-Default.crd.yaml
around lines 490–505 there are non-ASCII hyphens in the generated CRD
descriptions (lines ~498–499); to fix, update api/hypershift/v1beta1/aws.go to
replace any Unicode hyphen/dash characters with the ASCII hyphen-minus, save,
re-run the CRD code generation pipeline (codegen), and then verify no non-ASCII
hyphens remain by running the suggested ripgrep check (rg -nU "[\u2010-\u2015]"
cmd/install/assets/hypershift-operator/zz_generated.crd-manifests || true);
commit the updated source file and the regenerated CRD manifests.

tenancy:
description: |-
tenancy indicates if instance should run on shared or single-tenant hardware.
Expand All @@ -509,6 +525,12 @@ spec:
Hosts (tenancy 'host')
rule: 'has(self.tenancy) && self.tenancy == ''host'' ? !has(self.capacityReservation)
: true'
- message: spotMarketOptions is incompatible with capacityReservation
and requires tenancy to be 'default' or unset (not 'dedicated'
or 'host')
rule: 'has(self.spotMarketOptions) ? (!has(self.capacityReservation)
&& (!has(self.tenancy) || self.tenancy == ''default''))
: true'
resourceTags:
description: |-
resourceTags is an optional list of additional tags to apply to AWS node
Expand Down
22 changes: 22 additions & 0 deletions ...ts/hypershift-operator/zz_generated.crd-manifests/nodepools-TechPreviewNoUpgrade.crd.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -487,6 +487,22 @@ spec:
requires a Capacity Reservation ID
rule: 'has(self.marketType) && self.marketType == ''CapacityBlocks''
? has(self.id) : true'
spotMarketOptions:
description: |-
spotMarketOptions specifies options for using AWS Spot instances.
When specified, instances will be launched as Spot instances with the given configuration.
Mutually exclusive with capacityReservation, and tenancy must be unset or set to "default".
properties:
maxPrice:
description: |-
maxPrice defines the maximum price (USD per instance‑hour) you are willing to pay for a Spot instance.
If omitted, the On‑Demand price is used as the ceiling.
Example: "0.0739"
Format: up to 10 integer digits and up to 6 fractional digits; no leading zeros unless the value is "0"; scientific notation is not allowed.
maxLength: 17
pattern: ^(0|[1-9][0-9]{0,9})(\.[0-9]{1,6})?$
type: string
type: object
tenancy:
description: |-
tenancy indicates if instance should run on shared or single-tenant hardware.
Expand All @@ -509,6 +525,12 @@ spec:
Hosts (tenancy 'host')
rule: 'has(self.tenancy) && self.tenancy == ''host'' ? !has(self.capacityReservation)
: true'
- message: spotMarketOptions is incompatible with capacityReservation
and requires tenancy to be 'default' or unset (not 'dedicated'
or 'host')
rule: 'has(self.spotMarketOptions) ? (!has(self.capacityReservation)
&& (!has(self.tenancy) || self.tenancy == ''default''))
: true'
resourceTags:
description: |-
resourceTags is an optional list of additional tags to apply to AWS node
Expand Down
Loading