-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Respond 200 on empty HTTP request to support GCE ingress health check #3308
Conversation
Why wouldn't you do your health checks on an actual health check endpoint instead? |
Having a public HTTP health check endpoint would be a great feature. It should use some heuristic (like info from server_info) to return a meaningful response in addition to a status code. This 200 is just an indication that the rippled server is running and accepting connections on a particular port. It is not meant to denote health wrt network synchronization/load/etc. This was a suggestion from @nbougalis as a one-off build for a particular use case I have. My biggest motivation for opening the PR is really to have Travis run and see if this unexpectedly breaks anything. |
A non 200 response would also indicate that it is running and accepting connections. |
Responding with a 200 allows this dumb endpoint to be used with a GKE ingress https://cloud.google.com/kubernetes-engine/docs/concepts/ingress#health_checks The ingress allows changing the path of the health check, but not the accepted response code. The response code must be exactly 200. |
You can set a path for readiness probes there, the /crawl endpoint for example would be a good workaround for this patch. |
See #2809 for some discussion on this topic by the way. |
Markus, that's a good idea. Docs are here: https://xrpl.org/peer-crawler.html Example request: |
@MarkusTeufelberger @intelliot I really appreciate the feedback. I will prioritize #2809 to be discussed in the near future. This fix is simply to provide a 200 response for the load balancer. Lets review this PR while #2809 undergoes discussion. |
If this is for a load balancer, I think it might make sense to return 200 only if the server is synced with the network. Sidenote: the
|
Removing myself as reviewer. The C++ looks fine. But I'm not qualified to say if this is something that should be done for rippled. |
@nbougalis do we want this as part of 1.6? |
I think we should fold the idea behind this into #3365 and consider following the guidelines provided by @MarkusTeufelberger in that thread. I'd suggest that we just close this. |
#3307