-
Notifications
You must be signed in to change notification settings - Fork 557
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
✨ Added the CapacityReservation support #5047
✨ Added the CapacityReservation support #5047
Conversation
Welcome @athiruma! |
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. |
/ok-to-test |
aa8cee7
to
7ed3c02
Compare
api/v1beta1/awsmachine_types.go
Outdated
@@ -157,6 +157,10 @@ type AWSMachineSpec struct { | |||
// +optional | |||
// +kubebuilder:validation:Enum:=default;dedicated;host | |||
Tenancy string `json:"tenancy,omitempty"` | |||
|
|||
// CapacityReservationID specifies the capacity reservation resource id that should be |
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 you are missing the rest of the sentence :)
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
a934b6d
to
1af019d
Compare
api/v1beta1/awsmachine_types.go
Outdated
// CapacityReservationID specifies the instance that should be launched in the | ||
// reserved compute capacity. | ||
// +optional | ||
CapacityReservationID string `json:"capacityReservationId,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.
What's the motivation to add this field to v1beta1? In general, new fields should probably only go into v1beta2.
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 added the same based on the spot market options PR. I will remove that.
1af019d
to
11045bb
Compare
Fixed the lint Fixed the description Removed the API in v1
11045bb
to
dc07755
Compare
/test pull-cluster-api-provider-aws-test |
api/v1beta2/awsmachine_types.go
Outdated
// CapacityReservationID specifies the instance that should be launched in the | ||
// reserved compute capacity. |
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 does not specify an instance.
// CapacityReservationID specifies the instance that should be launched in the | |
// reserved compute capacity. | |
// CapacityReservationID specifies the target Capacity Reservation into which the instance should be launched. |
pkg/cloud/services/ec2/instances.go
Outdated
@@ -1119,6 +1122,22 @@ func filterGroups(list []string, strToFilter string) (newList []string) { | |||
return | |||
} | |||
|
|||
func getCapacityReservationSpecification(capacityReservationID string) *ec2.CapacityReservationSpecification { | |||
if capacityReservationID == "" { | |||
// Instance is not a CapacityReservation 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.
See above. There's "[EC2] instance" and "Capacity Reservation" as two separate terms. Let's not mix those.
// Instance is not a CapacityReservation instance | |
// Not targetting any specific Capacity Reservation |
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.
ok
pkg/cloud/services/ec2/instances.go
Outdated
// Set required values for CapacityReservation | ||
capacityReservationTargetOptions := &ec2.CapacityReservationTarget{} | ||
capacityReservationTargetOptions.SetCapacityReservationId(capacityReservationID) | ||
|
||
capacityReservationSpecification := &ec2.CapacityReservationSpecification{} | ||
capacityReservationSpecification.SetCapacityReservationTarget(capacityReservationTargetOptions) | ||
|
||
return capacityReservationSpecification |
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.
// Set required values for CapacityReservation | |
capacityReservationTargetOptions := &ec2.CapacityReservationTarget{} | |
capacityReservationTargetOptions.SetCapacityReservationId(capacityReservationID) | |
capacityReservationSpecification := &ec2.CapacityReservationSpecification{} | |
capacityReservationSpecification.SetCapacityReservationTarget(capacityReservationTargetOptions) | |
return capacityReservationSpecification | |
return &ec2.CapacityReservationSpecification{ | |
CapacityReservationTarget: &ec2.CapacityReservationTarget{ | |
CapacityReservationId: ptr.To[string](capacityReservationID), | |
} | |
} |
}, | ||
{ | ||
name: "with an valid CapacityReservationID specified", | ||
capacityReservationID: *aws.String(mockCapacityReservationID), |
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: *aws.String(mockCapacityReservationID), | |
capacityReservationID: mockCapacityReservationID, |
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 accepts only the *string. So it will be aws.String(mockCapacityReservationID)
expectedRequest: nil, | ||
}, | ||
{ | ||
name: "with an valid CapacityReservationID specified", |
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.
name: "with an valid CapacityReservationID specified", | |
name: "with a valid CapacityReservationID specified", |
11012a7
to
35cf423
Compare
Removed the tests by verify-gen
35cf423
to
5e44fa5
Compare
/lgtm |
/lgtm |
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
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AndiDog The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
What this PR does / why we need it:
Add a new field to API -
capacityReservationId
, then use it to create capacity reserved instances by initializing aCapacityReservationSpecification
and passing it toRunInstances()
.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: