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 test process rlimits #2977

Merged
merged 9 commits into from
Nov 12, 2024
Merged

Conversation

sat0ken
Copy link
Contributor

@sat0ken sat0ken commented Nov 4, 2024

This implements the process_rlimits validation in #361

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.

Clippy CI failing, also you are not setting the correct runtimetest arg as cmd in the process spec, so the internal validation won't run at all.

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Nov 6, 2024

Also, are there any safe alternatives to get the rlimits, maybe via nix crate? https://docs.rs/nix/latest/nix/sys/resource/fn.getrlimit.html
Please use safe alternatives instead of unsafe code wherever possible :)

@sat0ken
Copy link
Contributor Author

sat0ken commented Nov 8, 2024

I fixed the code, remove unsafe code and change to use nix.

bc23f93

@sat0ken
Copy link
Contributor Author

sat0ken commented Nov 10, 2024

@YJDoc2 HI, I've fixed the code according to the comments. Could you please check the code?

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.

two nitpicks, but not blocking change. I'll merge this PR by tom if you don't respond. (Even a comment not necessarily code change)
To be clear, the nitpicks are not major, so I'm fine for merging this as is, and will merge by tom unless you comment otherwise.

let (soft_limit, hard_limit) = getrlimit(change_resource_type(spec_rlimit.typ())).unwrap();
if spec_rlimit.hard() != hard_limit {
eprintln!(
"error type of {:?} hard rlimit expected {:?} , got {:?}",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"error type of {:?} hard rlimit expected {:?} , got {:?}",
"error: rlimit mismatch {:?} hard rlimit expected {:?} , got {:?}",

Same for soft limit

}
}

fn change_resource_type(resource_type: PosixRlimitType) -> Resource {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
fn change_resource_type(resource_type: PosixRlimitType) -> Resource {
fn to_nix_resource(resource_type: PosixRlimitType) -> Resource {

my reasoning is change_resource_type does not express what exactly we are changing it to, but from fn signature it is understandable, so not a strict change req.

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Nov 12, 2024

Going ahead and merging

@YJDoc2 YJDoc2 merged commit 5a9e78f into youki-dev:main Nov 12, 2024
27 checks passed
@github-actions github-actions bot mentioned this pull request Nov 6, 2024
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