Consolidate sysctl commands for kubelet #43005
Conversation
|
Hi @cmluciano. 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 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. I understand the commands that are listed here. |
k8s-ci-robot
added
the
cncf-cla: yes
|
This change is |
k8s-github-robot
added
size/M
pkg/util/sysctl/sysctl.go
Outdated
|
|
||
| VmOvercommitMemoryAlways = 1 // kernel performs no memory over-commit handling | ||
| VmPanicOnOOMInvokeOOMKiller = 0 // kernel calls the oom_killer function when OOM occurs | ||
|
|
||
| KernelPanicOnOopsAlways = 1 // kernel panics on kernel oops | ||
| KernelPanicRebootTimeout = 10 // seconds after a panic for the kernel to reboot | ||
|
|
||
| RootMaxKeysSetting = 1000000 // Needed since docker creates a new key per container | ||
| RootMaxBytesSetting = 25000000 // allocate 25 bytes per key * number of MaxKeys |
There was a problem hiding this comment.
I like the comment, but if RootMaxBytesSetting depends on RootMaxBytesSetting, actually reference it as 25 * RootMaxBytesSetting.
cmluciano
force-pushed
the
cml/consolidatesysctl
branch
from
0ab5941
to
7b1cf8d
Compare
|
@dchen1107 @vishh does this look reasonable to you? |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
k8s-github-robot
added
the
do-not-merge
cmluciano
force-pushed
the
cml/consolidatesysctl
branch
from
7b1cf8d
to
13e4cab
Compare
k8s-github-robot
removed
the
lgtm
|
Fixed the go-fmt spacing issue. Can I get a retest? |
|
@k8s-bot ok to test |
cmluciano
force-pushed
the
cml/consolidatesysctl
branch
2 times, most recently
from
8c361f1
to
c5b2181
Compare
|
ok builds stabilized now after fixing all go fmt problems |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/release-note-none |
k8s-ci-robot
added
release-note-none
|
@dchen1107 Would you like me to preserve your comment? |
|
The new test failure seems to be a random flake. I have not pushed any new changes. |
|
The build logs aren't pointing me to any specific test failures, just that it can't seem to save the junit results |
|
@vishh Can I get a retest? |
|
@k8s-bot test this |
These commands are important enough to be in the Kubelet itself. By default, Ubuntu 14.04 and Debian Jessie have these set to 200 and 20000. Without this setting, nodes are limited in the number of containers that they can start.
cmluciano
force-pushed
the
cml/consolidatesysctl
branch
from
c5b2181
to
bafabcb
Compare
k8s-github-robot
removed
the
lgtm
|
ok everything looks good to go now cc @vishh @dchen1107 |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
thockin
removed
the
do-not-merge
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cmluciano, thockin, vishh
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
k8s-github-robot
added
the
approved
|
kops failure simply timed out, rerunning to see if it is a flake @k8s-bot pull-kubernetes-e2e-kops-aws test this |
|
@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this |
|
@k8s-bot pull-kubernetes-federation-e2e-gce test this |
|
@cmluciano: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
|
@justinsb Do you know if kops is doing anything special with the sysctls? I'm not sure if I also need to change something in kops. |
|
/retest |
|
Automatic merge from submit-queue (batch tested with PRs 43005, 46660, 46385, 46991, 47103) |
What this PR does / why we need it:
These commands are important enough to be in the Kubelet itself.
By default, Ubuntu 14.04 and Debian Jessie have these set to 200 and
20000. Without this setting, nodes are limited in the number of
containers that they can start.
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close that issue when PR gets merged): fixes #26005Special notes for your reviewer:
I had a difficult time writing tests for this. It is trivial to create a fake sysctl for testing, but the Kubelet does not have any tests for the prior settings.
Release note: