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

Update readiness probe docs to match observed behaviour #49476

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

NovemberZulu
Copy link

@NovemberZulu NovemberZulu commented Jan 17, 2025

Description

This PR updates documentation about readiness probes to explicitly state that pods that are being terminating (i.e. .metadata.deletionTimestamp is set) still have Ready condition "True" and .status.Phase == "Running". I kept the exiting text, but looking at

  1. https://github.com/kubernetes/kubernetes/blob/f64b651ebae643d422f4625161dc415970e2c166/pkg/kubelet/prober/prober_manager.go#L298 and https://github.com/kubernetes/kubernetes/blob/f64b651ebae643d422f4625161dc415970e2c166/pkg/kubelet/prober/prober_manager.go#L339

  2. https://github.com/kubernetes/kubernetes/blob/f64b651ebae643d422f4625161dc415970e2c166/pkg/kubelet/prober/worker.go#L259

I'd say that liveness READINESS probes are processed the same way no matter is the pod is terminating or not. Please advice if we should rephrase the note more, or remove it entirely. Thank you!

EDIT: Confused readiness and liveness probes

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 17, 2025
@k8s-ci-robot
Copy link
Contributor

Welcome @NovemberZulu!

It looks like this is your first PR to kubernetes/website 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/website has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added the language/en Issues or PRs related to English language label Jan 17, 2025
@k8s-ci-robot k8s-ci-robot requested a review from tengqm January 17, 2025 15:54
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign sftim for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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

@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jan 17, 2025
Copy link

netlify bot commented Jan 17, 2025

Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit 68480f9
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/679a03e37830c10008533fbf
😎 Deploy Preview https://deploy-preview-49476--kubernetes-io-main-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@thisisharrsh
Copy link
Contributor

/sig docs

@k8s-ci-robot k8s-ci-robot added the sig/docs Categorizes an issue or PR as relevant to SIG Docs. label Jan 20, 2025
Comment on lines 540 to 541
to stop. Note that`Ready` condition in the Pod status is still `"True"`
and `.status.phase` is `"Running"` while the Pod is terminating.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd avoid putting “note that“ inside a node callout.
Try:

Suggested change
to stop. Note that`Ready` condition in the Pod status is still `"True"`
and `.status.phase` is `"Running"` while the Pod is terminating.
to stop. The`Ready` condition in the Pod status remains true, and `.status.phase`
remains Running, until the Pod is fully terminated.

Which of these do you think is true @NovemberZulu:

  • during Pod termination, the kubelet omits readiness probing, even when readiness probes are defined
  • during Pod termination, the kubelet continues readiness probing when readiness probes are defined, but ignores the result and always marks the Pod as ready
  • during Pod termination, the kubelet continues readiness probing when readiness probes are defined, but ignores the result and retains the Ready condition value that was in effect before termination began
  • during Pod termination, the kubelet continues readiness probing when readiness probes are defined, but ignores the result and retains the Ready condition value that was in effect before termination began
  • during Pod termination, the value Ready condition depends on whether the Pod uses Pod readiness+ and other factors not mentioned in the current docs
  • none of the above are correct

?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the PR, thank you for the suggestion!

Based on my current understanding of the code, I think the behaviour is:

  • during Pod termination, the kubelet continues readiness probing when readiness probes are defined and marks Pod as either ready or not ready depending on the result, i.e. none of the above

Comment on lines 540 to 541
to stop. The`Ready` condition in the Pod status remains true, and `.status.phase`
remains Running, until the Pod is fully terminated.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • What if there is a readiness probe defined? Is this sentence still true?
  • What is an unready state if not having condition Ready set true?

Copy link
Contributor

@network-charles network-charles Jan 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried it and the new sentence is false.

To re-simulate it, run the below.

apiVersion: v1
kind: Pod
metadata:
  name: test
spec:
  containers:
  - name: test
    image: nginx
    ports:
    - containerPort: 80
    readinessProbe:
      httpGet:
        path: /
        port: 80
      initialDelaySeconds: 5
      periodSeconds: 5
    lifecycle:
      preStop:
        exec:
          command: ["sh", "-c", "sleep 20"] 

Result at around 18-20 seconds

kubectl describe pod test 
...
Status:           Running
...
Conditions:
  Type                        Status
  PodReadyToStartContainers   False < 
  Initialized                 True 
  Ready                       False <
  ContainersReady             False <
  PodScheduled                True 

An unready state occurs when a readiness probe fails, but the container is running. For example, if you use a path that doesn't exist in an HTTP Get probe, the pod will be in an unready state.

apiVersion: v1
kind: Pod
metadata:
  name: test
spec:
  containers:
  - name: test
    image: nginx
    ports:
    - containerPort: 80
    readinessProbe:
      httpGet:
        path: /doesnotexist
        port: 80
      initialDelaySeconds: 5
      periodSeconds: 5
kubectl describe pod test 
...
Status:           Running
...
Conditions:
  Type                        Status
  PodReadyToStartContainers   True 
  Initialized                 True 
  Ready                       False <
  ContainersReady             False <
  PodScheduled                True 

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me reword. Ready condition doesn't turn unconditionally "True", but it doesn't turn unconditionally "False" either

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sftim

What is an unready state if not having condition Ready set true?

This is a good question. Based on "on deletion, the Pod automatically puts itself into an unready state regardless of whether the readiness probe exists" I assumed that Ready condition will turn to "False" when I delete the pod, but this is not the case. As discussed in in Termination of Pods, terminating pods don't receive traffic from LBs. I have an assumption that "pod is ready" <=> "pod accepts traffic", but it looks like this is not actually true. Should we remove the note altogether?

@NovemberZulu
Copy link
Author

I reworded the note to talk about traffic, since arguably this was the initial intention.

Comment on lines 537 to 540
necessarily need a readiness probe; when the POD is deleted, the corresponding endpoint
will have its `ready` status as `false`, so load balancers will not use it for regular
traffic. The endpoint remains in the unready state while it waits for the containers
in the Pod to stop.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should talk about serving and ready - or hyperlink to the relevant existing docs?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try to add details about endpoint conditions.

into an unready state regardless of whether the readiness probe exists.
The Pod remains in the unready state while it waits for the containers in the Pod
to stop.
necessarily need a readiness probe; when the POD is deleted, the corresponding endpoint
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Pod (not POD)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will fix

@NovemberZulu
Copy link
Author

Another attempt :)

Co-authored-by: Tim Bannister <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language sig/docs Categorizes an issue or PR as relevant to SIG Docs. 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.

5 participants