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

script/check-config.sh: fixes for cgroupv2 and v5.x kernels #2729

Merged
merged 5 commits into from
Jan 28, 2021

Conversation

kolyshkin
Copy link
Contributor

This fixes script/check-config.sh to

  • not produce false failures on cgroupv2;
  • not check removed options on recent 5.x kernels.

See individual commits for details.

@kolyshkin kolyshkin force-pushed the check-config branch 2 times, most recently from 1fe94e3 to 85a687e Compare January 11, 2021 20:37
Copy link
Member

@tianon tianon left a comment

Choose a reason for hiding this comment

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

Generally LGTM, just one minor nit 👍 ❤️

cgroupDir="$(dirname "$cgroupSubsystemDir")"
if [ -d "$cgroupDir/cpu" -o -d "$cgroupDir/cpuacct" -o -d "$cgroupDir/cpuset" -o -d "$cgroupDir/devices" -o -d "$cgroupDir/freezer" -o -d "$cgroupDir/memory" ]; then
echo "$(wrap_good 'cgroup hierarchy' 'properly mounted') [$cgroupDir]"
if [[ $(stat -f -c %t /sys/fs/cgroup 2>/dev/null) = "63677270" ]]; then
Copy link
Member

Choose a reason for hiding this comment

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

[[ isn't necessary here; technically we should do $() in a separate command to catch the failure, but in this case we don't actually care about the failure (and the else should cover it) so it's fine:

Suggested change
if [[ $(stat -f -c %t /sys/fs/cgroup 2>/dev/null) = "63677270" ]]; then
if [ "$(stat -f -c %t /sys/fs/cgroup 2>/dev/null)" = '63677270' ]; then

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, makes sense (I am unsure why I used [[ in the first place).

Fixed.

@tianon
Copy link
Member

tianon commented Jan 11, 2021

(Sorry, saw check-config.sh and assumed this was moby/moby and didn't realize it was runc until I had already commented 🙈 Hopefully forgivable since I'm the original author of the first version of this script? 😇 ❤️ Feel free to ignore me if maintainers here don't agree. 👍)

Before:

> Generally Necessary:
> - cgroup hierarchy: nonexistent??
>     (see https://github.com/tianon/cgroupfs-mount)

After:

> Generally Necessary:
> - cgroup hierarchy: cgroupv2

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
CONFIG_NF_NAT_IPV4 was removed in kernel commit 3bf195ae6037e310,
which made its way into v5.1-rc1. The functionality is now under
NF_NAT which we already check for.

Make the check for NF_NAT_IPV4 conditional.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
CONFIG_NF_NAT_NEEDED was removed in kernel commit 4806e975729f99c7,
which made its way into v5.2-rc1. The functionality is now under
NF_NAT which we already check for.

Make the check for NF_NAT_NEEDED conditional.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Kernel commit 2d1c498072de69e (which made its way into kernel v5.8-rc1)
removed CONFIG_MEMCG_SWAP_ENABLED Kconfig option, making swap accounting
always enabled (unless swapaccount=0 boot option is provided).

Make the check conditional.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
These config options are removed by kernel commit f382fb0bcef4,
which made its way into kernel v5.0-rc1.

Make the check conditional.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin
Copy link
Contributor Author

CI failure is #2425, fixed by #2723. Restarting...

=== RUN   TestExecInTTY
836
    execin_test.go:349: unexpected carriage-return in output "PID   USER     TIME  COMMAND\r\n    1 root      0:00 cat\r\n    6 root      0:00 ps\r\n"
837
--- FAIL: TestExecInTTY (0.10s)

if [ -d "$cgroupDir/cpu" -o -d "$cgroupDir/cpuacct" -o -d "$cgroupDir/cpuset" -o -d "$cgroupDir/devices" -o -d "$cgroupDir/freezer" -o -d "$cgroupDir/memory" ]; then
echo "$(wrap_good 'cgroup hierarchy' 'properly mounted') [$cgroupDir]"
if [ "$(stat -f -c %t /sys/fs/cgroup 2>/dev/null)" = "63677270" ]; then
echo "$(wrap_good 'cgroup hierarchy' 'cgroupv2')"
Copy link
Member

Choose a reason for hiding this comment

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

I think we need more validations for cgroup2, but it can be worked out later, so LGTM

@mrunalp mrunalp merged commit 8bbfde8 into opencontainers:master Jan 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants