Skip to content
This repository was archived by the owner on Jan 9, 2023. It is now read-only.

Comments

Set kubelet eviction-hard based on total memory#150

Merged
jetstack-ci-bot merged 5 commits intojetstack:masterfrom
charlieegan3:hard-eviction-target-issue-149
Apr 10, 2018
Merged

Set kubelet eviction-hard based on total memory#150
jetstack-ci-bot merged 5 commits intojetstack:masterfrom
charlieegan3:hard-eviction-target-issue-149

Conversation

@charlieegan3
Copy link
Contributor

@charlieegan3 charlieegan3 commented Mar 26, 2018 •
edited
Loading

The kubelet will default to memory.available<100Mi as the hard eviction
threshold. This makes that figure 5% of the total memory or 100Mi.
Whichever is greater.

Read from the default memory facts to set the kubelet eviction-hard
parameter.

Default facts:
https://puppet.com/docs/facter/3.10/core_facts.html#memory

Implements #149

What this PR does / why we need it:

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

Release note:

Sets a kubelet hard eviction target to 5% of the node's total memory.

@jetstack-bot jetstack-bot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 26, 2018
@jetstack-bot jetstack-bot requested a review from simonswine March 26, 2018 15:53
@jetstack-ci-bot jetstack-ci-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 26, 2018
@jetstack-ci-bot
Copy link
Contributor

jetstack-ci-bot commented Mar 26, 2018

@charlieegan3 PR needs rebase

@jetstack-ci-bot jetstack-ci-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 26, 2018
simonswine
simonswine previously requested changes Mar 27, 2018
View reviewed changes
Copy link
Contributor

@simonswine simonswine left a comment

Choose a reason for hiding this comment

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

Thanks for this, see my little change requests

puppet/modules/kubernetes/lib/puppet/functions/five_percent_of_total_ram.rb Outdated
end

def five_percent_of_total_ram(total_bytes)
five_percent = (total_bytes * 0.05 / 1_000_000).round
Copy link
Contributor

@simonswine simonswine Mar 27, 2018

Choose a reason for hiding this comment

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

I think with Mi you would need to divide by 1024^2

puppet/modules/kubernetes/templates/kubelet.service.erb Outdated
"--tls-cert-file=<%= @cert_file %>" \
"--tls-private-key-file=<%= @key_file %>" \
<% end -%>
--eviction-hard=memory.available<<%= @hard_eviction_memory_threshold %>
Copy link
Contributor

@simonswine simonswine Mar 27, 2018

Choose a reason for hiding this comment

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

I would feel safer with quoting the whole thing "--eviction[...]%>"

@charlieegan3
Copy link
Contributor Author

charlieegan3 commented Apr 5, 2018

Based on the quick verify failure I need to also have these changes in the module repo: jetstack/puppet-module-kubernetes#22 - so I'm trying that...

@charlieegan3 charlieegan3 reopened this Apr 6, 2018
@charlieegan3
Copy link
Contributor Author

charlieegan3 commented Apr 10, 2018

/test puppet-tarmak-acceptance-ubuntu v1.9

@charlieegan3 charlieegan3 reopened this Apr 10, 2018
@charlieegan3
Set kubelet eviction-hard based on total memory
16eaa98 
The kubelet will default to memory.available<100Mi as the hard eviction
threshold. This makes that figure 5% of the total memory or 100Mi.
Whichever is greater.

Read from the default memory facts to set the kubelet `eviction-hard`
parameter.

Default facts:
https://puppet.com/docs/facter/3.10/core_facts.html#memory

Implements #149
@charlieegan3
Code review changes
f22b260
@charlieegan3
Format command correctly with backslash
9df71dd
@charlieegan3
Quote whole eviction-hard argument
445c8f3
@charlieegan3
Copy link
Contributor Author

charlieegan3 commented Apr 10, 2018

I have a feeling that the ubuntu acceptance tests are also failing because of the changes to the puppet repo config.

I0410 10:55:34.503] Call:  /workspace/./test-infra/jenkins/../scenarios/execute.py --env FIXTURES_YML=.fixtures.yml.local -- make -C puppet/modules/tarmak acceptance-1-9-ubuntu

This looks like maybe the submodule isn't there for some reason. Either way, I'm not sure how it could be caused by the changes in this PR...

@simonswine
Copy link
Contributor

simonswine commented Apr 10, 2018

/test verify quick

@simonswine
Copy link
Contributor

simonswine commented Apr 10, 2018

/test puppet-tarmak-acceptance-ubuntu v1.9

@jetstack-bot jetstack-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Apr 10, 2018
@simonswine simonswine dismissed their stale review April 10, 2018 15:09

Issues have benn addressed

@simonswine
Copy link
Contributor

simonswine commented Apr 10, 2018

/lgtm

@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 10, 2018
@jetstack-bot
Copy link
Collaborator

jetstack-bot commented Apr 10, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: simonswine

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

The pull request process is described here

Details 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

@jetstack-bot jetstack-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 10, 2018
@jetstack-ci-bot
Copy link
Contributor

jetstack-ci-bot commented Apr 10, 2018

Automatic merge from submit-queue.

@jetstack-ci-bot jetstack-ci-bot merged commit 8bbe4bc into jetstack:master Apr 10, 2018
@simonswine simonswine mentioned this pull request Apr 13, 2018
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@simonswine simonswine simonswine left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@simonswine simonswine

@charlieegan3 charlieegan3

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm 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/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@charlieegan3 @jetstack-ci-bot @simonswine @jetstack-bot