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.

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

libct/cg/v1: work around CPU quota period set failure #3090

Merged
merged 2 commits into from
Aug 30, 2021
Merged

libct/cg/v1: work around CPU quota period set failure #3090

merged 2 commits into from
Aug 30, 2021

Conversation

kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Jul 14, 2021 •
edited

This is based on (and currently includes) #3175.

I. As reported in issue #3084, sometimes setting CPU quota period fails
when a new period is lower and a parent cgroup has CPU quota limit set.

This happens as in cgroup v1 the quota and the period can not be set
together (this is fixed in v2), and since the period is being set first,
new_limit = old_quota/new_period may be higher than the parent
cgroup limit.

The fix is to retry setting the period after the quota, to cover all
possible scenarios.

Add a test case to cover a regression caused by an earlier version of
this patch (ignoring a failure of setting invalid period when quota is
not set).

Fixes: #3084
Closes: #2044

II. Add a test case for an issue fixed by the previous commit.

Unfortunately, this is somewhat complicated as there's no easy way to
create a transient unit, so a binary, sd-helper, had to be added. On top
of that, an ability to create a parent/pod cgroup is added to
helpers.bash, which might be useful for future integration tests.

History

1.0 backport: #3115

@kolyshkin
Copy link
Contributor Author

A repro (which took me a while to write) fails as expected!

time="2021-07-15T01:24:25Z" level=error msg="failed to write "10000": write /sys/fs/cgroup/cpu,cpuacct/runc-cgroups-integration-test/pod_31883/test-cgroup-14305/cpu.cfs_period_us: invalid argument"
152

Adding the fix now.

@kolyshkin kolyshkin force-pushed the cfq_quota_period branch 3 times, most recently from 4d77f91 to 8dac477 Compare July 15, 2021 19:04
@kolyshkin kolyshkin changed the title [WIP] fix for #3084 [WIP] libct/cg/v1: workaround CPU quota period set failure Jul 15, 2021
@kolyshkin
Copy link
Contributor Author

Don't know how to create a transient scope from bash :( and this is required for centos 7's systemd.

@kolyshkin
Copy link
Contributor Author

Don't know how to create a transient scope from bash :( and this is required for centos 7's systemd.

In theory, I should be able to use dbus-send to call org.freedesktop.systemd1.Manager.StartTransientUnit but I can't figure out a way to.

Another way is to use libcontainer and have a small go binary to do that.

@kolyshkin kolyshkin mentioned this pull request Jul 16, 2021
Closed
@kolyshkin kolyshkin force-pushed the cfq_quota_period branch 3 times, most recently from b284685 to 956b9a8 Compare July 20, 2021 02:46
@kolyshkin
Copy link
Contributor Author

Another way is to use libcontainer and have a small go binary to do that.

Trying it as I can't think of any better way right now.

@kolyshkin kolyshkin force-pushed the cfq_quota_period branch 8 times, most recently from 5e6f41f to c1ebddf Compare July 20, 2021 23:21
@kolyshkin kolyshkin marked this pull request as ready for review July 20, 2021 23:42
@kolyshkin kolyshkin changed the title [WIP] libct/cg/v1: workaround CPU quota period set failure libct/cg/v1: workaround CPU quota period set failure Jul 21, 2021
@kolyshkin kolyshkin added the backport/todo/1.0 A PR in main branch which needs to be backported to release-1.0 label Jul 28, 2021
@kolyshkin kolyshkin mentioned this pull request Jul 28, 2021
Merged
@kolyshkin kolyshkin added this to To do in PRs to review Jul 29, 2021
@kolyshkin kolyshkin added this to the 1.1.0 milestone Aug 11, 2021
@kolyshkin
Copy link
Contributor Author

Ughm; the backport (#3115) is already merged, and this one has no LGTMs :-\ @opencontainers/runc-maintainers PTAL 🙏🏻

@kolyshkin kolyshkin mentioned this pull request Aug 12, 2021
Closed
@kolyshkin
Copy link
Contributor Author

@opencontainers/runc-maintainers PTAL 🙏🏻

thaJeztah
thaJeztah reviewed Aug 19, 2021
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

Didn't do a full review yet, but left a comment.

Also looks like the last commit is somewhat unrelated (could make sense separately)

contrib/cmd/sd-helper/helper.go Outdated
Comment on lines 16 to 22
// version will be populated by the Makefile, read from
// VERSION file of the source code.
var version = ""

// gitCommit will be the hash that the binary was built from
// and will be populated by the Makefile.
var gitCommit = ""
Copy link
Member

Choose a reason for hiding this comment

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

Will this utility only be used for tests? Wondering if adding version information is a bit too much for that (or even using urfave/cli for this).

Reason I'm looking at the version variable, is also that if we want to switch to //go:embed (#3163), the path to files to embed cannot traverse to parent directories (so //go:embed ../../VERSION won't work) 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just copied this bit (as well as using urlfave/cli) from recvtty.

Let me try to drop both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, PTAL @thaJeztah

@kolyshkin
Copy link
Contributor Author

Also looks like the last commit is somewhat unrelated (could make sense separately)

That's right, but it's harder to work on it if it's separate (since other commits in this PR also change the same file), and I don't want to forget about it.

Let me try to rework it so the shellcheck fixes are separate.

@kolyshkin
Copy link
Contributor Author

Let me try to rework it so the shellcheck fixes are separate.

Those fixes separated out to #3175 but this one depends on (and currently includes) it.

We can either review/merge #3175 first, and then I'll rebase, or merge this one as is and close #3175.

@kolyshkin kolyshkin mentioned this pull request Aug 23, 2021
Merged
@kolyshkin
libct/cg/v1: workaround CPU quota period set failure
1b2adcf 
As reported in issue 3084, sometimes setting CPU quota period fails
when a new period is lower and a parent cgroup has CPU quota limit set.

This happens as in cgroup v1 the quota and the period can not be set
together (this is fixed in v2), and since the period is being set first,
new_limit = old_quota/new_period may be higher than the parent cgroup
limit.

The fix is to retry setting the period after the quota, to cover all
possible scenarios.

Add a test case to cover a regression caused by an earlier version of
this patch (ignoring a failure of setting invalid period when quota is
not set).

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin
tests/int: add a "update cpu period with pod limit set" test
6394457 
Add a test case for an issue fixed by the previous commit.

Unfortunately, this is somewhat complicated as there's no easy way to
create a transient unit, so a binary, sd-helper, had to be added. On top
of that, an ability to create a parent/pod cgroup is added to
helpers.bash, which might be useful for future integration tests.

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

Rebased on top of merged #3175, PTAL @thaJeztah

thaJeztah
thaJeztah approved these changes Aug 27, 2021
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@kolyshkin
Copy link
Contributor Author

This fixes a real bug, and the backport to 1.0 was already merged and released in 1.0.2. @opencontainers/runc-maintainers PTAL

PRs to review automation moved this from To do to Reviewer approved Aug 30, 2021
@AkihiroSuda AkihiroSuda merged commit 11d141b into opencontainers:master Aug 30, 2021
PRs to review automation moved this from Reviewer approved to Done Aug 30, 2021
@kolyshkin kolyshkin added backport/done/1.0 A PR in main branch which was backported to release-1.0 and removed backport/todo/1.0 A PR in main branch which needs to be backported to release-1.0 labels Nov 30, 2021
@kolyshkin
Copy link
Contributor Author

For the reference, the same fix for crun: containers/crun#1188

@kolyshkin kolyshkin mentioned this pull request Apr 25, 2024
Merged
@kolyshkin kolyshkin mentioned this pull request May 21, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hk-vmg hk-vmg hk-vmg left review comments

@thaJeztah thaJeztah thaJeztah approved these changes

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

@cyphar cyphar Awaiting requested review from cyphar

@mrunalp mrunalp Awaiting requested review from mrunalp

Assignees
No one assigned
Labels
area/cgroupv1 area/ci area/systemd backport/done/1.0 A PR in main branch which was backported to release-1.0 kind/bug
Projects
PRs to review
Done
Milestone
1.1.0
Development

Successfully merging this pull request may close these issues.

Reducing CPU period fails for subsystems if existing parent has quota>0 with systemd driver
4 participants
@kolyshkin @thaJeztah @AkihiroSuda @hk-vmg