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

Add support for user-defined healthchecks #22719

Closed
wants to merge 13 commits into
base: master
from

Conversation

Projects
None yet
@talex5
Contributor

talex5 commented May 13, 2016

This PR adds support for user-defined health-check probes for Docker containers. It adds a HEALTHCHECK instruction to the Dockerfile syntax plus some corresponding "docker run" options. It can be used with a restart policy to automatically restart a container if the check fails.

The HEALTHCHECK instruction has two forms (more may be added later, e.g. HTTP):

  • HEALTHCHECK [OPTIONS] CMD command (check container health by running a command inside the container)
  • HEALTHCHECK NONE (disable any healthcheck inherited from the base image)

The HEALTHCHECK instruction tells Docker how to test a container to check that it is still working. This can detect cases such as a web server that is stuck in an infinite loop and unable to handle new connections, even though the server process is still running.

When a container has a healthcheck specified, it has a health status in addition to its normal status. This status is initially starting. When a health check passes, it becomes healthy. After a certain number of failures, it becomes unhealthy.

The options that can appear before CMD are:

  • --interval=DURATION (default: 30s)
  • --timeout=DURATION (default: 30s)
  • --grace=DURATION (default: 30s)
  • --retries=N (default: 1)
  • --exit-on-unhealthy=X (default: true)

The health check will first run interval seconds after the container is started, and then again interval seconds after each previous check completes.

If a single run of the check takes longer than timeout seconds then the check is considered to have failed.

If the health state is starting and a check started within grace seconds of the container's start time fails, the failure is ignored and the health state remains starting.

It takes retries consecutive failures of the health check for the container to be considered unhealthy.

If --exit-on-unhealthy is true then the container will exit as soon as it becomes unhealthy. The container may then be automatically restarted, depending on its restart policy.

For example, to check every five minutes or so that a web-server is able to serve the site's main page within three seconds:

HEALTHCHECK --interval=5m --grace=20s --timeout=3s --exit-on-unhealthy \
  CMD curl -f http://localhost/

(from https://github.com/talex5/docker/blob/healthcheck/docs/reference/builder.md#healthcheck)

The changes to "docker run" are described here:

https://github.com/talex5/docker/blob/healthcheck/docs/reference/run.md#healthcheck

Example

$ docker run --name=test -d \
    --health-cmd='stat /etc/passwd' \
    --health-interval=2s \
    --exit-on-unhealthy=false \
    busybox sleep 1d
$ sleep 2; docker inspect --format='{{.State.Health.Status}}' test
healthy
$ docker exec test rm /etc/passwd
$ sleep 2; docker inspect --format='{{json .State.Health}}' test
{
  "Status":"unhealthy",
  "FailingStreak":1,
  "LastCheckStart":"2016-05-09T11:09:09.673709108Z",
  "LastCheckEnd":"2016-05-09T11:09:09.786146142Z",
  "LastExitCode":1,
  "LastOutput":"stat: can't stat '/etc/passwd': No such file or directory\n"
}

The health status is also displayed in the docker ps output.

Description for the changelog: Add support for user-defined healthchecks

Closes #21142 and #21143.

@thaJeztah

This comment has been minimized.

Member

thaJeztah commented May 13, 2016

thanks @talex5!

ping @icecrime 😄

The options that can appear before `CMD` are:
* `--interval=DURATION` (default: `30s`)

This comment has been minimized.

@cpuguy83

cpuguy83 May 13, 2016

Contributor

These all seem to be runtime configurations that ought not be in the Dockerfile.

This comment has been minimized.

@justincormack

justincormack May 13, 2016

Contributor

It does seem a bit problematic. I might test my container on a fast unloaded machine with SSD and then you run it on a slow loaded machine with a hard drive and the startup time is much longer so it fails the health check (eg mysql seems to take a long time to start serving I noticed recently). It does seem odd to have values that are related to runtime performance in the dockerfile.

This comment has been minimized.

@stevvooe

stevvooe May 13, 2016

Contributor

delay?

This comment has been minimized.

@cpuguy83

cpuguy83 May 16, 2016

Contributor

I think I'm really a hard -1 on these flags in the Dockerfile.

The `HEALTHCHECK` instruction has two forms:
* `HEALTHCHECK [OPTIONS] CMD command` (check container health by running a command inside the container)

This comment has been minimized.

@cpuguy83

cpuguy83 May 13, 2016

Contributor

Having CMD here is really confusing.

I think it would be better have simply HEALTHCHECK <cmd> that would be the default health checking command, kind of like ENTRYPOINT but for healthchecks.

This comment has been minimized.

@talex5

talex5 May 13, 2016

Contributor

There will likely be other types in future. e.g. HEALTHCHECK HTTP port=80 or somesuch. Without the CMD, it will be hard to extend the syntax.

This comment has been minimized.

@tianon

tianon May 15, 2016

Member

Maybe RUN instead? Or go with simpler lowercase "types" (like "run") so that they clash less visually with other instructions?

This comment has been minimized.

@cpuguy83

cpuguy83 May 16, 2016

Contributor

Ok, I understand the case for CMD here.

@@ -5,6 +5,20 @@ import (
"github.com/docker/go-connections/nat"
)
// HealthConfig holds configuration settings for the HEALTHCHECK feature.
// For durations, a value of -1 means to inherit the default value.

This comment has been minimized.

@cpuguy83

cpuguy83 May 13, 2016

Contributor

Instead of the magic number, can the durations be pointers? nil Means inherit.

This comment has been minimized.

@talex5

talex5 May 16, 2016

Contributor

Done. Optional floats are now represented by nil.

This comment has been minimized.

@talex5

talex5 May 25, 2016

Contributor

At the request of @stevvooe I've removed the pointers again. Now that grace isn't an option, 0 is no longer a valid duration for any option and we can use zero for inherit.

@@ -24,6 +38,7 @@ type Config struct {
StdinOnce bool // If true, close stdin after the 1 attached client disconnects.
Env []string // List of environment variable to set in the container
Cmd strslice.StrSlice // Command to run when starting the container
Healthcheck *HealthConfig `json:",omitempty"` // Command to run to check container is healthy

This comment has been minimized.

@cpuguy83

cpuguy83 May 13, 2016

Contributor

Only 1 health check per container? Do you see a reason why someone would want more than 1?

I can think at least testing multiple listening ports.

This comment has been minimized.

@thaJeztah

thaJeztah May 13, 2016

Member

Wouldn't those be combined? (just thinking)

This comment has been minimized.

@justincormack

justincormack May 13, 2016

Contributor

You could with a script, but I guess just using curl as your healthcheck is quite nice.

This comment has been minimized.

@crosbymichael

crosbymichael May 13, 2016

Member

Why does this have an omitempty? How does it get marshaled to json and sent to the server?

This comment has been minimized.

@stevvooe

stevvooe May 13, 2016

Contributor

@justincormack I would expect support for multiple HEALTHCHECK clauses in a Dockerfile. Any reason not to?

This comment has been minimized.

@justincormack

justincormack May 13, 2016

Contributor

On 13 May 2016 21:20, "Stephen Day" notifications@github.com wrote:

In vendor/src/github.com/docker/engine-api/types/container/config.go:

@@ -24,6 +38,7 @@ type Config struct {
StdinOnce bool // If true, close stdin after
the 1 attached client disconnects.
Env []string // List of environment variable
to set in the container
Cmd strslice.StrSlice // Command to run when starting
the container

  • Healthcheck *HealthConfig json:",omitempty" // Command
    to run to check container is healthy

@justincormack I would expect support for multiple HEALTHCHECK clauses in
a Dockerfile. Any reason not to?

How would you then eg override one of them? Maybe it should be an array of
checks so you have to replace all of them?

This comment has been minimized.

@stevvooe

stevvooe May 13, 2016

Contributor

I see complexity this can bring on. I think the issue is that you may want to define a tcp, http and cmd based healthcheck. Perhaps, it is sufficient to let these coexist in a single command, eliding the need to have complex merge semantics of health declarations (and having to name the health checks to tell them apart).

This comment has been minimized.

@talex5

talex5 May 16, 2016

Contributor

@crosbymichael omitempty only omits the value if it's empty (nil). If it contains information, it will be converted to JSON.

This comment has been minimized.

@crosbymichael

crosbymichael May 16, 2016

Member

@talex5 thanks, i blanked out on this one, its correct

@cpuguy83

This comment has been minimized.

Contributor

cpuguy83 commented May 13, 2016

There doesn't seem to be anything documented on the protocol between docker and the probe.
I imagine this is just exit statuses, but would be good to document.

Do we want this to be a simple 0 == OK, not zero == bad or more robust ala nagios-style checks (ok, warning, critical, and status messages for reporting to the user)?

@cpuguy83

This comment has been minimized.

Contributor

cpuguy83 commented May 13, 2016

Images also often define the ports they listening on (EXPOSE), would we want to provide a flag that runs some default check on these ports (like open a TCP conn)? This would be similar to how -P automatically creates port forwards for all EXPOSE'd ports.

// exec the healthcheck command in the container.
// Returns the exit code and error message (if any)
func (*cmdProbe) run(ctx context.Context, d *Daemon, container *container.Container) (int, string) {

This comment has been minimized.

@crosbymichael

crosbymichael May 13, 2016

Member

In go, your second return value should be error here and not return strings

This comment has been minimized.

@talex5

talex5 May 16, 2016

Contributor

The error here is normally the error string returned by the probe rather than a Go error, although there are a few places where I handle the (unlikely) case of a Go error starting the probe by pretending that the error came from the probe itself. If we return these errors as a third return value, we'll just turn it into a string at the next opportunity anyway.

This comment has been minimized.

@stevvooe

stevvooe May 16, 2016

Contributor

@talex5 Errors in Go are just values. You can add the error content to the error that makes sense for the application. I would expect these to build on exec.ExitError or define a special error type to encapsulate the error. Use a stringly-typed approach is just calling for bugs.

This comment has been minimized.

@crosbymichael

crosbymichael May 16, 2016

Member

Ya, this return value should be an error 100%. Even if it is just a string you can use errors.New("my string") or fmt.Errorf("my %s", "string") but you should not return string especially for how you are using it.

Timeout float64 // Time to wait before considering the check to have hung.
Retries uint // Number of consecutive failures needed to consider a container as unhealthy.
// 0 means default.
ExitOnUnhealthy *bool `json:",omitempty"` // Stop container on entering unhealthy state. nil for default.

This comment has been minimized.

@crosbymichael

crosbymichael May 13, 2016

Member

Is there a reason why this is a nilable option? They way I see it is that its a yes or no entry and it being nil does not mean anything. I think it would be better if this could not be nil.

This comment has been minimized.

@stevvooe

stevvooe May 13, 2016

Contributor

I'm not sure this option should even be present...

Either way, default of false is sensible, so no need for the pointer.

This comment has been minimized.

@talex5

talex5 May 16, 2016

Contributor

(note: @aluzzardi asked for the default to be changed to true)

An image can turn this on or off. The user can set it on or off or inherit the default from the image, so we need three states here.

// OpenMonitorChannel creates and returns a new monitor channel. If there already is one,
// it returns nil.
func (s *Health) OpenMonitorChannel() chan struct{} {
if s.stop == nil {

This comment has been minimized.

@crosbymichael

crosbymichael May 13, 2016

Member

This is racy, you will need to protect this with a lock

This comment has been minimized.

@talex5

talex5 May 16, 2016

Contributor

Opening and closing the channel only happens from the event monitor thread, which already runs with the container locked.

This comment has been minimized.

@crosbymichael
// This channel does not buffer. Once the write succeeds, the monitor
// has read the stop request and will not make any further updates
// to c.State.Health.
s.stop <- struct{}{}

This comment has been minimized.

@crosbymichael

crosbymichael May 13, 2016

Member

You can just do a close(s.stop) then set it to nil instead of sending an empty struct

This comment has been minimized.

@talex5

talex5 May 16, 2016

Contributor

Using close will not ensure that the probe thread has stopped before continuing, leading to a possible race if it is just about to write to the health structure.

This comment has been minimized.

@crosbymichael

crosbymichael May 16, 2016

Member

Ok, i see how its being used

@@ -33,14 +33,14 @@ type copyBackend interface {
// stateBackend includes functions to implement to provide container state lifecycle functionality.
type stateBackend interface {
ContainerCreate(types.ContainerCreateConfig) (types.ContainerCreateResponse, error)
ContainerKill(name string, sig uint64) error
ContainerKill(name string, sig uint64, cancelRestartManager bool) error

This comment has been minimized.

@stevvooe

stevvooe May 13, 2016

Contributor

Make an options structure for this.

This comment has been minimized.

@talex5

talex5 May 16, 2016

Contributor

Done.

ContainerPause(name string) error
ContainerRename(oldName, newName string) error
ContainerResize(name string, height, width int) error
ContainerRestart(name string, seconds int) error
ContainerRm(name string, config *types.ContainerRmConfig) error
ContainerStart(name string, hostConfig *container.HostConfig) error
ContainerStop(name string, seconds int) error
ContainerStop(name string, seconds int, cancelRestartManager bool) error

This comment has been minimized.

@stevvooe

stevvooe May 13, 2016

Contributor

Make an options structure for this.

This comment has been minimized.

@stevvooe

stevvooe May 13, 2016

Contributor

Or at least document what cancelRestartManager means.

@@ -22,15 +22,16 @@ import (
)
var validCommitCommands = map[string]bool{

This comment has been minimized.

@stevvooe

stevvooe May 13, 2016

Contributor

Lose the bool type here and save some space: use struct{}.

This comment has been minimized.

@talex5

talex5 May 16, 2016

Contributor

The PR does not change this code. If you want to clean up nearby code, I it think should be done in a separate PR.

// HealthConfig holds configuration settings for the HEALTHCHECK feature.
// For durations, a value of -1 means to inherit the default value.
type HealthConfig struct {
Test strslice.StrSlice // Test to perform to check container is healthy.

This comment has been minimized.

@stevvooe

stevvooe May 13, 2016

Contributor

Kind, Type, Mode. Test doesn't really work here.

This comment has been minimized.

@talex5

talex5 May 16, 2016

Contributor

It's not just the kind. It's something like {"CMD", "check.sh"},

This comment has been minimized.

@stevvooe

stevvooe May 16, 2016

Contributor

That is no good. We don't want to multi-band this slice. There should be a separate type and data field.

GracePeriod float64 // Time after container start to assume failure means still starting.
Interval float64 // Time to wait between checks.
Timeout float64 // Time to wait before considering the check to have hung.
Retries uint // Number of consecutive failures needed to consider a container as unhealthy.

This comment has been minimized.

@stevvooe

stevvooe May 13, 2016

Contributor

No need for unsigned, just use int.

This comment has been minimized.

@talex5

talex5 May 16, 2016

Contributor

Why? Then I'd have to handle negative values somehow, and it's not clear what that would mean.

This comment has been minimized.

@stevvooe

stevvooe May 16, 2016

Contributor

You don't. There is no need for uint here. It just makes other code awkward.

Test strslice.StrSlice // Test to perform to check container is healthy.
// An empty slice means to inherit the default.
// Otherwise, Cmd[0] is "NONE" or "CMD".
GracePeriod float64 // Time after container start to assume failure means still starting.

This comment has been minimized.

@stevvooe

stevvooe May 13, 2016

Contributor

Please use time.Duration.

This comment has been minimized.

@talex5

talex5 May 16, 2016

Contributor

Unfortunately, Go converts durations to JSON by printing its internal format (integer nanoseconds), which would leak an implementation detail of the Go language into the protocol, so I converted to floating-point seconds instead as a more standard unit.

This comment has been minimized.

@stevvooe

stevvooe May 16, 2016

Contributor

Please use time.Duration. The integer nanoseconds format is portable and the time.Duration type's behavior is safe and well-known.

type Health struct {
Status string // States are: "starting", "healthy", "unhealthy"
FailingStreak uint64 // Number of consecutive failures
LastCheckStart time.Time // Zero if never started

This comment has been minimized.

@stevvooe

stevvooe May 13, 2016

Contributor

Create subtype Last, with these fields:

type HealthcheckResult {
  Start, End time.Time
  ExitCode int
  Output []byte // string may not be right type.
}

Then, use a slice for the latest n results:

type Health struct {
  Results []HealthCheckResult
}

Contents of results can be managed by policy or tweaked for ideal use case. Things like last N failures can be calculated by looking at entries.

This comment has been minimized.

@stevvooe

stevvooe May 13, 2016

Contributor

Also, may want to include an Err error field to keep track of any errors resolving health.

* `--timeout=DURATION` (default: `30s`)
* `--grace=DURATION` (default: `30s`)
* `--retries=N` (default: `1`)
* `--exit-on-unhealthy=X` (default: `true`)

This comment has been minimized.

@stevvooe

stevvooe May 13, 2016

Contributor

We can probably lose the preposition here. In general, I find the wording on this field to be awkward but I don't have a better suggestion.

This comment has been minimized.

@stevvooe

stevvooe May 16, 2016

Contributor

After some thought, this policy really belongs on the restart policy. The health check should just be about reporting status. The interpretation of the status should be part of the restart policy.

@@ -0,0 +1,9 @@
(from "debian")

This comment has been minimized.

@stevvooe

stevvooe May 13, 2016

Contributor

Disappointing that s-expressions have worked their way in. These should be table based tests but that seems to be out of scope of this review.

@aluzzardi

This comment has been minimized.

Member

aluzzardi commented May 31, 2016

You can have a []byte but it gets base64 encoded by the marshaler.

That's true in Go but it's very language specific.

What I meant by "you can't have a []byte there" is, at the end, that field is going to be a string no matter what. Whether we base64 encode or not is another question.

How do we currently encode output for other commands (e.g. logs)?

@stevvooe

This comment has been minimized.

Contributor

stevvooe commented May 31, 2016

What if we did Raw with []byte and Output with string?

@thaJeztah

This comment has been minimized.

Member

thaJeztah commented May 31, 2016

What if we did Raw with []byte and Output with string?

SGTM, would give both options; would Raw be limited in size?

@thaJeztah

This comment has been minimized.

Member

thaJeztah commented Jun 1, 2016

ping @aluzzardi @crosbymichael WDYT on @stevvooe's proposal?

Remove the arbitrary output limitation of 255 bytes
Use 4096 instead.

Signed-off-by: Thomas Leonard <thomas.leonard@docker.com>
@talex5

This comment has been minimized.

Contributor

talex5 commented Jun 1, 2016

I'm not sure what you mean about having both. Would we include the same data twice in each message, or somehow know where to send which data?

Another possibility: if a probe wants to generate binary data then it base64 encodes it itself. That way, probes that only want text don't need to do anything and the messages are human-readable. Probes that return binary data would need something at the client end to interpret it anyway, so decoding it wouldn't be much extra work. The data on the wire would be the same as if we'd used []byte in that case.

@stevvooe

This comment has been minimized.

Contributor

stevvooe commented Jun 1, 2016

@talex5 Yes, have both. One that is "human-readable", and hopefully properly sanitized, and one that is the raw, unprocessed output.

This will meet both the use cases of sending binary data and debugging problems with health check output during development. It is unreasonable to make users go through so much work just to get unprocessed command output.

Let's make sure this feature isn't limited to our imaginations but, instead, enables others'.

@cpuguy83

This comment has been minimized.

Contributor

cpuguy83 commented Jun 2, 2016

I do not see the purpose of supporting more than just simple output from a healthcheck, and certainly not storing both binary and human-readable....

I think we should support only something very simple for a human to use as debug info, or potentially remove support for capturing output from the healthcheck from this PR and implement separately.

- 0: success - the container is healthy and ready for use
- 1: unhealthy - the container is not working correctly
- 2: starting - the container is not ready for use yet, but is working correctly

This comment has been minimized.

@cpuguy83

cpuguy83 Jun 2, 2016

Contributor

Isn't this state really just any failure before the first success?
I would recommend removing it.

This comment has been minimized.

@talex5

talex5 Jun 2, 2016

Contributor

Without this, whatever is monitoring the status will need some timeout to decide that the container has failed to start. @justincormack (I think) pointed out that a simple timeout is not enough to cover e.g. a database that is busy performing recovery actions. So the idea of this state is to allow the container to tell the monitor that it needs more time, and avoid the monitor having to guess what the grace period should be.

This comment has been minimized.

@dannc

dannc Jun 2, 2016

A nit about the labels/description attached to the return codes. Some containers are "run once and die" in that they produce some artifact and exit. Take for example a container that generates a TLS certificate and writes it to a volume, which other containers subsequently mount in order to use the certificate.

I would propose something like:
0: succeeded - the container is healthy or has exited successfully
1: failed - the container is unhealthy or has exited unsuccessfully
2: starting (in-progress?) - the container is not ready for use yet or has not completed its task

The options that can appear before `CMD` are:
* `--interval=DURATION` (default: `30s`)

This comment has been minimized.

@cpuguy83

cpuguy83 Jun 2, 2016

Contributor

Did we really decide to keep these runtime related flags?

This comment has been minimized.

@talex5

talex5 Jun 2, 2016

Contributor

We agreed to get rid of grace, but I'm happy to remove the others too.

@crosbymichael

This comment has been minimized.

Member

crosbymichael commented Jun 2, 2016

For the output, in this PR we should keep it as is. The size has already been increased to 4096 which is much better than what it was before. Most commands that do health checks should be encouraged to write a simple reason why they are reporting as they did. "I returned unhealthy BECAUSE i could not connect to the database." We don't want to encourage checks to do a yolo operation and puke up some stacktrace to decipher.

println("could not connect to database");
return 1;

In the future after this has some user and they are wanting some type of more structured response than a limited string it would be effortless to add an unbounded []byte array to the output for this. It is something that is safe to defer until we gain feedback.

For the three states, simple health checks don't have to use the starting state if they don't want to. To the normal developer they can just return 1 or 0. If the app has a long startup or some type of complexity on boot then it can take advantage of this state and we don't have to infer any type of guesses that we would otherwise be doing. Lets to write software that guesses things, let the things that know the state tell us. You are not forced to use this state if you don't want to.

After looking at the code again, I think all these things are already implemented.

LGTM

@thaJeztah

This comment has been minimized.

Member

thaJeztah commented Jun 2, 2016

ok, enough bike-shedding. Let's go for it LGTM

ADD check.sh main.sh /app/
CMD /app/main.sh
HEALTHCHECK
HEALTHCHECK --interval=5s --timeout=3s --retries=1 \

This comment has been minimized.

@dongluochen

dongluochen Jun 2, 2016

Contributor

nit: I think a good practice is multiple failures like 3 to tolerate random failures. Giving an example of --retries=1 might not be appropriate.

This comment has been minimized.

@dongluochen

dongluochen Jun 2, 2016

Contributor

I suggest we set default retries to 3.

@dongluochen

This comment has been minimized.

Contributor

dongluochen commented Jun 2, 2016

PR looks good to me.

@thaJeztah

This comment has been minimized.

Member

thaJeztah commented Jun 2, 2016

@dongluochen I carried the PR, rebased/squashed and with vendor update in #23218. Let me know if you need those addressed in the PR, otherwise I'll do a follow-up

@dongluochen

This comment has been minimized.

Contributor

dongluochen commented Jun 2, 2016

Thanks @thaJeztah. Follow-up is good.

@mageddo

This comment has been minimized.

mageddo commented Jul 26, 2017

Guys what about the restart policy? I think that is very important, the reason was explained here. So will this feature be implemented?

@stevvooe

This comment has been minimized.

Contributor

stevvooe commented Jul 26, 2017

@mageddo This is a long closed pull request, carried elsewhere. If you have questions or a feature request, please file a new issue.

@thaJeztah

This comment has been minimized.

Member

thaJeztah commented Jul 26, 2017

Locking the conversation in this issue for the reason @stevvoe mentioned above; comments on closed issues and PRs easily go unnoticed - I'm locking the conversation to prevent that from happening

@moby moby locked and limited conversation to collaborators Jul 26, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.