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

Cgroup v1 pid integration tests #391

Merged
merged 6 commits into from
Oct 19, 2021
Merged

Cgroup v1 pid integration tests #391

merged 6 commits into from
Oct 19, 2021

Conversation

Furisto
Copy link
Collaborator

@Furisto Furisto commented Oct 16, 2021

  • Implements the cgroup v1 pid tests
  • Added more context to error messages and added a new function for checking if the container was created successfully
  • I realize this is subjective but I found Passed, Skipped and Failed more understandable for TestResult so I changed the names. I can change it back if you prefer the older names.
  • I think we should have a convention for the used cgroups path. I set it as [cgroup_root]/runtime-test/[test_name]
  • The spec does not state what should happen if the limit is zero or negative, but we and runc just set unlimited in this case. We should write down where the spec is ambiguous, so that we do not forget it. This can then be used to improve the spec. For now I just added //SPEC in front of such comments in the code. Other suggestions welcome.

#361

@codecov-commenter
Copy link

codecov-commenter commented Oct 16, 2021

Codecov Report

Merging #391 (04a8fa8) into main (66c2eab) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #391   +/-   ##
=======================================
  Coverage   76.93%   76.93%           
=======================================
  Files          52       52           
  Lines        8280     8280           
=======================================
  Hits         6370     6370           
  Misses       1910     1910           

@Furisto Furisto requested a review from YJDoc2 October 16, 2021 14:49
@YJDoc2 YJDoc2 mentioned this pull request Oct 17, 2021
@YJDoc2
Copy link
Collaborator

YJDoc2 commented Oct 17, 2021

Hey, I will take a good detailed look once again tomorrow, but some things that I noticed :

  • context messages and helper function is an excellent addition! Thank you for adding that !
  • Our TestResult enum was meant to follow after Rust's Result, with adding a Skip variant, so I kept names of others same as Result. I do understand that Passed, Skipped and Failed make more sense, but I also feel a bit that if we are using Result in the name, we should keep the variant names similar 😅 I'll think more on this...
  • Using a common convention for cgroup path is good, but the created cgroup is not removed once the test are over (runtime-test directory remains after tests are over). We can manually remove this with remove_dir, after all container processes are exited, but then we should add the clean up function. I don't feel leaving an extra cgroup directory (even if it doesn't give issues) on tester's system is a good thing.

youki_integration_test/src/tests/cgroups/pids.rs Outdated Show resolved Hide resolved
youki_integration_test/src/tests/cgroups/pids.rs Outdated Show resolved Hide resolved
@Furisto
Copy link
Collaborator Author

Furisto commented Oct 18, 2021

@YJDoc2 PTAL

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Oct 18, 2021

Hey @Furisto , so I think let the enum names be Passed,Skipped and Failed(e) ; in case we run into an issue with them, we can see later.
Please check this from my last comment :

Using a common convention for cgroup path is good, but the created cgroup is not removed once the test are over (runtime-test directory remains after tests are over). We can manually remove this with remove_dir, after all container processes are exited, but then we should add the clean up function. I don't feel leaving an extra cgroup directory (even if it doesn't give issues) on tester's system is a good thing.

And also can you mention the macro in readme of test_framework and the check_container_created function in the tests readme, under https://github.com/containers/youki/tree/main/youki_integration_test#utils-provided ?

Other than that I think the code looks good 👍

@Furisto
Copy link
Collaborator Author

Furisto commented Oct 18, 2021

@YJDoc2 Done

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 👍
Thank you!

@YJDoc2 YJDoc2 merged commit 9ed52d9 into youki-dev:main Oct 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants