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

Ready check does not include current database connectivity #831

Closed
3 of 6 tasks
Waidmann opened this issue Feb 7, 2022 · 14 comments · May be fixed by #1382
Closed
3 of 6 tasks

Ready check does not include current database connectivity #831

Waidmann opened this issue Feb 7, 2022 · 14 comments · May be fixed by #1382
Labels
bug Something is not working. good first issue A good issue to tackle when being a novice to the project. help wanted We are looking for help on this one.

Comments

@Waidmann
Copy link

Waidmann commented Feb 7, 2022

Preflight checklist

Describe the bug

The health/ready endpoint returns OK when database connectivity is no longer given. I would expect it to check this because according to the docs: This endpoint returns a 200 status code when the HTTP server is up running and the environment dependencies (e.g. the database) are responsive as well..

Reproducing the bug

  1. Setup postgres service in k8s cluster
  2. Deploy keto to cluster with dsn pointing to postgres service
  3. Kill postgres
  4. Call keto 'health/ready' endpoint -> Returns OK

However when I try to insert/query tuples I will obviously be greeted with an error code.

Relevant log output

No response

Relevant configuration

No response

Version

0.6.0-alpha.1

On which operating system are you observing this issue?

No response

In which environment are you deploying?

Kubernetes with Helm

Additional Context

No response

@Waidmann Waidmann added the bug Something is not working. label Feb 7, 2022
@zepatrik
Copy link
Member

Good point, that should really be the case.

@zepatrik zepatrik added good first issue A good issue to tackle when being a novice to the project. help wanted We are looking for help on this one. labels Feb 17, 2022
@zepatrik
Copy link
Member

The ready-checkers are registered here:

r.healthH = healthx.NewHandler(r.Writer(), config.Version, healthx.ReadyCheckers{})

Currently none are registered, which means that Keto appears healthy as soon as it runs.

@nickjn92
Copy link

From a kubernetes point of view, you dont want to include external dependencies, such as a database, in your readiness checks. Otherwise you might end up a in a cascading failures scenario where all pods are taken down and are unable to serve requests, and you are greeted with some generic error that does'nt really inform about whats causing the issue.
I believe the best practice is to rely on monitoring to determine whats causing the errors and if you need to wait for database to be up you can use a initContainer or lifecycle hooks

@zepatrik
Copy link
Member

Interesting standpoint, maybe @Demonsthere can give his opinion on this? Keto is generally not able to serve any request without a working database connection. Init migration jobs will also not complete, so you will end up in an error loop anyways on helm install.
But yeah, killing a pod just because the database is unavailable is also not helpful 🤔

@Demonsthere
Copy link
Contributor

Imho, from a deployment perspective:

  • keto should report a ready check once it has started and is running in a stable state, as we use a init job, which has to connect to the DB, and without which the keto main deployment won't even start, we kinda assume that if keto is running then the connection to db must have been working at least for the migration part. This could be improved with a verification in the ready check if we can open a connection to the DB
  • as for periodic health checks, imho a periodic downtime of the db can always happen, and as pointed we should not cascade restart all pods because of that, but maybe mark the pod as unhealthy with a more specific health check?

@zepatrik
Copy link
Member

Sounds good, so basically we would ping the database on startup and report as ready once that succeeded. Further ready checks will not ping the database again, but always return true.
Later we can add a check that pings the db periodically.

@mstrYoda
Copy link

In Kubernetes, we can define the failure threshold to retry before restarting pods. Also, we can define initialDelaySeconds to wait for some operational tasks to be complete before sending health/readiness requests.

IMHO, I think that adding database health check might be good as well.

@Demonsthere
Copy link
Contributor

In the helm charts the values for probes are exposed and can be configured to your liking :)

@Demonsthere
Copy link
Contributor

Edit: we actually run into a related issue some time ago 😅 which caused us to rethink the setup a bit. We now have exposed the option to change the probes to custom ones, as seen here in kratos, and will work on reworking the healthchecks in general

@aeneasr
Copy link
Member

aeneasr commented Jan 18, 2023

Isn't this solved now? I think one of the probes now checks DB connectivity

@zepatrik
Copy link
Member

They would have to be added here right?

r.healthH = healthx.NewHandler(r.Writer(), config.Version, healthx.ReadyCheckers{})

Maybe that was a different project, and we can transfer the change?

@aeneasr
Copy link
Member

aeneasr commented Jan 19, 2023

:O Yes, definitely, that needs to be checked! Otherwise we could run into an outage if we encounter one of those SQL connection bugs with cockroach that need a pod restart

https://github.com/ory/kratos/blob/4181fbc381b46df5cd79941f20fc885c7a1e1b47/driver/registry_default.go#L255-L273

@jonas-jonas

This comment was marked as duplicate.

@aran
Copy link

aran commented Dec 14, 2023

I just ran into an issue using Postgresql as backend, with calls to Keto reporting something like:

unable to fetch records...terminating connection due to administrator command (SQLSTATE 57P01) with gRPC code Unknown.

DB was up and retries didn't work. However, restarting the pod worked. I am wondering if there's a chance of this issue making it over the finish line?

aeneasr added a commit that referenced this issue Jul 4, 2024
@aeneasr aeneasr removed their assignment Sep 19, 2024
@aeneasr aeneasr closed this as completed Dec 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is not working. good first issue A good issue to tackle when being a novice to the project. help wanted We are looking for help on this one.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants