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

Added pods-per-core to kubelet. #25762 #25813

Merged
merged 1 commit into from
May 28, 2016
Merged

Added pods-per-core to kubelet. #25762 #25813

merged 1 commit into from
May 28, 2016

Conversation

rrati
Copy link

@rrati rrati commented May 18, 2016 •
edited by vishh
Loading

Added --pods-per-core to kubelet
#25762

@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/old-docs size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 18, 2016
@rrati rrati force-pushed the kubelet-pods-per-core branch from d9fac00 to 5c9ed79 Compare May 18, 2016 15:59
@timothysc timothysc assigned wojtek-t and unassigned brendandburns May 18, 2016
@timothysc
Copy link
Member

/cc @kubernetes/sig-node

@timothysc
Copy link
Member

@rrati needs a test.

wojtek-t
wojtek-t reviewed May 19, 2016
View reviewed changes
cmd/kubelet/app/options/options.go
@@ -258,4 +259,5 @@ func (s *KubeletServer) AddFlags(fs *pflag.FlagSet) {
fs.StringVar(&s.EvictionSoftGracePeriod, "eviction-soft-grace-period", s.EvictionSoftGracePeriod, "A set of eviction grace periods (e.g. memory.available=1m30s) that correspond to how long a soft eviction threshold must hold before triggering a pod eviction.")
fs.DurationVar(&s.EvictionPressureTransitionPeriod.Duration, "eviction-pressure-transition-period", s.EvictionPressureTransitionPeriod.Duration, "Duration for which the kubelet has to wait before transitioning out of an eviction pressure condition.")
fs.Int32Var(&s.EvictionMaxPodGracePeriod, "eviction-max-pod-grace-period", s.EvictionMaxPodGracePeriod, "Maximum allowed grace period (in seconds) to use when terminating pods in response to a soft eviction threshold being met. If negative, defer to pod specified value.")
fs.Int32Var(&s.PodsPerCore, "pods-per-core", s.PodsPerCore, "Number of Pods per core that can run on this Kubelet. A value of 0 disables this limit. Cannot exceed max-pods")
Copy link
Member

Choose a reason for hiding this comment

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

What happens if it will exceed max-pod?

Also, this cannot exeed max-pod or this * number of cores?

Copy link
Author

Choose a reason for hiding this comment

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

max-pods is the total # of pods allowed, so if pods per core * cores > max-pods, then the kubelet will report max-pods. Should I make the statement more clear?

Copy link
Member

Choose a reason for hiding this comment

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

yes please

@wojtek-t
Copy link
Member

lgtm - just one minor comment

@rrati
Copy link
Author

rrati commented May 19, 2016

tests with 2 core system:

kubelet --pods-per-core=20
kubectl describe node
Capacity:
pods: 40
cpu: 2

kubelet --pods-per-core=100
kubectl describe node
Capacity:
pods: 110
cpu: 2

kubelet --pods-per-core=10 --max-pods=10
kubectl describe node
Capacity:
pods: 10
cpu: 2

@rrati rrati force-pushed the kubelet-pods-per-core branch from 5c9ed79 to 5f584a9 Compare May 19, 2016 13:38
@rrati
Copy link
Author

rrati commented May 19, 2016

Logged issue #25875 to address automated testing

@wojtek-t wojtek-t added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 19, 2016
@wojtek-t
Copy link
Member

lgtm

@wojtek-t wojtek-t added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels May 19, 2016
@rrati rrati force-pushed the kubelet-pods-per-core branch from 5f584a9 to da62b8f Compare May 19, 2016 14:41
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 19, 2016
@timothysc timothysc added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 19, 2016
@timothysc timothysc mentioned this pull request May 19, 2016
Closed
@timothysc timothysc added this to the v1.3 milestone May 19, 2016
@timothysc timothysc added the priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done. label May 19, 2016
@rrati
Copy link
Author

rrati commented May 19, 2016

@k8s-bot Please unit test this issue #IGNORE

@rrati
Copy link
Author

rrati commented May 19, 2016

@k8s-bot unit test this issue #IGNORE

@rrati rrati force-pushed the kubelet-pods-per-core branch from da62b8f to e0bf300 Compare May 19, 2016 16:42
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 19, 2016
@wojtek-t
Copy link
Member

@yujuhong no, it will be 0 by default; it was just requested by RedHat. I would like to take advantage of it in the next release hopefully.

@wojtek-t wojtek-t added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 20, 2016
@rrati
Copy link
Author

rrati commented May 20, 2016

@k8s-bot test this issue #IGNORE

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 21, 2016
@rrati rrati force-pushed the kubelet-pods-per-core branch from e3c7978 to 9b6e26c Compare May 23, 2016 14:06
@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 23, 2016
@timothysc timothysc added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 23, 2016
@jeremyeder
Copy link

For docs:

"pods-per-core" is a simple scaling factor that allows us to get past the previous limit of 110 on larger systems.

OpenShift could lay down a config like so (via installer):

--pods-per-core=10
--max-pods=255

Which, when compared with openshift 3.2 max-pods=110, the effective behavior of the new pods-per-core algorithm:

For 1-10 cores, max-pods is reduced
For 11+ cores, max-pods is increased

Examples:

8 cores:
8 cores * pods-per-core(10) = 80
80 < max-pods, so the limit is 80 pods.

16 cores:
16 cores * pods-per-core(10) = 160
160 < max-pods, so the limit is 160 pods.

32 cores * pods-per-core(10) = 320
320 > max-pods, so max-pods wins and the limit is 255

@alex-mohr alex-mohr added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels May 25, 2016
@alex-mohr
Copy link
Contributor

Buildcop: Seems worth a release-note, so swapped labels. LMK if that's wrong.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 26, 2016
@rrati rrati force-pushed the kubelet-pods-per-core branch from 9b6e26c to 8164a31 Compare May 26, 2016 19:11
@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 26, 2016
@rrati rrati force-pushed the kubelet-pods-per-core branch from 8164a31 to 2d487f7 Compare May 27, 2016 11:10
@wojtek-t wojtek-t added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 27, 2016
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented May 28, 2016

GCE e2e build/test passed for commit 2d487f7.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 74b20cc into kubernetes:master May 28, 2016
rrati pushed a commit to rrati/kubernetes that referenced this pull request Jun 27, 2016
This is a continuation of kubernetes#25813. Changed default pods-per-…
5449bac 
…core to 10

and max-pods to 250
@rrati rrati mentioned this pull request Jun 27, 2016
Closed
@jeremyeder
Copy link

Should the code throw an error if running_pods > max_pods ?

@jeremyeder jeremyeder mentioned this pull request Jul 5, 2016
Closed
@yujuhong
Copy link
Contributor

yujuhong commented Jul 6, 2016

Should the code throw an error if running_pods > max_pods ?

That's a valid question. I think after a restart, kubelet will admit each pod again, and some of the pods will get rejected due the MaxPods restriction. The containers belonging to those pods will be stopped.

@warmchang
Copy link
Contributor

mark.

@rrati rrati mentioned this pull request Jul 10, 2020
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@wojtek-t wojtek-t

Labels
kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.3
Development

Successfully merging this pull request may close these issues.