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

Heapster sizing adjustments #22940

Merged
merged 1 commit into from
Mar 14, 2016
Merged

Heapster sizing adjustments #22940

merged 1 commit into from
Mar 14, 2016
+39 −33

Conversation

mwielgus
Copy link
Contributor

Do not merge yet.

@mwielgus mwielgus added sig/autoscaling Categorizes an issue or PR as relevant to SIG Autoscaling. do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. labels Mar 14, 2016
@mwielgus mwielgus added this to the v1.2 milestone Mar 14, 2016
@k8s-github-robot k8s-github-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 14, 2016
@k8s-github-robot
Copy link

Labelling this PR as size/M

@piosz piosz assigned gmarek and unassigned piosz Mar 14, 2016
@k8s-bot
Copy link

k8s-bot commented Mar 14, 2016

GCE e2e build/test passed for commit 07d5e67a5982073c36ddf5514daff6f15f3939bd.

gmarek
gmarek reviewed Mar 14, 2016
View reviewed changes
cluster/addons/cluster-monitoring/google/heapster-controller.yaml
{% set num_nodes = pillar.get('num_nodes', -1) -%}
{% if num_nodes >= 0 -%}
{% set heapster_memory = (200 + num_nodes * 3)|string + "Mi" -%}
{% set metrics_memory = (200 + num_nodes * 3)|string + "Mi" -%}
{% set eventer_memory = (200000 + num_nodes * 200)|string + "Ki" -%}
Copy link
Contributor

Choose a reason for hiding this comment

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

s/200000/200 * 1024/

@mwielgus mwielgus force-pushed the heapster-adjustments branch from 07d5e67 to 798f0bd Compare March 14, 2016 16:29
@k8s-bot
Copy link

k8s-bot commented Mar 14, 2016

GCE e2e build/test passed for commit 798f0bd7aeac09acfd0f1847a47638d95a63e7a1.

@mwielgus mwielgus mentioned this pull request Mar 14, 2016
Closed
@davidopp
Copy link
Member

please ping this thread when you think it is ready to merge

vishh
vishh reviewed Mar 14, 2016
View reviewed changes
cluster/addons/cluster-monitoring/googleinfluxdb/heapster-controller-combined.yaml
{% set num_nodes = pillar.get('num_nodes', -1) -%}
{% if num_nodes >= 0 -%}
{% set heapster_memory = (200 + num_nodes * 3)|string + "Mi" -%}
{% set metrics_memory = (200 + num_nodes * 4)|string + "Mi" -%}
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we increasing the memory requirement for heapster pod as a whole?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Just in case.
For 1000 node cluster we are able to operate without any sink at 2-2.3gb (with 30k pause and 2k system pods). GKE http output creation consumes +400MB. That brings us to 2.75. If couple events occur at once: scraping, gke scraping and pod relist then the temporary memory consumption can be bigger. So to be on the VERY safe side I decided to go with 4 multpilier.

As we did a lot of noise (=events) during the tests it became apparent that Eventer should also have more memory. List that happens inside of watch consumes LOTS of memory (we will try to get rid of it just after 1.2).

@mwielgus mwielgus force-pushed the heapster-adjustments branch from 798f0bd to f5f6a80 Compare March 14, 2016 18:45
@mwielgus mwielgus removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Mar 14, 2016
@mwielgus
Copy link
Contributor Author

Also bumping the image to beta2.

@mwielgus mwielgus force-pushed the heapster-adjustments branch from f5f6a80 to 6123df9 Compare March 14, 2016 19:00
@k8s-bot
Copy link

k8s-bot commented Mar 14, 2016

GCE e2e build/test passed for commit f5f6a80e89544ffe5fb77fa542ac699dd84b9514.

@k8s-bot
Copy link

k8s-bot commented Mar 14, 2016

GCE e2e build/test passed for commit 6123df9.

@davidopp davidopp added lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. cherrypick-candidate labels Mar 14, 2016
@davidopp
Copy link
Member

LGTM

@k8s-github-robot
Copy link

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

piosz
piosz reviewed Mar 14, 2016
View reviewed changes
cluster/addons/cluster-monitoring/googleinfluxdb/heapster-controller-combined.yaml
{% set num_nodes = pillar.get('num_nodes', -1) -%}
{% if num_nodes >= 0 -%}
{% set heapster_memory = (200 + num_nodes * 3)|string + "Mi" -%}
{% set metrics_memory = (200 + num_nodes * 4)|string + "Mi" -%}
{% set eventer_memory = (200000 + num_nodes * 500)|string + "Ki" -%}
Copy link
Member

Choose a reason for hiding this comment

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

/s/200000/200*1024

@k8s-bot
Copy link

k8s-bot commented Mar 14, 2016

GCE e2e build/test passed for commit 6123df9.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

k8s-github-robot pushed a commit that referenced this pull request Mar 14, 2016
Merge pull request #22940 from mwielgus/heapster-adjustments
f899a40 
Auto commit by PR queue bot
@k8s-github-robot k8s-github-robot merged commit f899a40 into kubernetes:master Mar 14, 2016
@bgrant0607
Copy link
Member

PR description still says "do not merge yet".

@bgrant0607
Copy link
Member

Not good for release-note generation

@bgrant0607 bgrant0607 added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Mar 15, 2016
@bgrant0607
Copy link
Member

And please ensure all PRs have a description or at least a reference to an issue.

@andyzheng0831 andyzheng0831 mentioned this pull request Mar 15, 2016
Merged
eparis pushed a commit to eparis/kubernetes that referenced this pull request Mar 15, 2016
@eparis
Merge pull request kubernetes#22940 from mwielgus/heapster-adjustments
be2ad9e 
Auto commit by PR queue bot
(cherry picked from commit f899a40)
@eparis eparis mentioned this pull request Mar 15, 2016
Merged
@k8s-cherrypick-bot
Copy link

Commit be2ad9e found in the "release-1.2" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this s an error find help to get your PR picked.

@mwielgus
Copy link
Contributor Author

@bgrant0607 Please merge it to 1.2.

@eparis eparis mentioned this pull request Mar 15, 2016
Merged
@piosz
Copy link
Member

piosz commented Mar 16, 2016

@mwielgus this is already merged to 1.2 branch.

shyamjvs pushed a commit to shyamjvs/kubernetes that referenced this pull request Dec 1, 2016
@eparis
Merge pull request kubernetes#22940 from mwielgus/heapster-adjustments
799fd34 
Auto commit by PR queue bot
(cherry picked from commit f899a40)
shouhong pushed a commit to shouhong/kubernetes that referenced this pull request Feb 14, 2017
@eparis
Merge pull request kubernetes#22940 from mwielgus/heapster-adjustments
e679323 
Auto commit by PR queue bot
(cherry picked from commit f899a40)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@gmarek gmarek

Labels
cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. sig/autoscaling Categorizes an issue or PR as relevant to SIG Autoscaling. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.2
Development

Successfully merging this pull request may close these issues.