-
Notifications
You must be signed in to change notification settings - Fork 355
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
Implement integration tests for cgroup v2 cpu #528
Conversation
Cpu weight should be set to maximum value if value exceeds the valid range.
Implement an equality macro that returns a result instead of panicking
Codecov Report
@@ Coverage Diff @@
## main #528 +/- ##
==========================================
- Coverage 61.09% 61.03% -0.07%
==========================================
Files 85 85
Lines 12532 12522 -10
==========================================
- Hits 7657 7643 -14
- Misses 4875 4879 +4 |
I suspect that the test error has nothing to do with my changes and the tests might have been broken for a while. The last time the tests were run was here, but the youki binary is actually not found even though no error is reported. This seems to have been fixed with this change, but this did not trigger the integration tests, so it was not detected there either. Investigating this further... |
sorry... 😭 maybe it is solved by this PR |
@utam0k PTAL |
/// Initialize the logger, must be called before accessing the logger | ||
/// Multiple parts might call this at once, but the actual initialization | ||
/// is done only once due to use of OnceCell | ||
pub fn init(log_file: Option<PathBuf>, debug: bool) -> Result<()> { |
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.
I think it is better that the output of the integration test should be handled by only stderr/stdout. If you want to write to the file, you can use redirect, tee, etc
Hey @Furisto any idea why the integration test error is happening? The fix is to use the release version of youki instead of debug, but the debug / release should not differ in functionalities right? The CI log indicates that there was an error in reading the container state, which should either work or be broken in both debug and release builds... 🤔 |
Logger should not log to file
@YJDoc2 There are three parts
The reason that it works in release and not in debug is that the log level in release and debug is different. Using the release flags is the simplest fix and we should test in release anyway, but I am planning to take another look at the issue once this PR is through. |
@utam0k Done |
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.
LGTM
Extends our integration with tests for cpu resource restrictions on cgroup v2 systems. It also enables logging for the tests in case you need to debug them.