Skip to content

test(contest): add update pids limit integration test#3595

Merged
saku3 merged 3 commits into
youki-dev:mainfrom
moz-sec:add_pids_limit_test
Jun 27, 2026
Merged

test(contest): add update pids limit integration test#3595
saku3 merged 3 commits into
youki-dev:mainfrom
moz-sec:add_pids_limit_test

Conversation

@moz-sec

@moz-sec moz-sec commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Description

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactoring (no functional changes)
  • Performance improvement
  • Test updates
  • CI/CD related changes
  • Other (please describe):

Testing

  • Added new unit tests
  • Added new integration tests
  • Ran existing test suite
  • Tested manually (please provide steps)

Related Issues

Imprement #3576

Additional Context

As noted in #3594, the test for --update pids-limit fails, so skipped it.

I have confirmed that the test passes in runc.

$ just validate-contest-runc update
sudo RUNTIME_KIND="runc" /home/user/youki/scripts/contest.sh runc update
/usr/sbin/runc
# Start group update
1 / 2 : update_cgroup_v2_common_limits_test : ok
2 / 2 : update_pids_limit_test : ok
# End group update

This integration test was created based on the following description in updates.bats.
https://github.com/opencontainers/runc/blob/da3d022b06dd96a898f6d0cc1df2773df91ea035/tests/integration/update.bats#L330-L359

@moz-sec moz-sec changed the title add update pids limit integration test test(contest): add update pids limit integration test Jun 16, 2026
@moz-sec

moz-sec commented Jun 16, 2026

Copy link
Copy Markdown
Contributor Author

@saku3
Sorry.
The update test is failing during parallel execution due to duplicate cgroup names.
Also, it appears that some of these changes overlap with the changes in #3592.
Therefore, would it be okay to wait until this PR is merged and then modify the program to match that implementation?

Signed-off-by: moz-sec <m0253c@gmail.com>
@moz-sec moz-sec force-pushed the add_pids_limit_test branch from 24b4e29 to 89edaec Compare June 25, 2026 23:47
Signed-off-by: moz-sec <m0253c@gmail.com>
@moz-sec

moz-sec commented Jun 27, 2026

Copy link
Copy Markdown
Contributor Author

@saku3
It looks like #3592 has been merged into main, so I merged it and then extracted it into pids_limit.rs.
Please review it.

@saku3 saku3 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you for the PR.

I left one comment. Please take a look when you have a chance.

.unwrap();
test_result!(check_cgroup_value(&cgroup_path, "pids.max", "max"));

// update pids.limit to 0, pids.max will become 1

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could we explain why pids-limit 0 maps to pids.max = 1?

It seems to follow runc’s behavior, but the reason is not obvious from the current comment.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Set pids.max = 0 in a cgroup, new processes will no longer be created, causing the container to fail to start.
For this reason, runc converts the value of --pids-limit to 1 if it is 0.
This test is designed to match that behavior of runc.

I added this explanation as a comment in pids_limit.rs.

Signed-off-by: moz-sec <m0253c@gmail.com>

@saku3 saku3 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks!

@saku3 saku3 merged commit 2090590 into youki-dev:main Jun 27, 2026
28 checks passed
@github-actions github-actions Bot mentioned this pull request Jun 23, 2026
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.

2 participants