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

libct/cgroups: support Cgroups.Resources.Unified #2584

Merged
merged 5 commits into from
Sep 26, 2020

Conversation

kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Sep 11, 2020

Add support for unified resource map (as per [1]), and add some test
cases for the new functionality.

[1] opencontainers/runtime-spec#1040

Fixes: #2563

The biggest issue, I think, is that the mapping of the "unified" parameters
to systemd properties is not implemented, meaning there can be cases
where systemd unit settings and in-kernel cgroup settings won't agree.

I don't think it can cause any issues, but it's just doesn't look right.

@kolyshkin
Copy link
Contributor Author

That intermittent download failure, again

 ---> b2ec3d2c2048
Step 13/23 : RUN curl -o /usr/local/bin/umoci -fsSL https://github.com/opencontainers/umoci/releases/download/v0.4.5/umoci.amd64     && chmod +x /usr/local/bin/umoci
 ---> Running in b2823a13a789
curl: (22) The requested URL returned error: 429 too many requests
The command '/bin/sh -c curl -o /usr/local/bin/umoci -fsSL https://github.com/opencontainers/umoci/releases/download/v0.4.5/umoci.amd64     && chmod +x /usr/local/bin/umoci' returned a non-zero code: 22
Makefile:62: recipe for target 'runcimage' failed

Wonder if there's a better alternative. For some reason, this never happens to any other URLs downloaded from github

@kolyshkin
Copy link
Contributor Author

Rebased

... to include unified resources support.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
As the underlying error message from iotuils.WriteFile already contains
file name, there's no need to put it, otherwise we end up with something
like:

	failed to write "val" to "/sys/fs/cgroup/.../file": open /sys/fs/cgroup/.../file: permission denied

With this patch, the error will be

	failed to write "val": open /sys/fs/cgroup/.../file: permission denied

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

@giuseppe @mrunalp PTAL

@kolyshkin
Copy link
Contributor Author

Added bats test with more unified parameters (using only resources.unified), and libct/int test case using read-only file (pids.current) as a key to check for error.

AkihiroSuda
AkihiroSuda previously approved these changes Sep 21, 2020
@AkihiroSuda
Copy link
Member

@giuseppe PTAL?

Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

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

just a question, otherwise LGTM

libcontainer/cgroups/fs2/fs2.go Outdated Show resolved Hide resolved
@giuseppe
Copy link
Member

LGTM

mrunalp
mrunalp previously approved these changes Sep 24, 2020
Copy link
Contributor

@mrunalp mrunalp left a comment

Choose a reason for hiding this comment

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

We need follow up for systemd but this looks like a good start. Thanks!

Add support for unified resource map (as per [1]), and add some test
cases for the new functionality.

[1] opencontainers/runtime-spec#1040

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Container test_cgroups_group was missing from the teardown.

Fixes: c91fe9a.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Note that various invalid keys are covered by test cases in
libcontainer/integration/exec_test.go

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

mrunalp commented Sep 25, 2020

@AkihiroSuda ptal

@AkihiroSuda AkihiroSuda merged commit 6e5320f into opencontainers:master Sep 26, 2020
@AkihiroSuda
Copy link
Member

@kolyshkin Could you open an issue for tracking systemd issue?

@kolyshkin
Copy link
Contributor Author

Could you open an issue for tracking systemd issue?

I'm not quite sure which way to go -- let's discuss in there

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

Successfully merging this pull request may close these issues.

Support native cgroupv2 spec
4 participants