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

Updated docs for EndpointSliceTerminatingCondition #38390

Closed
wants to merge 2 commits into from

Conversation

YashPimple
Copy link
Contributor

Resolved Issue #38387 Stale docs for EndpointSliceTerminatingCondition

Made a new PR, similar to #37913, but targeting the main branch as per Instructed.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 10, 2022
@k8s-ci-robot k8s-ci-robot requested a review from freehan December 10, 2022 17:55
@k8s-ci-robot k8s-ci-robot added the language/en Issues or PRs related to English language label Dec 10, 2022
@k8s-ci-robot k8s-ci-robot added the sig/docs Categorizes an issue or PR as relevant to SIG Docs. label Dec 10, 2022
@netlify
Copy link

netlify bot commented Dec 10, 2022

Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit 14420f6
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/63a3128a92dcc90009c36c99
😎 Deploy Preview https://deploy-preview-38390--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 settings.

Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

Thanks

LGTM

@mehabhalodiya
Copy link
Contributor

Thanks @YashPimple

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 21, 2022
@@ -87,16 +87,16 @@ The three conditions are `ready`, `serving`, and `terminating`.

#### Ready

`ready` is a condition that maps to a Pod's `Ready` condition. A running Pod with the `Ready`
condition set to `True` should have this EndpointSlice condition also set to `true`. For
`ready` is a condition that maps to a Pod's `ready` condition. A running Pod with the `ready`
Copy link
Contributor

Choose a reason for hiding this comment

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

No. Pod's condition is called Ready, not ready.

Copy link
Contributor

@mehabhalodiya mehabhalodiya Dec 21, 2022

Choose a reason for hiding this comment

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

@YashPimple Please do the changes according to the suggestion. It slipped away from my sight!

@sftim
Copy link
Contributor

sftim commented Dec 21, 2022

/lgtm cancel

We should not use the wrong condition name.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 21, 2022
@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 ask for approval from sftim by writing /assign @sftim in a comment. For more information see the Kubernetes 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

`ready` is a condition that maps to a Pod's `Ready` condition. A running Pod with the `Ready`
condition set to `True` should have this EndpointSlice condition also set to `true`. For
compatibility reasons, `ready` is NEVER `true` when a Pod is terminating. Consumers should refer
`Ready` is a condition that maps to a Pod's `Ready` condition. A running Pod with the `Ready`
Copy link
Contributor

Choose a reason for hiding this comment

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

Checked the source code. For Endpoints, the unofficial condition type is ready, for Pod, the official condition type is Ready. This inconsistency is annoying, but it is the fact to be documented.

Copy link
Contributor

Choose a reason for hiding this comment

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

unofficial

But part of Kubernetes, right?

We might want to (separately) sprinkle some note shortcodes to point out the oddness.

Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

From elsewhere in the file:

`Terminating` is a condition that indicates whether an endpoint is terminating.

Is that actually terminating?

Comment on lines +90 to +92
`Ready` is a condition that maps to a Pod's `Ready` condition. A running Pod with the `Ready`
condition set to `true` should have this EndpointSlice condition also set to `true`. For
compatibility reasons, `Ready` is NEVER `true` when a Pod is terminating. Consumers should refer
Copy link
Contributor

Choose a reason for hiding this comment

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

OK, so is this right?

Suggested change
`Ready` is a condition that maps to a Pod's `Ready` condition. A running Pod with the `Ready`
condition set to `true` should have this EndpointSlice condition also set to `true`. For
compatibility reasons, `Ready` is NEVER `true` when a Pod is terminating. Consumers should refer
For an EndpointSlice, `ready` is a condition that maps to a Pod's `Ready` condition. A running Pod with
the `Ready` condition set to `true` should have any related EndpointSlice `ready` condition (in all-lowercase)
also set to `true`. For compatibility reasons, `ready` for an EndpointSlice is **never** `true` when a Pod is
terminating. Consumers should refer

Comment on lines 94 to +95
this rule is for Services with `spec.publishNotReadyAddresses` set to `true`. Endpoints for these
Services will always have the `ready` condition set to `true`.
Services will always have the `Ready` condition set to `true`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
this rule is for Services with `spec.publishNotReadyAddresses` set to `true`. Endpoints for these
Services will always have the `ready` condition set to `true`.
Services will always have the `Ready` condition set to `true`.
this rule is for Services where `spec.publishNotReadyAddresses` is set to `true`. For such
Services, any endpoints always have the `ready` condition set to `true`.

(Aside: I wonder what serving is set to if a Service has .spec.publishNotReadyAddresses set to true)


`serving` is identical to the `ready` condition, except it does not account for terminating states.
`serving` is identical to the `Ready` condition, except it does not account for terminating states.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this right?

Suggested change
`serving` is identical to the `Ready` condition, except it does not account for terminating states.
The `serving` condition is almost identical to the `ready` condition. The difference is that
`serving` continues checking the actual Pod status during Pod termination, whereas `ready`
turns `false` the moment that the Pod starts to terminate.


`serving` is identical to the `ready` condition, except it does not account for terminating states.
`serving` is identical to the `Ready` condition, except it does not account for terminating states.
Consumers of the EndpointSlice API should check this condition if they care about pod readiness while
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Consumers of the EndpointSlice API should check this condition if they care about pod readiness while
Consumers of the EndpointSlice API should check `serving` if they care about pod readiness while

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 28, 2022
@k8s-ci-robot
Copy link
Contributor

@YashPimple: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@mehabhalodiya
Copy link
Contributor

👋🏻 @YashPimple Please do the changes suggested above and rebase your PR too.

Here are some references/tips:

Another option would be:
Close this pull request, and open a new one based on the main branch that makes the same changes. In the new PR, mention that it's a replacement for PR #38390

Thank you.

YashPimple added a commit to YashPimple/website that referenced this pull request Dec 30, 2022
@YashPimple YashPimple closed this Dec 30, 2022
@sftim
Copy link
Contributor

sftim commented Jan 4, 2023

Replaced by #38701

k8s-ci-robot pushed a commit that referenced this pull request Mar 20, 2023
* Rebased issue #38390

* Updated endpoint-slices.md

Resolved issue #38701

* Updated endpoint-slice.md

* endpoint-slice.md

* New endpoint-slice.md file

* Updated endpoint-slice.md
DonatoHorn pushed a commit to DonatoHorn/website that referenced this pull request Jun 25, 2023
* Rebased issue kubernetes#38390

* Updated endpoint-slices.md

Resolved issue kubernetes#38701

* Updated endpoint-slice.md

* endpoint-slice.md

* New endpoint-slice.md file

* Updated endpoint-slice.md
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 needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants