Skip to content

[Corollary to #4161] Redis Connection Check when Using Sentinel #4169

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

Merged

Conversation

dstarner
Copy link

@dstarner dstarner commented Feb 14, 2020

Fixes: #3985, #3984, and a bug introduced in #4161

Redis Sentinel support was adding to the settings and configuration, but the extras AppConfig was not updated to account for Sentinel when starting the application. Without this fix it shouldn't cause the application to crash, but to throw a false positive connection issue. This fixes the check to account for when sentinel is configured

@lampwins
Copy link
Contributor

I would actually be okay with ripping this check out. It isn't necessarily adding any value over a sanity check on application upstart. For instance, it does nothing to check that the connection is still open over time. Cacheops is already set to safely degrade if the connection fails and error messages will be logged for failed webhook requests on account of the redis connection.

@jeremystretch thoughts? This would also resolve #3985

@dstarner
Copy link
Author

I feel like if a check is ever needed (which i believe it shouldnt be) then it also shouldn't be in a blocking path that prevents local-only commands from running, like mentioned in #3985. If it's believed that the check can be fully removed, I don't mind reworking this PR to just remove the original check.

@dstarner
Copy link
Author

Or, I guess if we wanted to keep the check, in a similar fashion to IS_TESTING in the configuration, we could add either a REQUIRES_REDIS or LOCAL_ONLY variable that wraps the check...but that seems even hackier and it has a slight code smell to it.

@jeremystretch
Copy link
Member

I have no objection to removing the check entirely. I agree that it is not necessary.

@jeremystretch
Copy link
Member

We probably do want to add some error handling around webhooks at least, though. Without this check in place, taking any action which results in the queuing of a webhook will cause a server error if the Redis service is unreachable. I'm happy to work on that; just wanted to make a note of it here.

@jeremystretch jeremystretch merged commit 8d5ea5d into netbox-community:develop Feb 14, 2020
@lock lock bot locked as resolved and limited conversation to collaborators May 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

collectstatic now requires a redis server running
3 participants