Skip to content

Conversation

mzazrivec
Copy link
Contributor

@mzazrivec mzazrivec commented Sep 4, 2025

What type of PR is this?

/kind feature

What this PR does / why we need it:

This pull request adds support for AWS Capacity Reservations to the ROSAMachinePool custom resource.

The capacityReservationId is the string id of the AWS on-demand capacity reservation and it must be created in advance, before creating the node pool.

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 #

Special notes for your reviewer:

Checklist:

  • squashed commits
  • includes documentation
  • includes emoji in title
  • adds unit tests
  • adds or updates e2e tests

Release note:

New ROSAMachinePool parameter for specifying AWS capacity reservation.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 4, 2025
@k8s-ci-robot k8s-ci-robot added needs-priority needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 4, 2025
@k8s-ci-robot
Copy link
Contributor

Hi @mzazrivec. 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 /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-sigs/prow repository.

@mzazrivec mzazrivec changed the title ROSAMachinePool: support for capacity reservations ✨ ROSAMachinePool: support for capacity reservations Sep 4, 2025
@richardcase
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 6, 2025
@mzazrivec mzazrivec force-pushed the capacity_reservations_in_rosa_machinepool branch 4 times, most recently from 71a4d19 to b932828 Compare September 8, 2025 14:21

gomock "github.com/golang/mock/gomock"
v1 "github.com/openshift-online/ocm-sdk-go/clustersmgmt/v1"
v1 "github.com/openshift-online/ocm-api-model/clientapi/clustersmgmt/v1"
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to change this

Suggested change
v1 "github.com/openshift-online/ocm-api-model/clientapi/clustersmgmt/v1"
v1 "github.com/openshift-online/ocm-sdk-go/clustersmgmt/v1"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I put the change back. I remembered that this one change is generated by go generate and without it make verify-gen (part of CI) would be failing.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 18, 2025
@mzazrivec mzazrivec force-pushed the capacity_reservations_in_rosa_machinepool branch from b932828 to 0db2942 Compare September 19, 2025 10:20
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 19, 2025
@mzazrivec mzazrivec force-pushed the capacity_reservations_in_rosa_machinepool branch from 0db2942 to e08500c Compare September 19, 2025 12:16
@mzazrivec
Copy link
Contributor Author

/retest

@serngawy
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 19, 2025
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: f144ef64bc5215666857d3df0651429ad543b415

@serngawy
Copy link
Contributor

/approved

Copy link
Member

@damdo damdo left a comment

Choose a reason for hiding this comment

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

Looks reasonable, just one nit

Copy link
Member

@damdo damdo left a comment

Choose a reason for hiding this comment

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

Thanks

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 24, 2025
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 6a943899e6d7a84517eb734581cb6eeb60b14a91

@damdo
Copy link
Member

damdo commented Sep 24, 2025

This one should be ready now @nrb @richardcase

Copy link

@bchandra-ocp bchandra-ocp left a comment

Choose a reason for hiding this comment

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

I had an inquiry/feedback left in the comment.

type: string
capacityReservationID:
description: |-
CapacityReservationID specifies the ID of an AWS On-Demand Capacity Reservation.

Choose a reason for hiding this comment

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

My understanding is that CapacityReservationID can specify the ID of both AWS ODCR and CapacityBlocks for ML. ROSA will inquire the type of capacity reservation from AWS and populate the market type accordingly. If so, should we clarify that here in the description?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bchandra-ocp What description string are you proposing?

Choose a reason for hiding this comment

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

How about

CapacityReservationID specifies the ID of an AWS On-Demand Capacity Reservation and Capacity Blocks for ML

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bchandra-ocp Added.

@mzazrivec mzazrivec force-pushed the capacity_reservations_in_rosa_machinepool branch from e82eb26 to 8853c4b Compare September 29, 2025 18:25
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 29, 2025
@k8s-ci-robot k8s-ci-robot requested a review from damdo September 29, 2025 18:26
Copy link
Member

@damdo damdo left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 30, 2025
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: c3aa1db4d156871076d3feb4cf916163cbe5d60c

@damdo
Copy link
Member

damdo commented Sep 30, 2025

/assign @nrb @richardcase

Would you be able to add your review/labels here thanks!

@damdo
Copy link
Member

damdo commented Sep 30, 2025

/label tide/merge-method-squash

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Sep 30, 2025
@richardcase
Copy link
Member

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: richardcase

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 30, 2025
@mzazrivec
Copy link
Contributor Author

/retest

@damdo
Copy link
Member

damdo commented Sep 30, 2025

/test pull-cluster-api-provider-aws-e2e-blocking

@k8s-ci-robot k8s-ci-robot merged commit c278953 into kubernetes-sigs:main Sep 30, 2025
19 checks passed
openshift-cherrypick-robot pushed a commit to openshift-cherrypick-robot/cluster-api-provider-aws that referenced this pull request Oct 2, 2025
openshift-cherrypick-robot pushed a commit to openshift-cherrypick-robot/cluster-api-provider-aws that referenced this pull request Oct 2, 2025
@serngawy
Copy link
Contributor

serngawy commented Oct 6, 2025

/cherry-pick release-2.9

@k8s-infra-cherrypick-robot

@serngawy: #5649 failed to apply on top of branch "release-2.9":

Applying: ROSAMachinePool: support for capacity reservations
Using index info to reconstruct a base tree...
M	config/crd/bases/infrastructure.cluster.x-k8s.io_rosamachinepools.yaml
M	exp/controllers/rosamachinepool_controller.go
A	exp/utils/rosa_helper.go
M	go.mod
M	go.sum
M	test/mocks/ocm_client_mock.go
Falling back to patching base and 3-way merge...
Auto-merging test/mocks/ocm_client_mock.go
Auto-merging go.sum
Auto-merging go.mod
CONFLICT (modify/delete): exp/utils/rosa_helper.go deleted in HEAD and modified in ROSAMachinePool: support for capacity reservations. Version ROSAMachinePool: support for capacity reservations of exp/utils/rosa_helper.go left in tree.
Auto-merging exp/controllers/rosamachinepool_controller.go
Auto-merging config/crd/bases/infrastructure.cluster.x-k8s.io_rosamachinepools.yaml
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 ROSAMachinePool: support for capacity reservations

In response to this:

/cherry-pick release-2.9

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.

@serngawy
Copy link
Contributor

serngawy commented Oct 7, 2025

/cherry-pick release-2.9

@k8s-infra-cherrypick-robot

@serngawy: new pull request created: #5698

In response to this:

/cherry-pick release-2.9

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.

Projects

None yet

Development

Successfully merging this pull request may close these issues.