-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Cluster-autoscaler: cloud provider interface implementation for AWS #1377
Cluster-autoscaler: cloud provider interface implementation for AWS #1377
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
I signed it! |
CLAs look good, thanks! |
} | ||
|
||
// MaxSize returns maximum size of the node group. | ||
func (asg *Asg) MaxSize() int { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems we could just pull MaxSize and MinSize from the AutoScalingGroup when we initialize. It's kind of annoying that it must be specified twice, both here and in the ASG itself. I'm not sure if there would be a use case for that?
@mwielgus I think this was lifted pretty much verbatim from the GCE side. I assume (from my short search through the MIG docs) that MIGs don't allow you to specify a min/max?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iterion No, migs allow you only to specify the target number of instances.
We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm. |
I signed it! |
We haven't tested the most recent iteration, but there were only minor changes. We'll try to test it all again soon. |
Same thing on our side, hopefully this week we'll get a chance to test it. We're also looking into writing some e2e tests for the AWS implementation. |
I just tested it out and it works good. Turned up cluster using kops and was able to scale the worker ASG up/down as expected. |
@osxi could you please sign the CLA? |
// assert.NoError(t, err) | ||
// } | ||
|
||
// TODO: IncreaseSize |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please implement.
@mwielgus I signed the CLA but then I added a commit from @andrewsykim, who also signed the CLA. I'll implement the commented tests. |
Finally had some time today to test this out! Sorry it took so long, we had to upgrade to |
Node object should be automatically deleted by node controller once it is gone from the cloud provider: https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/node/nodecontroller.go#L469 |
Re-tested after adding the AWS SDK autoscaler interface around the I haven't experienced any dangling nodes. After chatting with @andrewsykim on Slack, it looks like that might be related to the use of EC2 Classic (where my testing took place on a VPC setup). |
|
@mwielgus I will fix that |
@mwielgus can you please manually approve the CLA? |
There is no approval for CLA. There must be something missing on your side. Please check if your git author settings are in line with your github profile. |
CLAs look good, thanks! |
The code looks ok. Please squash the commits to 1-2 commits (code / extra godeps) and we should be ready to merge. |
@mwielgus done |
LGTM |
Thank you for your contribution! :) |
You're very welcome! |
Automatic merge from submit-queue |
Awesome. Nice work @osxi and @andrewsykim ! |
Please remember to update Kubernetes kube_up.sh scripts for AWS as well :). |
Automatic merge from submit-queue Bump cluster autoscaler to 0.3.0-beta2 Contains belongs fix kubernetes-retired/contrib#1560 and AWS support kubernetes-retired/contrib#1377.
…/topic/aws-provider Automatic merge from submit-queue Cluster-autoscaler: cloud provider interface implementation for AWS Part of kubernetes-retired/contrib#1311 <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.kubernetes.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.kubernetes.io/reviews/kubernetes/contrib/1377) <!-- Reviewable:end -->
…/topic/aws-provider Automatic merge from submit-queue Cluster-autoscaler: cloud provider interface implementation for AWS Part of kubernetes-retired/contrib#1311 <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.kubernetes.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.kubernetes.io/reviews/kubernetes/contrib/1377) <!-- Reviewable:end -->
Part of #1311
This change is![Reviewable](https://camo.githubusercontent.com/2d899f4291d07d3cd2fa4aaae1e3b243f164c23fce87d30a589ace0d496a444c/68747470733a2f2f72657669657761626c652e6b756265726e657465732e696f2f7265766965775f627574746f6e2e737667)