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

Implement integration tests for cgroup v2 cpu #528

Merged
merged 13 commits into from
Dec 14, 2021

Conversation

Furisto
Copy link
Member

@Furisto Furisto commented Dec 11, 2021

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.

@codecov-commenter
Copy link

codecov-commenter commented Dec 11, 2021

Codecov Report

Merging #528 (712a07e) into main (12e61eb) will decrease coverage by 0.06%.
The diff coverage is 79.41%.

@@            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     

@Furisto
Copy link
Member Author

Furisto commented Dec 11, 2021

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...

@utam0k
Copy link
Member

utam0k commented Dec 12, 2021

sorry... 😭 maybe it is solved by this PR
#514

@Furisto
Copy link
Member Author

Furisto commented Dec 12, 2021

@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<()> {
Copy link
Member

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

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Dec 13, 2021

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... 🤔
If the exact reason is not yet found, I'll once try to git bisect tomorrow to try and get the faulty commit, and see if I can find anything from it.
Thanks :)

Logger should not log to file
@Furisto
Copy link
Member Author

Furisto commented Dec 13, 2021

@YJDoc2 There are three parts

  • I added a log statement some time ago that would at the start of main log the arguments with which youki was called
  • The logger implementation logs to stderr
  • The integration test calls the state command and fails if output was written to stderr

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.

@Furisto
Copy link
Member Author

Furisto commented Dec 13, 2021

@utam0k Done

Copy link
Member

@utam0k utam0k left a comment

Choose a reason for hiding this comment

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

LGTM

@utam0k utam0k merged commit 82c621e into containers:main Dec 14, 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.

4 participants