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

newlog: fix file naming for logs #4491

Merged
merged 1 commit into from
Dec 17, 2024

Conversation

europaul
Copy link
Contributor

Revert the file naming inside devUpload and keepSentQueue directories to the original format dev.log.<timestamp>.gz to keep the compatibility with previous versions of the newlogd.

This is necessary e.g. in cases when EVE goes through an unsuccessful update and has to fallback to a version before the new naming convention. This way the logic inside newlogd, loguploader and edge-view can still understand the log files inside /persist/newlog and not throw an error.

@eriknordmark
Copy link
Contributor

... understand the log files inside /persist/newlog and not throw an error.

More specifically, the error was a log.Fatal from loguploader after the fallback to the old version of EVE.

docs/LOGGING.md Outdated
@@ -115,7 +116,7 @@ The uploading is controlled on a scheduled timer. When the timer fires, the "log

The "loguploader" collects stats of round-trip delay, controller CPU load percentage and log batch processing time. The current EVE implementation does not use those stats in calculating the uploading timer values.

The already uploaded gzip files with app logs are moved to /persist/newlog/keepSentQueue directory or removed in case of dev.log.upload files.
The already uploaded gzip files with app logs are moved to /persist/newlog/keepSentQueue directory or removed in case of dev.log files.
Copy link
Contributor

Choose a reason for hiding this comment

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

All 'dev.log*' named files can not be removed. can they?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think they have a copy in the 'keep' directory already in the latest newlog change. so they should be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@naiming-zededa is right, but I understand that the sentence is confusing - I'll rewrite

Revert the file naming inside devUpload and keepSentQueue directories to
the original format `dev.log.<timestamp>.gz` to keep the compatibility
with previous versions of the newlogd.

Signed-off-by: Paul Gaiduk <paulg@zededa.com>
@europaul europaul force-pushed the logs-naming-safe-to-fallback branch from 8e46902 to 0949315 Compare December 17, 2024 11:01
Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

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

Run tests

Copy link
Member

@OhmSpectator OhmSpectator left a comment

Choose a reason for hiding this comment

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

Good that it fixes the current problem. Let's merge it soon.

But could we add some logic on top to avoid fatals with log formats in the future? For example, not ending up with a log.Fatal because of invalid log names, or maybe some cleanup logic to remove problematic files on request? I’m not sure where exactly this would fit, but just an idea...

@eriknordmark
Copy link
Contributor

But could we add some logic on top to avoid fatals with log formats in the future? For example, not ending up with a log.Fatal because of invalid log names, or maybe some cleanup logic to remove problematic files on request? I’m not sure where exactly this would fit, but just an idea...

Yes, but please lets do that as a separate PR.
Currently tests are failing because some tests run master or a PR for a while and then they go back to 13.4.X and fatal due to this issue. So I want master to be usable again first.

@eriknordmark eriknordmark merged commit 8dfe4ea into lf-edge:master Dec 17, 2024
46 of 58 checks passed
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