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

Respond 200 on empty HTTP request to support GCE ingress health check #3308

Closed
wants to merge 1 commit into from

Conversation

tlongwell
Copy link

@tlongwell tlongwell commented Mar 19, 2020

@MarkusTeufelberger
Copy link
Collaborator

Why wouldn't you do your health checks on an actual health check endpoint instead?

@tlongwell
Copy link
Author

tlongwell commented Mar 19, 2020

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.

@MarkusTeufelberger
Copy link
Collaborator

A non 200 response would also indicate that it is running and accepting connections.

@tlongwell
Copy link
Author

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.

@MarkusTeufelberger
Copy link
Collaborator

You can set a path for readiness probes there, the /crawl endpoint for example would be a good workaround for this patch.

@MarkusTeufelberger
Copy link
Collaborator

See #2809 for some discussion on this topic by the way.

@intelliot
Copy link
Collaborator

Markus, that's a good idea. Docs are here: https://xrpl.org/peer-crawler.html

Example request: https://s1.ripple.com:51235/crawl

@carlhua
Copy link
Contributor

carlhua commented Mar 23, 2020

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

@intelliot
Copy link
Collaborator

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 /crawl endpoint does provide a 200 response:

% curl -I https://s1.ripple.com:51235/crawl
HTTP/1.1 200 OK
Server: rippled-1.4.0
Content-Type: application/json
Connection: close
Transfer-Encoding: chunked

@HowardHinnant
Copy link
Contributor

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.

@HowardHinnant HowardHinnant removed their request for review April 1, 2020 15:22
@carlhua carlhua requested a review from nbougalis April 3, 2020 13:51
@carlhua
Copy link
Contributor

carlhua commented Apr 17, 2020

@nbougalis do we want this as part of 1.6?

@nbougalis
Copy link
Contributor

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.

@tlongwell tlongwell closed this Apr 24, 2020
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.

6 participants