Skip to content
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.

Sign up for GitHub

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

Jump to bottom

✨ Set Paused condition on reconciled resources status upon reconciliation being paused #5383

Merged
merged 1 commit into from
Mar 17, 2025
Merged

✨ Set Paused condition on reconciled resources status upon reconciliation being paused #5383

merged 1 commit into from
Mar 17, 2025

Conversation

theobarberbany
Copy link
Contributor

@theobarberbany theobarberbany commented Mar 11, 2025 •
edited
Loading

What type of PR is this?
/kind api-change

What this PR does / why we need it:

edit (@damdo):
To follow the recent updates in the CAPI Provider contract for InfraMachines: kubernetes-sigs/cluster-api#11275 we should set the Paused=true condition on the resource's status to acknowledge we detected the addition of the cluster.x-k8s.io/paused annotation on a resource and paused its reconciliation to fulfill the user's request.

AWSManagedCluster is handled in a separate PR as there are API changes involved. See #5394

Checklist:

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

Release note:

Set Paused condition on reconciled resources status upon pausing reconciliation (CAPI provider contract change)

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority labels Mar 11, 2025
@k8s-ci-robot
Copy link
Contributor

Welcome @theobarberbany!

It looks like this is your first PR to kubernetes-sigs/cluster-api-provider-aws 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api-provider-aws has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot
Copy link
Contributor

Hi @theobarberbany. 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.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 11, 2025
@damdo
Copy link
Member

damdo commented Mar 11, 2025

/assign @nrb

@damdo damdo changed the title Set Paused condition ✨ Set Paused condition Mar 11, 2025
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Mar 12, 2025
@damdo damdo changed the title ✨ Set Paused condition ✨ Set status.paused condition upon pausing reconciliation Mar 12, 2025
@damdo
Copy link
Member

damdo commented Mar 12, 2025

/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 Mar 12, 2025
@damdo damdo changed the title ✨ Set status.paused condition upon pausing reconciliation ✨ Set Paused condition on resource's status upon reconciliation being paused Mar 12, 2025
@damdo damdo changed the title ✨ Set Paused condition on resource's status upon reconciliation being paused ✨ Set Paused condition on AWSMachine/AWSCluster status upon pausing reconciliation Mar 12, 2025
@damdo damdo changed the title ✨ Set Paused condition on AWSMachine/AWSCluster status upon pausing reconciliation ✨ Set Paused condition on AWSMachine/AWSCluster status upon reconciliation being paused Mar 12, 2025
@damdo damdo marked this pull request as ready for review March 12, 2025 14:27
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 12, 2025
@nrb
Copy link
Contributor

nrb commented Mar 12, 2025

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

@damdo
Copy link
Member

damdo commented Mar 13, 2025

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

@damdo
Copy link
Member

damdo commented Mar 13, 2025

failed to create new managed VPC: failed to create vpc: The maximum number of VPCs has been reached

How often do we clean up the VPCs? cc. @richardcase

@theobarberbany theobarberbany force-pushed the paused-condition branch 3 times, most recently from e5e4cfa to d86d093 Compare March 14, 2025 12:42
@richardcase
Copy link
Member

failed to create new managed VPC: failed to create vpc: The maximum number of VPCs has been reached

How often do we clean up the VPCs? cc. @richardcase

aws-janitor should run after we finish using the account. This could also be that we are using more VPCs in general in the tests.....but this is e2e.

However, this is the unit tests which shouldn't be created any VPCs and so its probably coming from the resource tracking code we have in the tests.

@damdo
Copy link
Member

damdo commented Mar 17, 2025

Got it, thanks @richardcase , it looks like it is passing now :)

@damdo
Copy link
Member

damdo commented Mar 17, 2025

@theobarberbany are you happy with this now? Is this ready for a final review?

@damdo damdo changed the title ✨ Set Paused condition on AWSMachine/AWSCluster status upon reconciliation being paused ✨ Set Paused condition on reconciled resources status upon reconciliation being paused Mar 17, 2025
@theobarberbany theobarberbany mentioned this pull request Mar 17, 2025
Open
@richardcase
Copy link
Member

richardcase commented Mar 17, 2025 •
edited
Loading

@theobarberbany - thanks for this. Could you squash your commits please? I think we are then good to get this merged.

@theobarberbany
Sets paused condition on CAPA resources
ff89188 
Sets paused condition on AWSMachine

Sets paused on AWSCluster

Sets paused condition on AWSManagedMachinePool

Sets paused condition for ROSAMachinePool

Sets paused condition for ROSAControlPlane

Sets paused condition on AWSManagedControlPlane

Sets paused condition on EKSConfig

Adds paused helper functions

This change adds the paused helper utilities from upstream cluster api.
It modifies them to not require v1beta2conditions.

This is so we can use similar code until the conditions changes are out
of beta.
@theobarberbany
Copy link
Contributor Author

@richardcase Have squashed :)

@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 Mar 17, 2025
@nrb
Copy link
Contributor

nrb commented Mar 17, 2025

/lgtm

Thanks Theo!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 17, 2025
@k8s-ci-robot k8s-ci-robot merged commit c1764a9 into kubernetes-sigs:main Mar 17, 2025
17 checks passed
@theobarberbany theobarberbany deleted the paused-condition branch March 17, 2025 16:03
nrb added a commit to nrb/cluster-api-provider-aws that referenced this pull request Mar 19, 2025
@nrb
Allow controllers to patch clusters/status
ef5cca8 
While kubernetes-sigs#5394 and kubernetes-sigs#5383 added support for patching a cluster/status in the
cluster.x-k8s.io API group, neither added the patch permission for the
associated controllers.

This commit adds RBAC support for patching cluster/status

Signed-off-by: Nolan Brubaker <[email protected]>
@nrb nrb mentioned this pull request Mar 19, 2025
Merged
2 tasks
@richardcase richardcase mentioned this pull request Mar 21, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ankitasw Ankitasw Awaiting requested review from Ankitasw

@cnmcavoy cnmcavoy Awaiting requested review from cnmcavoy

@AndiDog AndiDog Awaiting requested review from AndiDog

@fiunchinho fiunchinho Awaiting requested review from fiunchinho

@richardcase richardcase Awaiting requested review from richardcase

@damdo damdo Awaiting requested review from damdo

@nrb nrb Awaiting requested review from nrb

Assignees

@nrb nrb

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. 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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@theobarberbany @k8s-ci-robot @damdo @nrb @richardcase