Fix AdoptedResources functionality for CapacityReservations #268

fizashaikh · 2025-05-20T00:30:31Z

Issue #, if available: aws-controllers-k8s/community#2485

Description of changes:
This PR adds the following hooks for CapacityReservations.

post_set_resource_identifiers - This hook is needed to set additional fields required for successfully adopting existing reservation.
sdk_read_many_post_set_output - This hook is required to correctly update instance count for reservations after they are adopted.
sdk_update_post_build_request - This hook is needed to add support to update/modify capacity reservations using the additionalInfo reserved field.

The new AdoptedResource config looks like follows,

apiVersion: services.k8s.aws/v1alpha1
kind: AdoptedResource
metadata:
  name: test-adopt-existing-capacity   # name of the adopted resource
spec: 
  aws:
    nameOrID: cr-XXXXXXXXX     # ID of the actual capacity reservation in AWS
    additionalKeys:
      instanceCount: "1"              # replace with the actual instance count from your reservation
      availabilityZone: "us-west-2a"  # replace with your actual AZ
      instancePlatform: "Linux/UNIX"  # match the actual platform
      instanceType: "t2.nano"         # match the actual instance type
  kubernetes:
    group: ec2.services.k8s.aws
    kind: CapacityReservation
    metadata:
      name: test-adopt-capacity # name of the K8s capacity reservation resource created by ack-ec2-controller
      namespace: default

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

… for CapacityReservations

ack-prow · 2025-05-20T00:30:41Z

Hi @fizashaikh. Thanks for your PR.

I'm waiting for a aws-controllers-k8s member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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/test-infra repository.

ack-prow · 2025-05-20T00:30:41Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: fizashaikh
Once this PR has been reviewed and has the lgtm label, please assign knottnt for approval by writing /assign @knottnt in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

…ionalInfo field is passed

rushmash91 · 2025-05-27T22:53:00Z

apis/v1alpha1/ack-generate-metadata.yaml

+  build_date: "2025-05-26T04:21:07Z"
+  build_hash: b57010693b861c793703028023ae75209a6af343
+  go_version: go1.24.0
+  version: v0.43.2-26-gb570106-dirty
 api_directory_checksum: 3df6921566512e42abdabd564c80aea5311d410a


We are currently on v 47.2 for code-gen

Hi @fizashaikh , Thank you for your contribution!

I see that that the code-generator version you are using is v0.43.2 dirty, can you update it to the latest version and regenerate the controller.

michaelhtm

Thank you @fizashaikh
left a few comments..
Also would you like to try out https://aws-controllers-k8s.github.io/community/docs/user-docs/features/#resourceadoption?

michaelhtm · 2025-05-27T22:57:40Z

templates/hooks/capacity_reservation/post_set_resource_identifiers.go.tpl

+	if instanceCount, ok := identifier.AdditionalKeys["instanceCount"]; ok {
+			parsedInstanceCount, err := strconv.ParseInt(instanceCount, 10, 64)
+			if err != nil {
+				return fmt.Errorf("failed to parse instanceCount: %v", err)
+			}
+			r.ko.Spec.InstanceCount = &parsedInstanceCount
+	}


Having an sdk_read_many_post_set_output.go would be enough instead of adding this one?

michaelhtm · 2025-05-27T23:00:30Z

templates/hooks/capacity_reservation/post_set_resource_identifiers.go.tpl

+	if instancePlatform, ok := identifier.AdditionalKeys["instancePlatform"]; ok {
+		r.ko.Spec.InstancePlatform = &instancePlatform
+	}
+
+	if instanceType, ok := identifier.AdditionalKeys["instanceType"]; ok {
+		r.ko.Spec.InstanceType = &instanceType
+	}
+
+	if availabilityZone, ok := identifier.AdditionalKeys["availabilityZone"]; ok {
+		r.ko.Spec.AvailabilityZone = &availabilityZone
+	}
+
+	if availabilityZoneID, ok := identifier.AdditionalKeys["availabilityZoneID"]; ok {
+		r.ko.Spec.AvailabilityZoneID = &availabilityZoneID
+	}


these fields seem to already be set in sdkFind..why set them again?

michaelhtm · 2025-05-27T23:00:38Z

templates/hooks/capacity_reservation/sdk_read_many_post_set_output.go.tpl

+	if ko.Status.TotalInstanceCount != nil {
+		ko.Spec.InstanceCount = ko.Status.TotalInstanceCount
+	}


michaelhtm · 2025-05-27T23:04:16Z

pkg/resource/capacity_reservation/sdk.go

+	if input.AdditionalInfo != nil {
+		input.InstanceCount = nil
+		input.EndDate = nil
+		input.EndDateType = ""
+		input.InstanceMatchCriteria = ""
+	}


with this change, AdditionalInfo may never be nil...would this be better?

Suggested change

if input.AdditionalInfo != nil {

input.InstanceCount = nil

input.EndDate = nil

input.EndDateType = ""

input.InstanceMatchCriteria = ""

}

if delta.DifferentAt("Spec.AdditionalInfo") {

input.InstanceCount = nil

input.EndDate = nil

input.EndDateType = ""

input.InstanceMatchCriteria = ""

} else {

input.AdditionalInfo = nil

}

This also depends on which field would we prioritize to update..

Fiza Shaikh added 3 commits May 16, 2025 11:55

fix_capacity_reservation_adoption: Fix AdoptedResources functionality…

606f97f

… for CapacityReservations

fix_capacity_reservation_adoption: Fix AdoptedResources functionality…

8498190

… for CapacityReservations

fix_capacity_reservation_adoption: Fix AdoptedResources functionality…

2ac3488

… for CapacityReservations

ack-prow bot requested a review from a-hilaly May 20, 2025 00:30

ack-prow bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 20, 2025

ack-prow bot requested a review from jlbutler May 20, 2025 00:30

fix_capacity_reservation_adoption: Fix updating reservations if addit…

0e47229

…ionalInfo field is passed

rushmash91 reviewed May 27, 2025

View reviewed changes

michaelhtm reviewed May 27, 2025

View reviewed changes

Merge branch 'main' into fix_capacity_reservation_adoption

5738a2b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix AdoptedResources functionality for CapacityReservations #268

Fix AdoptedResources functionality for CapacityReservations #268

Uh oh!

fizashaikh commented May 20, 2025 •

edited

Loading

Uh oh!

ack-prow bot commented May 20, 2025

Uh oh!

ack-prow bot commented May 20, 2025

Uh oh!

rushmash91 May 27, 2025

Uh oh!

rushmash91 May 27, 2025 •

edited

Loading

Uh oh!

michaelhtm left a comment

Uh oh!

michaelhtm May 27, 2025

Uh oh!

michaelhtm May 27, 2025

Uh oh!

michaelhtm May 27, 2025

Uh oh!

michaelhtm May 27, 2025

Uh oh!

michaelhtm May 27, 2025

Uh oh!

Uh oh!

Fix AdoptedResources functionality for CapacityReservations #268

Are you sure you want to change the base?

Fix AdoptedResources functionality for CapacityReservations #268

Uh oh!

Conversation

fizashaikh commented May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ack-prow bot commented May 20, 2025

Uh oh!

ack-prow bot commented May 20, 2025

Uh oh!

rushmash91 May 27, 2025

Choose a reason for hiding this comment

Uh oh!

rushmash91 May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelhtm left a comment

Choose a reason for hiding this comment

Uh oh!

michaelhtm May 27, 2025

Choose a reason for hiding this comment

Uh oh!

michaelhtm May 27, 2025

Choose a reason for hiding this comment

Uh oh!

michaelhtm May 27, 2025

Choose a reason for hiding this comment

Uh oh!

michaelhtm May 27, 2025

Choose a reason for hiding this comment

Uh oh!

michaelhtm May 27, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fizashaikh commented May 20, 2025 •

edited

Loading

rushmash91 May 27, 2025 •

edited

Loading