Add Docker Healthcheck

[zstadler]

Fixes #635

[scara]

Hi @zstadler,
thanks for your contribution, sounds sensitive.

IMHO it would be nice that tileserve-gl would expose an /health endpoint charged to check its actual health status e.g. able to access the file system where mbtiles files should be accessed from and whatever the code requires to run healthy with the fastest and light code execution path.

That being said, why not using something like:

HEALTHCHECK CMD curl --fail http://localhost:80/health || exit 1

?
Guessing 'cause neither curl nor wget are bundled by default there and you don't want to add them i.e. just playing w/ node, already there.

HTH,
Matteo

[zstadler]

The healthcheck responses are in the code since #140, about 6 years ago. IMHO, changing them would be more of an issue.

I assume that curl was included in the Docker image at the time. This PR is trying to re-enable the Docker healthcheck with minimal backward incompatibility. This also means leaving curl out of the Docker image.

[acalcutt]

@zstadler did the docker have a healtcheck endpoint at some point? doesn't look loke #140 had one, but maybe one was added after?

For the health checking, how does docker respond if tileserver is still starting and is giving a 503? does docker giver a 'starting' healthcheck status?

[zstadler]

@acalcutt ,

Yes, there is a healthcheck endpoint for 6 years now. Commit 5d93b1d closed #140 by adding the endpoint. The code is there.

The JS code in this PR serves as a replacement for curl, which is not part of the Docker image.

[scara]

Hello Everyone,

The healthcheck responses are in the code since #140, about 6 years ago. IMHO, changing them would be more of an issue.

@zstadler I agree w/ you, just wondering if we want someday to add something more detailed to be used for readiness and liveness (and startup) K8s probes too. Docker Engine does nothing with those fine probes.
The current health check could be used, in K8s, for readiness/startup not for proper liveness.
BTW I understand that you want to introduce "just" the Docker HEALTHCHECK` here using the current health check.

The JS code in this PR serves as a replacement for curl, which is not part of the Docker image.

@zstadler That's fine and super sensitive 💪 to avoid adding binaries in the current image: 👍.

how does docker respond if tileserver is still starting and is giving a 503? does docker giver a 'starting' healthcheck status?

@acalcutt yes, it's the way it works, https://docs.docker.com/engine/reference/builder/#healthcheck:

When a container has a healthcheck specified, it has a health status in addition to its normal status. This status is initially starting. Whenever a health check passes, it becomes healthy (whatever state it was previously in). After a certain number of consecutive failures, it becomes unhealthy.

HTH,
Matteo

[zstadler]

That's fine and super sensitive 💪 to avoid adding binaries in the current image: 👍.

@scara,
Do you think the addition in this PR should be avoided? If so, please explain.

[scara]

Hi @zstadler,

Do you think the addition in this PR should be avoided? If so, please explain.

This PR should land for those running the official image on Docker Engines: it does what it should does w/o adding extra binaries (curl or wget or nc) to run the health check docker command.

HTH,
Matteo

[acalcutt]

Are the changes in .gitattributes needed for this PR? while it looks like a good idea, I'm not sure it goes with this and should be done in a separate PR unless it is needed.

Other than that I am likely to approve this, as long as my local testing works and there are no objections to it. I think allowing docker to use this built in health feature is a good idea.

[vinayakkulkarni]

Suggested change

	if (res.statusCode == 200) {
	if (res.statusCode === 200) {

[zstadler]

The type of the response StatusCode is a number. IMO, === is not needed for comparing that number with 200.

[zstadler]

@acalcutt,

Are the changes in .gitattributes needed for this PR? while it looks like a good idea, I'm not sure it goes with this and should be done in a separate PR unless it is needed.

I've created #637 as a separate PR for .gitattributes. Please merge it first.

[acalcutt]

I tested this and looked at the healthcheck status with docker inspect and it was getting this error.

        "Health": {
            "Status": "unhealthy",
            "FailingStreak": 5,
            "Log": [
                {
                    "Start": "2022-11-06T21:02:35.280366945-05:00",
                    "End": "2022-11-06T21:02:35.375671196-05:00",
                    "ExitCode": 1,
                    "Output": "file:///usr/src/app/src/healthcheck.js:1\nvar http = require(\"http\");\n           ^\n\nReferenceError: require is not defined in ES module scope, you can use import instead\nThis file is being treated as an ES module because it has a '.js' file extension and '/usr/src/app/package.json' contains \"type\": \"module\". To treat it as a CommonJS script, rename it to use the '.cjs' file extension.\n    at file:///usr/src/app/src/healthcheck.js:1:12\n    at ModuleJob.run (node:internal/modules/esm/module_job:193:25)\n    at async Promise.all (index 0)\n    at async ESMLoader.import (node:internal/modules/esm/loader:526:24)\n    at async loadESM (node:internal/process/esm_loader:91:5)\n    at async handleMainPromise (node:internal/modules/run_main:65:12)\n"
                },`

I changed the require to an import in healthcheck.js to fix this, and now it is successful

"Health": { "Status": "healthy", "FailingStreak": 0, "Log": [ { "Start": "2022-11-06T21:12:31.753993927-05:00", "End": "2022-11-06T21:12:31.871100779-05:00", "ExitCode": 0, "Output": "STATUS: 200\n" } ] }
`