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

populate cloud labels with cluster autoscaler tags #3017

Merged
merged 1 commit into from
Jul 21, 2017

Conversation

sethpollack
Copy link
Contributor

No description provided.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 21, 2017
@justinsb
Copy link
Member

Happy to see this - thank you for writing it..

First though, I'm not sure this is the right place for the code. This will add those cloud labels explicitly to the instancegroup (which is OKish), but I think it will only do so on initial creation - if you change the labels later, it won't add them.

Also, we merge labels from the cluster. I think it's probably simpler to just add these labels silently as part of AWS ASG creation. Check out this line:

tags, err := b.CloudTagsForInstanceGroup(ig)
I suspect that is the right location for the logic.

I'm not sure what we do if the label or taint can't fit / isn't valid for the AWS tag. Hopefully people choose reasonable labels & taints, but we don't want to break people's clusters that chose difficult names. I haven't checked on the length limits / which characters are valid though....

Not sure if these labels are expected to propagate to the instances or not. Currently all our tags do propagate (https://github.com/kubernetes/kops/blob/master/upup/pkg/fi/cloudup/awstasks/autoscalinggroup.go#L170). I suspect it is harmless, so not going to block this PR for that.

@justinsb justinsb self-requested a review July 21, 2017 14:33
@justinsb justinsb self-assigned this Jul 21, 2017
@sethpollack
Copy link
Contributor Author

Ok, updated

Also

The maximum key length is 127 Unicode characters.
The maximum value length is 255 Unicode characters.

@justinsb
Copy link
Member

/lgtm

Thanks @sethpollack

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 21, 2017
@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 5e70036 into kubernetes:master Jul 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants