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

Log all services to memory #4587

Merged
merged 10 commits into from
Dec 7, 2022
Merged

Conversation

felipecrs
Copy link
Contributor

@felipecrs felipecrs commented Dec 3, 2022

  • All logs can be accessed from /dev/shm/logs/<service>, e.g. /dev/shm/logs/go2rtc/current.
  • NGINX now logs to stdout as well (like in Logs in UI #4562)
  • Frigate service isn't set in CMD anymore as this allows us to log it as well.
    • S6-Overlay does not seem to support customizing logging for CMD
  • Refs Logs in UI #4562 (comment)
  • Is depended on by Logs in UI #4562

@netlify
Copy link

netlify bot commented Dec 3, 2022

Deploy Preview for frigate-docs canceled.

Name Link
🔨 Latest commit 40046e9
🔍 Latest deploy log https://app.netlify.com/sites/frigate-docs/deploys/638e652983476f0008aa14f2

@NickM-27
Copy link
Sponsor Collaborator

NickM-27 commented Dec 3, 2022

Nice! That should be good

@felipecrs felipecrs changed the title Log all services to RAM Log all services to memory Dec 3, 2022
@felipecrs
Copy link
Contributor Author

felipecrs commented Dec 3, 2022

@NickM-27 the current behavior, which mimics s6-overlay defaults, is to rotate the current log every time it reaches 1MB, and keep at most 20 archive files.

As we are logging to memory, we should perhaps consider decreasing the number of archives. Perhaps 1 is enough?

https://skarnet.org/software/s6/s6-log.html

@NickM-27
Copy link
Sponsor Collaborator

NickM-27 commented Dec 3, 2022

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.

@felipecrs
Copy link
Contributor Author

Ok, I also agree.

@felipecrs
Copy link
Contributor Author

Done.

@felipecrs
Copy link
Contributor Author

On my tests, this is working. If you could give it a spin as well before merging, it would be nice.

@NickM-27
Copy link
Sponsor Collaborator

NickM-27 commented Dec 3, 2022

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

@felipecrs
Copy link
Contributor Author

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:

  • S6_SERVICES_GRACETIME (default = 3000): How long (in milliseconds) s6 should wait,
    at shutdown time, for services declared in /etc/services.d to die before proceeding
    with the rest of the shutdown.
  • S6_KILL_GRACETIME (default = 3000): How long (in milliseconds) s6 should wait, at the end of
    the shutdown procedure when all the processes have received a TERM signal, for them to die
    before sending a KILL signal to make sure they're dead.

So, do you think 3 seconds is enough for the Frigate process to exit gracefully or should we give it more time?

@NickM-27
Copy link
Sponsor Collaborator

NickM-27 commented Dec 4, 2022

I'd say definitely give it more time, don't want it to get killed mid storage maintenance or something.

@felipecrs
Copy link
Contributor Author

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.

@felipecrs felipecrs marked this pull request as draft December 4, 2022 16:35
@NickM-27
Copy link
Sponsor Collaborator

NickM-27 commented Dec 5, 2022

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?

@felipecrs
Copy link
Contributor Author

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.

@felipecrs
Copy link
Contributor Author

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.

@felipecrs
Copy link
Contributor Author

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.

@felipecrs felipecrs marked this pull request as ready for review December 5, 2022 13:42
@NickM-27
Copy link
Sponsor Collaborator

NickM-27 commented Dec 5, 2022

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?

@felipecrs
Copy link
Contributor Author

@NickM-27
Copy link
Sponsor Collaborator

NickM-27 commented Dec 5, 2022

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.

@felipecrs
Copy link
Contributor Author

Hm... in the devcontainer Python runs as vscode. Maybe that's why. But I think it was supposed to have access anyway. I will do some testing tonight, but if you identify some fix feel free to amend this PR.

@NickM-27
Copy link
Sponsor Collaborator

NickM-27 commented Dec 5, 2022

Hm... in the devcontainer Python runs as vscode. Maybe that's why. But I think it was supposed to have access anyway. I will do some testing tonight, but if you identify some fix feel free to amend this PR.

that's a fair point, I haven't tested in a full container if python will have access

@felipecrs
Copy link
Contributor Author

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?

@NickM-27
Copy link
Sponsor Collaborator

NickM-27 commented Dec 5, 2022

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

@felipecrs
Copy link
Contributor Author

@NickM-27 check my last commit. It should fix all issues now.

@NickM-27
Copy link
Sponsor Collaborator

NickM-27 commented Dec 5, 2022

Thanks that works great

@NickM-27 NickM-27 mentioned this pull request Dec 5, 2022
@NickM-27
Copy link
Sponsor Collaborator

NickM-27 commented Dec 5, 2022

@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 FRIGATE_... variables

@NickM-27
Copy link
Sponsor Collaborator

NickM-27 commented Dec 5, 2022

When I run printenv in the container I can see the env variables, but it seems that inside the frigate python process it is unable to see the variables

@felipecrs
Copy link
Contributor Author

Thanks. Should work now.

@blakeblackshear blakeblackshear merged commit 2a5ab77 into blakeblackshear:dev Dec 7, 2022
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