Implement RuntimeConfig CRI call #8722

marquiz · 2023-06-22T14:02:46Z

The rpc only reports one field, i.e. the cgroup driver, to kubelet.
Containerd determines the effective cgroup driver by looking at all
runtime handlers, starting from the default runtime handler (the rest in
alphabetical order), and returning the cgroup driver setting of the
first runtime handler that supports one. If no runtime handler supports
cgroup driver (i.e. has a config option for it) systemd is returned as a
fallback default.

This patch implements the CRI server side of Kubernetes KEP-4033:
https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/4033-group-driver-detection-over-cri

Refs kubernetes/kubernetes#118770

k8s-ci-robot · 2023-06-22T14:02:58Z

Hi @marquiz. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

marquiz · 2023-06-22T14:03:25Z

Early peek to an implementation
/cc @mikebrow

marquiz · 2023-06-22T14:29:49Z

/cc @haircommander

k8s-ci-robot · 2023-06-22T14:29:51Z

@marquiz: GitHub didn't allow me to request PR reviews from the following users: haircommander.

Note that only containerd members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @haircommander

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.

Required to support upcoming Kubernetes (v1.28) features. Signed-off-by: Markus Lehtonen <markus.lehtonen@intel.com>

marquiz · 2023-07-24T18:27:24Z

Ready for review

mikebrow · 2023-07-25T22:59:28Z

/ok-to-test

mikebrow

Looking good! A couple comments / questions, request for a chat. Cheers

pkg/cri/server/runtime_config.go

mikebrow · 2023-07-25T23:39:17Z

pkg/cri/server/runtime_config.go

+
+func (c *criService) getCgroupDriver(ctx context.Context) runtime.CgroupDriver {
+	// Go through the runtime handlers in a predictable order, starting from the
+	// default handler, others sorted in alphabetical order


let's have a conversation about defaulting... see if there isn't a better way than alphabetical, and / or see if there is an issue here:

Noting for the conversation that we probably need to consider adding validation against our config, and talk about other considerations if a runtime is requested that is not the same (different cgroup driver) than as reported via RuntimeConfig.

let's have a conversation about defaulting... see if there isn't a better way than alphabetical, and / or see if there is an issue here:

Let's have a chat.

Noting for the conversation that we probably need to consider adding validation against our config, and talk about other considerations if a runtime is requested that is not the same (different cgroup driver) than as reported via RuntimeConfig.

Sounds pretty much like converging back to the old faithful "global" systemd_cgroup setting albeit in a very complicated fashion

that might be the correct path until we can report said global and per/runtime ... and the validation would let admins know..

mikebrow

need to edit the config description...

https://github.com/containerd/containerd/blame/main/docs/cri/config.md#L13-L33

and possibly explain some down in the related runtime option fields for ShimCgroup and SystemdCgroup

AkihiroSuda · 2023-07-27T00:00:44Z

pkg/cri/sbserver/runtime_config.go

+		log.G(ctx).Debugf("runtime handler %q does not provide cgroup driver information", handler)
+	}
+
+	// If no runtime handlers have a setting, return systemd as the default


Needs to check whether systemd is running

Thanks @AkihiroSuda, I think this is the right thing to do. And, now that I think of it, maybe we should just drop the SystemdCgroup setting from the runc handler and just rely on auto-detection everywhere. Using cgroupfs when system.slice and folks are around feels like a mis-configuration to me 🤔

When SystemdCgroup is unspecified it can just detect the driver automatically.
Explicit configuration of SystemdCgroup should be still supported for compatibility reason.

Using cgroupfs when system.slice and folks are around feels like a mis-configuration to me 🤔

Still legit when the systemd unit is Delegate-d.

Still legit when the systemd unit is Delegate-d.

Hmm, that's true, in principle. But afaiu this scenario is not supported by kubelet at least. And, I think it's still a mis-configuration if the unit has Delegate=yes but you have SystemdCgroup=true (or vice versa if Delegate=off but you'd have SystemdCgroup=false. So, even in this scenario the right path would seem to be auto-detection (another level of auto-detection, i.e. auto-detection on the pod-sandbox unit level). WDYT @AkihiroSuda?

I implemented systemd autodetection in case no runc plugins are configured (i.e. we cannot get the setting from there).

Regarding whether SystemdCgroup is explicitly set in the configuration, I don't think there's any way to detect that. It's a bool that's false by default so 🤷‍♀️

pkg/cri/instrument/instrumented_service.go

fuweid · 2023-07-27T03:44:44Z

pkg/cri/sbserver/runtime_config.go

+func (c *criService) RuntimeConfig(ctx context.Context, r *runtime.RuntimeConfigRequest) (*runtime.RuntimeConfigResponse, error) {
+	resp := &runtime.RuntimeConfigResponse{
+		Linux: &runtime.LinuxRuntimeConfiguration{
+			CgroupDriver: c.getCgroupDriver(ctx),


Out of topic: The kubelet takes the responsibility to create pod cgroup and then the CRI fills the container in it. I think that is reason why there is easy to cause mismatch. IMO, the end goal should be like that kubelet just lets the CRI to maintain the real lifecycle of container and pod. Currently, it's split-brain.

I fully agree with this ❤️

pkg/cri/sbserver/runtime_config.go

marquiz · 2023-07-28T10:10:08Z

I updated the PR with auto-detection in case the driver cannot be determined from the runtime-handler options. Regarding ordering (in case the default runtime-handler doesn't provide the setting) I left the alphabetical ordering as there are no other/better suggestions. TBH, I think it should not be allowed to have multiple runc-based runtime classes with conflicting cgroup driver setting (different runtime classes having different SystemdCgroup) and could be rejected in config validation but I think that's subject of another PR.

PTAL @AkihiroSuda @mikebrow @fuweid

marquiz · 2023-07-28T10:45:17Z

I made one more change: made the cgroup driver detection code Linux-only, other OS'es resorting to a stub

The rpc only reports one field, i.e. the cgroup driver, to kubelet. Containerd determines the effective cgroup driver by looking at all runtime handlers, starting from the default runtime handler (the rest in alphabetical order), and returning the cgroup driver setting of the first runtime handler that supports one. If no runtime handler supports cgroup driver (i.e. has a config option for it) containerd falls back to auto-detection, returning systemd if systemd is running and cgroupfs otherwise. This patch implements the CRI server side of Kubernetes KEP-4033: https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/4033-group-driver-detection-over-cri Signed-off-by: Markus Lehtonen <markus.lehtonen@intel.com>

AkihiroSuda · 2023-08-01T09:53:54Z

docs/cri/config.md

+>
+> When determining the cgroup driver, containerd uses the `SystemdCgroup`
+> setting from runc-based runtime classes, starting from the default runtime
+> class. If no runc-based runtime classes have been configured containerd


Suggested change

> class. If no runc-based runtime classes have been configured containerd

> class. Starting with containerd v2.0, if no runc-based runtime classes have been configured containerd

This whole thing is "starting with containerd v2.0" (plus possibly later with v1.6.x as @mikebrow pointed out below). I could add e.g. a separate paragraph in the end of this note

The automatic cgroup driver configuration for kubelet feature is supported in containerd v2.0 and later.

WDYT @AkihiroSuda @mikebrow ?

Looks good, but I don't think we will backport this to v1.6 LTS

Thx @AkihiroSuda for quick response. I now added that separate paragraph at the end of the note. @mikebrow PTAL

Looks good, but I don't think we will backport this to v1.6 LTS

My reasoning to mention backport may be needed before 1.6LTS EOL:

The current alpha stage of this k8s feature has a feature gate that is disabled by default.. and thus the api is not required to be there for supported container runtimes (1.6, 1.7, and possibly 2.0). The current proposed beta plan of this feature shows the feature gate is enabled by default and the api is expected to be enabled by supported container runtimes. Thus requiring clouds take the special action of disabling the feature gate if they don't move up to containerd that supports it. Then at GA stage the feature gate is expected to removed leaving the feature on and container runtimes that don't have it would cause an error, (with that current plan). Plans can always change.

our 1.6 EOL is currently set at: max(February 15, 2025 or next LTS + 6 months) ...

Same LTS issue with many CRI features...

True, the current plan states that the kubelet fallback behavior (basically kubelet's cgroupDriver setting) would be dropped at GA. However, elsewhere it's stated that kubelet's cgroupDriver setting will only be dropped when support for the feature is adopted by CRI-O and containerd. So if we stick to this plan we wait for containerd LTS (and other non-EOL'd versions) to support this – either backport to v1.6 or then wait for v1.6 EOL. But as @mikebrow stated the plan can (and probably will) change as we get closer to GA and see how this plays out. E.g. we still keep the cgroupDriver fallback for some time even in GA...

Anyway, what it comes to this PR now at hand, I think we cannot mention v1.6 LTS here because it hasn't been backported, yet. We update the documentation if/when we backport this. RIght @mikebrow @AkihiroSuda ?

AkihiroSuda · 2023-08-01T09:54:22Z

docs/cri/config.md

+> When determining the cgroup driver, containerd uses the `SystemdCgroup`
+> setting from runc-based runtime classes, starting from the default runtime
+> class. If no runc-based runtime classes have been configured containerd
+> relies on auto-detection based on determining if systemd is running.


Suggested change

> relies on auto-detection based on determining if systemd is running.

> relies on auto-detection based on determining if systemd is running.

> containerd v1.x always defaults to non-systemd cgroupfs driver.

Hmm, I'm not sure about this 🧐 What I mean is that this PR only adds new functionality that instructs the kubelet which cgroup driver it should use (for pod sandbox-level cgroups). Containerd v1.x doesn't have this functionality. The PR doesn't change any internal configuration logic or defaults of containerd itself. There is no "containerd-wide" (or even "containerd.cri -wide") cgroup driver setting. The setting only exists in the runc runtime handler plugin options. For other runtime handlers containerd doesn't care (afaiu).

containerd v1.x doesn't have this functionality .. "yet". 1.6 is LTS and may need to support the api before EOL

mikebrow

LGTM

AkihiroSuda

LGTM

Backporting to v1.6 is probably fine if it is necessary for Kube && it doesn't break compatibility for existing clusters

marquiz · 2023-08-03T07:41:47Z

Thanks for the reviews @mikebrow @AkihiroSuda and @fuweid.

I think the PR title "postfix" ([cgroup driver now defaults to systemd]) was wrong/misleading so I removed it. This PR is not changing the default cgroup driver (which setting only exists in the runc runtime handler plugin). With the new RuntimeConfig RPC we need to tell something to the kubelet and in case no runc-based runtime classes are configured (where we could get the cgroup driver setting) we resort to detecting if systemd is running or not (this doesn't change any containerd behavior, we just tell this information to the kubelet).

mikebrow · 2023-08-03T20:57:19Z

@marquiz missing dco signature on the docs commit..

Document the user visible changes related to the new RuntimeConfig RPC. Signed-off-by: Markus Lehtonen <markus.lehtonen@intel.com>

marquiz · 2023-08-04T06:49:40Z

@marquiz missing dco signature on the docs commit..

Argh, thanks for pointing this out @mikebrow. Fixed now

fuweid

LGTM

dmcgowan · 2023-11-06T15:18:01Z

I'm going to remove the breaking label for now since there isn't enough explanation in the title to indicate what is breaking. It just appears as an addition. We can add a note to the 2.0 release notes explaining the cgroup change if there is user action required in some cases.

AkihiroSuda · 2023-11-06T15:22:00Z

docs/cri/config.md

+> When determining the cgroup driver, containerd uses the `SystemdCgroup`
+> setting from runc-based runtime classes, starting from the default runtime
+> class. If no runc-based runtime classes have been configured containerd
+> relies on auto-detection based on determining if systemd is running.


@dmcgowan

I added the impact/breaking label because this seems to break an existing node that is (mis)using non-systemd cgroup driver on systemd distro

I think we can add a note about it, it just looked confusing in the change log. It implies that the RuntimeConfig CRI call itself is a breaking change. #9340

We can clean up the language in future betas

Sorry, maybe I have misread the paragraph

#9340 (comment)

k8s-ci-robot added the needs-ok-to-test label Jun 22, 2023

k8s-ci-robot requested a review from mikebrow June 22, 2023 14:03

marquiz force-pushed the devel/cgroup-driver-autoconfig branch from 5715945 to 2163191 Compare June 22, 2023 14:29

go.mod: update cri-api to v1.28.0-beta.0

850b2e1

Required to support upcoming Kubernetes (v1.28) features. Signed-off-by: Markus Lehtonen <markus.lehtonen@intel.com>

marquiz force-pushed the devel/cgroup-driver-autoconfig branch from 2163191 to 276462d Compare July 24, 2023 11:52

marquiz marked this pull request as ready for review July 24, 2023 18:26

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Jul 25, 2023

mikebrow reviewed Jul 25, 2023

View reviewed changes

mikebrow mentioned this pull request Jul 25, 2023

docs: document kubelet cgroup driver detection from the runtime kubernetes/website#42160

Merged

marquiz force-pushed the devel/cgroup-driver-autoconfig branch 2 times, most recently from e6b7fbc to 3fa3834 Compare July 26, 2023 13:58

mikebrow reviewed Jul 26, 2023

View reviewed changes

mikebrow assigned mxpv and AkihiroSuda Jul 26, 2023

AkihiroSuda reviewed Jul 27, 2023

View reviewed changes

fuweid reviewed Jul 27, 2023

View reviewed changes

AkihiroSuda added impact/changelog impact/breaking labels Jul 27, 2023

marquiz force-pushed the devel/cgroup-driver-autoconfig branch from 3fa3834 to 0f21904 Compare July 28, 2023 09:51

marquiz force-pushed the devel/cgroup-driver-autoconfig branch from 0f21904 to 9302091 Compare July 28, 2023 10:42

AkihiroSuda reviewed Aug 1, 2023

View reviewed changes

marquiz force-pushed the devel/cgroup-driver-autoconfig branch 2 times, most recently from d60befd to 17ddeef Compare August 2, 2023 11:40

mikebrow approved these changes Aug 2, 2023

View reviewed changes

AkihiroSuda approved these changes Aug 2, 2023

View reviewed changes

AkihiroSuda changed the title ~~cri: implement RuntimeConfig rpc [Breaking change: cgroup driver now defaults to systemd]~~ cri: implement RuntimeConfig rpc [cgroup driver now defaults to systemd] Aug 2, 2023

marquiz changed the title ~~cri: implement RuntimeConfig rpc [cgroup driver now defaults to systemd]~~ cri: implement RuntimeConfig rpc Aug 3, 2023

docs: document CRI cgroup driver auto detection

c62abcb

Document the user visible changes related to the new RuntimeConfig RPC. Signed-off-by: Markus Lehtonen <markus.lehtonen@intel.com>

marquiz force-pushed the devel/cgroup-driver-autoconfig branch from 17ddeef to c62abcb Compare August 4, 2023 06:48

fuweid approved these changes Aug 4, 2023

View reviewed changes

fuweid merged commit 2b2195c into containerd:main Aug 4, 2023
43 checks passed

marquiz deleted the devel/cgroup-driver-autoconfig branch August 4, 2023 09:14

thaJeztah mentioned this pull request Sep 1, 2023

remove uses of github.com/runc/libcontainer/cgroups #9045

Merged

dmcgowan added the area/cri Container Runtime Interface (CRI) label Nov 1, 2023

dmcgowan changed the title ~~cri: implement RuntimeConfig rpc~~ Implement RuntimeConfig CRI call Nov 1, 2023

dmcgowan mentioned this pull request Nov 2, 2023

Prepare release notes for 2.0.0-beta.0 #9318

Merged

dmcgowan removed the impact/breaking label Nov 6, 2023

dmcgowan mentioned this pull request Nov 6, 2023

Explain new cgroup driver detection in 2.0 #9340

Closed

AkihiroSuda reviewed Nov 6, 2023

View reviewed changes

dmcgowan mentioned this pull request Apr 19, 2024

Prepare release notes for v2.0.0-rc.1 #10098

Merged

dmcgowan mentioned this pull request May 17, 2024

Prepare release notes for v2.0.0-rc.2 #10240

Merged

dmcgowan mentioned this pull request Jun 13, 2024

Prepare v2.0.0-rc.3 release #10331

Merged

dmcgowan mentioned this pull request Aug 27, 2024

Prepare release notes for v2.0.0-rc.4 #10643

Merged

AkihiroSuda mentioned this pull request Sep 19, 2024

use cgroup driver "systemd" by default #7319

Closed

dmcgowan mentioned this pull request Sep 30, 2024

Prepare release notes for v2.0.0-rc.5 #10752

Merged

	> class. If no runc-based runtime classes have been configured containerd
	> class. Starting with containerd v2.0, if no runc-based runtime classes have been configured containerd

	> relies on auto-detection based on determining if systemd is running.
	> relies on auto-detection based on determining if systemd is running.
	> containerd v1.x always defaults to non-systemd cgroupfs driver.

Implement RuntimeConfig CRI call #8722

Implement RuntimeConfig CRI call #8722

Conversation

marquiz commented Jun 22, 2023 • edited Loading

k8s-ci-robot commented Jun 22, 2023

marquiz commented Jun 22, 2023

marquiz commented Jun 22, 2023

k8s-ci-robot commented Jun 22, 2023

marquiz commented Jul 24, 2023

mikebrow commented Jul 25, 2023

mikebrow left a comment

Choose a reason for hiding this comment

mikebrow Jul 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikebrow left a 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

marquiz commented Jul 28, 2023

marquiz commented Jul 28, 2023

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

mikebrow left a comment

Choose a reason for hiding this comment

AkihiroSuda left a comment

Choose a reason for hiding this comment

marquiz commented Aug 3, 2023 • edited Loading

mikebrow commented Aug 3, 2023

marquiz commented Aug 4, 2023

fuweid left a comment

Choose a reason for hiding this comment

dmcgowan commented Nov 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marquiz commented Jun 22, 2023 •

edited

Loading

mikebrow Jul 25, 2023 •

edited

Loading

marquiz commented Aug 3, 2023 •

edited

Loading