Add Terminating Condition to EndpointSlice

[andrewsykim]

What type of PR is this?
/kind feature

What this PR does / why we need it:
Adds a serving and terminating condition to EndpointSlice as proposed in this KEP.

Which issue(s) this PR fixes:
https://github.com/kubernetes/enhancements/tree/master/keps/sig-network/1672-tracking-terminating-endpoints

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Add a 'serving' and `terminating` condition to the EndpointSlice API.

`serving` tracks the readiness of endpoints regardless of their terminating state. This is distinct from `ready` since `ready` is only true when pods are not terminating. 
`terminating` is true when an endpoint is terminating. For pods this is any endpoint with a deletion timestamp.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[andrewsykim]

/retest

[andrewsykim]

/retest

[andrewsykim]

/retest

[andrewsykim]

/retest

[andrewsykim]

/retest

[andrewsykim]

/retest

[andrewsykim]

/retest

[andrewsykim]

/retest

[andrewsykim]

/milestone v1.20
/assign @robscott

[andrewsykim]

@robscott comments addressed, PTAL

[thockin]

So the implied third state here is `!Ready && !Draining` which can only mean !serving, regardless of terminating or not. This is a little different than the discussion, but I am not sure it is meaningful. Andrew?

…

On Mon, Nov 2, 2020 at 11:27 AM Rob Scott ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pkg/apis/discovery/types.go <#92968 (comment)> : > + // terminating indicates that this endpoint is terminating. A nil value indicates an + // unknown state. Consumers should interpret this unknown state to mean that the + // endpoint is not terminating. Thanks for the context here, it was helpful to read through the previous conversation around this. One of the points that stood out to me was: Sending traffic to any pod that fails readiness seems like we're breaking API contract. I'm personally in favor of only falling back to {R, T} endpoints That makes it sound like there's no value in tracking endpoints that are !Ready && Terminating. As a variation of what Tim said above, maybe we could use 2 conditions that were defined as the following: ready: serving && !terminating draining: serving && terminating Those conditions should never be true at the same time. I'm not sure that draining is the appropriate term here, I just wanted to throw out an alternative to terminating since that may be too broad for what I'm suggesting. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#92968 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABKWAVCBUXN7WV2KGRWECMLSN4B3JANCNFSM4OWYAVFA> .

[andrewsykim]

Alternative naming proposal for conditions:

• ready = ready && !terminating
• readiness = ready (identical to serving but more explicitly tied to pod readiness, we would have to document why we have both a Ready and Readiness condition)
• terminating = terminating

Kinda awkward to have a ready and readiness condition, but at least readiness is more explicit about a Pod's ready condition as opposed to serving. We would have to document really well why readiness exists and how it differs from ready. Thoughts @thockin @robscott?

[andrewsykim]

Going to update the condition name to readyTerminating based on my comment here #92968 (comment), open to other naming suggestions though.

[k8s-ci-robot]

@andrewsykim: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-kubernetes-kubemark-e2e-gce-big	14fe44aa88a2412435cb85c1f9d9dfed35a92490	link	/test pull-kubernetes-kubemark-e2e-gce-big

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

[andrewsykim]

/retest

[andrewsykim]

@thockin @robscott collapsed serving and terminating into a single conditoin readyTerminating, PTAL

[thockin]

I'll leave the EPSlice logic and final LGTM to @robscott but API-wise I am OK with this.

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewsykim, 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:

• api/OWNERS [thockin]
• pkg/apis/OWNERS [thockin]
• pkg/controller/endpointslice/OWNERS [andrewsykim,thockin]
• pkg/controller/util/endpoint/OWNERS [andrewsykim,thockin]
• pkg/features/OWNERS [andrewsykim,thockin]
• pkg/registry/OWNERS [thockin]
• staging/src/k8s.io/api/OWNERS [thockin]

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

[robscott]

Thanks!

/lgtm