Skip to content
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.

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

cgroups: Fix race to load cgroup.hostRoot option #27561

Merged
merged 1 commit into from Aug 21, 2023

Conversation

kvaps
Copy link
Contributor

@kvaps kvaps commented Aug 17, 2023

While loading, the package pkg/cgroups/manager sets the global variable cgroupRoot from the pkg/cgroups package.
This occurs before the configuration is fully loaded and the correct path is set in the pkg/cgroups package.

This variable is later used by the validateCgroupPath function.
As a result, the validateCgroupPath function always works with the default value instead of the user-defined one.

While using --set cgroup.autoMount.enabled=false and --set cgroup.hostRoot=/sys/fs/cgroup options, this might lead strange errors like:

level=info msg="  --cgroup-root='/sys/fs/cgroup'" subsys=daemon
level=info msg="Mounted cgroupv2 filesystem at /sys/fs/cgroup" subsys=cgroups
level=warning msg="No valid cgroup base path found: socket load-balancing tracing with Hubble will not work.See the kubeproxy-free guide for more details." subsys=cgroup-manager

And path /sys/fs/cgroup/kubepods correctly existing.

cgroups: Fix race to load cgroup.hostRoot option

@kvaps kvaps requested a review from a team as a code owner August 17, 2023 10:44
@kvaps kvaps requested a review from aspsk August 17, 2023 10:44
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Aug 17, 2023
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Aug 17, 2023
@kvaps kvaps force-pushed the fix-cgroup-loader branch 2 times, most recently from d4982cd to 0d024c9 Compare August 17, 2023 10:53
@aspsk
Copy link
Contributor

aspsk commented Aug 17, 2023

Thanks @kvaps, the change looks great! However, it breaks the pkg/cgroups/manager/manager_test.go and the pkg/cgroups/manager/provider_test.go tests, as they use the variable you've removed. Could you please patch the tests as well?

@aspsk aspsk added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Aug 17, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Aug 17, 2023
@kvaps
Copy link
Contributor Author

kvaps commented Aug 17, 2023

Yeah, sorry, added it to provider_test.go

@aspsk
Copy link
Contributor

aspsk commented Aug 17, 2023

@kvaps thanks. Could you also please rebase on top of main?

While loading, the package 'pkg/cgroups/manager' sets the global variable 'cgroupRoot' from the 'pkg/cgroups' package.
This occurs before the configuration is fully loaded and the correct path is set in the 'pkg/cgroups' package.

This variable is later used by the 'validateCgroupPath' function.
As a result, the 'validateCgroupPath' function always works with the default value instead of the user-defined one.

Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
@kvaps
Copy link
Contributor Author

kvaps commented Aug 17, 2023

Sure, done

@aspsk
Copy link
Contributor

aspsk commented Aug 18, 2023

/test

Copy link
Contributor

@aspsk aspsk left a comment

Choose a reason for hiding this comment

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

Thanks!

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Aug 18, 2023
@joestringer joestringer added needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch affects/v1.13 This issue affects v1.13 branch affects/v1.14 This issue affects v1.14 branch labels Aug 21, 2023
@joestringer joestringer merged commit 603235d into cilium:main Aug 21, 2023
60 checks passed
@tklauser tklauser mentioned this pull request Aug 22, 2023
22 tasks
@tklauser tklauser added backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. and removed needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Aug 22, 2023
@joestringer joestringer added backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. and removed backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. labels Aug 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects/v1.13 This issue affects v1.13 branch affects/v1.14 This issue affects v1.14 branch backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. kind/community-contribution This was a contribution made by a community member. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants