-
Notifications
You must be signed in to change notification settings - Fork 351
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
Integration test: cgroup v1 relative-cpus tests #2898
base: main
Are you sure you want to change the base?
Integration test: cgroup v1 relative-cpus tests #2898
Conversation
Signed-off-by: posutsai <posutsai@gmail.com>
@YJDoc2 May I ask you to review this PR? |
Sure, I'll take a look at this 👍 |
Signed-off-by: posutsai <posutsai@gmail.com>
Sorry, I would like to make some refinement. @YJDoc2 , Could you please review later? |
Signed-off-by: posutsai <posutsai@gmail.com>
@posutsai , sure. May I request you to mark this as draft and ping me when this is ready to review?
|
Thank you. I have turned it to draft. Also I would like to know should I do the check in |
Signed-off-by: posutsai <posutsai@gmail.com>
Hi, @YJDoc2 I believe the review can resume. I use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some changes, overall ok 👍
Thanks :)
Signed-off-by: posutsai <posutsai@gmail.com>
Signed-off-by: posutsai <posutsai@gmail.com>
Hey, so I should have asked this a long time back, but even I'm catching this right now - What you have implemented is still using the absolute cgroup path right? This does not have anything corresponding to https://github.com/opencontainers/runtime-tools/blob/master/validation/linux_cgroups_relative_cpus/linux_cgroups_relative_cpus.go#L24 which sets the relative cgroup path. |
Hi @YJDoc2, please correct me if I am wrong. According to my understanding, the point of the unit test is to check correctness of the config related to relative CPU resource allocation e.g. |
Hey so from what I saw, if you check other |
Signed-off-by: posutsai <posutsai@gmail.com>
2fd67fe
to
f21eea0
Compare
Hi, sorry for late reply. I've modified the code to use |
No worries! I'll try to take a look at this today or so. |
Hey, sorry it took me a while to get to this, got busy with other stuff. So in your recent changes, you are correctly taking it using the function however you are not setting the cgroup path as relative in the spec. For reference I'd suggest to take a look at https://github.com/containers/youki/pull/2686/files# , and see how it is done there, specifically https://github.com/containers/youki/pull/2686/files#diff-682de1209b41c713baac9258d0206875be877e6453cba0834357f52050d0520eR18-R21 I'll also suggest to take a look at discussion there, it seems the original author might not be continuing with the PR, so if you're fine with it, you might be able to take that one up after this (if you want). |
@posutsai , ping! |
Hey @posutsai , let me know if you have any issues, or cannot work this anymore. It is ok if you are busy with something else, just let me know so we can proceed accordingly. Thanks :) |
Hi, I have implemented the test_relative_cpus function to contribute to #361 by referencing
linux_cgroups_relative_cpus.go
andcpu/v2.rs
. However, I encountered a few issues:The current implementation closely mirrors the logic in v2.rs. Since there isn't a clear distinction between v1 and v2 logic in linux_cgroups_relative_cpus.go, would it be better to extract the common logic into a shared module?
I am also unsure about the correct way to run the tests. The just test-contest command on the main branch fails on Ubuntu 20.04. Could you provide guidance on resolving this issue?
Thank you for your help. I have learned a lot from this process.