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

Set MaxPods when using Amazon VPC CNI Plugin #6058

Merged
merged 7 commits into from
Nov 26, 2018

Conversation

ripta
Copy link
Contributor

@ripta ripta commented Nov 8, 2018

This PR closes #5510 and replaces it by fixing the merge conflict and implementing the automatic generation for MaxPods, as I've found the original author's patch to be very valuable and would love to see it merged.

  • I imported the awsutils package from the VPC CNI plugin with the thought of avoiding maintaining values separate from the plugin. It adds a dependency, so let me know if it's not the best way.
  • I added the seen map to machine_types.go, because the Pricing API was doing weird things, where it returned the same instance family multiple times (in some cases, up to 3 times).

Also, @justinsb added a comment in the original PR about possibly lowering the MaxPods. I didn't make any changes here, but let me know if I should try to incorporate that. One one hand, if there are performance penalties with large numbers of pods, it sounds like the defaults should protect cluster operators from shooting themselves in the foot; OTOH, there may(?) exist legitimate use cases that people may have many small pods and want to pack more pods. Should it be a flag to allow folks to opt into running more pods than recommended (110 per node?)

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 8, 2018
@k8s-ci-robot
Copy link
Contributor

Hi @ripta. Thanks for your PR.

I'm waiting for a kubernetes 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 cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 8, 2018
@chrisz100
Copy link
Contributor

/ok-to-test

Thanks @ripta !

@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 Nov 8, 2018
@ripta
Copy link
Contributor Author

ripta commented Nov 8, 2018

/assign @andrewsykim

Ready to review.

@amitsaha
Copy link

amitsaha commented Nov 8, 2018

Thanks for this @ripta - i was updating the cluster template as part of my workflow (via scripting). This is great default behavior.

@chrisz100
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 8, 2018
@andrewsykim
Copy link
Member

Not familiar enough in this area to comment :)

/unassign
/assign @justinsb

@k8s-ci-robot k8s-ci-robot assigned justinsb and unassigned andrewsykim Nov 9, 2018
hack/machine_types/machine_types.go Outdated Show resolved Hide resolved
nodeup/pkg/model/kubelet.go Outdated Show resolved Hide resolved
hack/machine_types/machine_types.go Outdated Show resolved Hide resolved
nodeup/pkg/model/kubelet.go Show resolved Hide resolved
nodeup/pkg/model/kubelet.go Show resolved Hide resolved
@justinsb
Copy link
Member

Looks great, but I do think we should avoid going >= 110, unless the user explicitly specifies kubelet.MaxPods >= 110.

And in general I think we should do min(kubelet.MaxPods, instanceType.MaxPods)

I also think it would be nice to avoid vendoring all of aws-vpc-cni, so that we can get closer to having this instance type DB move out of kops into a file in some shared repo. Then hopefully aws-vpc-cni and kops and the other projects can all refer to that file :-)

@justinsb justinsb added this to the 1.12 milestone Nov 19, 2018
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 20, 2018
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 25, 2018
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 25, 2018
@ripta
Copy link
Contributor Author

ripta commented Nov 25, 2018

@justinsb - I addressed the comments and resolved the merge conflict. PTAL.

/retest

@justinsb
Copy link
Member

Thanks - let's get this into 1.11 then :-)

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 26, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrisz100, justinsb, ripta

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 Nov 26, 2018
@k8s-ci-robot k8s-ci-robot merged commit 0c8e557 into kubernetes:master Nov 26, 2018
@ripta ripta deleted the max-pods branch November 27, 2018 03:45
justinsb added a commit to justinsb/kops that referenced this pull request Nov 28, 2018
PR kubernetes#6058 addded dedup in a better way than I had previously done, so
remove my more complicated and now superfluous second dedup.
liranp pushed a commit to spotinst/kubernetes-kops that referenced this pull request Mar 2, 2019
PR kubernetes#6058 addded dedup in a better way than I had previously done, so
remove my more complicated and now superfluous second dedup.
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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants