Pods which have not "started" can not be "ready" #92196

thockin · 2020-06-16T17:39:36Z

Before this commit, containers which have both a startupProbe and a
readinessProbe are marked as ready=false during stratup, but
containers which have only a startupProbe are marked ready=true.
This doesn't make sense.

This commit only considers readiness if the container is considered to
have "started", which leaves ready=false while starting up.

/kind bug

Fixes #89995

Special notes for your reviewer:

I am NOT super familiar with this code area. I dug around to find this and empirically it seems to work.

Does this PR introduce a user-facing change?:

Containers which specify a `startupProbe` but not a `readinessProbe` were previously considered "ready" before the `startupProbe` completed, but are now considered "not-ready".

k8s-ci-robot · 2020-06-16T17:40:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: thockin

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:

~~pkg/kubelet/prober/OWNERS~~ [thockin]

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

matthyx · 2020-06-16T17:42:13Z

Thanks Tim. Let me have a look later today...

thockin · 2020-06-16T19:22:10Z

/retest

matthyx · 2020-06-16T19:28:38Z

Tests are fine, which show the poor coverage of these edge cases...
Looks good, but I will add some tests in a follow-up PR.
/lgtm
/hold
Thanks!

matthyx · 2020-06-16T19:29:38Z

you can cancel the hold when you are ready.

matthyx · 2020-06-16T19:46:46Z

Once merged we should look at the serial e2e test results as there is some coverage there.

SergeyKanzhelev · 2020-06-16T21:13:09Z

pkg/kubelet/prober/prober_manager.go

@@ -261,6 +249,20 @@ func (m *manager) UpdatePodStatus(podUID types.UID, podStatus *v1.PodStatus) {
 			started = !exists
 		}
 		podStatus.ContainerStatuses[i].Started = &started
+
+		if started {


If there is no way container can be ready, but not started, perhaps the status can be stored in a single variable instead of two independent flags?

No, there are more ramifications... I think it's clearer to keep both even just for the sake of explanation and ease of testing.

Do you mean in the API or in this function?

The API has shipped, we don't want to change that.

This commit was designed to be surgical - move the code block, add an if. Further cleanup may be possible, but it seems low-value to me - this code is pretty simple to read?

I meant API =). I commented before looked into it and understand it's seems to be hard to change. The only benefit is guaranteed consistency of the status. This method is ok. But there are more. For instance, this:

kubernetes/pkg/kubelet/status/status_manager.go

Line 241 in 323f348

containerStatus.Ready = ready

where Ready and Started are set seemingly independently.

That case (I think) is after readiness has been considered at a lower level, but I admit I am not 100% confident in this code area any more :(

thockin · 2020-06-17T00:02:31Z

/retest

verify failed after 2 hours and a LOT of logs

Before this commit, containers which have both a `startupProbe` and a `readinessProbe` are marked as `ready=false` during stratup, but containers which have only a `startupProbe` are marked `ready=true`. This doesn't make sense. This commit only considers readiness if the container is considered to have "started", which leaves `ready=false` while starting up.

k8s-ci-robot · 2020-06-17T04:57:03Z

New changes are detected. LGTM label has been removed.

matthyx · 2020-06-17T06:03:24Z

@thockin what's the difference between your initial commit and the force-pushed one? I don't see it...

matthyx · 2020-06-17T06:08:50Z

@SergeyKanzhelev there are 2 states, ready and started, and they represent 2 different notions that are monitored by different probes... started is a permanent state (once startupProbe succeeds it never changes) whereas ready depends on the result of the readinessProbe.

Please refer to the KEP (kubernetes/enhancements#950) or my lightning talk on the subject: https://youtu.be/wO1uy9QKNHQ

SergeyKanzhelev · 2020-06-17T06:55:06Z

@SergeyKanzhelev there are 2 states, ready and started, and they represent 2 different notions that are monitored by different probes... started is a permanent state (once startupProbe succeeds it never changes) whereas ready depends on the result of the readinessProbe.

Totally with you on this. My comment was that there are three acceptable states represented with four possible combinations of flags. Which led to this Ready, but not Started issue. And there are seems to be other places which set these flags independently without ensuring that container is not ending up in un-acceptable state.

I'm pretty sure the Ready flag was already defined when Started was introduced. So it wasn't much of a choice for redesigning flags. I made my comment before looking deeper into changes needed to forbid this un-acceptable state in compile time.

Also, great lightning talk.

matthyx · 2020-06-17T06:59:04Z

seems to be other places which set these flags independently without ensuring that container is not ending up in un-acceptable state.

Good point, should we create another issue, or put everything on #89995 - in that case this PR should not "fix" it alone.

heckad · 2020-06-17T10:32:38Z

Recently, I noticed that pods without a readiness probe continue to receive traffic while they are in terminating status. In this regard, I propose to add new state for startUp probe and make liveness and readiness probs work only for running state. If our pods in terminating or startUp state, they are should be not ready also if readiness probe didn't set. Olso add startUp probe an excellent start to add a new feature which should run depended pods only when pod started. For example, if bd not in startUp state, we don't start pods dependent from DB.

Why is it so important to solve this problem?
Because the readiness probe is responsible not only for sending traffic but also for making a decision when stopping old pods, if we don't have readiness probe then Kubernetes stopping old pods when new just start starting. Because I propose to add a new state and make readiness and licenses probes available only for running pods, not for pods in startUp state or in terminating states.

Example behaviour when pod got kill signal but have ready state

Conditions:
  Type              Status
  Initialized       True
  Ready             True
  ContainersReady   True
  PodScheduled      True
Volumes:
  default-token-frfdw:
    Type:        Secret (a volume populated by a Secret)
    SecretName:  default-token-frfdw
    Optional:    false
QoS Class:       BestEffort
Node-Selectors:  <none>
Tolerations:     node.kubernetes.io/not-ready:NoExecute for 300s
                 node.kubernetes.io/unreachable:NoExecute for 300s
Events:
  Type    Reason   Age   From                                  Message
  ----    ------   ----  ----                                  -------
  Normal  Killing  24s   kubelet, vmi310368.contaboserver.net  Stopping container backend-order-crud

Thanks for your attention!

matthyx · 2020-06-17T11:10:22Z

I noticed that pods without a readiness probe continue to receive traffic while they are in terminating status.

This has always been the case... when you don't specify a readinessProbe, you assume an always ready state.

I propose to add a new state and make readiness and liveness probes available only for running pods, not for pods in starting or terminating states.

This is clearly a different use case than what the startupProbe was designed to solve. If you feel strong enough you can take the point and start a KEP to modify the handling of the shutdown phase...

However, now that we have the startupProbe, people could almost do without a readinessProbe (if they don't care about removing/adding back the pod to the load balancer pool) if that case had been covered. Then it could be a feature (or follow up) of my KEP...

What do you think @thockin (from a philosophical point of view) ?

heckad · 2020-06-17T11:37:52Z

This has always been the case... when you don't specify a readinessProbe, you assume an always ready state.

Yes, I propose to change this behaviour. A pod should be ready only in running state and readiness probe should manage ready state also only for running state. In startUp and terminating state pod should be not ready. I think this is bad behaviour when requests can go to pods wich starting work and completing work. How do you think this is a good propose? This propose can solve many problems like this.

matthyx · 2020-06-17T11:43:45Z

I think this is bad behaviour when requests can go to pods wich starting work and completing work.

According to this page, it should not be the case: https://kubernetes.io/docs/concepts/workloads/pods/pod/#termination-of-pods

Even if the pod is still "ready" no new traffic should go to it... so if it's something you can reproduce I suggest you fill a bug report (which is something I can help you with).

heckad · 2020-06-17T11:58:18Z

Even if the pod is still "ready" no new traffic should go to it...

What mean "ready" for pods in termination state?

matthyx · 2020-06-17T12:06:10Z

What mean "ready" for pods in termination state?

I need to check in the kubelet code...

matthyx · 2020-06-17T12:54:35Z

/test pull-kubernetes-kubemark-e2e-gce-big
/test pull-kubernetes-e2e-kind
/test pull-kubernetes-verify

thockin · 2020-06-17T14:31:48Z

It was just a rebase - trying to unstick tests

dims · 2020-06-17T23:44:47Z

/hold Temporary hold to get prow/tide to get back on its feet. Feel free to remove hold in a few hours.

dims · 2020-06-17T23:59:20Z

/hold cancel

…6-upstream-release-1.18 Automated cherry pick of #92196: Pods which have not "started" can not be "ready"

thockin requested a review from matthyx June 16, 2020 17:39

thockin mentioned this pull request Jun 16, 2020

Unexpected startupProbe behavior. #89995

Closed

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 16, 2020

thockin added priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. sig/node Categorizes an issue or PR as relevant to SIG Node. labels Jun 16, 2020

k8s-ci-robot requested review from feiskyer and Random-Liu June 16, 2020 17:41

k8s-ci-robot added area/kubelet and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jun 16, 2020

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 16, 2020

k8s-ci-robot assigned matthyx Jun 16, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 16, 2020

matthyx approved these changes Jun 16, 2020

View reviewed changes

SergeyKanzhelev reviewed Jun 16, 2020

View reviewed changes

thockin removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 17, 2020

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 17, 2020

thockin added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 17, 2020

matthyx mentioned this pull request Jun 17, 2020

Add tests covering startup probe without readiness #92239

Merged

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 17, 2020

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 17, 2020

k8s-ci-robot merged commit 78b503d into kubernetes:master Jun 18, 2020

k8s-ci-robot added this to the v1.19 milestone Jun 18, 2020

github-actions bot mentioned this pull request Jun 23, 2020

Week Ending June 21, 2020 dev-obs/actus#183

Open

matthyx mentioned this pull request Jun 24, 2020

Automated cherry pick of #92196: Pods which have not "started" can not be "ready" #92477

Merged

SergeyKanzhelev mentioned this pull request Jun 29, 2020

REQUEST: New membership for SergeyKanzhelev kubernetes/org#1965

Closed

6 tasks

k8s-ci-robot added a commit that referenced this pull request Jul 9, 2020

Merge pull request #92477 from matthyx/automated-cherry-pick-of-#9219…

b7fa672

…6-upstream-release-1.18 Automated cherry pick of #92196: Pods which have not "started" can not be "ready"

Haar mentioned this pull request Sep 29, 2020

startupProbe readiness state update issues #95140

Closed

tcdowney mentioned this pull request Nov 24, 2020

use startupProbe instead of readinessProbe for rake-task based containers cloudfoundry/capi-k8s-release#110

Closed

andrewsykim mentioned this pull request Nov 30, 2020

Set ContainerStatuses[i].Ready to false when container not started #96805

Closed

thockin deleted the startup-probe-blocks-readiness branch August 2, 2021 04:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pods which have not "started" can not be "ready" #92196

Pods which have not "started" can not be "ready" #92196

thockin commented Jun 16, 2020 •

edited

Loading

k8s-ci-robot commented Jun 16, 2020

matthyx commented Jun 16, 2020

thockin commented Jun 16, 2020

matthyx commented Jun 16, 2020

matthyx commented Jun 16, 2020

matthyx commented Jun 16, 2020

SergeyKanzhelev Jun 16, 2020

matthyx Jun 16, 2020

thockin Jun 16, 2020

SergeyKanzhelev Jun 17, 2020

thockin Jun 17, 2020

thockin commented Jun 17, 2020

k8s-ci-robot commented Jun 17, 2020

matthyx commented Jun 17, 2020

matthyx commented Jun 17, 2020

SergeyKanzhelev commented Jun 17, 2020

matthyx commented Jun 17, 2020

heckad commented Jun 17, 2020 •

edited

Loading

matthyx commented Jun 17, 2020

heckad commented Jun 17, 2020 •

edited

Loading

matthyx commented Jun 17, 2020 •

edited

Loading

heckad commented Jun 17, 2020

matthyx commented Jun 17, 2020

matthyx commented Jun 17, 2020

thockin commented Jun 17, 2020

dims commented Jun 17, 2020

dims commented Jun 17, 2020

Pods which have not "started" can not be "ready" #92196

Pods which have not "started" can not be "ready" #92196

Conversation

thockin commented Jun 16, 2020 • edited Loading

k8s-ci-robot commented Jun 16, 2020

matthyx commented Jun 16, 2020

thockin commented Jun 16, 2020

matthyx commented Jun 16, 2020

matthyx commented Jun 16, 2020

matthyx commented Jun 16, 2020

SergeyKanzhelev Jun 16, 2020

Choose a reason for hiding this comment

matthyx Jun 16, 2020

Choose a reason for hiding this comment

thockin Jun 16, 2020

Choose a reason for hiding this comment

SergeyKanzhelev Jun 17, 2020

Choose a reason for hiding this comment

thockin Jun 17, 2020

Choose a reason for hiding this comment

thockin commented Jun 17, 2020

k8s-ci-robot commented Jun 17, 2020

matthyx commented Jun 17, 2020

matthyx commented Jun 17, 2020

SergeyKanzhelev commented Jun 17, 2020

matthyx commented Jun 17, 2020

heckad commented Jun 17, 2020 • edited Loading

matthyx commented Jun 17, 2020

heckad commented Jun 17, 2020 • edited Loading

matthyx commented Jun 17, 2020 • edited Loading

heckad commented Jun 17, 2020

matthyx commented Jun 17, 2020

matthyx commented Jun 17, 2020

thockin commented Jun 17, 2020

dims commented Jun 17, 2020

dims commented Jun 17, 2020

thockin commented Jun 16, 2020 •

edited

Loading

heckad commented Jun 17, 2020 •

edited

Loading

heckad commented Jun 17, 2020 •

edited

Loading

matthyx commented Jun 17, 2020 •

edited

Loading