Skip to content

add process rlimits fail test#3051

Merged
YJDoc2 merged 5 commits intoyouki-dev:mainfrom
ntkm61027:add-test-process-rlimits-fail
Jan 16, 2025
Merged

add process rlimits fail test#3051
YJDoc2 merged 5 commits intoyouki-dev:mainfrom
ntkm61027:add-test-process-rlimits-fail

Conversation

@ntkm61027
Copy link
Contributor

@ntkm61027 ntkm61027 commented Jan 14, 2025

Description

Implement integration test for process_rlimits_fail.
Port the test from runtime-tools/validation with a different approach.

While the original test in runtime-tools/validation/process_rlimits_fail/process_rlimits_fail.go tests by passing an invalid rlimits type (RLIMIT_TEST), this implementation takes a different approach since PosixRlimitBuilder uses a strictly typed enum (PosixRlimitType) that prevents invalid types at compile time.

Instead, this test validates the error handling by setting extremely high values (u64::MAX) for both hard and soft limits, which exceeds system limitations.

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

related #361 (process_rlimits_fail)

Signed-off-by: ntkm61027 <131166531+ntkm61027@users.noreply.github.com>
@ntkm61027 ntkm61027 force-pushed the add-test-process-rlimits-fail branch from ea48efa to 90d0a8c Compare January 14, 2025 13:38
@YJDoc2 YJDoc2 self-requested a review January 15, 2025 13:19
@YJDoc2
Copy link
Collaborator

YJDoc2 commented Jan 15, 2025

Hey @ntkm61027 , thanks for the PR!
I had a question with this, can you please help me understand it - In the original go test, it is setting rlimit for RLIMIT_TEST , which is not a valid rlimit and hence it fails. However in this PR, we are setting a valid rlimit, but setting it to u64::max . Can you tell me why it is supposed to fail? Is there a Linux internal limit on max value of this, and u64::max is greater than that, so this fails?

Currently the CI is passing, so the logic in your test seems to be correct, but I don't understand why it is working.
Maybe also add corresponding explaining comment in the code as well.

Signed-off-by: ntkm61027 <131166531+ntkm61027@users.noreply.github.com>
@ntkm61027 ntkm61027 force-pushed the add-test-process-rlimits-fail branch from d7163ea to a826e17 Compare January 16, 2025 03:23
@ntkm61027
Copy link
Contributor Author

@YJDoc2
Thank you for your question!
Let me clarify with the specific documentation from Linux man pages.

According to man 2 setrlimit, the EPERM error occurs when:

The caller tried to increase the hard RLIMIT_NOFILE limit above the maximum defined by /proc/sys/fs/nr_open

In our test case:

  1. We are using a valid rlimit type (RLIMIT_NOFILE)
  2. But setting it to u64::MAX (18446744073709551615), which is significantly higher than the system's nr_open limit (default: 1048576)
  3. The kernel correctly rejects this with EPERM because it exceeds the maximum allowed value

This approach still tests the OCI spec requirement that "The runtime MUST generate an error for any values which cannot be mapped to a relevant kernel interface", just from a different angle than the original Go test:

  • Original Go test: Tests with invalid type (RLIMIT_TEST)
  • This implementation: Tests with valid type but unmappable value (exceeding nr_open limit)

I added these implementation details as comments in the code as well.

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Jan 16, 2025

Hey @ntkm61027 thanks for the explanation and comments. That makes sense.
CI is failing due to format errors, can you run cargo fmt and fix the formatting.
Additionally, can you also put the kernel doc link that you have added in above comment in the code doc comment as well?
Apart from that, lgtm, so will merge after fix.

Thanks for your contribution!

Signed-off-by: ntkm61027 <131166531+ntkm61027@users.noreply.github.com>
Signed-off-by: ntkm61027 <131166531+ntkm61027@users.noreply.github.com>
@ntkm61027
Copy link
Contributor Author

ntkm61027 commented Jan 16, 2025

@YJDoc2
Oops, fixed it! Mind taking another look?
Thanks for the review!

I forgot to add the kernel doc link! Just a moment please!

Signed-off-by: ntkm61027 <131166531+ntkm61027@users.noreply.github.com>
Copy link
Collaborator

@YJDoc2 YJDoc2 left a comment

Choose a reason for hiding this comment

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

lgtm!

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Jan 16, 2025

Hey @ntkm61027 thanks for updating with changes. Also, congratulations on your first contribution to Youki!

@YJDoc2 YJDoc2 merged commit dad3c55 into youki-dev:main Jan 16, 2025
27 checks passed
@github-actions github-actions bot mentioned this pull request Jan 16, 2025
YamasouA pushed a commit to YamasouA/youki that referenced this pull request Feb 2, 2025
* add process rlimits fail test

Signed-off-by: ntkm61027 <131166531+ntkm61027@users.noreply.github.com>

* add comments to process_rlimits_fail test

Signed-off-by: ntkm61027 <131166531+ntkm61027@users.noreply.github.com>

* fix fmt

Signed-off-by: ntkm61027 <131166531+ntkm61027@users.noreply.github.com>

* fix comments to process_rlimits_fail test

Signed-off-by: ntkm61027 <131166531+ntkm61027@users.noreply.github.com>

* fix comments to process_rlimits_fail test

Signed-off-by: ntkm61027 <131166531+ntkm61027@users.noreply.github.com>

---------

Signed-off-by: ntkm61027 <131166531+ntkm61027@users.noreply.github.com>
Signed-off-by: Akiyama <akiakiskyhand@gmail.com>
moz-sec pushed a commit to moz-sec/youki that referenced this pull request Mar 29, 2025
* add process rlimits fail test

Signed-off-by: ntkm61027 <131166531+ntkm61027@users.noreply.github.com>

* add comments to process_rlimits_fail test

Signed-off-by: ntkm61027 <131166531+ntkm61027@users.noreply.github.com>

* fix fmt

Signed-off-by: ntkm61027 <131166531+ntkm61027@users.noreply.github.com>

* fix comments to process_rlimits_fail test

Signed-off-by: ntkm61027 <131166531+ntkm61027@users.noreply.github.com>

* fix comments to process_rlimits_fail test

Signed-off-by: ntkm61027 <131166531+ntkm61027@users.noreply.github.com>

---------

Signed-off-by: ntkm61027 <131166531+ntkm61027@users.noreply.github.com>
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

Comments