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

tool: revert log level of malformed gens #158

Merged
merged 2 commits into from
Apr 23, 2023

Conversation

nikstur
Copy link
Collaborator

@nikstur nikstur commented Apr 23, 2023

The message about malformed generations should semantically be a warning. However, since users might have hundreds of old and thus malformed generations and can do little about it, this should remain a debug message. This way the user is not spammed with no-op warnings while still enabling debugging.

This reverts the change to the log level made in #155

@RaitoBezarius
Copy link
Member

Can you add a change to enable debugging logs in our tests? That would be great for diagnosing and fixing in the future. :)

Copy link
Member

@RaitoBezarius RaitoBezarius left a comment

Choose a reason for hiding this comment

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

LGTM in the meantime

The message about malformed generatiosn should semantically be a
warning. However, since users might have hundres of old and thus
malformed generations and can do little about it, this should remain a
debug message. This way the user is not spammed with no-op warnings
while still enabling debugging.
@nikstur nikstur force-pushed the revert-warnlevel-malformed-gen branch from 7995558 to 8efc061 Compare April 23, 2023 21:28
@nikstur
Copy link
Collaborator Author

nikstur commented Apr 23, 2023

Can you add a change to enable debugging logs in our tests? That would be great for diagnosing and fixing in the future. :)

This should already be enabled: https://github.com/nix-community/lanzaboote/blob/master/rust/tool/tests/common/mod.rs#L132

If you want to enable logging for the unit tests, we can take a look at: https://github.com/d-e-s-o/test-log
I think this might be more of an interactive approach where you add the logger initialization for the test that fails/that you want to inspect. Maybe @blitz has some more ideas/expertise with logging in unit tests?

@nikstur nikstur merged commit efff933 into master Apr 23, 2023
@blitz blitz deleted the revert-warnlevel-malformed-gen branch April 24, 2023 13:33
@blitz
Copy link
Member

blitz commented Apr 24, 2023

Regarding the logging in unit tests: The more the better. But it looks like we are already doing that.

However, since users might have hundreds of old and thus malformed generations and can do little about it, this should remain a debug message.

They can GC? Having lots of generations which users cannot boot, because lzbt can't parse them should be something that users should be aware of. We can make the warning less scary, if that's the issue, but it would be really valuable for diagnosing issues in the field.

@nikstur
Copy link
Collaborator Author

nikstur commented Apr 24, 2023

Eventually I want a single log message (warning) when there are malformed generations. But in the loop I do not think it makes sense to emit a warning for every malformed generation. If we implement what @alois31 suggested in the Matrix channel we can emit this warning when the GC does not run because there are broken generations.

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.

None yet

3 participants