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

Support start_period for healthcheck in Docker Compose #475

Merged
merged 2 commits into from Aug 30, 2017

Conversation

@denverdino
Copy link
Contributor

commented Aug 28, 2017

Signed-off-by: Li Yi denverdino@gmail.com

- What I did
The --health-start-period duration is supported by docker run and docker service create since 17.05, but it is missing in the Docker Compose.

The PR adds the start_period for healthcheck to Docker Compose.

- How I did it

Adds the start_period for healthcheck to Docker Compose.

- How to verify it

Using the bellow compose file: docker-compose.yaml

version: "3.4"
services:
  foo:
    image: elasticsearch
    healthcheck:
      test: "curl --silent --fail localhost:9200/_cluster/health || exit 1"
      interval: 10s
      timeout: 2s
      retries: 5
      start_period: 15s

docker stack deploy -c docker-compose.yaml elasticsearch

- Description for the changelog

Support start_period for healthcheck in Docker Compose

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

@dnephin
Copy link
Collaborator

left a comment

You'll also need to update cli/compose/convert/service.go to accept the new field.

I believe this will require an updated to the swarmkit dependency in vendor.conf, which will probably be blocked on #465

@@ -324,7 +324,8 @@
{"type": "array", "items": {"type": "string"}}
]
},
"timeout": {"type": "string"}
"timeout": {"type": "string"},
"start_period": {"type": "string"}

This comment has been minimized.

Copy link
@dnephin

dnephin Aug 28, 2017

Collaborator

indentation looks off. This file uses spaces not tabs.

@@ -172,7 +172,7 @@ type HealthCheckConfig struct {
Timeout string
Interval string
Retries *uint64
StartPeriod string
StartPeriod string `mapstructure:"start_period"`

This comment has been minimized.

Copy link
@dnephin

dnephin Aug 28, 2017

Collaborator

This field should be a *time.Duration not a string. I just noticed that the other fields here are wrong. We'll need to fix that.

This comment has been minimized.

Copy link
@denverdino

denverdino Aug 29, 2017

Author Contributor

func convertHealthcheck(healthcheck *composetypes.HealthCheckConfig) (*container.HealthConfig, error)

will parse and convert the string into time.Duration

This comment has been minimized.

Copy link
@dnephin

dnephin Aug 29, 2017

Collaborator

It will yes, but it's an unnecessary step. The config should be of type *time.Duration. This is a mistake that was made in other fields of the HealthCheckConfig (that we need to fix), but there are other fields in the service that use *time.Duration correctly.

@codecov-io

This comment has been minimized.

Copy link

commented Aug 28, 2017

Codecov Report

Merging #475 into master will decrease coverage by 1.2%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master     #475      +/-   ##
==========================================
- Coverage   48.62%   47.41%   -1.21%     
==========================================
  Files         199      199              
  Lines       16396    16398       +2     
==========================================
- Hits         7972     7775     -197     
- Misses       8008     8228     +220     
+ Partials      416      395      -21

@denverdino denverdino force-pushed the AliyunContainerService:start_period branch from 68fb22c to e94b6c7 Aug 29, 2017

@GordonTheTurtle GordonTheTurtle removed the dco/no label Aug 29, 2017

@denverdino denverdino force-pushed the AliyunContainerService:start_period branch from e94b6c7 to 15d242e Aug 29, 2017

Support start_period for healthcheck in Docker Compose
Signed-off-by: Li Yi <denverdino@gmail.com>

@denverdino denverdino force-pushed the AliyunContainerService:start_period branch from 099e73c to 0abdad6 Aug 29, 2017

@docker docker deleted a comment from GordonTheTurtle Aug 29, 2017

@denverdino

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2017

Changed the type of interval, timeout and start_period of healthcheck from string to * time.Duration

Thanks

@dnephin
Copy link
Collaborator

left a comment

thanks, just one comment

"retries": {"type": "number"},
"test": {
"oneOf": [
{"type": "string"},
{"type": "array", "items": {"type": "string"}}
]
},
"timeout": {"type": "string"}
"timeout": {"type": "string", "format": "duration"}

This comment has been minimized.

Copy link
@dnephin

dnephin Aug 30, 2017

Collaborator

Normally we do not make any changes to previous versions of the schema. They should be frozen. Would you mind reverting this change in the previous versions?

This comment has been minimized.

Copy link
@denverdino

denverdino Aug 30, 2017

Author Contributor

Do you think the change on the datatype will impact the parsing for old version?

This comment has been minimized.

Copy link
@dnephin

dnephin Aug 30, 2017

Collaborator

Nope, it wont

Change the type of interval, timeout and start_period of healthcheck …
…from string to * time.Duration

Signed-off-by: Li Yi <denverdino@gmail.com>

@denverdino denverdino force-pushed the AliyunContainerService:start_period branch from e19ef5c to e02fcfd Aug 30, 2017

@denverdino

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2017

@dnephin Thanks for your comments, the change on the 3.0~3.3 schema is reverted

@vdemeester
Copy link
Member

left a comment

LGTM 🐮

@dnephin
Copy link
Collaborator

left a comment

LGTM

@dnephin dnephin merged commit e636a53 into docker:master Aug 30, 2017

8 checks passed

ci/circleci: cross Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: shellcheck Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
ci/circleci: validate Your tests passed on CircleCI!
Details
codecov/patch 66.66% of diff hit (target 50%)
Details
codecov/project Absolute coverage decreased by -1.2% but relative coverage increased by +18.04% compared to 65202d6
Details
dco-signed All commits are signed

@GordonTheTurtle GordonTheTurtle added this to the 17.08.0 milestone Aug 30, 2017

@mgansler mgansler referenced this pull request Sep 7, 2017
@mmariani

This comment has been minimized.

Copy link
Contributor

commented Sep 27, 2017

I was waiting for this feature, but I think I am missing something here.

start_period Additional property start_period is not allowed

$ docker version
Client:
 Version:      17.09.0-ce
 API version:  1.32
 Go version:   go1.8.3
 Git commit:   afdb6d4
 Built:        Tue Sep 26 22:42:45 2017
 OS/Arch:      linux/amd64

Server:
 Version:      17.09.0-ce
 API version:  1.32 (minimum version 1.12)
 Go version:   go1.8.3
 Git commit:   afdb6d4
 Built:        Tue Sep 26 22:41:24 2017
 OS/Arch:      linux/amd64
 Experimental: true
     [...]
     healthcheck:
       test: "curl --fail http://localhost:5601/ || exit 1"
       start_period: 10m

I checked the schema file in the 17.09 release of docker-ce and it seems correct.

@rdxmb

This comment has been minimized.

Copy link
Contributor

commented Oct 13, 2017

// edited: works with docker-compose file version 3.4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.