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

Docker Exit #43

Merged
merged 3 commits into from
Jan 12, 2022
Merged

Conversation

wandersonwhcr
Copy link
Collaborator

Context

This PR adds docs about how to implement HEALTHCHECK on Dockerfiles.

Also, adds a warning about HEALTHCHECK ignored by Kubernetes. I tried to search a great documentation on Internet about this behavior on K8S and reference it, but I didn't find something awesome.

Changes

  • Tests included
  • Documentation updated
  • Commit message is clear

@wandersonwhcr wandersonwhcr self-assigned this Dec 23, 2021
@wandersonwhcr wandersonwhcr added the enhancement New feature or request label Dec 23, 2021
@wandersonwhcr wandersonwhcr added this to the v0.6.0 milestone Dec 23, 2021
@wandersonwhcr
Copy link
Collaborator Author

@fernandesGabriel what do you think about this?

@wandersonwhcr
Copy link
Collaborator Author

Hey @s4mur4i what do you think about this?

@s4mur4i
Copy link
Contributor

s4mur4i commented Jan 5, 2022

Hey @s4mur4i what do you think about this?

In our environment we use kubernetes based environment. We have been working with following variables:

          readinessProbe:
            exec:
              command:
                - php-fpm-healthcheck
            initialDelaySeconds: 5
            periodSeconds: 30
            timeoutSeconds: 10
            successThreshold: 1
            failureThreshold: 3
          livenessProbe:
            exec:
              command:
                - php-fpm-healthcheck
            initialDelaySeconds: 5
            periodSeconds: 30
            timeoutSeconds: 10
            successThreshold: 1
            failureThreshold: 3
            ```
I think values can be raised a little to not DDOS php-fpm-healthcheck.
What I could recommend maybe:

--interval=10s
--timeout=5s
--start-period=5s

In our case at higher load we see false negatives because of lower timeouts. that's why we raised it.

@wandersonwhcr
Copy link
Collaborator Author

But about Docker, do you think this doc is interesting?

@wandersonwhcr wandersonwhcr linked an issue Jan 5, 2022 that may be closed by this pull request
@s4mur4i
Copy link
Contributor

s4mur4i commented Jan 6, 2022

One thing confuses me:
https://github.com/renatomefi/php-fpm-healthcheck/blob/master/php-fpm-healthcheck#L21-L25
according to doc here, you can have exit 2 for example, which should not be used.

So shouldn't that be handled somehow? or we can have unforeseen behavior.
Since I am on primarily on kubernetes, I cant really say if interesting or not.

@wandersonwhcr
Copy link
Collaborator Author

Yes, this is confusing, but I think our script must not handle this.

Docker only accepts exits 0 to healthy, 1 to unhealthy and 2 that should not be used. php-fpm-healthcheck can return other values because there is more problems that can be found instead of binary 0 or 1.

To resolve this, I added to docs a || exit 1 after our script, described here:

https://github.com/renatomefi/php-fpm-healthcheck/pull/43/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R167-R168

Thinking about the problem, yes, we can add a --docker argument, but I think we don't need to this right now.

@s4mur4i I asked your help because nobody is replying my messages 🤣 sorry to disturb you

@fernandesGabriel
Copy link

@wandersonwhcr sorry for the late reply.

Unfortunately, I don't think I can opiate much here. @s4mur4i configurations are close to mine by the way.

Regarding the use of HEALTHCHECK on Kubernetes, looks like they even explicitly disabled it some time ago: kubernetes/kubernetes#50796. However, in the Kubernetes context, I would still prefer to use liveness/readiness configurations instead of something defined on the Dockerfile.

@wandersonwhcr
Copy link
Collaborator Author

@fernandesGabriel ty for you reply 😄

Yes, this change also adds a warning about Kubernetes ignoring HEALTHCHECK command from Docker.

BTW, TY ❤️

@wandersonwhcr wandersonwhcr merged commit 06cc809 into renatomefi:master Jan 12, 2022
@wandersonwhcr wandersonwhcr deleted the feature/docker-exit branch January 12, 2022 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document and test Docker healthcheck
3 participants