add CONFIG_CGROUP_BPF to required kernel config #41

pacoxu · 2024-09-25T06:50:34Z

References:

Fixes #36

k8s-ci-robot · 2024-09-25T06:50:37Z

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

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

In response to this:

References:

support cgroup2 containerd/containerd#3799 (comment)

https://man7.org/linux/man-pages/man7/cgroups.7.html

https://www.kernel.org/doc/Documentation/admin-guide/cgroup-v2.rst

https://www.kernel.org/doc/Documentation/admin-guide/cgroup-v1/

Fixes #36

/cc @KentaTada @neolit123

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-sigs/prow repository.

pacoxu · 2024-09-25T06:52:23Z

As cgroup v1 is in maintenance mode but not removed, we should not remove the cgroup v1 related kernel config out.

However, should we show different message for CONFIG_CGROUP_BPF missing?
Or should we check if the user is in cgroup v1 or v2 at this point to decide which check policy to use?

neolit123

in #36 @KentaTada mentioned that we need to read another file to verify cgroupv2.

Unlike in v1, the kernel config alone cannot determine whether the required v2 features are enabled.

validators/types_unix.go

pacoxu · 2024-09-26T01:28:15Z

in #36 @KentaTada mentioned that we need to read another file to verify cgroupv2.

It seems that we need to check in different ways for cgroup v1 and cgroup v2.

For instance,

cgroup v2: check /sys/fs/cgroup/init.scope/cgroup.freeze
cgroup v1: check CONFIG_CGROUP_FREEZER.

/hold

pacoxu · 2024-09-26T02:50:59Z

	// /proc/cgroups is meaningless for v2
	// https://github.com/torvalds/linux/blob/v5.3/Documentation/admin-guide/cgroup-v2.rst#deprecated-v1-core-features
	if IsCgroup2UnifiedMode() {
		// "pseudo" controllers do not appear in /sys/fs/cgroup/cgroup.controllers.
		// - devices: implemented in kernel 4.15
		// - freezer: implemented in kernel 5.2
		// We assume these are always available, as it is hard to detect availability.
		pseudo := []string{"devices", "freezer"}
		data, err := ReadFile("/sys/fs/cgroup", "cgroup.controllers")
		if err != nil {
			return nil, err
		}
		subsystems := append(pseudo, strings.Fields(data)...)
		return subsystems, nil
	}

Runc code https://github.com/kubernetes/kubernetes/blob/e404a28fae6946483c5de0e9f4b4adf6e6f63f3b/vendor/github.com/opencontainers/runc/libcontainer/cgroups/utils.go#L100-L114

pacoxu · 2024-09-26T04:09:42Z

system-validators/validators/cgroup_validator_linux.go

Lines 141 to 156 in c4cf635

    
           func (c *CgroupsValidator) getCgroupV2Subsystems() ([]string, error) { 
        
           	// Some controllers are implicitly enabled by the kernel. 
        
           	// Those controllers do not appear in /sys/fs/cgroup/cgroup.controllers. 
        
           	// https://github.com/torvalds/linux/blob/v5.3/kernel/cgroup/cgroup.c#L433-L434 
        
           	// We assume these are always available, as it is hard to detect availability. 
        
           	// So, we hardcode the following as "pseudo" controllers. 
        
           	// - devices: implemented in kernel 4.15 
        
           	// - freezer: implemented in kernel 5.2 
        
           	pseudo := []string{"devices", "freezer"} 
        
           	data, err := ioutil.ReadFile(filepath.Join(unifiedMountpoint, "cgroup.controllers")) 
        
           	if err != nil { 
        
           		return nil, err 
        
           	} 
        
           	subsystems := append(pseudo, strings.Fields(string(data))...) 
        
           	return subsystems, nil 
        
           }

	pseudo := []string{"devices", "freezer"}

We currently use the similar logic here and add freezer as default.

system-validators/validators/types_unix.go

Lines 69 to 70 in c4cf635

    
           Cgroups: []string{"cpu", "cpuacct", "cpuset", "devices", "freezer", "memory", "pids"}, 
        
           CgroupsOptional: []string{

This checks the cgroup sub controllers.

neolit123 · 2024-09-26T07:09:30Z

aligning with RUNC seems good to me.

what about these comments about freeze vs freezer?
#36 (comment)

pacoxu · 2024-09-26T08:24:36Z

/unhold
For cgroup v1 cleanup, we should not do it now.

what about these comments about freeze vs freezer?
#36 (comment)

Updated.

neolit123

/lgtm
/approve
/hold

@KentaTada could you please have a look too?

neolit123 · 2024-09-26T09:03:01Z

For cgroup v1 cleanup, we should not do it now.

could you explain why?

pacoxu · 2024-09-26T09:10:47Z

For cgroup v1 cleanup, we should not do it now.

could you explain why?

Basically two reasons IMO:

Kubernetes 1.31: Moving cgroup v1 Support into Maintenance Mode, not deprecated.
Most OS distrbutions keeps the cgroup v1 related kernel config as is, and I think this may be for backward compatibility.

neolit123 · 2024-09-26T09:14:59Z

For cgroup v1 cleanup, we should not do it now.

could you explain why?

Basically two reasons IMO:

Kubernetes 1.31: Moving cgroup v1 Support into Maintenance Mode, not deprecated.

Most OS distrbutions keeps the cgroup v1 related kernel config as is, and I think this may be for backward compatibility.

ok, got it. you mean that we should not cleanup cgroups v1 because they are still widely used.

i tried finding @KentaTada 's slack to give him a ping but couldn't.
let's wait until end of next week maybe and if he does not reply we can go merge this PR as it is.

KentaTada · 2024-09-26T11:47:41Z

@KentaTada could you please have a look too?

I'll take a look at this pr by the first half of the next week.

pacoxu · 2024-10-15T03:07:45Z

I updated the PR accordingly and the only unresolved comment is https://github.com/kubernetes/system-validators/pull/41/files#r1800338762.

I am not sure how to confirm about the default mount point if we cannot get the mount info correctly.

validators/cgroup_validator_linux.go

KentaTada · 2024-10-16T03:32:59Z

@pacoxu
Thank you for improving this feature.
It looks good to me.

pacoxu · 2024-10-16T03:34:46Z

@pacoxu
Thank you for improving this feature.
It looks good to me.

Thanks for your detailed review and comments.
Wait for @neolit123 to double check if anything is missing.

validators/cgroup_validator_linux.go

pacoxu · 2024-10-16T07:54:28Z

I refactor the logic to just parse /proc/mounts to get the first cgroup path.

and I add a simple test for getUnifiedMountpoint( ).

BTW, the /proc/self/mountinfo file format is a little complex and I removed that part.

neolit123

this is much better @pacoxu !
mostly requesting minor fixes for text / comments in multiple places.

and only one Q about the file parser.

validators/cgroup_validator_linux.go

neolit123 · 2024-10-17T07:10:27Z

validators/cgroup_validator_linux.go

+				if len(cgroupV1MountPoint) == 0 {
+					cgroupV1MountPoint = fields[1]
+				}


is it ok to just do this?

Suggested change

if len(cgroupV1MountPoint) == 0 {

cgroupV1MountPoint = fields[1]

}

cgroupV1MountPoint = fields[1]

i.e. always update the var if more cgroups v1 mount points are found or should we only catch the first one?
(i don't know the format)

[root@10-29-14-249 ~]# cat /proc/mounts | grep cgroup tmpfs /sys/fs/cgroup tmpfs ro,seclabel,nosuid,nodev,noexec,mode=755 0 0 cgroup /sys/fs/cgroup/systemd cgroup rw,seclabel,nosuid,nodev,noexec,relatime,xattr,release_agent=/usr/lib/systemd/systemd-cgroups-agent,name=systemd 0 0 cgroup /sys/fs/cgroup/blkio cgroup rw,seclabel,nosuid,nodev,noexec,relatime,blkio 0 0 cgroup /sys/fs/cgroup/hugetlb cgroup rw,seclabel,nosuid,nodev,noexec,relatime,hugetlb 0 0 cgroup /sys/fs/cgroup/perf_event cgroup rw,seclabel,nosuid,nodev,noexec,relatime,perf_event 0 0 cgroup /sys/fs/cgroup/memory cgroup rw,seclabel,nosuid,nodev,noexec,relatime,memory 0 0 cgroup /sys/fs/cgroup/net_cls,net_prio cgroup rw,seclabel,nosuid,nodev,noexec,relatime,net_prio,net_cls 0 0 cgroup /sys/fs/cgroup/cpu,cpuacct cgroup rw,seclabel,nosuid,nodev,noexec,relatime,cpuacct,cpu 0 0 cgroup /sys/fs/cgroup/devices cgroup rw,seclabel,nosuid,nodev,noexec,relatime,devices 0 0 cgroup /sys/fs/cgroup/pids cgroup rw,seclabel,nosuid,nodev,noexec,relatime,pids 0 0 cgroup /sys/fs/cgroup/cpuset cgroup rw,seclabel,nosuid,nodev,noexec,relatime,cpuset 0 0 cgroup /sys/fs/cgroup/freezer cgroup rw,seclabel,nosuid,nodev,noexec,relatime,freezer 0 0

For cgroup v1, this is an example. I prefer to get the first one.

neolit123

/lgtm
/approve

should i create a new release after this?

k8s-ci-robot · 2024-10-17T07:57:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: neolit123, pacoxu

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [neolit123]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

pacoxu · 2024-10-17T08:04:25Z

should i create a new release after this?

Yes, as we have no todo items for v1.32.

neolit123 · 2024-10-17T08:05:22Z

unrelated fix, please LGTM

README.md: change the ML for SIG CL #42

will cut a release after it merges and send PR for k/k.

pacoxu · 2024-10-17T08:06:29Z

/unhold
Thanks @KentaTada @neolit123 for your reviewing.

neolit123 · 2024-10-17T08:07:14Z

i will also rename the branch of this repo to main

k8s-ci-robot requested a review from neolit123 September 25, 2024 06:50

pacoxu force-pushed the kernel-config-cgroupv2 branch from 5f9c5bb to c4cf635 Compare September 25, 2024 06:58

neolit123 reviewed Sep 25, 2024

View reviewed changes

validators/types_unix.go Show resolved Hide resolved

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 26, 2024

pacoxu force-pushed the kernel-config-cgroupv2 branch from c4cf635 to e50ca40 Compare September 26, 2024 08:13

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 26, 2024

pacoxu force-pushed the kernel-config-cgroupv2 branch from e50ca40 to 51eb654 Compare September 26, 2024 08:22

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 26, 2024

pacoxu force-pushed the kernel-config-cgroupv2 branch from 51eb654 to 5843314 Compare September 26, 2024 08:33

neolit123 reviewed Sep 26, 2024

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 26, 2024

k8s-ci-robot assigned neolit123 Sep 26, 2024

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 26, 2024

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 15, 2024

pacoxu force-pushed the kernel-config-cgroupv2 branch from 95629f0 to b8d518a Compare October 15, 2024 05:54

neolit123 reviewed Oct 15, 2024

View reviewed changes

pacoxu force-pushed the kernel-config-cgroupv2 branch 3 times, most recently from 4ea6136 to 26b2c98 Compare October 15, 2024 07:19

pacoxu commented Oct 15, 2024

View reviewed changes

validators/cgroup_validator_linux.go Outdated Show resolved Hide resolved

pacoxu force-pushed the kernel-config-cgroupv2 branch 3 times, most recently from c34f1f4 to 0e215f9 Compare October 15, 2024 08:25

neolit123 reviewed Oct 16, 2024

View reviewed changes

pacoxu force-pushed the kernel-config-cgroupv2 branch from 0e215f9 to 8bf7873 Compare October 16, 2024 07:51

KentaTada mentioned this pull request Oct 16, 2024

Add blog article about cgroup v2 kubernetes/website#47342

Closed

neolit123 reviewed Oct 17, 2024

View reviewed changes

parse /proc/mounts for the first cgroup path and add a UT

4a1c1a3

pacoxu force-pushed the kernel-config-cgroupv2 branch from 8bf7873 to 4a1c1a3 Compare October 17, 2024 07:50

neolit123 reviewed Oct 17, 2024

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 17, 2024

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 17, 2024

k8s-ci-robot merged commit c4f0414 into kubernetes:master Oct 17, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add CONFIG_CGROUP_BPF to required kernel config #41

add CONFIG_CGROUP_BPF to required kernel config #41

pacoxu commented Sep 25, 2024

k8s-ci-robot commented Sep 25, 2024

pacoxu commented Sep 25, 2024

neolit123 left a comment

pacoxu commented Sep 26, 2024

pacoxu commented Sep 26, 2024

pacoxu commented Sep 26, 2024

neolit123 commented Sep 26, 2024 •

edited

Loading

pacoxu commented Sep 26, 2024

neolit123 left a comment

neolit123 commented Sep 26, 2024

pacoxu commented Sep 26, 2024 •

edited

Loading

neolit123 commented Sep 26, 2024

KentaTada commented Sep 26, 2024

pacoxu commented Oct 15, 2024

KentaTada commented Oct 16, 2024

pacoxu commented Oct 16, 2024

pacoxu commented Oct 16, 2024 •

edited

Loading

neolit123 left a comment

neolit123 Oct 17, 2024

pacoxu Oct 17, 2024

neolit123 left a comment

k8s-ci-robot commented Oct 17, 2024

pacoxu commented Oct 17, 2024

neolit123 commented Oct 17, 2024 •

edited

Loading

pacoxu commented Oct 17, 2024

neolit123 commented Oct 17, 2024

add CONFIG_CGROUP_BPF to required kernel config #41

add CONFIG_CGROUP_BPF to required kernel config #41

Conversation

pacoxu commented Sep 25, 2024

k8s-ci-robot commented Sep 25, 2024

pacoxu commented Sep 25, 2024

neolit123 left a comment

Choose a reason for hiding this comment

pacoxu commented Sep 26, 2024

pacoxu commented Sep 26, 2024

pacoxu commented Sep 26, 2024

neolit123 commented Sep 26, 2024 • edited Loading

pacoxu commented Sep 26, 2024

neolit123 left a comment

Choose a reason for hiding this comment

neolit123 commented Sep 26, 2024

pacoxu commented Sep 26, 2024 • edited Loading

neolit123 commented Sep 26, 2024

KentaTada commented Sep 26, 2024

pacoxu commented Oct 15, 2024

KentaTada commented Oct 16, 2024

pacoxu commented Oct 16, 2024

pacoxu commented Oct 16, 2024 • edited Loading

neolit123 left a comment

Choose a reason for hiding this comment

neolit123 Oct 17, 2024

Choose a reason for hiding this comment

pacoxu Oct 17, 2024

Choose a reason for hiding this comment

neolit123 left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 17, 2024

pacoxu commented Oct 17, 2024

neolit123 commented Oct 17, 2024 • edited Loading

pacoxu commented Oct 17, 2024

neolit123 commented Oct 17, 2024

neolit123 commented Sep 26, 2024 •

edited

Loading

pacoxu commented Sep 26, 2024 •

edited

Loading

pacoxu commented Oct 16, 2024 •

edited

Loading

neolit123 commented Oct 17, 2024 •

edited

Loading