Skip to content
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 --health-* flags to service create and update #27369

Merged
merged 2 commits into from Oct 28, 2016
Merged

Conversation

cezarsa
Copy link
Contributor

@cezarsa cezarsa commented Oct 14, 2016

- 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
Copy link
Member

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 thaJeztah added the status/failing-ci Indicates that the PR in its current state fails the test suite label Oct 17, 2016
@thaJeztah
Copy link
Member

ping @stevvooe @aluzzardi @talex5

@cezarsa
Copy link
Contributor Author

cezarsa commented Oct 17, 2016

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
Copy link
Member

@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
Copy link
Contributor Author

cezarsa commented Oct 18, 2016

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

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

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

@vdemeester
Copy link
Member

@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
Copy link
Contributor

Also ping @dongluochen and @aaronlehmann FYI!

@GordonTheTurtle GordonTheTurtle added the dco/no Automatically set by a bot when one of the commits lacks proper signature label Oct 27, 2016
@GordonTheTurtle GordonTheTurtle added the dco/no Automatically set by a bot when one of the commits lacks proper signature label Oct 27, 2016
@GordonTheTurtle GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Oct 27, 2016
@cezarsa
Copy link
Contributor Author

cezarsa commented Oct 27, 2016

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
Copy link
Contributor

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.

Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🐸
@cezarsa needs a rebase though 😅

Signed-off-by: Cezar Sa Espinola <cezarsa@gmail.com>
@thaJeztah
Copy link
Member

looks like it's rebased

Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Moving to docs review

@vdemeester
Copy link
Member

@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)

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 <cezarsa@gmail.com>
@cezarsa
Copy link
Contributor Author

cezarsa commented Oct 28, 2016

Done. :)

@vdemeester
Copy link
Member

@cezarsa perfect ! Thanks !
docs LGTM 🐸

@vdemeester vdemeester merged commit f860289 into moby:master Oct 28, 2016
thaJeztah added a commit to thaJeztah/docker that referenced this pull request Jan 16, 2017
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 <github@gone.nl>
dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017
Add --health-* flags to service create and update
andrewhsu pushed a commit to docker/docker-ce that referenced this pull request May 19, 2017
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 <github@gone.nl>
Upstream-commit: 5d2722f83db9e301c6dcbe1c562c2051a52905db
Component: cli
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants