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

tests/int: swap-related fixes #4188

Merged
merged 5 commits into from
Jun 7, 2024
Merged

tests/int: swap-related fixes #4188

merged 5 commits into from
Jun 7, 2024

Conversation

kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Feb 2, 2024 •
edited
Loading

  • Fixes a few minor issues with integration tests.
  • cgroup v2: do not set swap to 0 or unlimited when it's not available.

See individual commits for details.

Fixes #4166

@kolyshkin kolyshkin mentioned this pull request Feb 2, 2024
Closed
@lifubang lifubang mentioned this pull request Mar 3, 2024
Closed
lifubang
lifubang requested changes Mar 3, 2024
View reviewed changes
Copy link
Member

@lifubang lifubang left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Sorry, when we set memory to -1 in cgroupv2, it means set both memory and swap limit to max:

runc/tests/integration/update.bats

Lines 119 to 121 in a837eeb

# try to remove memory limit
runc update test_update --memory -1
[ "$status" -eq 0 ]

@kolyshkin
Copy link
Contributor Author

Sorry, when we set memory to -1 in cgroupv2, it means set both memory and swap limit to max:

@lifubang I'm not sure why this comment is related to this PR. In here I am just fixing some issues with the test.

@kolyshkin
Copy link
Contributor Author

Sorry, when we set memory to -1 in cgroupv2, it means set both memory and swap limit to max:

runc/tests/integration/update.bats

Lines 119 to 121 in a837eeb

# try to remove memory limit
runc update test_update --memory -1
[ "$status" -eq 0 ]

Indeed it does remove both memory and swap limits in v2, and the test is checking it (see commit commit 18ebc51).

IOW it's not a bug, it's a feature.

(and I still don't understand how this question relates to this PR)

@lifubang
Copy link
Member

and I still don't understand how this question relates to this PR

Because with your patch, I still can get an error when test this case.

This is because:
https://github.com/opencontainers/runc/blob/main/libcontainer/cgroups/utils.go#L414-L420
--memory -1 means set memory=max & swap=max.

so, need to move this test to the if condition check:

runc/tests/integration/update.bats

Line 120 in a837eeb

runc update test_update --memory -1

@kolyshkin
Copy link
Contributor Author

and I still don't understand how this question relates to this PR

Because with your patch, I still can get an error when test this case.

Thanks, I think I understand it now. Added two more commits, untested locally, please give it a try @lifubang.

@kolyshkin kolyshkin requested a review from lifubang March 27, 2024 05:01
@kolyshkin
Copy link
Contributor Author

and I still don't understand how this question relates to this PR

Because with your patch, I still can get an error when test this case.

Thanks, I think I understand it now. Added two more commits, untested locally, please give it a try @lifubang.

@lifubang PTAL

@lifubang
Copy link
Member

PTAL

I'll check it later, once I'm sitting in the front of my PC with an old kernel.

@kolyshkin kolyshkin added this to the 1.2.0 milestone Jun 6, 2024
@kolyshkin @lifubang
tests/int/update: fix v2 swap check
e530b2a 
If swap is disabled, we should not run swap tests.

Fixes 4166.

Signed-off-by: Kir Kolyshkin <[email protected]>
@kolyshkin @lifubang
tests/int: fixup find statements
8626c71 
1. Add "-maxdepth 2" to not dive too deep into cgroup hierarchy.

2. Add "-type f" to look for a regular file.

Signed-off-by: Kir Kolyshkin <[email protected]>
@kolyshkin @lifubang
tests/int/helpers: fix cgroups_swap check for v2
dbb011e 
In case of cgroup v2, there's no memory.swap.max in the top-level
cgroup, so we have to use find.

Signed-off-by: Kir Kolyshkin <[email protected]>
@kolyshkin @lifubang
libct/cg/fs/v2: ignore setting swap in some cases
4209439 
When swap is being disabled (as set to 0), or set to max, ignore
non-existent memory.swap.max cgroup file.

If swap is being set explicitly to some value, do return an error like
before.

Signed-off-by: Kir Kolyshkin <[email protected]>
@kolyshkin @lifubang
tests/cgroups: separate cgroup v2 swap test
3083bd4 
There are cgroup v2 systems out there that do not have cgroup swap enabled,
and this test will probably fail in there.

Move it to a separate case, guarded with requires cgroups_swap.

Signed-off-by: Kir Kolyshkin <[email protected]>
@lifubang lifubang merged commit a4b0857 into opencontainers:main Jun 7, 2024
39 checks passed
@lifubang
Copy link
Member

lifubang commented Jun 7, 2024

Maybe the forth commit(4209439) should be backported to release-1.1?

@lifubang lifubang mentioned this pull request Jun 10, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

@lifubang lifubang lifubang approved these changes

@cyphar cyphar Awaiting requested review from cyphar

Assignees
No one assigned
Labels
area/cgroupv2 area/ci
Projects
None yet
Milestone
1.2.0
Development

Successfully merging this pull request may close these issues.

[ci] We should check memory.swap.max exists or not for cgroupv2
3 participants
@kolyshkin @lifubang @AkihiroSuda