tell kubelet which cgroup driver to use

Is this an instruction, or a hint?

an instruction -> reworded to instructed

?

(I think we'd support an override mechanism, but maybe I'm wrong)

No we don't. There should be no reason to override as the cgroup driver setting of and the kubelet and runtime really must be aligned (the same). Anything else is misconfiguration and effectively a "broken" (partially or fully) node

agreed. it may make some sense to let a CR expose cgroup driver config to the user. but the kubelet cgroup driver CLI flag and v1beta1 config option must be removed eventually. it should be automatically detected.

to me, it makes sense to outline this deprecation work in the kep and then log an issue in k/k for sig node to track. but(!!) these cli/config options are so widely used that inevitably, and despite release notes, the removal will break some users.

it may make some sense to let a CR expose cgroup driver config to the user

they could potentially do this by having a CR that configures the CRI implementation, but I think the larger intention of this enhancement is to take the configuration of cgroup driver away from kubelet+kubernetes in general

Proposal updated: now the "override" mechanism would be to disable the feature gate. @sftim this resolved?

I still recommend the wording change, unless there's a reason it makes the KEP less useful

Hmm, might be hair-splitting but I feel that "allow" changes the aim of this proposal. The goal is for kubelet to obey what runtime tells it to do (when the feature is enabled), not to opt out for something else. Maybe it's my non-native language skills... WDYT @haircommander @sftim ?

yeah I agree @marquiz

AIUI we do not publish a list of supported vs. unsupported container runtimes.

(We do tell people what we test against, but I don't think we - Kubernetes - make a public claim about compatibility with some runtimes and not others).

True. Though in practice we only have a few implementations. We could drop the deprecation part of the proposal, as the flag is still valid and necessary with runtime versions that doesn't support this. Any thoughts @haircommander?

We could also be explicit in what we mean by all. Members of the cri-o and containerd community have signed-off on the proposal. I am not aware of cri-dockerd's awareness. We could change the wording to

to be dropped when support for the field is adopted by CRI-O and containerd.

I now dropped the sentence about deprecating --cgroup-driver. I think dropping the flag will require wide/ubiquitous adoption of "fresh" versions of the container runtimes that support this feature. Simpler to not speculate on that in this KEP. WDYT @haircommander @sftim ?

tbh I think we should deprecate so users have warning that they should remove it from their scripts and the like. I don't think we need to be explicit about when we will drop it.

The --cgroup-driver option has been deprecated and will be dropped in a future release. Please upgrade to a CRI implementation that supports cgroup-driver detection.

or something

OK, added back with the following wording:

Further, the kubeletConfig field and `--cgroup-driver` flag will be
marked as deprecated, to be dropped when support for the feature is adopted by
CRI-O and containerd. Usage of the deprecated setting will produce a log
message, e.g.:

"cgroupDriver option has been deprecated and will be dropped in a future release. Please upgrade to a CRI implementation that supports cgroup-driver detection."

How about exposing a cgroup driver time series? It'd be low cardinality I think.

In practice, this setting not expected to change. Metrics for configuration settings deserves a separate proposal, if desired, I think. @haircommander

If we're loading it from third party software, then surely it's not a configuration setting?

(imagine if you update to a new system image for nodes with a different container runtime, and now you have a mix of cgroup v2 and the future cgroup v3 - you might care)

yeah I guess I also find myself wondering how useful such a metric would be. cgroup version (1, 2, or future 3) is not covered in the scope of this KEP--it's automatically discovered by both kubelet and cri. I don't see any reason a cluster admin would want a metric to see the cgroup driver.

How about displaying it in Node Status?

Would be possible, of course, but I'd like to keep Kubernetes API changes out of the scope of this KEP if possible.

As well as changing the CRI protocol, I'd expect to see a feature gate (even one that's on by default) to control whether the kubelet falls back to manual configuration or honors what the container runtime reports.

If available the cgroup driver information received from the container runtime will take precedence over cgroupDriver setting from the kubelet config (or --cgroup-driver command line flag).

It doesn't sound like there's any other way to opt out. In that case we might want to mark the feature as beta from the off.

I really can't think of a scenario where manual override would be desired. If the settings are not aligned the node is misconfigured and not working properly. This is exactly the (commmon'ish) misconfiguration scenario what we're trying to prevent with the proposal. You're right with the straight to beta 🤔 Thoughts @haircommander?

personally, I don't think a feature gate is necessary. I think the kubelet should be robust enough to fallback to its own cgroup-driver for a couple of releases while broad adoption of newer cri's takes place. Eventually, it can drop support for its own cgroup-driver. we shouldn't drop the flag until we're confident the runtimes are present.

It doesn't sound like there's any other way to opt out

I don't think we should provide a way to opt-out tbh. the runtime already needs to be configured manually for a cgroup driver, so the cluster admin already needs to know that cgroup-driver should be configured. The only difference is the cluster admin now has one place fewer to do that configuration.

feature flags are not meant to be used as opt-in or opt-out flags, they are meant to graduate and eventually being removed. They are used to give the project the opportunity to roll back changes on multiple releases, if something unexpected happen on this feature and we introduce this change directly, we'll be releasing a core component with a bug that we can only solve with a new release.

As Tim said, when we are confident with the change sometimes we start with a feature gate enabled by default in beta, but this change is introducing a behavior change, it must have a feature gate and provide a graduation criteria (it should work for containerd and crio per example and no bugs reported during one release, per example) IMHO

Another thing that a feature gate can help with is if we need to change implementation details around how we merge or override we are protected and can stay in alpha to do that.

OK, looks like people want this so I added feature gate (KubeletCgroupDriverFromCRI) in the proposal. It is proposed to be enabled by default, and thus KEP maturity would be beta directly. @sftim @aojea @mrunalp did I get it right?

Probably silly question. Why don't we want to delegate sandbox cgroups management to a runtime? If this is doable we don't need to synchronise this between Kubelet and runtime anymore.

Is it because we also want to control the namespaces that are associated with things in the sandbox cgroup?

I don't think it's a silly question. I think we'll get back to that at some other KEP :) That's just soo much bigger topic and "problem" to tackle. And would take time to adopt even if we had the code merged in the runtimes' mainline now.

Is it because we also want to control the namespaces that are associated with things in the sandbox cgroup?

I think it's for historical reasons (docker) but others probably know better than me.

If I know these version values, and I don't have a detailed understanding of the behavior of my preferred container runtime, can I work out whether or not the kubelet is automatically learning which cgroup driver to use?

If not, I'd like us to add that. It's much easier for us to document “look at this metrics query” or “look for this log line” vs. “go and read the docs and / or source code for some third party component”.

We should add this for the first release where the new behavior will be on by default, and optionally for any earlier releases.

This question is not the same as can I work out which driver is in use; it's about working out whether or not the kubelet is overriding my configuration. (It is also useful to know which driver is in use).

would having the output of crictl info display the cgroupDriver be sufficient? it currently reports the runtime status info and would likely be extended to include it, which would provide a runtime agnostic way of polling

I added a mention of crictl info for determining the runtime-side support. WDYT @sftim

This now states:

Kubelet and container runtime version. The crictl tool can be used to determine if the container runtime supports the feature (crictl info).

@sftim do you consider this as resolved?

I think we should split this into a separate CRI call.

@mrunalp I think in this case in this case we should add a streaming interface for the runtime to be able to notify about dynamic changes (not for the cgroup driver but for other potentially more dynamic stuff in the future). Adding yet another endpoint that get's periodically polled doesn't sound reasonable to me. WDYT? @haircommander ?

I think the agreeement was for this KEP to be simple and for this use case. We could add a streaming interface later if needed for other use cases?

yeah I think something worth noting is we're not preparred in this KEP to react to the runtime changing its mind. I don't think the kubelet should update the cgroup driver if the cri changes without a kubelet restart. from that perspective, separating the calls may be appropriate

I don't think the kubelet should update the cgroup driver if the cri changes

I agree. But I was thinking this from the CRI API pov. So we would add the simple "unary" rpc for now and then later with the dynamic stuff add a parallel streaming rpc (for basically the same data, I think). Is this the way to go?

yeah I find myself leaning towards having a separate API call for the streaming stuff, part to reduce the info going over the wire, part to make clear how the kubelet will react to the data

OK, changed the proposal to add a new RuntimeConfig rpc for querying the runtime configuration. @mrunalp @haircommander WDYT?

This should be a map that takes runtime handler names as the keys:
https://github.com/kubernetes/enhancements/pull/3858/files#diff-057b8627f24bc6a0742b51b4fea113938a3440eafe255edecc834f3301c008fcR471

Regarding the runtime configuration we'd prolly need a "system-wide config" that is independent of the runtime class and the runtime handler specific stuff. I think the cgroup driver would fall in the first category – even though at least in containerd it is technically possible to have different cgroup driver for different runtime handlers I can't think of a practical scenario where this would make sense (and kubelet can anyways handle only one)

Also, I think we've tried to prefer lists with named fields over maps also in CRI API.

Thoughts @mrunalp @haircommander @mikebrow ?

kubelet can anyways handle only one

👍

Then this does not need to be a map, but I still expect the proto file to have a comment line to state that this is a global config that is decoupled from individual runtime handlers.

I added a comment along these lines in the proposal (in the .proto diff)

This is an important change of behavior, so if I have kubelet with hardcoded cgroup driver in version X and I upgrade to version X+1 I may have my kubelet running with a different configuration without noticing?

I'd argue in this case N+1 would then actually fix your node. Logically there should be only one configuration point for this setting. Having it both in kubelet and container runtime is a constant source of problems as if your runtime has configured driver Y and kubelet has Z then your node is borken

non-blocking comment: since there is a feature gate, the operator can see which nodes it is enabled on via a metric. Please note this in later KEP update.