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

Also set new (non-beta/non-deprecated) labels in buildGenericLabels #4053

Merged
merged 2 commits into from
Aug 2, 2021
Merged

Also set new (non-beta/non-deprecated) labels in buildGenericLabels #4053

merged 2 commits into from
Aug 2, 2021

Conversation

codablock
Copy link
Contributor

This fixes issues when an ASG has 0 nodes yet and node labels are built
using buildGenericLabels and the node-template labels.

Issues include (anti-)affinity and nodeSelectors for the given labels,
giving false-negative results for candidate nodes, which leads to ASGs
never scaling up.

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

Welcome @codablock!

It looks like this is your first PR to kubernetes/autoscaler 🎉. 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/autoscaler 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 k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label May 3, 2021
@codablock
Copy link
Contributor Author

I see that 1.21 is underway. Is there a chance to still get this into 1.21? It might also make sense to backport this to older versions, as it looks like all versions are affected by this.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 3, 2021
@codablock
Copy link
Contributor Author

After looking into other cloud providers, I realised that all cloud providers have this same issue. I decided to reimplement the solution in a generic way (based on updateDeprecatedTemplateLabels) and force-pushed this.

@codablock codablock mentioned this pull request May 3, 2021
Closed
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 6, 2021
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 6, 2021
@codablock codablock mentioned this pull request May 6, 2021
Merged
@gjtempleton gjtempleton mentioned this pull request May 23, 2021
Merged
@gjtempleton
Copy link
Member

Thanks for the PR and putting in the work to make it a generic solution.

There's a couple of AWS related conflicts after the merging of #3848, however other than it looks good to me from the AWS side.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 23, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 23, 2021
@codablock
Copy link
Contributor Author

@gjtempleton fyi, I've resolved the merge conflicts in the meantime.

@codablock
Copy link
Contributor Author

@gjtempleton Hello. Just wanted to check if there is any update on planning to merge this one? My understand is that we're ready for merge.

@gjtempleton
Copy link
Member

gjtempleton commented Jul 8, 2021 •
edited
Loading

Sorry for the delay!

Thanks for the nudge. All looks good on the AWS side.

You'll still need approval from one of the higher-level approvers of the wider CA codebase as well as mentioned by the CI-robot, I'm only an approver on the AWS cloud provider code.
/assign aleksandra-malinowska
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 8, 2021
elmiko
elmiko reviewed Jul 29, 2021
View reviewed changes
Copy link
Contributor

@elmiko elmiko left a comment

Choose a reason for hiding this comment

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

overall this looks nice to me, just a minor spacing issue on the tests, otherwise i'm /lgtm

cluster-autoscaler/cloudprovider/gce/templates_test.go Outdated Show resolved Hide resolved
codablock added 2 commits July 29, 2021 16:45
@codablock
Introduce UpdateDeprecatedTemplateLabels to set beta/deprecated labels
8f11490 
And at the same time only set stable labels in all buildGenericLabels
implementations.

This fixes issues when a node group has 0 nodes yet and node labels are
built using buildGenericLabels and the node-template labels.

Issues include (anti-)affinity and nodeSelectors for the given labels,
giving false-negative results for candidate nodes, which leads to ASGs
never scaling up.
@codablock
Remove obsolete comment
6d84abf 
arch is not hardcoded anymore
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 29, 2021
@elmiko
Copy link
Contributor

elmiko commented Jul 29, 2021

thanks @codablock !
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 29, 2021
feiskyer
feiskyer approved these changes Aug 2, 2021
View reviewed changes
Copy link
Member

@feiskyer feiskyer 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
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: codablock, feiskyer

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 Aug 2, 2021
@k8s-ci-robot k8s-ci-robot merged commit 21fc0c1 into kubernetes:master Aug 2, 2021
@codablock codablock deleted the old-labels branch August 2, 2021 07:12
@jutley jutley mentioned this pull request Feb 24, 2022
Closed
evansheng pushed a commit to airbnb/autoscaler that referenced this pull request Mar 15, 2022
@k8s-ci-robot
Merge pull request kubernetes#4053 from codablock/old-labels
28855f9 
Also set new (non-beta/non-deprecated) labels in buildGenericLabels
evansheng pushed a commit to airbnb/autoscaler that referenced this pull request Mar 26, 2022
@k8s-ci-robot
Merge pull request kubernetes#4053 from codablock/old-labels
e27b079 
Also set new (non-beta/non-deprecated) labels in buildGenericLabels
@evansheng evansheng mentioned this pull request Mar 26, 2022
Merged
jiancheung pushed a commit to airbnb/autoscaler that referenced this pull request Jul 29, 2022
@k8s-ci-robot
Merge pull request kubernetes#4053 from codablock/old-labels
23b70ca 
Also set new (non-beta/non-deprecated) labels in buildGenericLabels
jiancheung pushed a commit to airbnb/autoscaler that referenced this pull request Jul 29, 2022
@k8s-ci-robot
Merge pull request kubernetes#4053 from codablock/old-labels
1ae1410 
Also set new (non-beta/non-deprecated) labels in buildGenericLabels
jiancheung pushed a commit to airbnb/autoscaler that referenced this pull request Jul 29, 2022
@k8s-ci-robot
Merge pull request kubernetes#4053 from codablock/old-labels
545e9d9 
Also set new (non-beta/non-deprecated) labels in buildGenericLabels
akirillov pushed a commit to airbnb/autoscaler that referenced this pull request Oct 27, 2022
@k8s-ci-robot
Merge pull request kubernetes#4053 from codablock/old-labels
d39ccae 
Also set new (non-beta/non-deprecated) labels in buildGenericLabels
@akirillov akirillov mentioned this pull request Oct 27, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elmiko elmiko elmiko left review comments

@feiskyer feiskyer feiskyer approved these changes

@aleksandra-malinowska aleksandra-malinowska Awaiting requested review from aleksandra-malinowska

@Jeffwan Jeffwan Awaiting requested review from Jeffwan

Assignees

@elmiko elmiko

@aleksandra-malinowska aleksandra-malinowska

@gjtempleton gjtempleton

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/cluster-autoscaler 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. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@codablock @k8s-ci-robot @gjtempleton @elmiko @feiskyer @jbartosik @aleksandra-malinowska