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

Add healthcheck for go2rtc service #5545

Merged
merged 12 commits into from
Feb 19, 2023

Conversation

felipecrs
Copy link
Contributor

@felipecrs felipecrs commented Feb 19, 2023

Also:

  • Upgrade s6-overlay from 3.1.3.0 to 3.1.4.0.
  • Allow go2rtc to restart within the container's lifetime
  • Fix startup and finish logs not being collected by s6, thus not shown in Frigate UI

@netlify
Copy link

netlify bot commented Feb 19, 2023

Deploy Preview for frigate-docs canceled.

Name Link
🔨 Latest commit 0593c9e
🔍 Latest deploy log https://app.netlify.com/sites/frigate-docs/deploys/63f271eaedfa1a000893b8d6

@felipecrs felipecrs marked this pull request as draft February 19, 2023 16:42
felipecrs and others added 2 commits February 19, 2023 13:43
Co-authored-by: Nicolas Mowen <nickmowen213@gmail.com>
Co-authored-by: Nicolas Mowen <nickmowen213@gmail.com>
@felipecrs
Copy link
Contributor Author

For some reason this new service isn't even starting for me. I'm trying to debug what's happening.

@felipecrs
Copy link
Contributor Author

@NickM-27, we can probably speed up the go2rtc initialization if we only create the config once. For that, I'd create the config in /dev/shm, and on next initializations I'd check if the config exists already and if it does, use it rather than recreate it.

What do you think?

@NickM-27
Copy link
Sponsor Collaborator

@NickM-27, we can probably speed up the go2rtc initialization if we only create the config once. For that, I'd create the config in /dev/shm, and on next initializations I'd check if the config exists already and if it does, use it rather than recreate it.

What do you think?

Yeah I think that makes sense

@felipecrs
Copy link
Contributor Author

Ok, so this is working. If I copy and paste the healthcheck's run contents to frigate's run contents, it works great.

However, s6 is not starting the go2rtc-healthcheck service itself and I really don't understand why.

This should be the only missing thing.

@NickM-27
Copy link
Sponsor Collaborator

Ok, so this is working. If I copy and paste the healthcheck's run contents to frigate's run contents, it works great.

However, s6 is not starting the go2rtc-healthcheck service itself and I really don't understand why.

This should be the only missing thing.

do you need to add the producer-for to the go2rtc part like it is set for the logs?

@felipecrs
Copy link
Contributor Author

I'm checking here.

Another thing, since go2rtc is now allowed to restart, we need to be more careful during initialization:

If go2rtc config for example is invalid, go2rtc will keep restarting forever. We should handle this case and make the container exit, I think.

Or maybe not?

@NickM-27
Copy link
Sponsor Collaborator

I'm checking here.

Another thing, since go2rtc is now allowed to restart, we need to be more careful during initialization:

If go2rtc config for example is invalid, go2rtc will keep restarting forever. We should handle this case and make the container exit, I think.

Or maybe not?

I think we should definitely fail if the create_config.py errors out, but if go2rtc doesn't like the config then that is something the create_config.py script should handle to ensure we only provide go2rtc with a valid config.

@felipecrs
Copy link
Contributor Author

felipecrs commented Feb 19, 2023

I don't think this is possible, because the create_config.py will pass whatever the user has set directly to go2rtc. Which may be a valid YAML but not a valid config.

i.e. we need to invoke go2rtc

@felipecrs
Copy link
Contributor Author

do you need to add the producer-for to the go2rtc part like it is set for the logs?

That fixed the issue, but I think this is probably a bug of s6-overlay. I should probably report it :(

@NickM-27
Copy link
Sponsor Collaborator

I don't think this is possible, because the create_config.py will pass whatever the user has set directly to go2rtc. Which may be a valid YAML but not a valid config.

i.e. we need to invoke go2rtc

actually I don't think it will be an issue. If the frigate config contains invalid yaml then frigate itself will not keep running. Personally, I think if the user provides an invalid config it is okay. It may change in the future but for now I believe go2rtc does not actually do yaml validation. For example, if you add

go2rtc:
  logs:
    ...

it should be log but go2rtc runs and just ignores it

@NickM-27
Copy link
Sponsor Collaborator

do you need to add the producer-for to the go2rtc part like it is set for the logs?

That fixed the issue, but I think this is probably a bug of s6-overlay. I should probably report it :(

Not that I know s6 super well but I think it makes sense that it would be required. all the other ones seem to require producer-for or consumer-for

@felipecrs
Copy link
Contributor Author

felipecrs commented Feb 19, 2023

all the other ones seem to require producer-for or consumer-for

Only because they make use of s6's logging services. I was not trying to integrate the healthcheck with the logging service, but it turns out to be a nice thing.

Now the logs from both go2rtc and the healthcheck are merged and can be seen from Frigate UI.

Also, some initialization logs were being hidden and they are now part of the logs that can be seen from Frigate UI.

@felipecrs felipecrs marked this pull request as ready for review February 19, 2023 18:08
@felipecrs
Copy link
Contributor Author

felipecrs commented Feb 19, 2023

I don't think this is possible, because the create_config.py will pass whatever the user has set directly to go2rtc. Which may be a valid YAML but not a valid config.
i.e. we need to invoke go2rtc

actually I don't think it will be an issue. If the frigate config contains invalid yaml then frigate itself will not keep running. Personally, I think if the user provides an invalid config it is okay. It may change in the future but for now I believe go2rtc does not actually do yaml validation. For example, if you add

go2rtc:
  logs:
    ...

it should be log but go2rtc runs and just ignores it

Ok, let's revisit this later if we find a need then.

Copy link
Sponsor Collaborator

@NickM-27 NickM-27 left a comment

Choose a reason for hiding this comment

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

Been running this for a few minutes and all seems well, the logging improvements are nice as it should give some more info as well

@felipecrs
Copy link
Contributor Author

Maybe we can decrease the time window to 30s?

@NickM-27
Copy link
Sponsor Collaborator

Maybe we can decrease the time window to 30s?

I think that would be fine, I definitely wouldn't go lower than that though. Curious what @blakeblackshear thinks

@blakeblackshear
Copy link
Owner

30s seems fine. I don't see any reason this would add significant overhead.

@blakeblackshear blakeblackshear merged commit a8c567d into blakeblackshear:dev Feb 19, 2023
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