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_oom_score_adj #2987

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

saku3
Copy link

@saku3 saku3 commented Nov 13, 2024

This implements the process_oom_score_adj validation in #361 .

@saku3 saku3 force-pushed the add-test-process_oom_score_adj branch 2 times, most recently from b03083d to 9307ee1 Compare November 13, 2024 09:26
@YJDoc2
Copy link
Collaborator

YJDoc2 commented Nov 13, 2024

Hey, there are several linting issues, you can check by running cargo clippy -- -D warnings and to fix formatting cargo fmt.

More importantly the test added in this PR is failing with error container stderr was not empty, found : Unexpected oom_score_adj, expected: 500 found: 500 Not sure why, there might be some spaces ? You can try adding .trim().

Also, would it be possible to set the oom score randomly, withing a range of say 300-700 ? You can check https://github.com/youki-dev/youki/pull/2978/files# to see how it is done there.

Signed-off-by: Yusuke Sakurai <[email protected]>
@saku3 saku3 force-pushed the add-test-process_oom_score_adj branch from 9307ee1 to 8eace0c Compare November 14, 2024 03:22
@saku3
Copy link
Author

saku3 commented Nov 14, 2024

@YJDoc2 Thank you for the review!
I've fixed the code according to the comments.
I confirmed locally that running the just validate-contest-runc command results in process_oom_score_adj: ok.
Could you please check the code?

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Nov 14, 2024

Hey @saku3 Thanks for the quick update! So the oom test is now passing, but the formatting and linting still fails. Can you run cargo fmt and check just lint ?

Apart from that the PR looks good to me, so once that is fixed, I think this is good to merge. Thanks :)

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