-
Notifications
You must be signed in to change notification settings - Fork 667
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
Use Systemd Journal API for all logs endpoints in API #4972
Conversation
4aed656
to
e68ef3c
Compare
e68ef3c
to
c2f40c5
Compare
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.
Mostly minor comments although there is an issue with the addon logs one that requires some tweaks, see the comment below.
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
90c31c1
to
afeea5b
Compare
This PR should also address this older RFC: #3309 |
Replace all logs endpoints using container logs with wrapped advanced_logs function, adding possibility to get logs from previous boots and following the logs. Supervisor logs are an excetion where Docker logs are still used - in case an exception is raised while accessing the Systemd logs, they're used as fallback - otherwise we wouldn't have an easy way to see what went wrong.
afeea5b
to
8f1515c
Compare
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.
Looks great 👍
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.
This is changing the content type from binary to text. Are the current users (probably only frontend?) able to handle this?
Supervisor logs are an excetion where Docker logs are still used - in case an exception is raised while accessing the Systemd logs, they're used as fallback - otherwise we wouldn't have an easy way to see what went wrong.
I guess the reverse can be true to. But yeah, since we try journald logs first, it will just not be noticeable in this case.
Yes, frontend doesn't really care about the content type. Host logs used text in the previous version as well, now it's consistent for all
I expect the journald erros to be rather persistent (until a reboot, etc.) than intermittent. And Docker logs are used only as the fallback, so if it also fails, you're done :) |
Proposed change
Replace all logs endpoints using container logs with wrapped advanced_logs function, adding possibility to get logs from previous boots and following the logs. Supervisor logs are an excetion where Docker logs are still used - in case an exception is raised while accessing the Systemd logs, they're used as fallback - otherwise we wouldn't have an easy way to see what went wrong.
Type of change
Additional information
Checklist
ruff format supervisor tests
)If API endpoints of add-on configuration are added/changed: