Skip to content
This repository was archived by the owner on Feb 22, 2022. It is now read-only.

[incubator/vault] Remove liveness probe #11320

Closed
wants to merge 2 commits into from

Conversation

lawliet89
Copy link
Contributor

@lawliet89 lawliet89 commented Feb 11, 2019

What this PR does / why we need it:

The readiness probe already runs throughout the lifetime of the pod and
will also signal the health of the pod.

Also, using a "bare" TCP probe results in many error messages from Vault
in the form of 2019-02-11T08:51:56.900Z [INFO] http: TLS handshake error from 172.19.9.1:49392: EOF

There is probably no point in duplicating the same httpGet check in
the liveness probe too.

This is an alternative to #9462. I don't see a point in doing the exact same check.

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • DCO signed
  • Chart Version bumped
  • Variables are documented in the README.md

The readiness probe already runs throughout the lifetime of the pod and
will also signal the health of the pod.

Also, using a "bare" TCP probe results in many error messages from Vault
in the form of `2019-02-11T08:51:56.900Z [INFO] http: TLS handshake
error from 172.19.9.1:49392: EOF`

There is probably no point in duplicating the same `httpGet` check in
the liveness probe too.

Signed-off-by: Yong Wen Chua <lawliet89@users.noreply.github.com>
@helm-bot helm-bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). labels Feb 11, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: lawliet89
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: sameersbn

If they are not already assigned, you can assign the PR to them by writing /assign @sameersbn in a comment when ready.

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:

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

@helm-bot helm-bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 25, 2019
Signed-off-by: Yong Wen Chua <lawliet89@users.noreply.github.com>
@helm-bot helm-bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 25, 2019
@lawliet89
Copy link
Contributor Author

lawliet89 commented Feb 25, 2019

Can I get someone to review/merge this please? I've fixed the conflicts.

@cpanato
Copy link
Member

cpanato commented Mar 7, 2019

the readiness probe run just one to let k8s know it is ready to accept traffic and liveness run during the lifetime of the pod to let the scheduler it is alive otherwise kill the pod.
i did not get why do you want to remove the liveness check

@lawliet89
Copy link
Contributor Author

lawliet89 commented Mar 7, 2019

Doesn't the readiness probe run for the entire lifetime of the pod? At least that's what the docs says.

In any case I could always add it back but it shouldn't be a bare tcp connection because it causes a lot of noise in the logs wrt bad tls handshakes.

@cpanato
Copy link
Member

cpanato commented Mar 7, 2019

@lawliet89 my understanding
liveness: "Many applications running for long periods of time eventually transition to broken states, and cannot recover except by being restarted. Kubernetes provides liveness probes to detect and remedy such situations" from https://kubernetes.io/docs/tasks/configure-pod-container/configure-liveness-readiness-probes/#define-a-liveness-command

readiness "Sometimes, applications are temporarily unable to serve traffic. For example, an application might need to load large data or configuration files during startup, or depend on external services after startup. In such cases, you don’t want to kill the application, but you don’t want to send it requests either. Kubernetes provides readiness probes to detect and mitigate these situations. A pod with containers reporting that they are not ready does not receive traffic through Kubernetes Services." from https://kubernetes.io/docs/tasks/configure-pod-container/configure-liveness-readiness-probes/#define-readiness-probes

@jbialy
Copy link
Contributor

jbialy commented Mar 7, 2019

@lawliet89 I just got my PR merged (#9462) that should address the tcp handshake errors by hitting the vault health endpoint using http/https.

@lawliet89
Copy link
Contributor Author

Great. Then I guess there's no need for this PR.

@lawliet89 lawliet89 closed this Mar 7, 2019
@mkozjak
Copy link

mkozjak commented Mar 9, 2019

@jbialy please check my review here: #9462 (review)
With 0.15.0 the pod gets in a CrashLoopBackOff, since k8s is killing it based on 503 and it shouldn't be, because that error means that Vault is in a sealed state.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants