contrib: update k8s liveness and readiness probes #4405
Conversation
* Fix liveness probe for dgraph alpha. * Add liveness probe to dgraph zero. * Update k8s manifests to include both liveness and readiness probes. Signed-off-by: fristonio <deepshpathak@gmail.com>
fristonio
requested review from
danielmai,
manishrjain and
a team
as code owners
dgraph/cmd/zero/http.go
Outdated
| x.AddCorsHeaders(w) | ||
|
|
||
| w.WriteHeader(http.StatusOK) | ||
| w.Write([]byte("OK")) |
There was a problem hiding this comment.
Error return value of w.Write is not checked (from errcheck)
Signed-off-by: fristonio <deepshpathak@gmail.com>
There was a problem hiding this comment.
The title of this PR is a bit misleading. It adds a /health endpoint to Zero and changes the /health endpoint for Dgraph Alpha. The PR name and description should reflect that.
The k8s configs should continue working for the latest release for Dgraph Alpha. We can either split up the K8s config change to a separate PR and wait for the next release to support these new endpoints, or we update the docs to link to the K8s config that's on the current github.com/dgraph-io/dgraph/tree/release/$latestversion branch instead of github.com/dgraph-io/dgraph/tree/master.
Reviewable status: 0 of 5 files reviewed, 2 unresolved discussions (waiting on @danielmai, @fristonio, and @manishrjain)
dgraph/cmd/alpha/run.go, line 280 at r2 (raw file):
x.AddCorsHeaders(w) _, ok := r.URL.Query()["live"]
Why is a live query parameter necessary here? Why not have health checking as plain /health?
There was a problem hiding this comment.
The title of this PR is a bit misleading. It adds a /health endpoint to Zero and changes the /health endpoint for Dgraph Alpha. The PR name and description should reflect that.
I will update the PR title and pull request description to better explain the changes.
We can either split up the K8s config change to a separate PR and wait for the next release to support these new endpoints, or we update the docs to link to the K8s config that's on the current.
I think it makes more sense if the docs point to the release version of the dgraph rather than master, for any documentation change or issue fixes we can backport them to the required version and they would still reflect in the documentation.
Reviewable status: 0 of 5 files reviewed, 2 unresolved discussions (waiting on @danielmai, @fristonio, and @manishrjain)
dgraph/cmd/alpha/run.go, line 280 at r2 (raw file):
Previously, danielmai (Daniel Mai) wrote…
Why is a
livequery parameter necessary here? Why not have health checking as plain/health?
We need two separate endpoints, one for liveness and one for readiness. The live get query parameter describes if the request is for liveness check or readiness. For liveness check, we assume that if the webserver is able to serve any traffic it is live and for this, we make a request with live GET parameter. For readiness, however, we take into consideration the fact that the server in some cases is not able to serve traffic even if the container is live, like when bootstrapping badger, or sharing raft state snapshot, etc. This approach is similar to what cockroach DB has. You can take a look at #4383 for discussion around this.
fristonio
changed the title
There was a problem hiding this comment.
I agree that we should make the links specific to the docs version.
Reviewable status: 0 of 5 files reviewed, 2 unresolved discussions (waiting on @fristonio and @manishrjain)
dgraph/cmd/alpha/run.go, line 280 at r2 (raw file):
Previously, fristonio (Deepesh Pathak) wrote…
We need two separate endpoints, one for liveness and one for readiness. The
liveget query parameter describes if the request is for liveness check or readiness. For liveness check, we assume that if the webserver is able to serve any traffic it is live and for this, we make a request withliveGET parameter. For readiness, however, we take into consideration the fact that the server in some cases is not able to serve traffic even if the container is live, like when bootstrapping badger, or sharing raft state snapshot, etc. This approach is similar to what cockroach DB has. You can take a look at #4383 for discussion around this.
Understood, thanks.
* Fix liveness probe for dgraph alpha. * Add liveness probe to dgraph zero. * Update k8s manifests to include both liveness and readiness probes. (cherry picked from commit ed072d0)
- Adding health endpoints to v1.1.x Signed-off-by: Prashant Shahi <coolboi567@gmail.com>
The liveness probe can be triggered by providing a
live=1GET parameter to the existing/healthendpoint. The/healthendpoint, however, corresponds to a readiness probe. This is similar to how cockroach DB handles these probes.This will require an update in the dgraph docker image
dgraph/dgraph.Fixes #4378
This change is