Docker Support for Consul Health Checks #1343

diptanu · 2015-10-26T20:11:14Z

This PR adds support for running script based health checks inside Docker Containers.

Users have to configure the checks by passing the container id, the script to run and the shell. We default to /bin/sh or override that with $SHELL in case the user doesn't specify the shell while configuring the check.

Ex -

{
  "check": {
    "id": "mem-util",
    "name": "Memory utilization",
    "script": "/usr/local/bin/check_mem.py",
    "docker_container_id": "54432bad1fc77bc030943de76e037c0a95a70e0b24da27570750f92b5e682b62",
    "shell": "/bin/bash",
    "interval": "10s"
  }
}

…'t respond while running checks

salehe · 2015-10-26T20:17:07Z

I'm asking out of curiosity. How is this different from using "docker exec" in a standard script check?

diptanu · 2015-10-26T20:21:47Z

@salehe Not very different at all, but that depends on users generating scripts for every container running on the system. This exposes an API which makes this easy to run checks programatically, for example Nomad could use Consul agent to run health checks for a container that it is managing.

slackpad · 2015-10-26T20:32:15Z

command/agent/agent.go

+				Logger:            a.logger,
+			}
+			dockerCheck.Start()
+			a.checkDockers[check.CheckID] = dockerCheck
 		}


I'd add a final else that returns an error that the type is unknown.

@slackpad The previous behaviour defaulted to Script check, perhaps we should keep that?

Yeah that would be ok, too.

slackpad · 2015-10-26T21:53:34Z

One other thing - will need to updates the docs as well - it's all under /website.

… find out if the check is a Docker check

slackpad · 2015-10-26T23:15:50Z

command/agent/check.go

@@ -64,7 +68,7 @@ func (c *CheckType) IsTTL() bool {

 // IsMonitor checks if this is a Monitor type
 func (c *CheckType) IsMonitor() bool {
-	return c.Script != "" && c.Interval != 0
+	return c.Script != "" && c.DockerContainerId == "" && c.Interval != 0


Actually this made me realize how sloppy the current else is. We should probably add the script check back to Docker and make an explicit check for IsMonitor and have an else that fails. Right now we will roll into creating a monitor when these requirements aren't met. Sorry for the flip-flop on this one.

slackpad · 2015-10-26T23:31:31Z

command/agent/check.go

+	// Collect the output
+	output, _ := circbuf.NewBuffer(CheckBufSize)
+
+	err = c.dockerClient.StartExec(exec.ID, docker.StartExecOptions{Detach: false, Tty: false, OutputStream: output, ErrorStream: output})


Is this Tty option redundant with the one above? Also, does OutputStream trump the Attach* ones?

Is this Tty option redundant with the one above?

I think this is an idiosyncrasy of the API. When you run via CLI some of these actions are merged (e.g. creating the exec and starting the exec) but via the API they are separate actions so you end up specifying the same setting twice. I haven't traced this all the way through but I think it's fine to specify twice for now.

slackpad · 2015-10-26T23:42:31Z

command/agent/check_test.go

+}
+
+func TestDockerCheckWhenExecReturnsSuccessExitCode(t *testing.T) {
+	expectDockerCheckStatus(t, &fakeDockerClientWithNoErrors{}, "passing")


Would be good to throw in a warning case as well, since there's a code path for it.

slackpad · 2015-10-26T23:45:15Z

command/agent/check_test.go

+	mock := &MockNotify{
+		state:   make(map[string]string),
+		updates: make(map[string]int),
+		output:  make(map[string]string),


Would it be pretty easy to make sure the output gets plumbed through? With the mock it's kind of lame but it would test the basic logic of the check.

Docker Support for Consul Health Checks

armon · 2015-10-27T16:18:08Z

LGTM @diptanu @slackpad Can we make sure the website docs get updated for this change as well

slackpad · 2015-10-27T21:55:17Z

@diptanu it looks like this change broke some of the BSD targets (from a full make):

8 errors occurred:
--> freebsd/amd64 error: exit status 2
Stderr: # github.com/fsouza/go-dockerclient/external/github.com/Sirupsen/logrus
../../fsouza/go-dockerclient/external/github.com/Sirupsen/logrus/terminal_freebsd.go:10: ioctlReadTermios redeclared in this block
        previous declaration at ../../fsouza/go-dockerclient/external/github.com/Sirupsen/logrus/terminal_bsd.go:7
../../fsouza/go-dockerclient/external/github.com/Sirupsen/logrus/terminal_freebsd.go:12: Termios redeclared in this block
        previous declaration at ../../fsouza/go-dockerclient/external/github.com/Sirupsen/logrus/terminal_bsd.go:9

--> netbsd/386 error: exit status 2
Stderr: # github.com/fsouza/go-dockerclient/external/github.com/Sirupsen/logrus
../../fsouza/go-dockerclient/external/github.com/Sirupsen/logrus/text_formatter.go:28: undefined: IsTerminal

--> freebsd/arm error: exit status 2
Stderr: # github.com/fsouza/go-dockerclient/external/github.com/Sirupsen/logrus
../../fsouza/go-dockerclient/external/github.com/Sirupsen/logrus/terminal_freebsd.go:10: ioctlReadTermios redeclared in this block
        previous declaration at ../../fsouza/go-dockerclient/external/github.com/Sirupsen/logrus/terminal_bsd.go:7
../../fsouza/go-dockerclient/external/github.com/Sirupsen/logrus/terminal_freebsd.go:12: Termios redeclared in this block
        previous declaration at ../../fsouza/go-dockerclient/external/github.com/Sirupsen/logrus/terminal_bsd.go:9

--> freebsd/386 error: exit status 2
Stderr: # github.com/fsouza/go-dockerclient/external/github.com/Sirupsen/logrus
../../fsouza/go-dockerclient/external/github.com/Sirupsen/logrus/terminal_freebsd.go:10: ioctlReadTermios redeclared in this block
        previous declaration at ../../fsouza/go-dockerclient/external/github.com/Sirupsen/logrus/terminal_bsd.go:7
../../fsouza/go-dockerclient/external/github.com/Sirupsen/logrus/terminal_freebsd.go:12: Termios redeclared in this block
        previous declaration at ../../fsouza/go-dockerclient/external/github.com/Sirupsen/logrus/terminal_bsd.go:9

--> openbsd/386 error: exit status 2
Stderr: # github.com/fsouza/go-dockerclient/external/github.com/docker/docker/pkg/system
../../fsouza/go-dockerclient/external/github.com/docker/docker/pkg/system/stat_unsupported.go:16: s.Mtimespec undefined (type *syscall.Stat_t has no field or method Mtimespec)
# github.com/fsouza/go-dockerclient/external/github.com/Sirupsen/logrus
../../fsouza/go-dockerclient/external/github.com/Sirupsen/logrus/terminal_openbsd.go:5: ioctlReadTermios redeclared in this block
        previous declaration at ../../fsouza/go-dockerclient/external/github.com/Sirupsen/logrus/terminal_bsd.go:7
../../fsouza/go-dockerclient/external/github.com/Sirupsen/logrus/terminal_openbsd.go:7: Termios redeclared in this block
        previous declaration at ../../fsouza/go-dockerclient/external/github.com/Sirupsen/logrus/terminal_bsd.go:9

--> netbsd/amd64 error: exit status 2
Stderr: # github.com/fsouza/go-dockerclient/external/github.com/Sirupsen/logrus
../../fsouza/go-dockerclient/external/github.com/Sirupsen/logrus/text_formatter.go:28: undefined: IsTerminal

--> netbsd/arm error: exit status 2
Stderr: # github.com/fsouza/go-dockerclient/external/github.com/Sirupsen/logrus
../../fsouza/go-dockerclient/external/github.com/Sirupsen/logrus/text_formatter.go:28: undefined: IsTerminal

--> openbsd/amd64 error: exit status 2
Stderr: # github.com/fsouza/go-dockerclient/external/github.com/docker/docker/pkg/system
../../fsouza/go-dockerclient/external/github.com/docker/docker/pkg/system/stat_unsupported.go:16: s.Mtimespec undefined (type *syscall.Stat_t has no field or method Mtimespec)
# github.com/fsouza/go-dockerclient/external/github.com/Sirupsen/logrus
../../fsouza/go-dockerclient/external/github.com/Sirupsen/logrus/terminal_openbsd.go:5: ioctlReadTermios redeclared in this block
        previous declaration at ../../fsouza/go-dockerclient/external/github.com/Sirupsen/logrus/terminal_bsd.go:7
../../fsouza/go-dockerclient/external/github.com/Sirupsen/logrus/terminal_openbsd.go:7: Termios redeclared in this block
        previous declaration at ../../fsouza/go-dockerclient/external/github.com/Sirupsen/logrus/terminal_bsd.go:9

make: *** [all] Error 1

cbednarski · 2015-10-27T22:03:45Z

Hm.. I'm not sure whether this is (still) a problem in logrus or a problem because go-dockerclient has ventured an old version, but it may be easier to build flag this feature for now since Docker doesn't run on BSD anyway.

diptanu added 8 commits October 22, 2015 17:48

Implemented Docker health checks

80ad971

Merge branch 'master' into f-docker-check

24ed164

Implemented Docker health checks

71ede8a

Registering the Exec with Docker Daemon everytime the check is invoked

83db728

Fixed merge conflicts

2632bb3

Adding a debug log to indicate the exit code of failed check

d695012

Marking the state of a service as critical if the Docker Daemon doesn…

40f72a8

…'t respond while running checks

Added some tests for docker check

31cdf4f

slackpad reviewed Oct 26, 2015
View reviewed changes

diptanu added 4 commits October 26, 2015 15:00

Extracted the logic of figuring out the shell and fixing the logic to…

809e9f5

… find out if the check is a Docker check

Defaulting to Monitor check

5f8f531

Collect and truncate the output from docker exec

4c1818e

Updated the comment for CheckType

b4af7f4

slackpad reviewed Oct 26, 2015
View reviewed changes

Making sure the script is not empty if it's a docker check

f5f5ed0

slackpad reviewed Oct 26, 2015
View reviewed changes

Not adding the docker check if we couldn't create the client

423f7fb

slackpad reviewed Oct 26, 2015
View reviewed changes

diptanu added 7 commits October 26, 2015 16:54

Fixed the Fake Docker client to simulate Exec start failures

9efbd1a

Added a test for exit code 1 with docker exec

5827865

Added a test to check if we are properly truncating docker exec outputs

f0c783d

Fixed the tests

1e240b5

Making an explicit check to test whether a check is of type Monitor

471442e

Added a test for selecting shell from env

2fdcf1a

Forcing the Env variable to empty while testing the default shell logic

3d68d06

diptanu added a commit that referenced this pull request Oct 27, 2015

Merge pull request #1343 from hashicorp/f-docker-check

ab7ab8e

Docker Support for Consul Health Checks

diptanu merged commit ab7ab8e into master Oct 27, 2015

diptanu deleted the f-docker-check branch October 27, 2015 04:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Docker Support for Consul Health Checks #1343

Docker Support for Consul Health Checks #1343

diptanu commented Oct 26, 2015

salehe commented Oct 26, 2015

diptanu commented Oct 26, 2015

slackpad Oct 26, 2015

diptanu Oct 26, 2015

slackpad Oct 26, 2015

slackpad commented Oct 26, 2015

slackpad Oct 26, 2015

slackpad Oct 26, 2015

cbednarski Oct 27, 2015

slackpad Oct 26, 2015

slackpad Oct 26, 2015

armon commented Oct 27, 2015

slackpad commented Oct 27, 2015

cbednarski commented Oct 27, 2015

Docker Support for Consul Health Checks #1343

Docker Support for Consul Health Checks #1343

Conversation

diptanu commented Oct 26, 2015

salehe commented Oct 26, 2015

diptanu commented Oct 26, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slackpad commented Oct 26, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armon commented Oct 27, 2015

slackpad commented Oct 27, 2015

cbednarski commented Oct 27, 2015