Fix handling of healthcheck from image #3539

stefanb2 · 2019-07-09T19:45:35Z

proposed fix for Does not schedule healthcheck for images with HEALTHCHECK #3525 cmd/podman/shared/create.go:
If the image doesn't provide any options, e.g. interval, timeout, etc.,
then apply the standard defaults when creating the container. Now podman
correctly schedules the healtcheck service & timer for the container.
proposed fix for podman incompatiblity with healthcheck from docker image #3507 libpod/healthcheck.go
- remove duplicate check, already called in HealthCheck()
- reject zero-length command list
- check for Docker command list, starting with NONE, CMD or CMD-SHELL
- otherwise pass command list as-is to container

openshift-ci-robot · 2019-07-09T19:45:56Z

Hi @stefanb2. Thanks for your PR.

I'm waiting for a containers or openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

haircommander · 2019-07-09T19:48:52Z

/ok-to-test

rh-atomic-bot · 2019-07-09T19:53:45Z

Can one of the admins verify this patch?
I understand the following commands:

bot, add author to whitelist
bot, test pull request
bot, test pull request once

libpod/healthcheck.go

cmd/podman/shared/create.go

stefanb2 · 2019-07-10T05:04:03Z

/test special_testing_rootless

mheon · 2019-07-10T12:55:11Z

That one's a known flake. For reference, the /retest commands don't work on most tests - you need to go into the page for the failing test, and click the restart link

stefanb2 · 2019-07-10T18:22:38Z

Tests with Docker V1 images, generated on Fedora 30 with default docker installation. F30 podman package rebuilt with WIP patches included.

Dockerfile HEALTHCHECK line
output from podman image inspect <IMAGE> | fgrep HEALTHCHECK
output from podman container inspect --format {{json .Config.HealthCheck}} test
output from systemctl --user show <CONTAINER ID>.timer | fgrep TimersMonotonic=
output from podman --log-level=debug healthcheck run test 2>&1 | fgrep "executing health check command"

Healthcheck disabled

1 - HEALTHCHECK NONE
2 - HEALTHCHECK \u0026{[\"NONE\"] \"0s\" \"0s\" '\\x00'}
3 - null
4 - timer not found (i.e. none scheduled)
5 - ERRO[0000] container <CONTAINER ID> has no defined healthcheck

Healthcheck with one command

1 - HEALTHCHECK CMD /bin/true
2 - HEALTHCHECK \u0026{[\"CMD-SHELL\" \"/bin/true\"] \"0s\" \"0s\" '\\x00'}
3 - {"Test":["CMD-SHELL","/bin/true"],"Interval":30000000000,"Timeout":30000000000,"Retries":3}
4 - TimersMonotonic={ OnUnitInactiveUSec=30s ...
5 - executing health check command /bin/sh -c /bin/true for <CONTAINER ID>

Healthcheck with 1-element command list

1 - HEALTHCHECK CMD ["/bin/true"]
2 - HEALTHCHECK \u0026{[\"CMD\" \"/bin/true\"] \"0s\" \"0s\" '\\x00'}
3 - {"Test":["CMD","/bin/true"],"Interval":30000000000,"Timeout":30000000000,"Retries":3}
4 - TimersMonotonic={ OnUnitInactiveUSec=30s ...
5 - executing health check command /bin/true for <CONTAINER ID>

Healthcheck with 2-element command list

1 - HEALTHCHECK CMD ["/bin/echo", "test"]
2 - HEALTHCHECK \u0026{[\"CMD\" \"/bin/echo\" \"test\"] \"0s\" \"0s\" '\\x00'}
3 - {"Test":["CMD","/bin/echo","test"],"Interval":30000000000,"Timeout":30000000000,"Retries":3}
4 - TimersMonotonic={ OnUnitInactiveUSec=30s ...
5 - executing health check command /bin/echo test for <CONTAINER ID>

Healthcheck with one command and non-default interval

1 - HEALTHCHECK --interval=40s CMD /bin/true
2 - HEALTHCHECK \u0026{[\"CMD-SHELL\" \"/bin/true\"] \"40s\" \"0s\" '\\x00'}
3 - {"Test":["CMD-SHELL","/bin/true"],"Interval":40000000000,"Timeout":30000000000,"Retries":3}
4 - TimersMonotonic={ OnUnitInactiveUSec=40s
5 - executing health check command /bin/sh -c /bin/true for <CONTAINER ID>

Healthcheck with one command and non-default timeout

1 - HEALTHCHECK --timeout=50s CMD /bin/true
2 - HEALTHCHECK \u0026{[\"CMD-SHELL\" \"/bin/true\"] \"0s\" \"50s\" '\\x00'}
3 - {"Test":["CMD-SHELL","/bin/true"],"Interval":30000000000,"Timeout":50000000000,"Retries":3}
4 - TimersMonotonic={ OnUnitInactiveUSec=30s ...
5 - executing health check command /bin/sh -c /bin/true for <CONTAINER ID>

Healthcheck with one command and non-default retries

1 - HEALTHCHECK --retries=10 CMD /bin/true
2 - HEALTHCHECK \u0026{[\"CMD-SHELL\" \"/bin/true\"] \"0s\" \"0s\" '\\n'}
3 - {"Test":["CMD-SHELL","/bin/true"],"Interval":30000000000,"Timeout":30000000000,"Retries":10}
4 - TimersMonotonic={ OnUnitInactiveUSec=30s ...
5 - executing health check command /bin/sh -c /bin/true for <CONTAINER ID>

libpod/healthcheck.go

stefanb2 · 2019-07-11T07:11:57Z

Tests with Docker V2 images, generated on Fedora 30 with Docker CE 19.03 installation. F30 podman package rebuilt with WIP patches included.

Dockerfile HEALTHCHECK line
output from podman image inspect <IMAGE> | fgrep HEALTHCHECK
output from podman container inspect --format {{json .Config.HealthCheck}} test
output from systemctl --user show <CONTAINER ID>.timer | fgrep TimersMonotonic=
output from podman --log-level=debug healthcheck run test 2>&1 | fgrep "executing health check command"

Healthcheck disabled

1 - HEALTHCHECK NONE
2 - HEALTHCHECK \u0026{[\"NONE\"] \"0s\" \"0s\" \"0s\" '\\x00'}
3 - null
4 - timer not found (i.e. none scheduled)
5 - ERRO[0000] container <CONTAINER ID> has no defined healthcheck

Healthcheck with one command

1 - HEALTHCHECK CMD /bin/true
2 - HEALTHCHECK \u0026{[\"CMD-SHELL\" \"/bin/true\"] \"0s\" \"0s\" \"0s\" '\\x00'}
3 - {"Test":["CMD-SHELL","/bin/true"],"Interval":30000000000,"Timeout":30000000000,"Retries":3}
4 - TimersMonotonic={ OnUnitInactiveUSec=30s ...
5 - executing health check command /bin/sh -c /bin/true for <CONTAINER ID>

Healthcheck with 1-element command list

1 - HEALTHCHECK CMD ["/bin/true"]
2 - HEALTHCHECK \u0026{[\"CMD\" \"/bin/true\"] \"0s\" \"0s\" \"0s\" '\\x00'}
3 - {"Test":["CMD","/bin/true"],"Interval":30000000000,"Timeout":30000000000,"Retries":3}
4 - TimersMonotonic={ OnUnitInactiveUSec=30s ...
5 - executing health check command /bin/true for <CONTAINER ID>

Healthcheck with 2-element command list

1 - HEALTHCHECK CMD ["/bin/echo", "test"]
2 - HEALTHCHECK \u0026{[\"CMD\" \"/bin/echo\" \"test\"] \"0s\" \"0s\" \"0s\" '\\x00'}
3 - {"Test":["CMD","/bin/echo","test"],"Interval":30000000000,"Timeout":30000000000,"Retries":3}
4 - TimersMonotonic={ OnUnitInactiveUSec=30s ...
5 - executing health check command /bin/echo test for <CONTAINER ID>

Healthcheck with one command and non-default interval

1 - HEALTHCHECK --interval=40s CMD /bin/true
2 - HEALTHCHECK \u0026{[\"CMD-SHELL\" \"/bin/true\"] \"40s\" \"0s\" \"0s\" '\\x00'}
3 - {"Test":["CMD-SHELL","/bin/true"],"Interval":40000000000,"Timeout":30000000000,"Retries":3}
4 - TimersMonotonic={ OnUnitInactiveUSec=40s
5 - executing health check command /bin/sh -c /bin/true for <CONTAINER ID>

Healthcheck with one command and non-default timeout

1 - HEALTHCHECK --timeout=50s CMD /bin/true
2 - HEALTHCHECK \u0026{[\"CMD-SHELL\" \"/bin/true\"] \"0s\" \"50s\" \"0s\" '\\x00'}
3 - {"Test":["CMD-SHELL","/bin/true"],"Interval":30000000000,"Timeout":50000000000,"Retries":3}
4 - TimersMonotonic={ OnUnitInactiveUSec=30s ...
5 - executing health check command /bin/sh -c /bin/true for <CONTAINER ID>

Healthcheck with one command and non-default start period

1 - HEALTHCHECK --start-period=60s CMD /bin/true
2 - HEALTHCHECK \u0026{[\"CMD-SHELL\" \"/bin/true\"] \"0s\" \"0s\" \"1m0s\" '\\x00'}
3 - {"Test":["CMD-SHELL","/bin/true"],"StartPeriod":60000000000,"Interval":30000000000,"Timeout":30000000000,"Retries":3}
4 - TimersMonotonic={ OnUnitInactiveUSec=30s ...
5 - executing health check command /bin/sh -c /bin/true for <CONTAINER ID>

Healthcheck with one command and non-default retries

1 - HEALTHCHECK --retries=10 CMD /bin/true
2 - HEALTHCHECK \u0026{[\"CMD-SHELL\" \"/bin/true\"] \"0s\" \"0s\" \"0s\" '\\n'
3 - {"Test":["CMD-SHELL","/bin/true"],"Interval":30000000000,"Timeout":30000000000,"Retries":10}
4 - TimersMonotonic={ OnUnitInactiveUSec=30s ...
5 - executing health check command /bin/sh -c /bin/true for <CONTAINER ID>

stefanb2 · 2019-07-11T12:26:17Z

From my PoV this PR is ready for final review & approval.

cmd/podman/shared/create.go

libpod/healthcheck.go

mheon · 2019-07-11T15:53:35Z

/approve

openshift-ci-robot · 2019-07-11T15:53:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mheon, stefanb2

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [mheon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

stefanb2 · 2019-07-11T17:47:28Z

Healthcheck with empty command

1 - HEALTHCHECK CMD [""]
2 - HEALTHCHECK \u0026{[\"CMD\" \"\"] \"0s\" \"0s\" \"0s\" '\\x00'}
3 - {"Test":["CMD",""],"Interval":30000000000,"Timeout":30000000000,"Retries":3}
4 - TimersMonotonic={ OnUnitInactiveUSec=30s ...
5 - ERRO[0000] container <CONTAINER ID> has no defined healthcheck

i.e. podman does create the unit & timer, but any attempt to start the healthcheck is rejected by podman. Hence there are no attempts logged in the container and its health state stays at Starting.

- remove duplicate check, already called in HealthCheck() - reject zero-length command list and empty command string as errorneous - support all Docker command list keywords: NONE, CMD or CMD-SHELL - use Docker default "/bin/sh -c" for CMD-SHELL Fixes containers#3507 Signed-off-by: Stefan Becker <chemobejk@gmail.com>

If the image doesn't provide any options, e.g. interval, timeout, etc., then apply the Docker defaults when creating the container. Otherwise the defaults will be left 0 and podman doesn't schedule the healtcheck service & timer for the container or incorrectly reports unhealthy state when the check is executed. Fixes containers#3525 Signed-off-by: Stefan Becker <chemobejk@gmail.com>

If the image was built with "HEALTHCHECK NONE" then we should create a container without healthcheck configuration. Otherwise executing the healthcheck on the container will return "unhealthy" instead of the correct error message that the container doesn't have a healthcheck. We also ignore the healthcheck configuration if the command list is empty or the command string is empty. Fixes containers#3525 Signed-off-by: Stefan Becker <chemobejk@gmail.com>

An image with "HEALTHCHECK CMD ['']" is valid but as there is no command defined the healthcheck will fail. Reject such a configuration. Fixes containers#3507 Signed-off-by: Stefan Becker <chemobejk@gmail.com>

stefanb2 · 2019-07-16T04:53:23Z

Can I have a LGTM from the reviewers?

mheon · 2019-07-16T13:50:51Z

LGTM

mheon · 2019-07-16T13:51:07Z

@rhatdan @giuseppe @vrothberg PTAL

rhatdan · 2019-07-16T14:19:43Z

/lgtm

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 9, 2019

openshift-ci-robot requested review from rhatdan and TomSweeneyRedHat July 9, 2019 19:45

openshift-ci-robot added the size/S label Jul 9, 2019

openshift-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 9, 2019

openshift-ci-robot added ok-to-test and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 9, 2019

rhatdan reviewed Jul 9, 2019

View reviewed changes

libpod/healthcheck.go Outdated Show resolved Hide resolved

stefanb2 commented Jul 9, 2019

View reviewed changes

cmd/podman/shared/create.go Outdated Show resolved Hide resolved

stefanb2 force-pushed the topic-pr-3507-3525 branch from d52d0d0 to d94fbca Compare July 9, 2019 21:06

openshift-ci-robot added size/M and removed size/S labels Jul 9, 2019

stefanb2 force-pushed the topic-pr-3507-3525 branch from d94fbca to f30eb45 Compare July 9, 2019 21:15

stefanb2 force-pushed the topic-pr-3507-3525 branch from f30eb45 to 6f0fdc6 Compare July 10, 2019 06:55

mheon reviewed Jul 10, 2019

View reviewed changes

libpod/healthcheck.go Outdated Show resolved Hide resolved

mheon reviewed Jul 10, 2019

View reviewed changes

libpod/healthcheck.go Outdated Show resolved Hide resolved

stefanb2 force-pushed the topic-pr-3507-3525 branch from 6f0fdc6 to 5f700e1 Compare July 11, 2019 08:09

stefanb2 changed the title ~~WIP fix handling of healthcheck from image~~ Fix handling of healthcheck from image Jul 11, 2019

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 11, 2019

rhatdan reviewed Jul 11, 2019

View reviewed changes

cmd/podman/shared/create.go Show resolved Hide resolved

cmd/podman/shared/create.go Show resolved Hide resolved

libpod/healthcheck.go Show resolved Hide resolved

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 11, 2019

stefanb2 force-pushed the topic-pr-3507-3525 branch from 5f700e1 to f0c24e5 Compare July 11, 2019 17:40

stefanb2 added 4 commits July 16, 2019 07:01

healthcheck: reject empty commands

5ed2de1

An image with "HEALTHCHECK CMD ['']" is valid but as there is no command defined the healthcheck will fail. Reject such a configuration. Fixes containers#3507 Signed-off-by: Stefan Becker <chemobejk@gmail.com>

stefanb2 force-pushed the topic-pr-3507-3525 branch from f0c24e5 to 5ed2de1 Compare July 16, 2019 04:01

openshift-ci-robot assigned rhatdan Jul 16, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 16, 2019

openshift-merge-robot merged commit 386ffd2 into containers:master Jul 16, 2019

mheon added the Release Notes 1.5.0 label Jul 16, 2019

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 26, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix handling of healthcheck from image #3539

Fix handling of healthcheck from image #3539

stefanb2 commented Jul 9, 2019

openshift-ci-robot commented Jul 9, 2019

haircommander commented Jul 9, 2019

rh-atomic-bot commented Jul 9, 2019

stefanb2 commented Jul 10, 2019

mheon commented Jul 10, 2019

stefanb2 commented Jul 10, 2019 •

edited

Loading

stefanb2 commented Jul 11, 2019

stefanb2 commented Jul 11, 2019

mheon commented Jul 11, 2019

openshift-ci-robot commented Jul 11, 2019

stefanb2 commented Jul 11, 2019

stefanb2 commented Jul 16, 2019

mheon commented Jul 16, 2019

mheon commented Jul 16, 2019

rhatdan commented Jul 16, 2019

Fix handling of healthcheck from image #3539

Fix handling of healthcheck from image #3539

Conversation

stefanb2 commented Jul 9, 2019

openshift-ci-robot commented Jul 9, 2019

haircommander commented Jul 9, 2019

rh-atomic-bot commented Jul 9, 2019

stefanb2 commented Jul 10, 2019

mheon commented Jul 10, 2019

stefanb2 commented Jul 10, 2019 • edited Loading

stefanb2 commented Jul 11, 2019

stefanb2 commented Jul 11, 2019

mheon commented Jul 11, 2019

openshift-ci-robot commented Jul 11, 2019

stefanb2 commented Jul 11, 2019

stefanb2 commented Jul 16, 2019

mheon commented Jul 16, 2019

mheon commented Jul 16, 2019

rhatdan commented Jul 16, 2019

stefanb2 commented Jul 10, 2019 •

edited

Loading