-
Notifications
You must be signed in to change notification settings - Fork 579
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.
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
✨ Add the support for capacity blocks #5211
base: main
Are you sure you want to change the base?
Conversation
Hi @athiruma. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your PR @athiruma 👍
Left some suggestions to improve the overall readability.
8ae7c7b
to
d34e59b
Compare
@damdo?? |
/ok-to-test |
/test pull-cluster-api-provider-aws-test |
d34e59b
to
f21fca7
Compare
/test pull-cluster-api-provider-aws-e2e |
/test pull-cluster-api-provider-aws-e2e |
f21fca7
to
05baf2d
Compare
/test pull-cluster-api-provider-aws-e2e |
/test pull-cluster-api-provider-aws-e2e-eks |
1 similar comment
/test pull-cluster-api-provider-aws-e2e-eks |
The eks test is still flakey. I'll take a look at it tomorrow, I think I know where we can improve it |
/test pull-cluster-api-provider-aws-e2e-eks |
/test pull-cluster-api-provider-aws-e2e |
1 similar comment
/test pull-cluster-api-provider-aws-e2e |
/retest |
@athiruma: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. |
c975830
to
6c1c6bd
Compare
api/v1beta2/awsmachine_types.go
Outdated
// UseCapacityBlock enables usage of pre-purchased compute capacity (capacity blocks) with AWS Capacity Reservations. | ||
// If enabled, CapacityReservationID must be specified to identify the target reservation. | ||
// +optional | ||
UseCapacityBlock *bool `json:"useCapacityBlock,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed, if all it is doing is enabling the capacity reservation ID field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need this to use for AWS capacity blocks for ml.
@damdo ptal ? |
b5997f8
to
7a2948f
Compare
@@ -197,6 +197,14 @@ type AWSMachineSpec struct { | |||
// CapacityReservationID specifies the target Capacity Reservation into which the instance should be launched. | |||
// +optional | |||
CapacityReservationID *string `json:"capacityReservationId,omitempty"` | |||
|
|||
// MarketType specifies the type of market for the EC2 instance. Valid values include: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// MarketType specifies the type of market for the EC2 instance. Valid values include: | |
// marketType specifies the type of market for the EC2 instance. Valid values include: |
api/v1beta2/awsmachine_types.go
Outdated
// If this value is selected, CapacityReservationID must be specified to identify the target reservation. | ||
// If MarketType is not specified and SpotMarketOptions is provided, the MarketType defaults to "spot". | ||
// +optional | ||
MarketType *MarketType `json:"marketType,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be a pointer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should allow empty values so I thought we should use a pointer here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you care about the difference between marketType: ""
and the field not being present at all? The controller will treat those two representations in the same way won't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I see then I'll make a change. Thanks
api/v1beta2/types.go
Outdated
const ( | ||
// MarketTypeOnDemand is a MarketType enum value | ||
MarketTypeOnDemand MarketType = "on-demand" | ||
|
||
// MarketTypeSpot is a MarketType enum value | ||
MarketTypeSpot MarketType = "spot" | ||
|
||
// MarketTypeCapacityBlock is a MarketType enum value | ||
MarketTypeCapacityBlock MarketType = "capacity-block" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kubernetes API conventions state that enum values should be PascalCase. We prefer this and then to convert to upstream APIs (EC2) in this case so that there's consistency across Kube like APIs. So these should really be OnDemand, Spot and CapacityBlock
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
} | ||
|
||
// MarketType describes the market type of an Instance | ||
type MarketType string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll need a kubebuilder:validation:Enum
marker on here to make sure the correct schema is generated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
api/v1beta2/awsmachine_webhook.go
Outdated
@@ -361,6 +362,14 @@ func (r *AWSMachine) validateNetworkElasticIPPool() field.ErrorList { | |||
return allErrs | |||
} | |||
|
|||
func (r *AWSMachine) validateInstanceMarketType() field.ErrorList { | |||
var allErrs field.ErrorList | |||
if ptr.Deref(r.Spec.MarketType, "") == MarketTypeCapacityBlock && r.Spec.SpotMarketOptions != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this also check MarketType is not ondemand if SpotMarketOptions != nil? i.e. the check should be that the only valid when SpotMarketOptions != nil is MarketType==spot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh yeah good catch thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
@@ -197,6 +197,14 @@ type AWSMachineSpec struct { | |||
// CapacityReservationID specifies the target Capacity Reservation into which the instance should be launched. | |||
// +optional | |||
CapacityReservationID *string `json:"capacityReservationId,omitempty"` | |||
|
|||
// MarketType specifies the type of market for the EC2 instance. Valid values include: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we describe in gdoc what's the default behaviour if I don't set spot or capacityBlock? i.e on demand
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
7a2948f
to
c1d94dc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some documentation fixes for the new casing and LGTM from the API perspective
api/v1beta2/awsmachine_webhook.go
Outdated
func (r *AWSMachine) validateInstanceMarketType() field.ErrorList { | ||
var allErrs field.ErrorList | ||
if r.Spec.MarketType == MarketTypeCapacityBlock && r.Spec.SpotMarketOptions != nil { | ||
allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "MarketType"), "MarketType set to CapacityBlock and spotMarketOptions cannot be used together")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "MarketType"), "MarketType set to CapacityBlock and spotMarketOptions cannot be used together")) | |
allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "marketType"), "marketType set to CapacityBlock and spotMarketOptions cannot be used together")) |
api/v1beta2/awsmachine_webhook.go
Outdated
allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "MarketType"), "MarketType set to CapacityBlock and spotMarketOptions cannot be used together")) | ||
} | ||
if r.Spec.MarketType == MarketTypeOnDemand && r.Spec.SpotMarketOptions != nil { | ||
allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "MarketType"), "setting MarketType to OnDemand and spotMarketOptions cannot be used together")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "MarketType"), "setting MarketType to OnDemand and spotMarketOptions cannot be used together")) | |
allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "marketType"), "setting marketType to OnDemand and spotMarketOptions cannot be used together")) |
api/v1beta2/types.go
Outdated
@@ -261,8 +261,32 @@ type Instance struct { | |||
// CapacityReservationID specifies the target Capacity Reservation into which the instance should be launched. | |||
// +optional | |||
CapacityReservationID *string `json:"capacityReservationId,omitempty"` | |||
|
|||
// marketType specifies the type of market for the EC2 instance. Valid values include: | |||
// "on-demand" (default): The instance runs as a standard On-Demand instance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// "on-demand" (default): The instance runs as a standard On-Demand instance. | |
// "OnDemand" (default): The instance runs as a standard On-Demand instance. |
api/v1beta2/types.go
Outdated
|
||
// marketType specifies the type of market for the EC2 instance. Valid values include: | ||
// "on-demand" (default): The instance runs as a standard On-Demand instance. | ||
// "spot": The instance runs as a Spot instance. When SpotMarketOptions is provided, the MarketType defaults to "spot". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// "spot": The instance runs as a Spot instance. When SpotMarketOptions is provided, the MarketType defaults to "spot". | |
// "Spot": The instance runs as a Spot instance. When spotMarketOptions is provided, the marketType defaults to "Spot". |
api/v1beta2/types.go
Outdated
// marketType specifies the type of market for the EC2 instance. Valid values include: | ||
// "on-demand" (default): The instance runs as a standard On-Demand instance. | ||
// "spot": The instance runs as a Spot instance. When SpotMarketOptions is provided, the MarketType defaults to "spot". | ||
// "capacity-block": The instance utilizes pre-purchased compute capacity (capacity blocks) with AWS Capacity Reservations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// "capacity-block": The instance utilizes pre-purchased compute capacity (capacity blocks) with AWS Capacity Reservations. | |
// "CapacityBlock": The instance utilizes pre-purchased compute capacity (capacity blocks) with AWS Capacity Reservations. |
api/v1beta2/types.go
Outdated
// "spot": The instance runs as a Spot instance. When SpotMarketOptions is provided, the MarketType defaults to "spot". | ||
// "capacity-block": The instance utilizes pre-purchased compute capacity (capacity blocks) with AWS Capacity Reservations. | ||
// If this value is selected, CapacityReservationID must be specified to identify the target reservation. | ||
// If MarketType is not specified and SpotMarketOptions is provided, the MarketType defaults to "spot". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// If MarketType is not specified and SpotMarketOptions is provided, the MarketType defaults to "spot". | |
// If marketType is not specified and spotMarketOptions is provided, the marketType defaults to "Spot". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
8f0b86f
to
a1fa3fd
Compare
fixed |
@@ -128,6 +128,19 @@ type AWSLaunchTemplate struct { | |||
// PrivateDNSName is the options for the instance hostname. | |||
// +optional | |||
PrivateDNSName *infrav1.PrivateDNSName `json:"privateDnsName,omitempty"` | |||
|
|||
// CapacityReservationID specifies the target Capacity Reservation into which the instance should be launched. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// CapacityReservationID specifies the target Capacity Reservation into which the instance should be launched. | |
// capacityReservationID specifies the target Capacity Reservation into which the instance should be launched. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, but this is supposed to be exactly the other way around. Go doc comments are meant to start with the name they describe, as shown for example in the rest of the file. Since the name is capital CapacityReservationID
, it's also spelled this way in the comment. Same for all new fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is not true for Kubernetes APIs, it is convention to use the serialized version within the godoc so that commands like kubectl explain
use the same language as the end user will be familiar with. Cluster API has also adopted this convention as well, see kubernetes-sigs/cluster-api#11238
I had assumed it was being adopted community wide?
a1fa3fd
to
af2ddda
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
InstanceType: "test", | ||
}, | ||
}, | ||
wantErr: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be an error because CapacityReservationID
isn't filled. If I'm right, an additional positive/valid case should also be added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed validating the capacityReservationId
in the webhook when the marketType is set to CapacityBlock. Instead, I raised the error in the getInstanceMarketOptions
method. It would be better to include the validation in the webhook as well. Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
}, | ||
}, | ||
}, | ||
wantErr: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, this is an error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added the webhook and added one more test cases.
pkg/cloud/services/ec2/instances.go
Outdated
return nil, nil | ||
default: | ||
// Invalid MarketType provided | ||
return nil, errors.Errorf("invalid MarketType %s, must be spot/capacity-block or empty", i.MarketType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return nil, errors.Errorf("invalid MarketType %s, must be spot/capacity-block or empty", i.MarketType) | |
return nil, errors.Errorf("invalid MarketType %q", i.MarketType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
func getLaunchTemplateInstanceMarketOptionsRequest(i *expinfrav1.AWSLaunchTemplate) (*ec2.LaunchTemplateInstanceMarketOptionsRequest, error) { | ||
if i.MarketType != "" && i.MarketType != infrav1.MarketTypeCapacityBlock && i.SpotMarketOptions != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function does the same thing as getInstanceMarketOptionsRequest
. Sad that the distinct types don't allow it to easily be one function.
From comparing both functions, I think this is a bug:
func getLaunchTemplateInstanceMarketOptionsRequest(i *expinfrav1.AWSLaunchTemplate) (*ec2.LaunchTemplateInstanceMarketOptionsRequest, error) { | |
if i.MarketType != "" && i.MarketType != infrav1.MarketTypeCapacityBlock && i.SpotMarketOptions != nil { | |
func getLaunchTemplateInstanceMarketOptionsRequest(i *expinfrav1.AWSLaunchTemplate) (*ec2.LaunchTemplateInstanceMarketOptionsRequest, error) { | |
if i.MarketType != "" && i.MarketType == infrav1.MarketTypeCapacityBlock && i.SpotMarketOptions != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh, thanks. fixed
@@ -1076,6 +1076,7 @@ func TestCreateLaunchTemplateVersion(t *testing.T) { | |||
awsResourceReference []infrav1.AWSResourceReference | |||
expect func(m *mocks.MockEC2APIMockRecorder) | |||
wantErr bool | |||
MarketType *string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MarketType *string | |
marketType *string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
af2ddda
to
66769de
Compare
New changes are detected. LGTM label has been removed. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR added the support for CapacityBlocks for ML, by adding this we can get the support for capacity reservations of capacity blocks for ML provided by AWS.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes # #5045
Special notes for your reviewer:
Checklist:
Release note: