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

Backporting AWS ignored labels and capacity memory changes to 1.16 #2851

Merged
merged 3 commits into from
Feb 28, 2020
Merged

Backporting AWS ignored labels and capacity memory changes to 1.16 #2851

merged 3 commits into from
Feb 28, 2020

Conversation

maximerenou50
Copy link
Contributor

This PR is about backporting changes from master to 1.16. This is required when using EKS 1.14 with node groups because:

  • EKS 1.14 (which is currently the latest version) does not work with CA 1.17
  • Sometimes the memory capacity between instances within worker groups can differ quite a lot
  • EKS add a label that confused CA

I was able to test this against a 3 AZs group set which now spread the instances across all AZs.

Finally, I would like to get a new release of 1.16, can you guide me through the process of doing that? (or is it just something that happens by itself periodically?)

cdmurph32 and others added 3 commits February 20, 2020 15:26
@cdmurph32
Raise maximum memory capacity difference.
fa1f032 
AWS M5 instance types may differ in memory capacity by more than 128MB.
@cdmurph32
Add additional AWS labels.
2b615e1 
Whitelist additional node labels for AWS CNI custom networking and
EC2 lifecycle.

Move AWS ignored node labels to AWS specific file.
Add eks.amazonaws.com/nodegroup label to awsIgnoredLabels
b74297d
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 20, 2020
@maximerenou50
Copy link
Contributor Author

Hello,
Any update on this? I'm only backporting changes, not introducing anything new, so hopefully, it should be fairly straightforward. Please let me know if you need more information.
Thanks in advance!

@piteur
Copy link

piteur commented Feb 24, 2020

Heads up on that MR, I will benefit from it as we are facing the same issue!

Please consider :)

@MaciekPytel
Copy link
Contributor

/approve
cc: @Jeffwan - this is AWS specific, so I don't want to lgtm without pinging you first. If you're ok with this let's merge it.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MaciekPytel

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 Feb 25, 2020
@Jeffwan
Copy link
Contributor

Jeffwan commented Feb 26, 2020

@maximerenou50 Can you give original PR info?

Sometimes the memory capacity between instances within worker groups can differ quite a lot

Could you give nodeInfo, I'm fine to increase limit but we really want to resolve it internally. I do see some instance has memory difference. need to get more data points.

@maximerenou50
Copy link
Contributor Author

maximerenou50 commented Feb 26, 2020 •
edited
Loading

Hi @Jeffwan ,

This is the original PR: #2462
For my scenario, I noticed on AWS m5.xlarge instances a difference of 176MB. I do not know the reason for that, at first, I thought it was between AZ but actually it also happened within the same AZ.

Please let me know if you need more information.
Thanks

@Jeffwan
Copy link
Contributor

Jeffwan commented Feb 28, 2020

/lgtm

Yeah. I would suggest you to include original PR info in the cherry-pick. Like cherry-pick #no. - PR title. It would be helpful for us to manage back port changes for every release.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 28, 2020
@Jeffwan
Copy link
Contributor

Jeffwan commented Feb 28, 2020

@MaciekPytel Changes looks good to me, could you help approve this PR?

@k8s-ci-robot k8s-ci-robot merged commit f1b1898 into kubernetes:cluster-autoscaler-release-1.16 Feb 28, 2020
@maximerenou50 maximerenou50 deleted the feature/aws_labels_memory branch March 2, 2020 07:51
@maximerenou50 maximerenou50 mentioned this pull request Mar 5, 2020
Closed
@2ZZ 2ZZ mentioned this pull request Mar 27, 2020
Closed
@Jeffwan
Copy link
Contributor

Jeffwan commented Mar 27, 2020

original PR: https://github.com/kubernetes/autoscaler/pull/2462/files

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

@losipiuk losipiuk Awaiting requested review from losipiuk

Assignees

@Jeffwan Jeffwan

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. 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.
6 participants
@maximerenou50 @piteur @MaciekPytel @k8s-ci-robot @Jeffwan @cdmurph32