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

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)

Copy link
Contributor

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

will parse and convert the string into time.Duration

Copy link
Contributor

Choose a reason for hiding this comment

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

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

codecov-io 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

Signed-off-by: Li Yi <denverdino@gmail.com>
@denverdino
Copy link
Contributor Author

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

Thanks

Copy link
Contributor

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

thanks, just one comment

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

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope, it wont

…from string to * time.Duration

Signed-off-by: Li Yi <denverdino@gmail.com>
@denverdino
Copy link
Contributor Author

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

Copy link
Collaborator

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

Copy link
Contributor

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

LGTM

@mmariani
Copy link
Contributor

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

rdxmb 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
Development

Successfully merging this pull request may close these issues.

7 participants