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

libs:logging: Fix logger #8547

Merged
merged 1 commit into from Dec 4, 2023
Merged

Conversation

jodh-intel
Copy link
Contributor

@jodh-intel jodh-intel commented Dec 1, 2023

PR #8311 inadvertently broke the logging since no log messages below the Info level are logged now, regardless of the requested log level.

Resolve the issue by storing the requested log level in the RuntimeComponentLevelFilter and using that level in the log() function, rather than hard-coding Info as the default where no entry is found in the FILTER_RULE hashmap.

Fixes: #8546.

Signed-off-by: James O. D. Hunt james.o.hunt@intel.com

PR kata-containers#8311 inadvertently broke the logging since no log messages below the
`Info` level are logged now, regardless of the requested log level.

Resolve the issue by storing the requested log level in the
`RuntimeComponentLevelFilter` and using that level in the `log()`
function, rather than hard-coding `Info` as the default where no entry
is found in the `FILTER_RULE` hashmap.

Fixes: kata-containers#8546.

Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>
@katacontainersbot katacontainersbot added the size/small Small and simple task label Dec 1, 2023
@jodh-intel
Copy link
Contributor Author

It's worth noting that the logger tests were failing. We need to find a way to re-introduce the changes I added on #2970 in a reliable way. Part of the problem is that our build system doesn't currently distinguish between golang and rust components. If it did, we could make all rust components depend on running the logger test suite before building the component. But that's not reliable as developers can in some instances just run cargo build which bypasses that.

Another approach would be to have a rust "build script" that we add to all rust components to force-run the logger tests. That would involve duplication though.

@jodh-intel
Copy link
Contributor Author

This PR will need to go into the next release.

Copy link
Contributor

@cmaf cmaf left a comment

Choose a reason for hiding this comment

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

@jodh-intel jodh-intel added safe-to-test Add to PR after manually reviewing to allow certain extra checks to run ok-to-test labels Dec 1, 2023
@jodh-intel
Copy link
Contributor Author

/test

@jodh-intel jodh-intel merged commit 7beab11 into kata-containers:main Dec 4, 2023
168 of 238 checks passed
@jodh-intel
Copy link
Contributor Author

#8554 raised to address the issue of re-enabling the feature where we always run the logger tests (always, I say! ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-test safe-to-test Add to PR after manually reviewing to allow certain extra checks to run size/small Small and simple task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

libs: logging: Fix logger
4 participants