Skip to content

tests/int: swap-related fixes#4188

Merged
lifubang merged 5 commits intoopencontainers:mainfrom
kolyshkin:ci-swap
Jun 7, 2024
Merged

tests/int: swap-related fixes#4188
lifubang merged 5 commits intoopencontainers:mainfrom
kolyshkin:ci-swap

Conversation

@kolyshkin
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

kolyshkin commented Mar 18, 2024

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
Copy Markdown
Contributor Author

kolyshkin commented Mar 18, 2024

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
Copy Markdown
Member

lifubang commented Mar 25, 2024

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
Copy Markdown
Contributor Author

kolyshkin commented Mar 26, 2024

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
Copy Markdown
Contributor Author

kolyshkin commented May 13, 2024

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
Copy Markdown
Member

lifubang commented May 16, 2024

PTAL

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

lifubang
lifubang requested changes May 19, 2024
View reviewed changes
Comment thread libcontainer/cgroups/fs2/memory.go
@kolyshkin kolyshkin added this to the 1.2.0 milestone Jun 6, 2024
kolyshkin added 5 commits June 6, 2024 18:52
@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 <kolyshkin@gmail.com>
@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 <kolyshkin@gmail.com>
@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 <kolyshkin@gmail.com>
@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 <kolyshkin@gmail.com>
@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 <kolyshkin@gmail.com>
@lifubang lifubang merged commit a4b0857 into opencontainers:main Jun 7, 2024
@lifubang
Copy link
Copy Markdown
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

@lifubang lifubang lifubang approved these changes

@AkihiroSuda AkihiroSuda AkihiroSuda 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