Skip to content

Remove cgroup v1/v2 only Kernel Config checks - #53

Merged
k8s-ci-robot merged 1 commit into
kubernetes:mainfrom
pacoxu:cr-fix-cgroup
Sep 16, 2025
Merged

Remove cgroup v1/v2 only Kernel Config checks#53
k8s-ci-robot merged 1 commit into
kubernetes:mainfrom
pacoxu:cr-fix-cgroup

Conversation

@pacoxu

@pacoxu pacoxu commented Sep 16, 2025

Copy link
Copy Markdown
Member

https://man7.org/linux/man-pages/man7/cgroups.7.html (search in Linux 5.2 and earlier).)

#51 (comment)

To make it cherry-pickable, I opened this PR.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. labels Sep 16, 2025
@k8s-ci-robot k8s-ci-robot added sig/node Categorizes an issue or PR as relevant to SIG Node. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 16, 2025
Comment thread validators/types_unix.go Outdated
Comment on lines +61 to +65
{Name: "CGROUP_BPF"}, // cgroups v2
{Name: "CGROUP_CPUACCT"}, // cgroups v1 cpuacct
{Name: "CGROUP_DEVICE"}, // cgroups v1 device
{Name: "CGROUP_FREEZER"}, // cgroups v1 freezer
{Name: "CGROUP_HUGETLB", Description: "Required for hugetlb cgroup."}, // cgroups v1 hugetlb

@neolit123 neolit123 Sep 16, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding these under optional it's still incorrect. it will warn for about options required (optional=warn) for both v1 and v2.

we definitely need the other PR.

but if we are backporting something i think it's better to remove them from config options and rely on the controller checks only. because controller enabled == kernel config enabled, right?
#51 (comment)

if we don't want to backport anything we could say it's a known bug and that they must skip preflight.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

Backport may be helpful to avoid some errors. But #41 was merged for system validator v1.9.0 which is kubernetes v1.32. (released in Dec 2024.)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iiuc, we need to backport this commit to multiple sv and k8s versions.

i can cut the new sv releases, but would leave the backports to you.

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

@neolit123 neolit123 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you are going with removing the options, best to rename the commit and pr name.

@pacoxu pacoxu changed the title mark cgroup v1/v2 only Kernel Config as optional Remove cgroup v1/v2 only Kernel Config checks Sep 16, 2025
@pacoxu

pacoxu commented Sep 16, 2025

Copy link
Copy Markdown
Member Author

/hold

Let me confirm this again.

@k8s-ci-robot 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 16, 2025
@pacoxu

pacoxu commented Sep 16, 2025

Copy link
Copy Markdown
Member Author

if you are going with removing the options, best to rename the commit and pr name.

This was addressed.

If we backport this, it means users can pass the check without CGROUP_BPF for cgroup v2; and can pass without those CPUACCT/DEVICE/FREEZER for cgroup v1. Is that a bigger issue?

@pacoxu

pacoxu commented Sep 16, 2025

Copy link
Copy Markdown
Member Author

if you are going with removing the options, best to rename the commit and pr name.

This was addressed.

If we backport this, it means users can pass the check without CGROUP_BPF for cgroup v2; and can pass without those CPUACCT/DEVICE/FREEZER for cgroup v1. Is that a bigger issue?

Sorry. My fault. I forgot the cgroup controller checks.

It would be OK.

/unhold

@k8s-ci-robot 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 16, 2025
@pacoxu

pacoxu commented Sep 16, 2025

Copy link
Copy Markdown
Member Author

I created a CR #54 here. [EDITED, wrong PR number]

@neolit123

neolit123 commented Sep 16, 2025

Copy link
Copy Markdown
Member

I created a CR #53 here.

i'm getting a bit confused.

i think we need to do this:

  • have a single PR / commit that removes all v1 and v2 options, leaves only {Name: "CGROUPS"},
  • we merge it in main here and backport it to SV versions, then backport it to k/k
  • it makes sure users of v1 or v2 don't get any errors if they are missing options
  • the other bigger PR can split the config options, we release a new MINOR here and add it to the latest k/k release

does that plan make sense?

@pacoxu

pacoxu commented Sep 16, 2025

Copy link
Copy Markdown
Member Author

I created a CR #53 here.

i'm getting a bit confused.

i think we need to do this:

  • have a single PR / commit that removes all v1 and v2 options, leaves only {Name: "CGROUPS"},
  • we merge it in main here and backport it to SV versions, then backport it to k/k
  • it makes sure users of v1 or v2 don't get any errors if they are missing options
  • the other bigger PR can split the config options, we release a new MINOR here and add it to the latest k/k release

does that plan make sense?

Totally agree.

@pacoxu

pacoxu commented Sep 16, 2025

Copy link
Copy Markdown
Member Author

Updated this PR and the CR #54

@neolit123 neolit123 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 16, 2025
@k8s-ci-robot

Copy link
Copy Markdown
Contributor

[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

Details Needs approval from an approver in each of these files:

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

@k8s-ci-robot
k8s-ci-robot merged commit 3d931b9 into kubernetes:main Sep 16, 2025
3 checks passed
@neolit123

neolit123 commented Sep 16, 2025

Copy link
Copy Markdown
Member

Updated this PR and the CR #54

i will cut 1.9.2 soon

@neolit123

Copy link
Copy Markdown
Member

@pacoxu

pacoxu commented Sep 16, 2025

Copy link
Copy Markdown
Member Author

1.9.2 https://github.com/kubernetes/system-validators/releases/tag/v1.9.2

do we not need this in 1.10 as well?

Yes @neolit123 v1.34 is using v1.10.1

@neolit123

Copy link
Copy Markdown
Member

1.9.2 https://github.com/kubernetes/system-validators/releases/tag/v1.9.2
do we not need this in 1.10 as well?

Yes @neolit123 v1.34 is using v1.10.1

ok please send CR for release-1.10 and will create a new release there too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/node Categorizes an issue or PR as relevant to SIG Node. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants