Add capacity reservation to spec

[Skarlso]

Description

Closes #5258

Note, that this adds a bit more than just the ID to be more flexible and preempt future requests of "just add the ARN"... :)

Checklist

• Added tests that cover your change (if possible)
• Added/modified documentation as required (such as the README.md, or the userdocs directory)
• Manually tested
• Made sure the title of the PR is a good description that can go into the release notes
• (Core team) Added labels for change area (e.g. area/nodegroup) and kind (e.g. kind/improvement)

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯

• Backfilled missing tests for code in same general area 🎉
• Refactored something and made the world a better place 🌟

[Skarlso]

Anyone knows how to update the docs for the config reference?
I thought build-pages would do the trick, but it didn't.

[Himangini]

looks like the schema is correctly updated in the docs

[Himangini]

LGTM 👍🏻

[nikimanoledaki]

A couple of comments/change requests! :)
We could also add validation that CapacityReservationPreference is set to a valid value, which is either open or none, and add a note about this in the schema.

[nikimanoledaki]

Suggested change

	CapacityReservationID *string `json:"capacityReservationId,omitempty"`
	CapacityReservationID *string `json:"capacityReservationID,omitempty"`

shouldn't this be uppercase ID?

[Skarlso]

Sure. :D Adjust the tests too :)

[nikimanoledaki]

The CapacityReservationSpecification docs specify that:

You can specify only one parameter at a time. If you specify CapacityReservationPreference and CapacityReservationTarget, the request fails.

We should add validation that not both are set at the same time.

Stale review because I'm applying the changes and will require a review from someone else.

[nikimanoledaki]

I tried creating it manually with just CapacityReservationTarget and both CapacityReservationTargetId and CapacityReservationResourceGroupArn, and got this error:

You cannot specify a value for CapacityReservationId and CapacityReservationResourceGroupArn in the same request. Specify either a CapacityReservationId or a CapacityReservationResourceGroupArn and try again. 

I will add more validation to split it further since those 2 fields also cannot be set at the same time.

[Himangini]

I tried creating it manually with just CapacityReservationTarget and both CapacityReservationTargetId and CapacityReservationResourceGroupArn, and got this error:

You cannot specify a value for CapacityReservationId and CapacityReservationResourceGroupArn in the same request. Specify either a CapacityReservationId or a CapacityReservationResourceGroupArn and try again. 

I will add more validation to split it further since those 2 fields also cannot be set at the same time.

@nikimanoledaki The original ticket was just about adding the support for CapacityReservationID, I think we should just stick to that and not go overboard with other settings. If we get more requests in the future we'll see how to add support for them. Sounds good?

Note, that this adds a bit more than just the ID to be more flexible and preempt future requests of "just add the ARN"... :)

Stale

[Skarlso]

@nikimanoledaki The original ticket was just about adding the support for CapacityReservationID, I think we should just stick to that and not go overboard with other settings. If we get more requests in the future we'll see how to add support for them. Sounds good?

I don't think she added anything new right now, right? She just improved the UX by returning an error if the fields are defined together. ARN was already there. Thanks, Niki, great work! :) Thanks for shepherding my little baby. :D

[nikimanoledaki]

@Himangini The change was simple: adding 1 more validation and updating the tests. I just finished it and pushed the changes.

Regarding scope, the feature request was to add support for Capacity Reservations. This is what this PR does, including Preference, ID, and Resource Group.

IDs are part of the Target, and I think (and I agree with Gergely) that it would not be a complete feature if we did not add Preference and Resource Group as well - it's 2 more fields of the same CloudFormation struct.

[Himangini]

@Himangini The change was simple: adding 1 more validation and updating the tests. I just finished it and pushed the changes.

Thats awesome, thanks for that.

Regarding scope, the feature request was to add support for Capacity Reservations (Preference, ID, and Resource Group). This is what this PR does.

With my earlier comment, I was saying I don't think we talked about adding support for ARN during planning, I am guessing this PR is adding extra bits as mentioned in the quote below and all I was saying was if it's creating more problems let's skip the ARN support.

Note, that this adds a bit more than just the ID to be more flexible and preempt future requests of "just add the ARN"... :)

[nikimanoledaki]

@Himangini Thank you for raising the question regarding scope. Thankfully the extra bits only took some validation to make this feature complete and this PR is ready to be reviewed 👍

[nikimanoledaki]

Manually tested that it works:

apiVersion: eksctl.io/v1alpha5
kind: ClusterConfig

metadata:
  name: nm-cap-res-target-id-test
  region: eu-north-1

nodeGroups:
  - name: ng-1
    instanceType: t3.small
    desiredCapacity: 1
    availabilityZones: ["eu-north-1a"]
    capacityReservation:
      capacityReservationTarget:
        capacityReservationID: "cr-07497b08a30634133"

It used the Capacity Reservation (0/1 available) 🎉 🥳

[Himangini]

👍🏻

[tanvp112]

looks like the schema is correctly updated in the docs

Hi @Himangini , couldn't find the said field at the online page. Can you provide the link to the screenshot you captured? Would like to understand if this new feature applies to both node group and managed node group?

Thanks.

[Skarlso]

Hello @tanvp112!

Yes, it applies to both, managed and unmanaged.

The website will not have it yet, as this was just released recently in an RC. :) Please wait for a full release for the docs update. :)