[kubelet] Introduce --protect-kernel-defaults flag to make the tunable behaviour configurable #27874

ingvagabund · 2016-06-22T14:25:55Z

Let's make the default behaviour of kernel tuning configurable. The default behaviour is kept modify as has been so far.

This change is

ingvagabund · 2016-06-22T14:37:12Z

@derekwaynecarr

ingvagabund · 2016-06-22T14:37:19Z

@kubernetes/rh-cluster-infra

derekwaynecarr · 2016-06-22T14:55:19Z

cmd/kubelet/app/options/options.go

@@ -272,4 +274,6 @@ func (s *KubeletServer) AddFlags(fs *pflag.FlagSet) {
 	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. The total number of Pods on this Kubelet cannot exceed max-pods, so max-pods will be used if this calculation results in a larger number of Pods allowed on the Kubelet. A value of 0 disables this limit.")
+	fs.StringVar(&s.KernelTunableBehavior, "kernel-tunable-behaviour", s.KernelTunableBehavior, "Default kubelet behaviour for kernel tunning. Valid values: warning, modify, error")


s/tunning/tuning

It would be good to mention here that default tunable behavior is modify if not set.

Why the word "Default"?

I feel that kubelet should error out if the kernel configuration does not match its expectation. Proceeding might be OK for some configuration options, but in the long run it will be make it hard to reason kubelet behavior.

I'm OK with the flag if it can be made a boolean.

`--kernel-auto-configuration=true` When set to `true` kubelet will configure the kernel to meet its feature requirements. When set to `false` kubelet will fail if the kernel configuration is in-compatible.

And if warn isn't the default, it doesn't seem all that harmful to add support for it

Wondering whether there is a clear distinction between required and recommended tunables. The former can result in an error when set wrong, the latter to a warning. Having only required tunables makes people to choose the "warn" mode (which is effectively not supportable inside of a product).

all 3 nits addressed

Each and every option we add needs to ideally be tested. The lesser options we add, the easier it is to maintain in general.
If a need arises to support warning, we can consider it then..
As @derekwaynecarr pointed out, it will be difficult to track the various kernel configs that kubelet will depend on over time.

@vishh warning is no longer supported, only error and modified. Is it sufficient for the moment?

sttts · 2016-06-22T15:11:47Z

cmd/kubelet/app/server.go

@@ -1005,3 +1012,16 @@ func parseResourceList(m utilconfig.ConfigurationMap) (api.ResourceList, error)
 	}
 	return rl, nil
 }
+
+func parseKernelTunableBehaviour(behaviour string) (cm.KernelTunableBehavior, error) {


s/behaviour/b/g

vishh · 2016-07-19T00:38:13Z

cmd/kubelet/app/options/options.go

@@ -278,4 +279,5 @@ func (s *KubeletServer) AddFlags(fs *pflag.FlagSet) {
 	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. The total number of Pods on this Kubelet cannot exceed max-pods, so max-pods will be used if this calculation results in a larger number of Pods allowed on the Kubelet. A value of 0 disables this limit.")
+	fs.StringVar(&s.KernelTunableBehavior, "kernel-tunable-behaviour", s.KernelTunableBehavior, "Kubelet behaviour for kernel tuning. Valid values are 'modify', 'error'. Defaults to 'modify'")


I'd still prefer this flag to be a boolen. Until we know for a fact that warning mode is required, this can be a boolean.
For example, --set-kernel-defaults=[true].

ingvagabund · 2016-08-01T13:04:19Z

So, still something needs to be re-generated

sttts · 2016-08-01T13:24:23Z

@ingvagabund Jenkins is happy now

ingvagabund · 2016-08-01T13:51:01Z

@sttts finally, now it's time for the re-base game.

ingvagabund · 2016-08-01T13:51:56Z

@vishh updated, changed to boolean flag --protect-kernel-defaults, if not set => modify, if set => error

vishh · 2016-08-01T19:42:22Z

cmd/kubelet/app/server.go

@@ -1020,3 +1027,11 @@ func parseResourceList(m utilconfig.ConfigurationMap) (api.ResourceList, error)
 	}
 	return rl, nil
 }
+
+func parseKernelTunableBehaviour(protect bool) cm.KernelTunableBehavior {


nit: Why keep this config as a string when the user input is a boolean?

Since there are three behaviours. At the moment, the behaviour flag is boolean. Later, it can be changed/removed/extended with more flags. In order to make the transition from input to any of the three (and in future more) behaviour generic, I kept the parse function here.

The problem is that kubelet is consumed in different methods. One if via the cmd/kubelet package. It also gets linked into other binaries. I'd prefer not supporting the warn option at all for now for the same reasons mentioned above. Changing internal code is much simpler in the future than the CLI, so we can switch to a string easily in the future.

Updated the NodeConfig to take bool instead of string. Rebased.

vishh · 2016-08-01T19:42:45Z

cc @mtaufen --> FYI: This PR adds another config object to kubelet.

vishh · 2016-08-01T19:43:00Z

@ingvagabund Just one more nit/comment. Otherwise LGTM. Sorry for the delay

ncdc · 2016-08-10T20:24:35Z

@ingvagabund mind addressing the comment so we can get this merged?

…eBehavior configurable

ingvagabund · 2016-08-11T11:39:52Z

The hack/update-all.sh is black magic. The generated-protobuf is run within a container with one golang. The rest is run on a host with local golang. It starts to be funny, when both golangs have different location of standard library. Container expects /usr/local/golang in GOROOT, Fedora /usr/lib/golang. Which is not obviously correctly handled.

Almost done re-generating.

k8s-bot · 2016-08-11T12:29:12Z

GCE e2e build/test passed for commit 587b1f8.

ingvagabund · 2016-08-11T14:09:12Z

@vishh PTAL

ncdc · 2016-08-12T13:56:53Z

@vishh bump 😄

vishh · 2016-08-12T19:21:04Z

lgtm. Thanks @ingvagabund!

k8s-github-robot · 2016-08-12T20:27:41Z

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

k8s-bot · 2016-08-12T21:10:56Z

GCE e2e build/test passed for commit 587b1f8.

k8s-github-robot · 2016-08-12T21:11:25Z

Automatic merge from submit-queue

googlebot added the cla: yes label Jun 22, 2016

k8s-github-robot assigned vishh Jun 22, 2016

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Jun 22, 2016

ingvagabund force-pushed the kubelet-kernel-tunning-behaviour-new-flags branch from 4b5992b to 53c0488 Compare June 22, 2016 14:49

derekwaynecarr reviewed Jun 22, 2016
View reviewed changes

k8s-github-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Jun 22, 2016

sttts reviewed Jun 22, 2016
View reviewed changes

ingvagabund force-pushed the kubelet-kernel-tunning-behaviour-new-flags branch from 53c0488 to 2c231d8 Compare June 23, 2016 09:57

ingvagabund force-pushed the kubelet-kernel-tunning-behaviour-new-flags branch from f36b8b2 to e97149d Compare July 1, 2016 15:16

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 13, 2016

vishh reviewed Jul 19, 2016
View reviewed changes

ingvagabund force-pushed the kubelet-kernel-tunning-behaviour-new-flags branch from e97149d to a9d4f6e Compare July 26, 2016 11:49

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 26, 2016

ingvagabund force-pushed the kubelet-kernel-tunning-behaviour-new-flags branch 2 times, most recently from 7629805 to 6220206 Compare July 26, 2016 13:03

k8s-github-robot mentioned this pull request Jul 26, 2016

[k8s.io] Deployment scaled rollout should not block on annotation check {Kubernetes e2e suite} #29559

Closed

k8s-github-robot added do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. retest-not-required-docs-only and removed retest-not-required-docs-only labels Jul 28, 2016

ingvagabund force-pushed the kubelet-kernel-tunning-behaviour-new-flags branch from 6220206 to 0994e01 Compare August 1, 2016 12:22

sttts added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. release-note-label-needed labels Aug 1, 2016

ingvagabund changed the title ~~WIP: kubelet: introduce --kernel-tunable-behaviour to make the behaviour configurable~~ [kubelet] Introduce --protect-kernel-defaults flag to make the tunable behaviour configurable Aug 1, 2016

vishh reviewed Aug 1, 2016
View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 9, 2016

ingvagabund force-pushed the kubelet-kernel-tunning-behaviour-new-flags branch from 0994e01 to b09757e Compare August 11, 2016 10:20

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 11, 2016

kubelet: introduce --protect-kernel-defaults to make the KernelTunabl…

eb967ad

…eBehavior configurable

Autogenerate

587b1f8

ingvagabund force-pushed the kubelet-kernel-tunning-behaviour-new-flags branch from b09757e to 587b1f8 Compare August 11, 2016 11:50

vishh added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 12, 2016

k8s-github-robot merged commit f279e61 into kubernetes:master Aug 12, 2016

ingvagabund deleted the kubelet-kernel-tunning-behaviour-new-flags branch September 16, 2016 08:34

dims mentioned this pull request Aug 3, 2017

expose "KernelTunableWarn" in kublet config #50110

Closed

pacoxu mentioned this pull request Aug 16, 2021

Kubelet sysctl settings cannot be overridden #74151

Open

andypitcher mentioned this pull request Aug 22, 2023

Update Hardening Guides to reflect removal of "protect-kernel-defaults" check in RKE/RKE2/K3S CIS 1.7 Profiles rancher/rancher#42154

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[kubelet] Introduce --protect-kernel-defaults flag to make the tunable behaviour configurable #27874

[kubelet] Introduce --protect-kernel-defaults flag to make the tunable behaviour configurable #27874

ingvagabund commented Jun 22, 2016 •

edited by k8s-oncall

Loading

ingvagabund commented Jun 22, 2016

ingvagabund commented Jun 22, 2016

derekwaynecarr Jun 22, 2016

aveshagarwal Jun 22, 2016

sttts Jun 22, 2016

vishh Jun 22, 2016

ncdc Jun 22, 2016

ncdc Jun 22, 2016

sttts Jun 23, 2016

ingvagabund Jun 23, 2016

vishh Jun 23, 2016

ingvagabund Jul 1, 2016

sttts Jun 22, 2016

ingvagabund Jun 23, 2016

vishh Jul 19, 2016

ingvagabund commented Aug 1, 2016

sttts commented Aug 1, 2016

ingvagabund commented Aug 1, 2016

ingvagabund commented Aug 1, 2016

vishh Aug 1, 2016

ingvagabund Aug 2, 2016

vishh Aug 5, 2016

ingvagabund Aug 11, 2016

vishh commented Aug 1, 2016

vishh commented Aug 1, 2016

ncdc commented Aug 10, 2016

ingvagabund commented Aug 11, 2016

k8s-bot commented Aug 11, 2016

ingvagabund commented Aug 11, 2016

ncdc commented Aug 12, 2016

vishh commented Aug 12, 2016

k8s-github-robot commented Aug 12, 2016

k8s-bot commented Aug 12, 2016

k8s-github-robot commented Aug 12, 2016

[kubelet] Introduce --protect-kernel-defaults flag to make the tunable behaviour configurable #27874

[kubelet] Introduce --protect-kernel-defaults flag to make the tunable behaviour configurable #27874

Conversation

ingvagabund commented Jun 22, 2016 • edited by k8s-oncall Loading

ingvagabund commented Jun 22, 2016

ingvagabund commented Jun 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ingvagabund commented Aug 1, 2016

sttts commented Aug 1, 2016

ingvagabund commented Aug 1, 2016

ingvagabund commented Aug 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vishh commented Aug 1, 2016

vishh commented Aug 1, 2016

ncdc commented Aug 10, 2016

ingvagabund commented Aug 11, 2016

k8s-bot commented Aug 11, 2016

ingvagabund commented Aug 11, 2016

ncdc commented Aug 12, 2016

vishh commented Aug 12, 2016

k8s-github-robot commented Aug 12, 2016

k8s-bot commented Aug 12, 2016

k8s-github-robot commented Aug 12, 2016

ingvagabund commented Jun 22, 2016 •

edited by k8s-oncall

Loading