Skip to content

cgroup v1 and v2 kernel config check according to isCgroupV2 #52

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Sep 17, 2025
Merged

cgroup v1 and v2 kernel config check according to isCgroupV2 #52

merged 1 commit into from
Sep 17, 2025
+42 −2

Conversation

pacoxu
Copy link
Member

@pacoxu pacoxu commented Sep 15, 2025 •
edited
Loading

@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 15, 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/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 15, 2025
@pacoxu
Copy link
Member Author

pacoxu commented Sep 15, 2025

/cc @neolit123 @KentaTada

@k8s-ci-robot
Copy link
Contributor

@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:

/cc @neolit123 @KentaTada

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.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 15, 2025
@pacoxu pacoxu marked this pull request as draft September 15, 2025 06:47
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 15, 2025
@pacoxu pacoxu force-pushed the fix-cgroup-v1-v2 branch 2 times, most recently from a0481d6 to c917173 Compare September 15, 2025 06:58
@pacoxu pacoxu marked this pull request as ready for review September 15, 2025 06:58
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 15, 2025
neolit123
neolit123 reviewed Sep 15, 2025
View reviewed changes
Copy link
Member

@neolit123 neolit123 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

thanks for the PR @pacoxu

given we are adding new fields it means we cannot backport the fix to older SV releases...(according to the k8s backport rules at least...)
this solution can be backported but it's less ideal i guess
#51 (comment)

up to you to decide what is better here.

validators/cgroup_validator_other.go
}

// getUnifiedMountpoint is a no-op for non-Linux OSes.
func getUnifiedMountpoint(path string) (string, bool, error) {
Copy link
Member

Choose a reason for hiding this comment

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

hm, how did this compile before?
weren't we supposed to get an error if this function was missing on non-Linux?

Copy link
Member Author

Choose a reason for hiding this comment

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

getUnifiedMountpoint is implemented in linux only and was only used in that linux go as well.
However, we want to use it in validators/kernel_validator.go in this PR.

  • Adding new validators/kernel_validator_linux.go and validators/kernel_validator_other.go is too complex. So I added this here.

@neolit123 neolit123 mentioned this pull request Sep 16, 2025
Merged
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 16, 2025
@neolit123 neolit123 mentioned this pull request Sep 17, 2025
Merged
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 17, 2025
neolit123
neolit123 reviewed Sep 17, 2025
View reviewed changes
validators/cgroup_validator_other.go

var _ Validator = &CgroupsValidator{}

const mountsFilePath = ""
Copy link
Member

Choose a reason for hiding this comment

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

this is not needed? the definition of mountsFilePath is only required in the linux file, as far as i can tell.

Copy link
Member

Choose a reason for hiding this comment

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

never mind, it's referenced in validators/kernel_validator.go

neolit123
neolit123 reviewed Sep 17, 2025
View reviewed changes
Copy link
Member

@neolit123 neolit123 left a comment

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 17, 2025
@k8s-ci-robot
Copy link
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

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 ce18350 into kubernetes:main Sep 17, 2025
3 checks passed
@neolit123
Copy link
Member

new release and branch
https://github.com/kubernetes/system-validators/releases/tag/v1.11.0
https://github.com/kubernetes/system-validators/tree/release-1.11

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

Reviewers

@neolit123 neolit123 neolit123 left review comments

@carlory carlory Awaiting requested review from carlory

@SataQiu SataQiu Awaiting requested review from SataQiu

Assignees

@neolit123 neolit123

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/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[need-info] Does kubeadm require CONFIG_CGROUP_CPUACCT/DEVICE/FREEZER even with cgroups v2?

3 participants

@pacoxu @k8s-ci-robot @neolit123