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.

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

✨ Add support for DefaultCooldown to AWSMachinePool #2006

Merged
merged 1 commit into from
Oct 14, 2020

Conversation

arghya88
Copy link
Contributor

@arghya88 arghya88 commented Oct 3, 2020

What this PR does / why we need it:

Add support for configuring cooldowns https://docs.aws.amazon.com/autoscaling/ec2/userguide/Cooldown.html to AWSMachinePool

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 #1987

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 3, 2020
@k8s-ci-robot
Copy link
Contributor

Welcome @arghya88!

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

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 3, 2020
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 3, 2020
@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 Oct 3, 2020
@arghya88 arghya88 force-pushed the defaultcooldown-asg branch 2 times, most recently from 09e7e64 to 102936b Compare October 3, 2020 13:04
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 3, 2020
@arghya88 arghya88 force-pushed the defaultcooldown-asg branch from a1014a4 to 570e6ed Compare October 3, 2020 13:34
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 3, 2020
@arghya88 arghya88 force-pushed the defaultcooldown-asg branch 2 times, most recently from 9792eb0 to 661ec69 Compare October 3, 2020 14:01
@arghya88 arghya88 force-pushed the defaultcooldown-asg branch from 661ec69 to 41c5cf8 Compare October 5, 2020 11:45
@arghya88 arghya88 force-pushed the defaultcooldown-asg branch from 41c5cf8 to 07f0615 Compare October 5, 2020 14:04
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 5, 2020
@arghya88
Copy link
Contributor Author

arghya88 commented Oct 5, 2020

/retest

@arghya88 arghya88 force-pushed the defaultcooldown-asg branch from 07f0615 to 90992df Compare October 5, 2020 14:40
@arghya88
Copy link
Contributor Author

arghya88 commented Oct 6, 2020

@randomvariable what do you think of this in it's current state? Let me know if any change is required like moving the defaulting to controller instead of relying on kubebuilder.

@richardcase
Copy link
Member

richardcase commented Oct 8, 2020

Let me know if any change is required like moving the defaulting to controller instead of relying on kubebuilder.

Thanks @arghya88 . I think we need to add a validation check in the webhook to make sure that nothing crazy like a negative duration is specified and so we can fail early.

@arghya88
Copy link
Contributor Author

arghya88 commented Oct 8, 2020

@richardcase as I can see there is no webhook for AWSMachinePool but there is one for AWSManagedMachinePool. So we need to create a new webhook for AWSMachinePool because I added the DefaultCoolDown in AWSMachinePoolSpec not in AWSManagedMachinePoolSpec?

@richardcase
Copy link
Member

Good point @arghya88....i'd missed that, sorry.

I think we should create the webhooks for AWSMachinePool and do this Duration check along with other validation checks for AWSMachinePool.

But perhaps we should create a separate issue for adding the webhooks for AWSMachinePool? What do you think @randomvariable ?

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 9, 2020
@arghya88 arghya88 force-pushed the defaultcooldown-asg branch from 1fa5b34 to 9668869 Compare October 9, 2020 07:55
@arghya88 arghya88 force-pushed the defaultcooldown-asg branch 2 times, most recently from 47d4dfd to 47cc284 Compare October 9, 2020 08:17
@arghya88
Copy link
Contributor Author

arghya88 commented Oct 9, 2020

@richardcase I am getting this error


Boilerplate header is wrong for: /home/prow/go/src/sigs.k8s.io/cluster-api-provider-aws/exp/api/v1alpha3/awsmachinepool_webhook.go
make: *** [Makefile:486: verify-boilerplate] Error 1

@richardcase
Copy link
Member

richardcase commented Oct 9, 2020

I am getting this error

You will need to add the license header to the new webhook file. For example see this

@richardcase
Copy link
Member

Any new files need to have the license comment in them.

@arghya88 arghya88 force-pushed the defaultcooldown-asg branch from 47cc284 to f6cecf2 Compare October 9, 2020 08:26
@arghya88
Copy link
Contributor Author

arghya88 commented Oct 9, 2020

Any new files need to have the license comment in them.

Got it. Thanks for the guidance.

@arghya88
Copy link
Contributor Author

arghya88 commented Oct 9, 2020

/retest

@arghya88 arghya88 force-pushed the defaultcooldown-asg branch from f6cecf2 to ba73d6d Compare October 9, 2020 08:57
@arghya88
Copy link
Contributor Author

arghya88 commented Oct 9, 2020

@randomvariable I have done below changes. PTAL

  1. Made it optional
  2. Setting default value in a webhook now
  3. Validating negative value in the webhook

cc @richardcase

@arghya88
Copy link
Contributor Author

/assign @davidewatson

@richardcase
Copy link
Member

Seems ok to me

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 12, 2020
@randomvariable
Copy link
Member

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: randomvariable

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 Oct 14, 2020
@k8s-ci-robot k8s-ci-robot merged commit 32d06c8 into kubernetes-sigs:master Oct 14, 2020
@arghya88 arghya88 deleted the defaultcooldown-asg branch October 30, 2020 03:03
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. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for DefaultCooldown options to AWSMachinePool
5 participants