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

podman incompatiblity with healthcheck from docker image #3507

Closed
stefanb2 opened this issue Jul 8, 2019 · 17 comments
Closed

podman incompatiblity with healthcheck from docker image #3507

stefanb2 opened this issue Jul 8, 2019 · 17 comments
Labels
kind/bug Categorizes issue or PR as related to a bug. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.

Comments

@stefanb2
Copy link
Contributor

stefanb2 commented Jul 8, 2019

Is this a BUG REPORT or FEATURE REQUEST? (leave only one on its own line)

/kind bug

Description

Podman interprets the HEALTHCHECK section in a Docker image incorrectly. Instead of running the expected command line docker-healthcheck the healthcheck tries to execute CMD.

Steps to reproduce the issue:

I'm using docker images that have been created using this Dockerfile snippet

COPY docker-healthcheck /usr/local/bin/
HEALTHCHECK CMD ["docker-healthcheck"]

According to Dockerfile reference documentation the above is correct. The healthcheck works without problem when I use the image with docker.

When I use podman image inspect on the docker image I see:

"History": [
    ...
    {
        "created": "2019-01-24T15:17:53.220742027Z",
        "created_by": "/bin/sh -c #(nop)  HEALTHCHECK \u0026{[\"CMD\" \"docker-healthcheck\"] \"0s\" \"0s\" \"0s\" '\\x00'}",
        "empty_layer": true
    },
    ...
]

On the container created from that image podman container inspect shows:

"Config": {
    ...
    "Healthcheck": {
       "Test": [
          "CMD",
          "docker-healthcheck"
       ]
    }
} 

i.e. the CMD from the original HEALTHCHECK line was not removed.

Describe the results you received:

Executing podman healthcheck run returns code 125 and when inspecting the log in the container I see:

"Healthcheck": {
    "Status": "unhealthy",
    "FailingStreak": 1,
    "Log": [
        {
            "Start": "2019-07-08T10:51:52.203856914+03:00",
            "End": "2019-07-08T10:51:52.355135943+03:00",
            "ExitCode": -1,
            "Output": "time=\"2019-07-08T10:51:52+03:00\" level=error msg=\"exec failed: container_linux.go:346: starting container process caused \\\"exec: \\\\\\\"CMD\\\\\\\": executable file not found in $PATH\\\"\\n\"\nexec failed: container_linux.go:346: starting container process caused \"exec: \\\"CMD\\\": executable file not found in $PATH\"\n"
        }
    ]
}

Describe the results you expected:

podman healthcheck run should trigger the command docker-healthcheck inside the container.

Additional information you deem important (e.g. issue happens only occasionally):

Output of podman version:

Version:            1.4.5-dev
RemoteAPI Version:  1
Go Version:         go1.12.6
OS/Arch:            linux/amd64

Output of podman info --debug:

debug:
  compiler: gc
  git commit: ""
  go version: go1.12.6
  podman version: 1.4.5-dev
host:
  BuildahVersion: 1.9.0
  Conmon:
    package: podman-1.4.4-1.fc30.x86_64
    path: /usr/libexec/podman/conmon
    version: 'conmon version 0.2.0, commit: 41010e63c287618b1dc34ee11d10d268e2feeefe'
  Distribution:
    distribution: fedora
    version: "30"
  MemFree: 19378372608
  MemTotal: 33400737792
  OCIRuntime:
    package: runc-1.0.0-93.dev.gitb9b6cc6.fc30.x86_64
    path: /usr/bin/runc
    version: |-
      runc version 1.0.0-rc8+dev
      commit: e3b4c1108f7d1bf0d09ab612ea09927d9b59b4e3
      spec: 1.0.1-dev
  SwapFree: 32002535424
  SwapTotal: 32002535424
  arch: amd64
  cpus: 12
  hostname: beckst-lnx
  kernel: 5.1.16-300.fc30.x86_64
  os: linux
  rootless: true
  uptime: 17h 25m 39.96s (Approximately 0.71 days)
registries:
  blocked: null
  insecure: null
  search:
  - docker.io
  - registry.fedoraproject.org
  - quay.io
  - registry.access.redhat.com
  - registry.centos.org
store:
  ConfigFile: /home/stefanb/.config/containers/storage.conf
  ContainerStore:
    number: 4
  GraphDriverName: overlay
  GraphOptions:
  - overlay.mount_program=/usr/bin/fuse-overlayfs
  GraphRoot: /home/stefanb/.local/share/containers/storage
  GraphStatus:
    Backing Filesystem: extfs
    Native Overlay Diff: "false"
    Supports d_type: "true"
    Using metacopy: "false"
  ImageStore:
    number: 6
  RunRoot: /tmp/1000
  VolumePath: /home/stefanb/.local/share/containers/storage/volumes
@openshift-ci-robot openshift-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Jul 8, 2019
@stefanb2 stefanb2 changed the title podman incompatible with healtcheck from docker image podman incompatiblity with healtcheck from docker image Jul 8, 2019
@stefanb2
Copy link
Contributor Author

stefanb2 commented Jul 8, 2019

Just to make sure: the healthcheck script in the image is not broken.

If I manually create the container from the given image and override the healthcheck command:

$ podman run --name=test ... --healthcheck-command docker-healthcheck ...
$ podman healthcheck run test; echo $?
healthy
0 

Inspection on the container:

...
            "Healthcheck": {
                "Status": "healthy",
                "FailingStreak": 0,
                "Log": [
                    {
                        "Start": "2019-07-08T12:02:11.086603816+03:00",
                        "End": "2019-07-08T12:02:11.337532264+03:00",
                        "ExitCode": 0,
                        "Output": ""
                    }
                ]
            }
...
            "Healthcheck": {
                "Test": [
                    "docker-healthcheck"
                ],
                "Interval": 30000000000,
                "Timeout": 30000000000,
                "Retries": 3
            }

@stefanb2
Copy link
Contributor Author

stefanb2 commented Jul 8, 2019

I guess to be 100% compatible podman should also understand ['CMD-SHELL', 'some shell string']

@stefanb2 stefanb2 changed the title podman incompatiblity with healtcheck from docker image podman incompatiblity with healthcheck from docker image Jul 8, 2019
@mheon
Copy link
Member

mheon commented Jul 8, 2019

Hm. I'm not actually sure whether this is a Buildah problem (creating the image) or a c/image problem (parsing the healthcheck out of the image). Are you building these images yourself with Podman, or pulling them down from a registry?

@stefanb2
Copy link
Contributor Author

stefanb2 commented Jul 8, 2019

I guess I could have stated this clearer in my description.

These are docker build generated images pulled from our company internal repository via podman pull. No Buildah is involved.

@mheon
Copy link
Member

mheon commented Jul 8, 2019

Alright, it's a parsing issue, then. Thanks.

@rhatdan
Copy link
Member

rhatdan commented Jul 8, 2019

Thanks @stefanb2, any chance you could open a PR to fix?

@mheon
Copy link
Member

mheon commented Jul 8, 2019

@rhatdan I think this is a c/image thing - we might not have proper support for this format of healthcheck there...

We might want to poke @mtrmac about this one

@mtrmac
Copy link
Collaborator

mtrmac commented Jul 8, 2019

It’s literally a < 3 minute check to find https://github.com/containers/libpod/blob/61c000a1d3bc58bbd669626ab67a9552088f3242/libpod/healthcheck.go#L115 and see that it handles neither NONE nor CMD, so I’m not sure how the shoutthought of c/image could ever come up.

(c/image does not deal with the config other than json.Unmarshal (and schema1/schema2 conversions); the health check text literally does not appear in c/image, other than a declaration of the field and test fixtures.)

@baude
Copy link
Member

baude commented Jul 8, 2019

this is podman ... ill take a look

@baude
Copy link
Member

baude commented Jul 8, 2019

oh and @mheon , you poked the bear!

@mheon
Copy link
Member

mheon commented Jul 8, 2019

Oops. My bad.

stefanb2 added a commit to stefanb2/libpod that referenced this issue Jul 8, 2019
Make the documentation agree with the code.

Related containers#3507
stefanb2 added a commit to stefanb2/libpod that referenced this issue Jul 8, 2019
Make the documentation agree with the code.

Related containers#3507

Signed-off-by: Stefan Becker <chemobejk@gmail.com>
@muayyad-alsadi
Copy link
Contributor

muayyad-alsadi commented Jul 8, 2019

for what I see here

https://docs.docker.com/engine/reference/run/#healthcheck

$ docker run --name=test -d \
    --health-cmd='stat /etc/passwd || exit 1' \
    --health-interval=2s \
    busybox sleep 1d

it's using the shell, I wonder how I can tell it not to use default shell ?

please when you are done here, please ping me on podman-compose to make it speak same language

containers/podman-compose#23

@muayyad-alsadi
Copy link
Contributor

from what I see I guess podman should parse the value of --health-cmd the following way

  • it takes a single string value after it
  • if it starts with [ and can be parsed as json list, consider it a list of arguments starting with command path
  • else (either a string that does not start with [ or can't be parsed as json, then it's a shell string to be passed as a single param to ["/bin/sh", "-c", here ] )

examples

$ podman run --health-cmd='stat /etc/passwd || exit 1'  ...
$ podman run --health-cmd='[ -f /etc/passwd] || exit 1'  ...
$ podman run --health-cmd='echo monkey banana'  ...
$ podman run --health-cmd='["/usr/bin/curl", "localhost/healthz"]'  ...

stefanb2 added a commit to stefanb2/libpod that referenced this issue Jul 9, 2019
- 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

Signed-off-by: Stefan Becker <chemobejk@gmail.com>
stefanb2 added a commit to stefanb2/libpod that referenced this issue Jul 9, 2019
- 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

Signed-off-by: Stefan Becker <chemobejk@gmail.com>
stefanb2 added a commit to stefanb2/libpod that referenced this issue Jul 9, 2019
- 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

Signed-off-by: Stefan Becker <chemobejk@gmail.com>
stefanb2 added a commit to stefanb2/libpod that referenced this issue Jul 10, 2019
- remove duplicate check, already called in HealthCheck()
- reject zero-length command list 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>
stefanb2 added a commit to stefanb2/libpod that referenced this issue Jul 11, 2019
- remove duplicate check, already called in HealthCheck()
- reject zero-length command list 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>
stefanb2 added a commit to stefanb2/libpod that referenced this issue Jul 11, 2019
- 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>
stefanb2 added a commit to stefanb2/libpod that referenced this issue Jul 11, 2019
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
Copy link
Contributor Author

$ podman run --health-cmd='stat /etc/passwd || exit 1'  ...
$ podman run --health-cmd='[ -f /etc/passwd] || exit 1'  ...
$ podman run --health-cmd='echo monkey banana'  ...
$ podman run --health-cmd='["/usr/bin/curl", "localhost/healthz"]'  ...

@muayyad-alsadi see PR #3508 for the rename of the options to be compatible with Docker CLI.

Your idea goes beyond what Docker CLI implements. docker run ... --health-cmd="XYZ" ... is always translated to the container HealthCheck configuration ["CMD-SHELL", "XYZ"], i.e. the health check executed inside the container will be /bin/sh -c "XYZ".

It does not parse the option value as JSON string, i.e. --health-cmd="['A', 'B']" is translated to ["CMD-SHELL", "['A', 'B']"].

To be 100% compatible with Docker CLI, podman should not split the option value on white space as it currently does. With Docker CLI --health-cmd="/bin/false || echo test" leads to /bin/sh -c "/bin/false || echo test" (result: healthy), whereas with podman it leads to /bin/false || echo test (result: unhealthy).

@muayyad-alsadi
Copy link
Contributor

Totally agree that we should not split and stay drop in replacement.

But how to pass ["CMD", "/bin/foo", "funny", "unescaped", "args"]? as in docker compose files?
And support both cmd and cmd shell at the same time? Is there another --health-something that I'm not aware of? Please see podman-compose still open ticket.

@stefanb2
Copy link
Contributor Author

docker-compose doesn't have this problem as it is a Docker client, i.e. it talks directly to the daemon. Therefore it can pass down any value for healthcheck['test'] that passes validation. I'm not sure if libpod(?) also offers such an API, i.e. podman-compose could be rewritten to use that instead of the podman CLI.

If that is not possible then I guess a new PR requesting support for JSON values for --health-cmd in podman is in order. IMHO it wouldn't break backward compatibility.

@mheon
Copy link
Member

mheon commented Jul 12, 2019

It's not unprecedented for us to support alternative formats - for --entrypoint for example, we allow JSON arrays to set multi-element entrypoints, using the same flag as we do for single-element ones. While we definitely need to retain compatibility with Docker, we're hardly opposed to extending our CLI where sensible.

Still, I think our first course of action should be to make the CLI compatible with Docker. We can look into supporting additional features from the command line from there.

stefanb2 added a commit to stefanb2/libpod that referenced this issue Jul 13, 2019
Fix Docker CLI compatibility issue: the "--healthcheck-command" option
value should not be split but instead be passed as single string to
"/bin/sh -c <opt>".

On the other hand implement the same extension as is already available
for "--entrypoint", i.e. allow the option value to be a JSON array of
strings. This will make life easier for tools like podman-compose.

Continuation of containers#3455 & containers#3507
stefanb2 added a commit to stefanb2/libpod that referenced this issue Jul 13, 2019
Fix Docker CLI compatibility issue: the "--healthcheck-command" option
value should not be split but instead be passed as single string to
"/bin/sh -c <opt>".

On the other hand implement the same extension as is already available
for "--entrypoint", i.e. allow the option value to be a JSON array of
strings. This will make life easier for tools like podman-compose.

Continuation of containers#3455 & containers#3507
stefanb2 added a commit to stefanb2/libpod that referenced this issue Jul 13, 2019
Fix Docker CLI compatibility issue: the "--healthcheck-command" option
value should not be split but instead be passed as single string to
"/bin/sh -c <opt>".

On the other hand implement the same extension as is already available
for "--entrypoint", i.e. allow the option value to be a JSON array of
strings. This will make life easier for tools like podman-compose.

Continuation of containers#3455 & containers#3507

Signed-off-by: Stefan Becker <chemobejk@gmail.com>
stefanb2 added a commit to stefanb2/libpod that referenced this issue Jul 13, 2019
Fix Docker CLI compatibility issue: the "--healthcheck-command" option
value should not be split but instead be passed as single string to
"/bin/sh -c <opt>".

On the other hand implement the same extension as is already available
for "--entrypoint", i.e. allow the option value to be a JSON array of
strings. This will make life easier for tools like podman-compose.

Updated "--healthcheck-command" option values in tests accordingly.

Continuation of containers#3455 & containers#3507

Signed-off-by: Stefan Becker <chemobejk@gmail.com>
stefanb2 added a commit to stefanb2/libpod that referenced this issue Jul 14, 2019
Fix Docker CLI compatibility issue: the "--healthcheck-command" option
value should not be split but instead be passed as single string to
"CMD-SHELL", i.e. "/bin/sh -c <opt>".

On the other hand implement the same extension as is already available
for "--entrypoint", i.e. allow the option value to be a JSON array of
strings. This will make life easier for tools like podman-compose.

Updated "--healthcheck-command" option values in tests accordingly.

Continuation of containers#3455 & containers#3507

Signed-off-by: Stefan Becker <chemobejk@gmail.com>
stefanb2 added a commit to stefanb2/libpod that referenced this issue Jul 16, 2019
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>
@github-actions 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 24, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/bug Categorizes issue or PR as related to a bug. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

No branches or pull requests

7 participants