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

add process rlimits fail test #3051

Merged
merged 5 commits into from
Jan 16, 2025

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