Add --health-* flags to service create and update by cezarsa · Pull Request #27369 · moby/moby

cezarsa · 2016-10-14T00:09:29Z

- What I did

This PR is related to #25303 and adds --health-* and --no-healthcheck flags to service create and update commands.

- How I did it

The ContainerSpec was modified to include a HealthConfig instance which is them serialized to swarmkit. I also created a PR in swarmkit (moby/swarmkit#1641) which needs to be merged before this PR can be accepted.

For service create the behavior is similar to docker run with the same checks validating conflicting arguments. On service update it's also valid to set things like --health-cmd "" and --health-timeout 0 to allow inheriting these attributes from the image. Setting --no-healthcheck will reset the HealthConfig to the NONE test and remove every other attribute.

- How to verify it

Some test case were added to check if the commands are behaving correctly.

- Description for the changelog

Add --health-* and --no-healthcheck to service create and update

- A picture of a cute animal (not mandatory but encouraged)
🐱

thaJeztah · 2016-10-17T05:37:40Z

Looks like there's some golint errors;

00:10:17 ---> Making bundle: validate-lint (in bundles/1.13.0-dev/validate-lint)
00:10:17 Errors from golint:
00:10:17 cli/command/service/opts.go:72:6: exported type PositiveDurationOpt should have comment or be unexported
00:10:17 cli/command/service/opts.go:76:1: exported method PositiveDurationOpt.Set should have comment or be unexported
00:10:17 
00:10:17 Please fix the above errors. You can test via "golint" and commit the result.

thaJeztah · 2016-10-17T05:38:00Z

ping @stevvooe @aluzzardi @talex5

cezarsa · 2016-10-17T12:48:13Z

Thanks, I've fixed the lint errors. However, the code still doesn't compile on CI because it depends on moby/swarmkit#1641. If the swarmkit PR is approved (and updated on vendor) I'll rebase this PR and it should compile cleanly.

thaJeztah · 2016-10-17T21:01:27Z

@cezarsa to make CI pass during review, you can add a second commit that makes those changes in the vendored files. The "vendor" CI will fail, but all other tests can pass then. Once we're in "code review", and moby/swarmkit#1641 is merged, you can then update the PR to update the vendor.sh an get the actual changes from SwarmKit

cezarsa · 2016-10-18T01:21:19Z

Thanks for the tip @thaJeztah. Tests are passing now.

thaJeztah · 2016-10-25T00:29:23Z

oh boy, looks like this needs a rebase now. 😢

vdemeester · 2016-10-27T17:03:28Z

@cezarsa something is missing somehow. The healthcheck I provide for the service doesn't end-up into the container(s) created.

$ docker run -d --health-cmd="stat /etc/passwd" --health-interval=10s --health-retries=2 busybox top
$ docker service create --name test --health-cmd="stat /etc/passwd" --health-interval=10s --health-retries=2 busybox top
# … wait for a bit
$ docker ps
CONTAINER ID        IMAGE               COMMAND             CREATED              STATUS                    PORTS               NAMES
95b46e278bfb        busybox             "top"               14 seconds ago       Up 13 seconds (healthy)                       hungry_colden
7501f04e3610        busybox:latest      "top"               About a minute ago   Up About a minute                             test.1.lalhjh6envegchv54qjrkts4u
$ docker inspect --format={{.State.Health}} 95b
{healthy 0 [0xc42037e280 0xc42037e2d0 0xc42037e320 0xc42037e370 0xc42037e3c0]}
$ docker inspect --format={{.State.Health}} 750
<nil>

As shown in inspect or in ps, the container that was created as part of the service has no health check.

icecrime · 2016-10-27T17:12:32Z

Also ping @dongluochen and @aaronlehmann FYI!

cezarsa · 2016-10-27T21:34:51Z

Sorry @vdemeester, there was a bit of code missing. I just added it in 3d0132f

I made this mistake because similar code exists in swarmkit and I thought the daemon was using it, sorry. (I guess it will use in the future?)

Just tested it now and everything seems to be working now:

$ docker service create --health-cmd 'exit 1' busybox tail -f /dev/null
lb3l6fxxdehc8n65k172wpacy
$ docker ps
CONTAINER ID        IMAGE               COMMAND                  CREATED                  STATUS                                     PORTS                    NAMES
9528efa658fc        busybox:latest      "tail -f /dev/null"      Less than a second ago   Up Less than a second (health: starting)                            hungry_montalcini.1.moaaurb2ugxc2cktv4qnabpoo
$ docker inspect --format={{.State.Health}} 95
{starting 0 []}
$ docker inspect --format={{.Config.Healthcheck.Test}} 95
[CMD-SHELL exit 1]

dongluochen · 2016-10-27T22:36:17Z

I test this code and it looks good to me. One small problem would be the default value for healthcheck interval and retries are not shown. But we haven't done it in docker run.

vdemeester

LGTM 🐸
@cezarsa needs a rebase though 😅

Signed-off-by: Cezar Sa Espinola <[email protected]>

thaJeztah · 2016-10-28T03:46:25Z

looks like it's rebased

cpuguy83

LGTM

Moving to docs review

vdemeester · 2016-10-28T16:40:36Z

@cezarsa could you squash all the commits that are not related to vendoring ? (this way we have one commit for vendoring, one for the actual diff)

thaJeztah

LGTM (after squashing)

A HealthConfig entry was added to the ContainerSpec associated with the service being created or updated. Signed-off-by: Cezar Sa Espinola <[email protected]>

cezarsa · 2016-10-28T17:28:40Z

Done. :)

vdemeester · 2016-10-28T17:30:34Z

@cezarsa perfect ! Thanks !
docs LGTM 🐸

Pull request moby#27745 added support for the client to talk to older versions of the daemon. Various flags were added to docker 1.13 that are not compatible with older daemons. This PR adds annotations to those flags, so that they are automatically hidden if the daemon is older than docker 1.13 (API 1.25). Not all new flags affect the API (some are client-side only). The following PR's added new flags to docker 1.13 that affect the API; - moby#23430 added `--cpu-rt-period`and `--cpu-rt-runtime` - moby#27800 / moby#25317 added `--group` / `--group-add` / `--group-rm` - moby#27702 added `--network` to `docker build` - moby#25962 added `--attachable` to `docker network create` - moby#27998 added `--compose-file` to `docker stack deploy` - moby#22566 added `--stop-timeout` to `docker run` and `docker create` - moby#26061 added `--init` to `docker run` and `docker create` - moby#26941 added `--init-path` to `docker run` and `docker create` - moby#27958 added `--cpus` on `docker run` / `docker create` - moby#27567 added `--dns`, `--dns-opt`, and `--dns-search` to `docker service create` - moby#27596 added `--force` to `docker service update` - moby#27857 added `--hostname` to `docker service create` - moby#28031 added `--hosts`, `--host-add` / `--host-rm` to `docker service create` and `docker service update` - moby#28076 added `--tty` on `docker service create` / `docker service update` - moby#26421 added `--update-max-failure-ratio`, `--update-monitor` and `--rollback` on `docker service update` - moby#27369 added `--health-cmd`, `--health-interval`, `--health-retries`, `--health-timeout` and `--no-healthcheck` options to `docker service create` and `docker service update` Signed-off-by: Sebastiaan van Stijn <[email protected]>

Add --health-* flags to service create and update

Pull request moby/moby#27745 added support for the client to talk to older versions of the daemon. Various flags were added to docker 1.13 that are not compatible with older daemons. This PR adds annotations to those flags, so that they are automatically hidden if the daemon is older than docker 1.13 (API 1.25). Not all new flags affect the API (some are client-side only). The following PR's added new flags to docker 1.13 that affect the API; - moby/moby#23430 added `--cpu-rt-period`and `--cpu-rt-runtime` - moby/moby#27800 / moby/moby#25317 added `--group` / `--group-add` / `--group-rm` - moby/moby#27702 added `--network` to `docker build` - moby/moby#25962 added `--attachable` to `docker network create` - moby/moby#27998 added `--compose-file` to `docker stack deploy` - moby/moby#22566 added `--stop-timeout` to `docker run` and `docker create` - moby/moby#26061 added `--init` to `docker run` and `docker create` - moby/moby#26941 added `--init-path` to `docker run` and `docker create` - moby/moby#27958 added `--cpus` on `docker run` / `docker create` - moby/moby#27567 added `--dns`, `--dns-opt`, and `--dns-search` to `docker service create` - moby/moby#27596 added `--force` to `docker service update` - moby/moby#27857 added `--hostname` to `docker service create` - moby/moby#28031 added `--hosts`, `--host-add` / `--host-rm` to `docker service create` and `docker service update` - moby/moby#28076 added `--tty` on `docker service create` / `docker service update` - moby/moby#26421 added `--update-max-failure-ratio`, `--update-monitor` and `--rollback` on `docker service update` - moby/moby#27369 added `--health-cmd`, `--health-interval`, `--health-retries`, `--health-timeout` and `--no-healthcheck` options to `docker service create` and `docker service update` Signed-off-by: Sebastiaan van Stijn <[email protected]> Upstream-commit: 5d2722f83db9e301c6dcbe1c562c2051a52905db Component: cli

GordonTheTurtle added the status/0-triage label Oct 14, 2016

thaJeztah added status/1-design-review and removed status/0-triage labels Oct 17, 2016

thaJeztah added the status/failing-ci Indicates that the PR in its current state fails the test suite label Oct 17, 2016

cezarsa force-pushed the hc branch from 8401fdc to f403045 Compare October 17, 2016 12:18

vdemeester added the status/needs-vendoring label Oct 17, 2016

cezarsa force-pushed the hc branch from f403045 to d327387 Compare October 17, 2016 21:19

thaJeztah removed the status/failing-ci Indicates that the PR in its current state fails the test suite label Oct 18, 2016

vdemeester added the area/swarm label Oct 18, 2016

cezarsa force-pushed the hc branch 2 times, most recently from 4d4c8d4 to 3a97971 Compare October 25, 2016 13:10

thaJeztah mentioned this pull request Oct 25, 2016

[epic] add more options to service create / service update #25303

Open

thaJeztah added this to the 1.13.0 milestone Oct 26, 2016

thaJeztah added status/2-code-review and removed status/1-design-review labels Oct 26, 2016

thaJeztah assigned vdemeester Oct 26, 2016

cezarsa force-pushed the hc branch 2 times, most recently from 13ecf11 to ba9bb00 Compare October 26, 2016 21:35

cezarsa force-pushed the hc branch from ba9bb00 to 4ba408f Compare October 27, 2016 21:19

GordonTheTurtle added the dco/no Automatically set by a bot when one of the commits lacks proper signature label Oct 27, 2016

cezarsa force-pushed the hc branch from 4ba408f to d0bcff6 Compare October 27, 2016 21:20

GordonTheTurtle added the dco/no Automatically set by a bot when one of the commits lacks proper signature label Oct 27, 2016

cezarsa force-pushed the hc branch from d0bcff6 to 625c3f2 Compare October 27, 2016 21:26

GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Oct 27, 2016

vdemeester mentioned this pull request Oct 27, 2016

service create kills the task when the image has HEALTHCHECK and it takes more than 25 seconds to start #26498

Closed

vdemeester approved these changes Oct 28, 2016

View reviewed changes

vdemeester removed the status/needs-vendoring label Oct 28, 2016

vendor: update swarmkit

fd657a1

Signed-off-by: Cezar Sa Espinola <[email protected]>

cezarsa force-pushed the hc branch from 625c3f2 to 7f1e42c Compare October 28, 2016 02:44

cpuguy83 approved these changes Oct 28, 2016

View reviewed changes

cpuguy83 added status/3-docs-review and removed status/2-code-review labels Oct 28, 2016

thaJeztah approved these changes Oct 28, 2016

View reviewed changes

Add --health-* commands to service create and update

7bd2611

A HealthConfig entry was added to the ContainerSpec associated with the service being created or updated. Signed-off-by: Cezar Sa Espinola <[email protected]>

cezarsa force-pushed the hc branch from 7f1e42c to 7bd2611 Compare October 28, 2016 17:27

vdemeester added status/4-merge and removed status/3-docs-review labels Oct 28, 2016

vdemeester merged commit f860289 into moby:master Oct 28, 2016

thaJeztah added the impact/changelog label Jan 16, 2017

thaJeztah mentioned this pull request Jan 16, 2017

Add version annotation to various flags added in 1.13 #30186

Merged

dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017

Merge pull request moby#27369 from cezarsa/hc

4403644

Add --health-* flags to service create and update

Conversation

cezarsa commented Oct 14, 2016

Uh oh!

thaJeztah commented Oct 17, 2016

Uh oh!

thaJeztah commented Oct 17, 2016

Uh oh!

cezarsa commented Oct 17, 2016

Uh oh!

thaJeztah commented Oct 17, 2016

Uh oh!

cezarsa commented Oct 18, 2016

Uh oh!

thaJeztah commented Oct 25, 2016

Uh oh!

vdemeester commented Oct 27, 2016

Uh oh!

icecrime commented Oct 27, 2016

Uh oh!

cezarsa commented Oct 27, 2016

Uh oh!

dongluochen commented Oct 27, 2016

Uh oh!

vdemeester left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Oct 28, 2016

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

vdemeester commented Oct 28, 2016

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

cezarsa commented Oct 28, 2016

Uh oh!

vdemeester commented Oct 28, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

vdemeester left a comment •

edited

Loading