-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Log all services to memory #4587
Conversation
✅ Deploy Preview for frigate-docs canceled.
|
Nice! That should be good |
@NickM-27 the current behavior, which mimics s6-overlay defaults, is to rotate the As we are logging to memory, we should perhaps consider decreasing the number of archives. Perhaps 1 is enough? |
Yeah I agree that's better archive, might be worth increasing the size a bit to 10MB or so so there's a decent history for users to view. |
Ok, I also agree. |
Done. |
On my tests, this is working. If you could give it a spin as well before merging, it would be nice. |
Sure, I can do it tomorrow night or Monday morning, I'm out right now |
There's another thing to take into consideration here. Since Frigate isn't run as CMD anymore, s6-overlay now apply some more generic rules to it. Most importantly:
So, do you think 3 seconds is enough for the Frigate process to exit gracefully or should we give it more time? |
I'd say definitely give it more time, don't want it to get killed mid storage maintenance or something. |
Ok, I think there's a bug in s6-overlay which will affect us: Let's wait for the closure of it before merging this PR. |
is it this PR that causes that issue or the update to s6 v3? |
That's a good question. I got to test. My assumption is that it's this PR, since I'm switching Frigate from CMD to a service. |
I tested here, the same issue happens when using CMD (which means this PR is NOT introducing this issue). Now I'll test now with S6 v2. |
Ok, same issue with s6-overlay v2 which means it was always there. This may actually be the reason for multiple random database corruptions that we found people reporting (including me). Therefore, this PR can be merged as it brings no additional issues. |
how does this work with the dev container? are we still able to start / stop frigate python process as expected and it isn't started automatically? |
@NickM-27 Nothing should change for the devcontainer because of: |
Thanks @felipecrs I'm seeing an error, when I try to view the contents of the log files from the frigate python process it does not have appropriate permissions, the python process will need to be able to read the log file contents. |
Hm... in the devcontainer Python runs as |
that's a fair point, I haven't tested in a full container if python will have access |
Another thing, have in mind that, since the Frigate process is not being supervised in the devcontainer, there won't be any logs for it. Perhaps we should have a fake Frigate service running in the devcontainer just printing some dummy text, so that the log files gets actually created. What do you think? |
Yeah I am aware of that, I'm not sure it's necessary since the logs will be visible in the console (so it isn't needed) and the logs being gone won't cause any real issues. Happy to hear other's opinions though |
@NickM-27 check my last commit. It should fix all issues now. |
Thanks that works great |
@felipecrs I don't think frigate is being started with env variables with this PR or at least the frigate config can not see any |
When I run |
Thanks. Should work now. |
/dev/shm/logs/<service>
, e.g./dev/shm/logs/go2rtc/current
.CMD
anymore as this allows us to log it as well.