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

centos9+systemd: setting cpu-period resets cpu.idle #3786

Closed
kolyshkin opened this issue Mar 27, 2023 · 3 comments · Fixed by #3788
Closed

centos9+systemd: setting cpu-period resets cpu.idle #3786

kolyshkin opened this issue Mar 27, 2023 · 3 comments · Fixed by #3788

Comments

@kolyshkin
Copy link
Contributor

kolyshkin commented Mar 27, 2023

Description

One of our integration tests is failing on CentOS Stream 9 when using systemd cgroup driver.

What happens is cpu.idle setting is getting reset from 1 to 0 when setting cpu-period.

Steps to reproduce the issue

Minimal repro in bats:

#!/usr/bin/env bats

load helpers

function teardown() {
        teardown_bundle
}

function setup() {
        setup_busybox
        set_cgroups_path
}

@test "update cgroup cpu.idle" {
        requires cgroups_cpu_idle
        [ $EUID -ne 0 ] && requires rootless_cgroup

        runc run -d --console-socket "$CONSOLE_SOCKET" test_update
        [ "$status" -eq 0 ]

        check_cgroup_value "cpu.idle" "0"
        runc update --cpu-idle 1 test_update
        check_cgroup_value "cpu.idle" "1"

        # test update other option won't impact on cpu.idle
        runc update --cpu-period 10000 test_update
        [ "$status" -eq 0 ]
        check_cgroup_value "cpu.idle" "1"
}

Describe the results you received and expected

[root@localhost runc]# export RUNC_USE_SYSTEMD=yes
[root@localhost runc]# bats  tests/integration/u2.bats 
1..1
not ok 1 update cgroup cpu.idle
# (from function `check_cgroup_value' in file tests/integration/helpers.bash, line 265,
#  in test file tests/integration/u2.bats, line 28)
#   `check_cgroup_value "cpu.idle" "1"' failed
# runc spec (status=0):
# 
# runc run -d --console-socket /tmp/bats-run-102847/runc.ooH08F/tty/sock test_update (status=0):
# 
# current 0 !? 0
# runc update --cpu-idle 1 test_update (status=0):
# 
# current 1 !? 1
# runc update --cpu-period 10000 test_update (status=0):
# 
# current 0 !? 1

What version of runc are you using?

[root@localhost runc]# ./runc --version
runc version 1.1.0+dev
commit: e6cc9a67
spec: 1.1.0-rc.1
go: go1.19
libseccomp: 2.5.2

Host OS information

[root@localhost runc]# cat /etc/os-release 
NAME="CentOS Stream"
VERSION="9"
ID="centos"
ID_LIKE="rhel fedora"
VERSION_ID="9"
PLATFORM_ID="platform:el9"
PRETTY_NAME="CentOS Stream 9"
ANSI_COLOR="0;31"
LOGO="fedora-logo-icon"
CPE_NAME="cpe:/o:centos:centos:9"
HOME_URL="https://centos.org/"
BUG_REPORT_URL="https://bugzilla.redhat.com/"
REDHAT_SUPPORT_PRODUCT="Red Hat Enterprise Linux 9"
REDHAT_SUPPORT_PRODUCT_VERSION="CentOS Stream"

Host kernel information

[root@localhost runc]# uname -a
Linux localhost 5.14.0-282.el9.x86_64 #1 SMP PREEMPT_DYNAMIC Thu Feb 23 14:35:36 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
@kolyshkin
Copy link
Contributor Author

And the reason is this PR systemd/systemd#23299 which adds support for cpu.idle. It adds a special value idle for CPUWeight parameter. In case CPUWeight="idle" is set, cpu.idle is set to 1, otherwise it is set to 0.

So, our test case which checks that setting --cpu-period (which is translated to CPUWeight) does not affect cpu.idle fails.

@kolyshkin
Copy link
Contributor Author

This was added to systemd v252. Currently F37 is shipped with systemd v251, and CentOS 9 comes with systemd v252 -- thus we don't see the failure on Fedora.

@kolyshkin
Copy link
Contributor Author

Kernel API (not documented elsewhere): https://lore.kernel.org/all/[email protected]/

kolyshkin added a commit to kolyshkin/runc that referenced this issue Mar 28, 2023
Systemd v252 (available in CentOS Stream 9 in our CI) added support
for setting cpu.idle (see [1]). The way it works is:
 - if CPUWeight == 0, cpu.idle is set to 1;
 - if CPUWeight != 0, cpu.idle is set to 0.

This behavior breaks the existing test case, as described in [2].

To fix, skip the last check in the test case in case a newer systemd
is used.

Fixes: opencontainers#3786

[1] systemd/systemd#23299
[2] opencontainers#3786

Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin added a commit to kolyshkin/runc that referenced this issue Mar 28, 2023
Systemd v252 (available in CentOS Stream 9 in our CI) added support
for setting cpu.idle (see [1]). The way it works is:
 - if CPUWeight == 0, cpu.idle is set to 1;
 - if CPUWeight != 0, cpu.idle is set to 0.

This commit implements setting cpu.idle in systemd cgroup driver via a
unit property. In case CPUIdle is set to non-zero value, the driver sets
adds CPUWeight=0 property, which will result in systemd setting cpu.idle
to 1.

Unfortunately, there's no way to set cpu.idle to 0 without also changing
the CPUWeight value, so the driver doesn't do anything if CPUIdle is
explicitly set to 0. This case is handled by the fs driver which is
always used as a followup to setting systemd unit properties.

Also, handle cpu.idle set via unified map. In case it is set to non-zero
value, add CPUWeight=0 property, and ignore cpu.weight (otherwise we'll
get two different CPUWeight properties set).

Add a unit test for new values in unified map, and an integration test case.

[1] systemd/systemd#23299
[2] opencontainers#3786

Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin added a commit to kolyshkin/runc that referenced this issue Mar 28, 2023
Systemd v252 (available in CentOS Stream 9 in our CI) added support
for setting cpu.idle (see [1]). The way it works is:
 - if CPUWeight == 0, cpu.idle is set to 1;
 - if CPUWeight != 0, cpu.idle is set to 0.

This commit implements setting cpu.idle in systemd cgroup driver via a
unit property. In case CPUIdle is set to non-zero value, the driver sets
adds CPUWeight=0 property, which will result in systemd setting cpu.idle
to 1.

Unfortunately, there's no way to set cpu.idle to 0 without also changing
the CPUWeight value, so the driver doesn't do anything if CPUIdle is
explicitly set to 0. This case is handled by the fs driver which is
always used as a followup to setting systemd unit properties.

Also, handle cpu.idle set via unified map. In case it is set to non-zero
value, add CPUWeight=0 property, and ignore cpu.weight (otherwise we'll
get two different CPUWeight properties set).

Add a unit test for new values in unified map, and an integration test case.

[1] systemd/systemd#23299
[2] opencontainers#3786

Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin added a commit to kolyshkin/runc that referenced this issue Mar 28, 2023
Systemd v252 (available in CentOS Stream 9 in our CI) added support
for setting cpu.idle (see [1]). The way it works is:
 - if CPUWeight == 0, cpu.idle is set to 1;
 - if CPUWeight != 0, cpu.idle is set to 0.

This behavior breaks the existing test case, as described in [2].

To fix, skip the last check in the test case in case a newer systemd
is used.

Fixes: opencontainers#3786

[1] systemd/systemd#23299
[2] opencontainers#3786

Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin added a commit to kolyshkin/runc that referenced this issue Mar 28, 2023
Systemd v252 (available in CentOS Stream 9 in our CI) added support
for setting cpu.idle (see [1]). The way it works is:
 - if CPUWeight == 0, cpu.idle is set to 1;
 - if CPUWeight != 0, cpu.idle is set to 0.

This commit implements setting cpu.idle in systemd cgroup driver via a
unit property. In case CPUIdle is set to non-zero value, the driver sets
adds CPUWeight=0 property, which will result in systemd setting cpu.idle
to 1.

Unfortunately, there's no way to set cpu.idle to 0 without also changing
the CPUWeight value, so the driver doesn't do anything if CPUIdle is
explicitly set to 0. This case is handled by the fs driver which is
always used as a followup to setting systemd unit properties.

Also, handle cpu.idle set via unified map. In case it is set to non-zero
value, add CPUWeight=0 property, and ignore cpu.weight (otherwise we'll
get two different CPUWeight properties set).

Add a unit test for new values in unified map, and an integration test case.

[1] systemd/systemd#23299
[2] opencontainers#3786

Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin added a commit to kolyshkin/runc that referenced this issue Mar 29, 2023
Systemd v252 (available in CentOS Stream 9 in our CI) added support
for setting cpu.idle (see [1]). The way it works is:
 - if CPUWeight == 0, cpu.idle is set to 1;
 - if CPUWeight != 0, cpu.idle is set to 0.

This commit implements setting cpu.idle in systemd cgroup driver via a
unit property. In case CPUIdle is set to non-zero value, the driver sets
adds CPUWeight=0 property, which will result in systemd setting cpu.idle
to 1.

Unfortunately, there's no way to set cpu.idle to 0 without also changing
the CPUWeight value, so the driver doesn't do anything if CPUIdle is
explicitly set to 0. This case is handled by the fs driver which is
always used as a followup to setting systemd unit properties.

Also, handle cpu.idle set via unified map. In case it is set to non-zero
value, add CPUWeight=0 property, and ignore cpu.weight (otherwise we'll
get two different CPUWeight properties set).

Add a unit test for new values in unified map, and an integration test case.

[1] systemd/systemd#23299
[2] opencontainers#3786

Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin added a commit to kolyshkin/runc that referenced this issue Mar 31, 2023
Systemd v252 (available in CentOS Stream 9 in our CI) added support
for setting cpu.idle (see [1]). The way it works is:
 - if CPUWeight == 0, cpu.idle is set to 1;
 - if CPUWeight != 0, cpu.idle is set to 0.

This behavior breaks the existing test case, as described in [2].

To fix, skip the last check in the test case in case a newer systemd
is used.

Fixes: opencontainers#3786

[1] systemd/systemd#23299
[2] opencontainers#3786

Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin added a commit to kolyshkin/runc that referenced this issue Mar 31, 2023
Systemd v252 (available in CentOS Stream 9 in our CI) added support
for setting cpu.idle (see [1]). The way it works is:
 - if CPUWeight == 0, cpu.idle is set to 1;
 - if CPUWeight != 0, cpu.idle is set to 0.

This commit implements setting cpu.idle in systemd cgroup driver via a
unit property. In case CPUIdle is set to non-zero value, the driver sets
adds CPUWeight=0 property, which will result in systemd setting cpu.idle
to 1.

Unfortunately, there's no way to set cpu.idle to 0 without also changing
the CPUWeight value, so the driver doesn't do anything if CPUIdle is
explicitly set to 0. This case is handled by the fs driver which is
always used as a followup to setting systemd unit properties.

Also, handle cpu.idle set via unified map. In case it is set to non-zero
value, add CPUWeight=0 property, and ignore cpu.weight (otherwise we'll
get two different CPUWeight properties set).

Add a unit test for new values in unified map, and an integration test case.

[1] systemd/systemd#23299
[2] opencontainers#3786

Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin added a commit to kolyshkin/runc that referenced this issue Apr 3, 2023
Systemd v252 (available in CentOS Stream 9 in our CI) added support
for setting cpu.idle (see [1]). The way it works is:
 - if CPUWeight == 0, cpu.idle is set to 1;
 - if CPUWeight != 0, cpu.idle is set to 0.

This behavior breaks the existing test case, as described in [2].

To fix, skip the last check in the test case in case a newer systemd
is used.

Fixes: opencontainers#3786

[1] systemd/systemd#23299
[2] opencontainers#3786

Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin added a commit to kolyshkin/runc that referenced this issue Apr 3, 2023
Systemd v252 (available in CentOS Stream 9 in our CI) added support
for setting cpu.idle (see [1]). The way it works is:
 - if CPUWeight == 0, cpu.idle is set to 1;
 - if CPUWeight != 0, cpu.idle is set to 0.

This commit implements setting cpu.idle in systemd cgroup driver via a
unit property. In case CPUIdle is set to non-zero value, the driver sets
adds CPUWeight=0 property, which will result in systemd setting cpu.idle
to 1.

Unfortunately, there's no way to set cpu.idle to 0 without also changing
the CPUWeight value, so the driver doesn't do anything if CPUIdle is
explicitly set to 0. This case is handled by the fs driver which is
always used as a followup to setting systemd unit properties.

Also, handle cpu.idle set via unified map. In case it is set to non-zero
value, add CPUWeight=0 property, and ignore cpu.weight (otherwise we'll
get two different CPUWeight properties set).

Add a unit test for new values in unified map, and an integration test case.

[1] systemd/systemd#23299
[2] opencontainers#3786

Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin added a commit to kolyshkin/runc that referenced this issue Apr 4, 2023
Systemd v252 (available in CentOS Stream 9 in our CI) added support
for setting cpu.idle (see [1]). The way it works is:
 - if CPUWeight == 0, cpu.idle is set to 1;
 - if CPUWeight != 0, cpu.idle is set to 0.

This behavior breaks the existing test case, as described in [2].

To fix, skip the last check in the test case in case a newer systemd
is used.

Fixes: opencontainers#3786

[1] systemd/systemd#23299
[2] opencontainers#3786

Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin added a commit to kolyshkin/runc that referenced this issue Apr 4, 2023
Systemd v252 (available in CentOS Stream 9 in our CI) added support
for setting cpu.idle (see [1]). The way it works is:
 - if CPUWeight == 0, cpu.idle is set to 1;
 - if CPUWeight != 0, cpu.idle is set to 0.

This commit implements setting cpu.idle in systemd cgroup driver via a
unit property. In case CPUIdle is set to non-zero value, the driver sets
adds CPUWeight=0 property, which will result in systemd setting cpu.idle
to 1.

Unfortunately, there's no way to set cpu.idle to 0 without also changing
the CPUWeight value, so the driver doesn't do anything if CPUIdle is
explicitly set to 0. This case is handled by the fs driver which is
always used as a followup to setting systemd unit properties.

Also, handle cpu.idle set via unified map. In case it is set to non-zero
value, add CPUWeight=0 property, and ignore cpu.weight (otherwise we'll
get two different CPUWeight properties set).

Add a unit test for new values in unified map, and an integration test case.

[1] systemd/systemd#23299
[2] opencontainers#3786

Signed-off-by: Kir Kolyshkin <[email protected]>
jiusanzhou pushed a commit to jiusanzhou/runc that referenced this issue Apr 6, 2023
Systemd v252 (available in CentOS Stream 9 in our CI) added support
for setting cpu.idle (see [1]). The way it works is:
 - if CPUWeight == 0, cpu.idle is set to 1;
 - if CPUWeight != 0, cpu.idle is set to 0.

This behavior breaks the existing test case, as described in [2].

To fix, skip the last check in the test case in case a newer systemd
is used.

Fixes: opencontainers#3786

[1] systemd/systemd#23299
[2] opencontainers#3786

Signed-off-by: Kir Kolyshkin <[email protected]>
jiusanzhou pushed a commit to jiusanzhou/runc that referenced this issue Apr 6, 2023
Systemd v252 (available in CentOS Stream 9 in our CI) added support
for setting cpu.idle (see [1]). The way it works is:
 - if CPUWeight == 0, cpu.idle is set to 1;
 - if CPUWeight != 0, cpu.idle is set to 0.

This commit implements setting cpu.idle in systemd cgroup driver via a
unit property. In case CPUIdle is set to non-zero value, the driver sets
adds CPUWeight=0 property, which will result in systemd setting cpu.idle
to 1.

Unfortunately, there's no way to set cpu.idle to 0 without also changing
the CPUWeight value, so the driver doesn't do anything if CPUIdle is
explicitly set to 0. This case is handled by the fs driver which is
always used as a followup to setting systemd unit properties.

Also, handle cpu.idle set via unified map. In case it is set to non-zero
value, add CPUWeight=0 property, and ignore cpu.weight (otherwise we'll
get two different CPUWeight properties set).

Add a unit test for new values in unified map, and an integration test case.

[1] systemd/systemd#23299
[2] opencontainers#3786

Signed-off-by: Kir Kolyshkin <[email protected]>
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 a pull request may close this issue.

1 participant