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

Refactor s6 scripts to the new format #5135

Merged
merged 14 commits into from
Jan 18, 2023
Merged

Refactor s6 scripts to the new format #5135

merged 14 commits into from
Jan 18, 2023

Conversation

felipecrs
Copy link
Contributor

@felipecrs felipecrs commented Jan 17, 2023

Fixes

  • This fixes a long standing issue that was causing s6 to abruptly kill the Frigate process during exit process. Now it properly waits as long as needed for Frigate to exit gracefully, which should help to prevent database corruptions.

  • Prevent go2rtc from restarting without restarting the whole container. This can't happen, otherwise the birdseye restream feed will be lost, as it's added only during Frigate startup.

  • Never tolerate go2rtc to exit, even with status code 0. If it exits, the container will also exit (docker takes care of restarting after the container exits). The only situation in which this is tolerated is when Frigate also exited with status code 0 (probably because someone requested a restart from the UI).

  • Also, similarly never tolerates Frigate to exit. If Frigate exits, the container is also now exited (docker takes care of restarting after the container exits).

References

@netlify
Copy link

netlify bot commented Jan 17, 2023

Deploy Preview for frigate-docs canceled.

Name Link
🔨 Latest commit 55abdb4
🔍 Latest deploy log https://app.netlify.com/sites/frigate-docs/deploys/63c7e9cd3bd21f0008bebe97

@felipecrs
Copy link
Contributor Author

I was never able to build the Frigate image anymore since the tensorrt was added. I need to investigate why, so I can also test this PR.

@felipecrs
Copy link
Contributor Author

felipecrs commented Jan 17, 2023

Wow. I actually needed binfmt installed. Then I was able to build the Frigate container. That's weird... anyway, here's how to install it:

docker run --privileged --rm tonistiigi/binfmt --install all

No, sorry, never mind. I was actually calling the wrong make target.

@felipecrs
Copy link
Contributor Author

Ok, I think this is working. This also fixes an issue that always existed, that was causing the container to exit abruptly instead of waiting for Frigate to gracefully finish.

@felipecrs felipecrs marked this pull request as ready for review January 17, 2023 15:16
@felipecrs
Copy link
Contributor Author

Alright, I did some more testing. Logging and everything else seems to be working normally.

This however does not take down the container if go2rtc exits with 0. I'm working on it now.

@felipecrs
Copy link
Contributor Author

Done. Some demos:

Before

Code_j9UCdR0XCy.mp4

After

Code_AQQPoM4DSr.mp4

@felipecrs
Copy link
Contributor Author

felipecrs commented Jan 17, 2023

Actually this is now handling the case when Frigate exits with exit code 0 and makes the container exits with exit code 1.

Question: which exit code is Frigate exiting with when restarting from the UI?

@NickM-27
Copy link
Collaborator

Actually this is now handling the case when Frigate exits with exit code 0 and makes the container exits with exit code 1.

Question: which exit code is Frigate exiting with when restarting from the UI?

def restart_frigate():

@felipecrs felipecrs marked this pull request as draft January 17, 2023 16:28
@felipecrs felipecrs marked this pull request as ready for review January 17, 2023 16:54
@felipecrs
Copy link
Contributor Author

Ok, from me this is now ready to be reviewed.

@NickM-27 NickM-27 changed the title Refator s6 scripts to the new format Refactor s6 scripts to the new format Jan 17, 2023
Dockerfile Outdated Show resolved Hide resolved
Co-authored-by: Felipe Santos <[email protected]>
@blakeblackshear blakeblackshear merged commit 02df2a8 into blakeblackshear:dev Jan 18, 2023
@NickM-27
Copy link
Collaborator

@felipecrs restart from container doesn't seem to be working for me. It stops frigate but the container keeps running

@felipecrs felipecrs deleted the migrate-s6-scripts branch January 18, 2023 14:29
@felipecrs
Copy link
Contributor Author

That's weird. I tested it. Did you restart from Frigate UI? I'll test again.

@NickM-27
Copy link
Collaborator

That's weird. I tested it. Did you restart from Frigate UI? I'll test again.

I used the save config & restart option. Frigate would not come up and I checked the logs and go2rtc was still running

@felipecrs
Copy link
Contributor Author

Code_bpoVRhY7BB.mp4

@felipecrs
Copy link
Contributor Author

Is there any chance you forgot to rebuild the container?

@felipecrs
Copy link
Contributor Author

Oh... I think this might be possible in the devcontainer. I'll push a fix.

@NickM-27
Copy link
Collaborator

No, I did a full fresh build. It was also working before the changes.

Oh... I think this might be possible in the devcontainer. I'll push a fix.

I am not using devcontainer for these changes I am using a full E2E docker running on my main server

@NickM-27
Copy link
Collaborator

It looks like it gets to waiting for detection process to exit and then never finishes from there even after 2 minutes

@felipecrs
Copy link
Contributor Author

Yes. It takes a while indeed. That's why I also changed the text from UI.

BTW, go2rtc only stops AFTER frigate now, not before. That's why go2rtc was still running in your container.

And, I think we should fix this from Python side, not from s6 side, because it's Python who manages the detection process.

@felipecrs
Copy link
Contributor Author

The only reason why this was working before was because s6 was mistakenly not waiting for Frigate to exit.

@NickM-27
Copy link
Collaborator

NickM-27 commented Jan 18, 2023

And, I think we should fix this from Python side, not from s6 side, because it's Python who manages the detection process.

I do agree but at the same time there is always a case where the process might get stuck and I think S6 should force stop the container at some point

also I let the container go for 5 minutes and it never restarted

@felipecrs
Copy link
Contributor Author

That's valid. I can add a timeout, this is a supported feature from s6. Will do it later. But if we could improve in Python side, it would also probably speed-up the whole process.

@felipecrs
Copy link
Contributor Author

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.

3 participants