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

Support running integration tests on cgroupv2 hosts #3143

Merged
merged 2 commits into from Jul 30, 2022

Conversation

bobbypage
Copy link
Collaborator

Adds a few fixes to the integration tests to support running on cgroupv2. Now running make docker-test-integration on a cgroupv2 host should work fine.

@bobbypage bobbypage force-pushed the cg2-e2e branch 2 times, most recently from 7b1cf42 to bdc0932 Compare July 29, 2022 00:12
@bobbypage
Copy link
Collaborator Author

@tstapler ptal

@@ -211,7 +212,15 @@ func TestDockerContainerSpec(t *testing.T) {
assert := assert.New(t)

assert.True(containerInfo.Spec.HasCpu, "CPU should be isolated")
assert.Equal(cpuShares, containerInfo.Spec.Cpu.Limit, "Container should have %d shares, has %d", cpuShares, containerInfo.Spec.Cpu.Limit)
if cgroups.IsCgroup2UnifiedMode() {
// see https://github.com/google/cadvisor/blob/24e7a9883d12f944fd4403861707f4bafcaf4f3d/container/common/helpers.go#L249-L260
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add more context to this comment. Is it pointing out how shares are calculated?

If so, perhaps replace // see with // see how cpu shares are calculated

Copy link
Collaborator Author

@bobbypage bobbypage Jul 29, 2022

Choose a reason for hiding this comment

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

updated comment to be a bit more clear, ptal :)

Copy link
Contributor

Choose a reason for hiding this comment

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

What a beautiful comment :D, thanks.

# otherwise writing subtree_control fails with EBUSY.
# An error during moving non-existent process (i.e., "cat") is ignored.
mkdir -p /sys/fs/cgroup/init
xargs -rn1 < /sys/fs/cgroup/cgroup.procs > /sys/fs/cgroup/init/cgroup.procs || :
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this xargs block do?

I can see that -r is do not run the command if there is no stdin, and -n1 is max args 1? but I don't see a command that its executing.

Copy link
Collaborator Author

@bobbypage bobbypage Jul 29, 2022

Choose a reason for hiding this comment

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

I copied this over from https://github.com/moby/moby/blob/master/hack/dind :)

So the idea here, is that for docker in docker for cgroupv2 we need to be move all the processes in a child cgroup or else we will hit the runc/cgroupv2 limitation mentioned here - containerd/containerd#6659 (comment)

The goal here is to first create a new cgroup (mkdir -p /sys/fs/cgroup/init). Then we want to copy all of pids one by one from /sys/fs/cgroup/cgroup.procs to the new child cgroup /sys/fs/cgroup/init/cgroup.procs (the xargs -rn1 ensures the list of pids is split one by one).

There is no command being passed to xargs, so as a result it will simply echo each pid entry (see https://unix.stackexchange.com/questions/110134/what-does-xargs-do-if-its-used-without-any-parameter)

Copy link
Contributor

Choose a reason for hiding this comment

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

Neat, so xargs allows you to copy the file but throw an error if the source doesn't have any lines in it.

The new comment is helpful thanks!

On cgroupv2, we need to use the helper function to locate the
controller.

Signed-off-by: David Porter <porterdavid@google.com>
Fix a few of the integration tests to be cgroupv2 compatible.

Signed-off-by: David Porter <porterdavid@google.com>
@bobbypage
Copy link
Collaborator Author

Updated based on your feedback, ptal again

@k8s-ci-robot
Copy link
Collaborator

@bobbypage: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-cadvisor-e2e dd027fd link true /test pull-cadvisor-e2e

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Copy link
Contributor

@tstapler tstapler left a comment

Choose a reason for hiding this comment

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

Thanks for adding the extra context

lgtm

@bobbypage bobbypage merged commit 5d6557a into google:master Jul 30, 2022
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.

None yet

3 participants