Updating Topology Aware Subsetting KEP for 1.21 #2355

robscott · 2021-01-28T03:23:12Z

This includes adding a PRR, adding a new syncs metric, and a variety of additional cleanup.

See also: #2354
Enhancement Issue: #2004

/sig network
/cc @andrewsykim @bowei @freehan
/assign @thockin @wojtek-t

thockin

What happens if someone enables subsetting but we have a very old kube-proxy? It may see duplicate endpoints - are we confident that old kube-proxy de-dups endpoints (from slices)? Do we have tests backing that up?

thockin · 2021-01-28T17:40:50Z

keps/sig-network/2004-topology-aware-subsetting/README.md

 ### Configuration

-This proposal builds off of the [EndpointSlice API](https://kubernetes.io/docs/concepts/services-networking/endpoint-slices/).
+A new `endpointslice.kubernetes.io/subsetting` annotation will be supported on


You say later the controller config, but can we say something here like "...the default behavior will be decided by the system." ?

Long term, this annotation becomes the escape hatch for people who need to disable it, and not something that most users specify. In fact, let's call that out as the long-term goal? "Most users should not need to specify this at all, and will get topology-aware balancing by default".

Should we not have a controller flag at all here? I was thinking that having a controller flag would provide a way to experiment with the default value here in advance of a k8s transition to "Auto" by default.

Good question. How nervous should we be about the default becoming "auto"? Thinking about the best-case timelines:

Suppose we do not add any controller flag:

Q1 1.21 = alpha + opt-in by annotation

Q2 1.22 = beta + opt-in by annotation

Q3 1.23 = GA + opt-out by annotation

The riskiest change is 22->23, I think. In fact it seems so risky that we may want to drag out beta + opt-in for several releases, pushing out GA. Q4 is blech, so really GA would be Q1'22 earliest.

Suppose we decouple the controller default (via flag or just another gate):

Q1 1.21 = alpha + opt-in by annotation

Q2 1.22 = beta + opt-in by annotation

Q3 1.23 = GA + opt-in by annotation

Q1 1.25 = change default flag value to "auto"

Now we get to GA the feature without flipping the default.

As I wrote this I realized that maybe your question is whether we need a flag or just a different feature gate? I think we can start without such a flag - we can always add flags, but dropping them is hard

Good call, updated KEP to say any change to default behavior will be covered by a future KEP.

keps/sig-network/2004-topology-aware-subsetting/README.md

This includes adding a PRR, adding a new syncs metric, and a variety of additional cleanup.

thockin

/lgtm

thockin · 2021-01-29T16:50:36Z

keps/sig-network/2004-topology-aware-subsetting/README.md

+      `endpointslice.kubernetes.io/subsetting` annotation on Service.
+    - Will enabling / disabling the feature require downtime of the control
+      plane?
+      No


For PRR reviewers - does this include "needs a restart to flip the gate" ?

I think what we're saying here is:

you can enable/disable feature by feature-gate switch (above - which requires restart)

you can enable/disable the feature for your individual single Service (on service by service basis) - and that doesn't require restart

[Assuming that was the intention, can you please make it explicit that it's on Service by Service basis?]

thockin · 2021-01-29T16:55:47Z

/approve

I am not PRR reviewer, but KEP body LGTM

k8s-ci-robot · 2021-01-29T16:56:01Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: robscott, thockin
To complete the pull request process, please assign wojtek-t after the PR has been reviewed.
You can assign the PR to them by writing /assign @wojtek-t in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

keps/prod-readiness/OWNERS
~~keps/sig-network/OWNERS~~ [thockin]

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

wojtek-t · 2021-01-29T17:01:36Z

Will do the PRR review on Monday.

wojtek-t · 2021-02-01T08:43:31Z

keps/sig-network/2004-topology-aware-subsetting/README.md

 #### Feature Gate

 This functionality will be guarded by the `TopologyAwareSubsetting` feature
 gate. This gate should not be enabled if the `ServiceTopology` gate is enabled.


I think I don't understand this.

Doesn't this mean that we have no way to graduate both features to Beta (as we enable Beta by default)?

wojtek-t · 2021-02-01T09:13:50Z

keps/sig-network/2004-topology-aware-subsetting/README.md

   - `endpoints_require` pool is empty.
 4. If `endpoints_available` pool is empty and `endpoints_require` pool is not
-   empty, downgrade to `Random` approach.
+   empty, downgrade to `Disabled` approach.


[Comment to this whole subsection - not to this line].

How we're planning to reduce the churn on EndpointSlices?
Please correct me if I'm wrong, by my understanding is that this algorithm will be run each time when we will be computing slicing for a given service.

So let's say I have all my endpoints in zone A (say 30 endpoints). How you're going to achieve the situation that e.g. when adding 1 endpoint only one EndpointSlice will get updated, not potentially all 3 of them?

[This algorithm doesn't seem to take inco acount a current assignment...]

wojtek-t · 2021-02-01T09:21:57Z

keps/sig-network/2004-topology-aware-subsetting/README.md

+      `endpointslice.kubernetes.io/subsetting` annotation on Service.
+    - Will enabling / disabling the feature require downtime of the control
+      plane?
+      No


I think what we're saying here is:

you can enable/disable feature by feature-gate switch (above - which requires restart)

you can enable/disable the feature for your individual single Service (on service by service basis) - and that doesn't require restart

[Assuming that was the intention, can you please make it explicit that it's on Service by Service basis?]

wojtek-t · 2021-02-01T09:25:05Z

keps/sig-network/2004-topology-aware-subsetting/README.md

+      No
+
+* **Does enabling the feature change any default behavior?**
+  In most cases, this feature will be intentionally enabled on specific Services


Given this sentence from the above:
"When this annotation is either unspecified or not set to Auto, the Disabled behavior will be default."

The answer for this question is just "no" :)

This is a nice explanation, but I think there is a huge benefit of keeping the answers as short as possible - we don't necessary want to explain all the details here.
So let's simply change to something like:

"No - it requires to explicitly opting in for the feature by setting the annotation".

wojtek-t · 2021-02-01T09:30:40Z

keps/sig-network/2004-topology-aware-subsetting/README.md

+
+* **Will enabling / using this feature result in any new API calls?**
+  This will result in more EndpointSlices which would result in more create and
+  update calls.


Can we be more specific?
E.g. can we provide an upper bound on increase? (e.g. say that there will be no more than #zones times more than current operations (if that is even true)). Or something like that...

robscott · 2021-02-05T05:26:08Z

Thanks for all the great feedback on this PR! These discussions have encouraged me to try a slightly different approach with #2434. I will revisit this PR after the KEP freeze to get them into a deferred state and capture most of the #2434. Closing this PR temporarily to avoid any confusion.

/close

k8s-ci-robot · 2021-02-05T05:26:21Z

@robscott: Closed this PR.

In response to this:

Thanks for all the great feedback on this PR! These discussions have encouraged me to try a slightly different approach with #2434. I will revisit this PR after the KEP freeze to get them into a deferred state and capture most of the #2434. Closing this PR temporarily to avoid any confusion.

/close

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.

k8s-ci-robot assigned thockin and wojtek-t Jan 28, 2021

k8s-ci-robot added the sig/network Categorizes an issue or PR as relevant to SIG Network. label Jan 28, 2021

k8s-ci-robot requested review from andrewsykim, bowei and freehan January 28, 2021 03:23

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 28, 2021

robscott force-pushed the topology-aware-subsetting-1-21 branch from cc351b7 to 28cd98d Compare January 28, 2021 03:25

robscott mentioned this pull request Jan 28, 2021

Updating Topology Aware Proxying KEP for 1.21 release cycle #2354

Closed

robscott force-pushed the topology-aware-subsetting-1-21 branch from 28cd98d to f9c1a31 Compare January 28, 2021 03:27

robscott mentioned this pull request Jan 28, 2021

Topology Aware Subsetting #2004

Closed

thockin reviewed Jan 28, 2021

View reviewed changes

Updating Topology Aware Subsetting KEP for 1.21

2fdd5ac

This includes adding a PRR, adding a new syncs metric, and a variety of additional cleanup.

robscott force-pushed the topology-aware-subsetting-1-21 branch from f9c1a31 to 2fdd5ac Compare January 29, 2021 06:31

thockin reviewed Jan 29, 2021

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 29, 2021

wojtek-t reviewed Feb 1, 2021

View reviewed changes

robscott mentioned this pull request Feb 5, 2021

Adding KEP 2433: Topology Aware Hints #2434

Merged

k8s-ci-robot closed this Feb 5, 2021

Updating Topology Aware Subsetting KEP for 1.21 #2355

Updating Topology Aware Subsetting KEP for 1.21 #2355

Uh oh!

Conversation

robscott commented Jan 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thockin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thockin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thockin commented Jan 29, 2021

Uh oh!

k8s-ci-robot commented Jan 29, 2021

Uh oh!

wojtek-t commented Jan 29, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robscott commented Feb 5, 2021

Uh oh!

k8s-ci-robot commented Feb 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

robscott commented Jan 28, 2021 •

edited

Loading