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

Fix Heapster and Metrics Server configuration to enable overriding resource requirements. #56965

Merged
merged 2 commits into from
Dec 18, 2017
Merged

Fix Heapster and Metrics Server configuration to enable overriding resource requirements. #56965

merged 2 commits into from
Dec 18, 2017

Conversation

kawych
Copy link
Contributor

@kawych kawych commented Dec 8, 2017 •
edited
Loading

What this PR does / why we need it:
Configure resources for Heapster and Metrics Servier using Component Config. This will enable overriding default resource requirements for these components.

Release note:

Fix Heapster configuration and Metrics Server configuration to enable overriding default resource requirements.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 8, 2017
@kawych
Copy link
Contributor Author

kawych commented Dec 8, 2017

/unassign @DirectXMan12
/assign @mikedanese

@kawych kawych changed the title [WIP] Fix Heapster and Metrics Server configuration to enable overriding resource requirements. Fix Heapster and Metrics Server configuration to enable overriding resource requirements. Dec 11, 2017
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 11, 2017
@kawych
Copy link
Contributor Author

kawych commented Dec 11, 2017

@piosz @mikedanese
This PR is now ready for review. PTAL

@mikedanese
Copy link
Member

/approve no-issue

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 14, 2017
@kawych
Copy link
Contributor Author

kawych commented Dec 15, 2017

/unassign @piosz
/assign @x13n

@k8s-ci-robot k8s-ci-robot assigned x13n and unassigned piosz Dec 15, 2017
x13n
x13n reviewed Dec 15, 2017
View reviewed changes
cluster/addons/cluster-monitoring/googleinfluxdb/heapster-controller-combined.yaml Outdated
- --cpu={{ base_metrics_cpu }}
- --extra-cpu={{ metrics_cpu_per_node }}m
- --memory={{ base_metrics_memory }}
- --extra-memory={{ metrics_memory_per_node }}Mi
- --extra-memory={{metrics_memory_per_node}}Mi
Copy link
Member

Choose a reason for hiding this comment

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

Why? Either keep the spaces in all params or remove from all of them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@x13n
Copy link
Member

x13n commented Dec 15, 2017

One small nit, but otherwise looks good.

@x13n
Copy link
Member

x13n commented Dec 18, 2017

/lgtm
/retest

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kawych, mikedanese, x13n

Associated issue requirement bypassed by: mikedanese

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@kawych
Copy link
Contributor Author

kawych commented Dec 18, 2017

/retest

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 57324, 56931, 57000, 57150, 56965). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 55a6003 into kubernetes:master Dec 18, 2017
@kawych kawych mentioned this pull request Dec 19, 2017
Merged
k8s-github-robot pushed a commit that referenced this pull request Dec 21, 2017
Merge pull request #57390 from kawych/automated-cherry-pick-of-#56965…
6618bdf 
…-upstream-release-1.9

Automatic merge from submit-queue.

Automated cherry pick of #56965: Use pod nanny configured with ComponentConfig in Heapster

Cherry pick of #56965 on release-1.9.

#56965: Use pod nanny configured with ComponentConfig in Heapster
x13n
x13n reviewed Jan 5, 2018
View reviewed changes
cluster/addons/cluster-monitoring/google/heapster-controller.yaml
command:
- /pod_nanny
- --config-dir=/etc/config
- --cpu=100m
Copy link
Member

Choose a reason for hiding this comment

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

@kawych Actually, this flag no longer exists in the new version, so this won't work. Please create a new PR to remove unused flags.

Copy link
Contributor Author

@kawych kawych Jan 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@MaciekPytel

It will work, I've tested it :) But it looks like I've submitted the code into incorrect branch: https://github.com/kubernetes/autoscaler/blob/addon-resizer-release-1/addon-resizer/nanny/main/pod_nanny.go#L45
I think we just need to cherrypick this to 1.8 branch. I can do a PR later today/early on monday.

Copy link
Member

Choose a reason for hiding this comment

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

Ack. We'll need image version bump here later anyway.

x13n
x13n reviewed Jan 5, 2018
View reviewed changes
cluster/addons/cluster-monitoring/google/heapster-controller.yaml
@@ -73,6 +99,9 @@ spec:
requests:
cpu: 50m
memory: {{ nanny_memory }}
volumeMounts:
- name: heapster-config-volume
mountMath: /etc/config
Copy link
Member

Choose a reason for hiding this comment

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

s/Math/Path

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops! Will submit a fix shortly.

x13n
x13n reviewed Jan 5, 2018
View reviewed changes
cluster/addons/cluster-monitoring/google/heapster-controller.yaml
@@ -73,6 +99,9 @@ spec:
requests:
cpu: 50m
memory: {{ nanny_memory }}
volumeMounts:
- name: heapster-config-volume
Copy link
Member

Choose a reason for hiding this comment

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

No such thing as heapster-config-volume

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Defined below, but now I've noticed duplicated "volumes:" section - maybe that's why? Will submit a fix

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

@x13n x13n x13n left review comments

Assignees

@x13n x13n

@mikedanese mikedanese

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. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@kawych @mikedanese @x13n @k8s-ci-robot @k8s-github-robot @DirectXMan12 @piosz