cartesi / rollups

Cartesi Rollups
30 stars 12 forks source link

Health check standardization #128

Closed GMKrieger closed 1 year ago

GMKrieger commented 1 year ago

Issue #29

This PR implements the functionality requested on the issue, that each service has its own health check setup, and that there's an explicit flag that enables or disables them.

There's more work to be done here, particularly in using Rust utilities to reduce repeated code. However, due to release schedules, this work will be done in another PR.

GMKrieger commented 1 year ago

The health check code for the host runner, inspect server, graphql server, and state server is missing. Hehehe

The servers have their own health check, we don't need to initiate another routine for them. I will add to the host runner.

gligneul commented 1 year ago

The servers have their own health check, we don't need to initiate another routine for them. I will add to the host runner.

I think we should standardize all services, regardless if they have a server or not.

GMKrieger commented 1 year ago

The servers have their own health check, we don't need to initiate another routine for them. I will add to the host runner.

I think we should standardize all services, regardless if they have a server or not.

Ok, so a couple of issues. First, the state-server config file is on the state-server registry, so it's not something we can easily modify here. The inspect-server doesn't use clap, but structopt; it's totally viable to make the changes, however it'll take more time.

Still waiting on what exactly is the implementation on the host-runner.

renan061 commented 1 year ago

True, let's not worry about the state-server for now.

endersonmaia commented 1 year ago

Sorry for the late response, I couldn´t review this, since I was out-of-office.

This --help output seems confusing to me:

❯ docker run -ti --rm ghcr.io/cartesi/rollups-indexer:main --help
...
      --no-indexer-healthcheck <HEALTHCHECK_DISABLED>
          Enable health check [env: HEALTHCHECK_DISABLED=]
      --indexer-healthcheck-port <HEALTHCHECK_PORT>
          Port of health check [env: HEALTHCHECK_PORT=] [default: 22024]

First, the --no-indexer-healthcheck doesn't match the HEALTHCHECK_DISABLED env variable name, which is a pattern used on other arguments.

Also the comment should be Disable health check.

Finally, the environment variables don't have a prefix and will collide with other services running on the same environment.

As we can see in the rollups-dispatcher --help output.

      --no-dispatcher-healthcheck <HEALTHCHECK_DISABLED>
          Enable health check [env: HEALTHCHECK_DISABLED=]
      --dispatcher-healthcheck-port <HEALTHCHECK_PORT>
          Port of health check [env: HEALTHCHECK_PORT=] [default: 22022]

I didn't exercise the services but just commented on the --help output.

gligneul commented 1 year ago

@endersonmaia, thanks for the feedback. I will take a look myself.

renan061 commented 1 year ago

141 fixes some of these and removes the repeated code.