Adding documentation for Topology Aware Hints #27032

robscott · 2021-03-13T00:37:34Z

This adds documentation for the new Topology Aware Hints feature and also notes that Service Topology is deprecated.

ServiceTopology Enhancement Issue: Topology aware routing of services enhancements#536
TopologyAwareHints Enhancement Issue: Topology Aware Routing enhancements#2433

/cc @andrewsykim @sftim
/sig network

k8sio-netlify-preview-bot · 2021-03-13T00:37:38Z

Deploy preview for kubernetes-io-vnext-staging processing.

Building with commit b9ec368

https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/6064f7b4d270fc0007f201ff

reylejano · 2021-03-13T01:02:36Z

/assign @PI-Victor

content/en/docs/concepts/services-networking/topology-aware-hints.md

thockin · 2021-03-16T17:38:44Z

content/en/docs/concepts/services-networking/topology-aware-hints.md

+
+## Constraints
+
+* Topology Aware Hints are not compatible with `internalTrafficPolicy=Local`,


Is it invalid to spec eTP: Cluster and iTP: Local ? I don't think that's disallowed? In that case, topology hints modulate the meaning of "Cluster", no?

You're completely right. There's a bug in the implementation that makes these exclusive, but that should not be the case.

Follow up: kubernetes/kubernetes#100313

if my understanding is correct, before the fix for kubernetes/kubernetes#100313, it's not compatible with externalTrafficPolicy=Local as well?

Updated, thanks!

thockin · 2021-03-16T17:40:26Z

content/en/docs/concepts/services-networking/topology-aware-hints.md

+  use both features in the same cluster on different Services, just not on the
+  same Service.
+
+* This approach will not work well for Services that have a large proportion of


The APPROACH might, but the implementation will need to adapt to handle it, right?

For an alpha feature, it's OK to document current shortcomings without making a commitment about future improvements to address them (we don't want to make promises unless we're sure of delivering on them).

This is worth further discussion, but the only ways I can think to adapt to this would be new metrics that could enable feedback-based balancing or extra configuration.

What I meant is that the implementation could evolve to consider feedback and the hints API would still be appropriate

sftim

Hi @robscott

Thanks for this PR.
The feature looks really useful, I can see myself wanting that next week actually.

This will need some changes before we can merge it I'm afraid. Any feedback I've marked as a nit is optional; the other stuff pretty much does need fixing though.

content/en/docs/concepts/services-networking/topology-aware-hints.md

sftim · 2021-03-16T18:50:29Z

content/en/docs/concepts/services-networking/topology-aware-hints.md

+  use both features in the same cluster on different Services, just not on the
+  same Service.
+
+* This approach will not work well for Services that have a large proportion of


For an alpha feature, it's OK to document current shortcomings without making a commitment about future improvements to address them (we don't want to make promises unless we're sure of delivering on them).

sftim · 2021-03-21T17:21:49Z

content/en/docs/concepts/services-networking/service-topology.md

+Service Topology is deprecated as of Kubernetes v1.21, and will be removed in
+v1.22. Topology Aware Hints was introduced in Kubernetes v1.21 and provides
+similar functionality.


Do we also need to update https://kubernetes.io/docs/reference/using-api/deprecation-guide/ ?

That's a good question. I think that specific doc is focused on deprecations of API versions, not fields. I know we'll have a deprecation notice in the 1.21 release notes, but maybe we need something more visible in the docs?

/cc @liggitt

Yeah, that page is currently focused on entirely deprecated API versions that require migration to another API version. Since v1beta1 EndpointSlices will stop being served in 1.25, and the v1 replacement is ready, they should be marked as deprecated, and added to that doc, along with a mention of the topology field not being writeable in v1.

PI-Victor · 2021-03-22T16:02:03Z

Hi @robscott , thank you for having your Doc PR ready for review, friendly reminder about the upcoming doc related dates for the 1.21 release:
Upcoming doc related dates for the 1.21 release:

~~Docs Ready for Review deadline is March 24~~
Docs Ready to Merge deadline is March 31

sftim · 2021-03-26T18:52:59Z

@kubernetes/sig-network-pr-reviews how's this looking?

preview of the main new content

reylejano · 2021-03-29T06:34:42Z

Hi @andrewsykim @bowei @dcbw @thockin @wojtek-t , since this documentation PR is related to SIG Network KEP 2433 Topology Aware Hints , as reviewers or approvers of the KEP please provide a technical review for this PR by March 31 to get this into the release. Thank you!

sftim · 2021-03-30T17:05:09Z

As this is documenting an alpha feature, it's OK to have a few documentation rough edges.

We do want:

factual accuracy, no obvious incorrect statements etc
a clear indication that the feature is alpha (DONE!)
enough detail that someone can decide whether or not that feature is relevant; if they find it's on in their cluster, should they worry, etc.
accurate feature gate & related details

I think that's the minimum. Obviously going beyond that is better and welcome.

We will consider PRs against dev-1.21 even after the initial docs merge, and because this is website is continuously delivered, we'll also consider the ~~same~~ equivalent commits post-release if you rebase against the primary branch.

TL;DR: So we don't need this PR to be completely finished feature docs, so long as the minimum is done well.

sftim · 2021-03-30T17:06:21Z

BTW @robscott I do think this documentation is already good and well beyond the minimum, thanks for being so thorough!

robscott · 2021-03-30T17:36:38Z

I think this PR is just waiting for technical signoff at this point. @thockin or @andrewsykim can you add a technical LGTM to this?

content/en/docs/concepts/services-networking/service-topology.md

andrewsykim · 2021-03-31T14:37:35Z

content/en/docs/concepts/services-networking/topology-aware-hints.md

+   real-time feedback. It is still possible for individual endpoints to become
+   overloaded.
+
+3. **One or more Nodes has insufficient information:** If any node does not have


Is this true even when a single node may be missing topology labels for a short period of time?

In other words, during that short period of time, would kube-proxy flap between topolgoy-aware hints and all endpoints?

Yes, that is what is is for now.

Yep, the current approach is very quick to bail out. Would like to try to find some kind of acceptable threshold for missing data, but that's been hard to quantify so far.

andrewsykim · 2021-03-31T14:45:48Z

content/en/docs/concepts/services-networking/topology-aware-hints.md

+## Using Topology Aware Hints
+
+You can enable Topology Aware Hints for a Service by setting the
+`service.kubernetes.io/topology-aware-hints` annotation to `auto`. This tells


Sorry if this was covered in the PR review, I was unable to review it due to other priorities :(

Do we "drop annotations" like we do field when the feature gate is off, or is the annotation just ignored in this case?

Yep, the annotation is a way for users to opt into this feature on a Service, so dropping that opt-in would not be good here.

content/en/docs/concepts/services-networking/service-topology.md

reylejano · 2021-03-31T14:52:08Z

@robscott please review and address andrewsykim's comments. I also made a few suggestions.
Thank you!

thockin

Overall good, some smaller comments

content/en/docs/concepts/services-networking/service-topology.md

content/en/docs/concepts/services-networking/topology-aware-hints.md

thockin · 2021-03-31T19:44:05Z

content/en/docs/concepts/services-networking/topology-aware-hints.md

+## Using Topology Aware Hints
+
+You can enable Topology Aware Hints for a Service by setting the
+`service.kubernetes.io/topology-aware-hints` annotation to `auto`. This tells


thockin · 2021-03-31T19:44:28Z

content/en/docs/concepts/services-networking/topology-aware-hints.md

+## Using Topology Aware Hints
+
+You can enable Topology Aware Hints for a Service by setting the
+`service.kubernetes.io/topology-aware-hints` annotation to `auto`. This tells


"Auto" or "auto"? It should be "Auto" probably

Unfortunately it is "auto", should I try to fix that in upstream? https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/endpointslice/utils.go#L395

Probably, and now you get to handle both!

Covered by kubernetes/kubernetes#100728

content/en/docs/concepts/services-networking/topology-aware-hints.md

thockin · 2021-03-31T19:48:14Z

content/en/docs/concepts/services-networking/topology-aware-hints.md

+   real-time feedback. It is still possible for individual endpoints to become
+   overloaded.
+
+3. **One or more Nodes has insufficient information:** If any node does not have


Yes, that is what is is for now.

content/en/docs/tasks/administer-cluster/enabling-service-topology.md

thockin · 2021-03-31T19:52:22Z

/assign

robscott · 2021-03-31T21:00:14Z

Thanks for the reviews @andrewsykim @thockin and @reylejano! This is ready for another round of review when you have time.

content/en/docs/concepts/services-networking/topology-aware-hints.md

thockin

/lgtm

k8s-ci-robot · 2021-03-31T22:31:46Z

LGTM label has been added.

Git tree hash: 82c662891624c9cc186ea3775704212ecfd8028c

reylejano · 2021-03-31T23:48:14Z

/approve

k8s-ci-robot · 2021-03-31T23:48:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: reylejano

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:

~~content/en/OWNERS~~ [reylejano]

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 added this to the 1.21 milestone Mar 13, 2021

k8s-ci-robot requested review from andrewsykim and sftim March 13, 2021 00:37

k8s-ci-robot added sig/network Categorizes an issue or PR as relevant to SIG Network. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 13, 2021

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. language/en Issues or PRs related to English language sig/docs Categorizes an issue or PR as relevant to SIG Docs. labels Mar 13, 2021

k8s-ci-robot assigned PI-Victor Mar 13, 2021

laverya reviewed Mar 15, 2021

View reviewed changes

content/en/docs/concepts/services-networking/topology-aware-hints.md Outdated Show resolved Hide resolved

robscott force-pushed the topology-aware-hints branch 2 times, most recently from 87e4cef to e158206 Compare March 15, 2021 21:38

thockin reviewed Mar 16, 2021

View reviewed changes

robscott mentioned this pull request Mar 16, 2021

Traffic Policy and Topology Aware Routing should not be exclusive kubernetes/kubernetes#100313

Closed

sftim reviewed Mar 16, 2021

View reviewed changes

robscott force-pushed the topology-aware-hints branch 2 times, most recently from 3e49aff to f319635 Compare March 20, 2021 00:42

sftim reviewed Mar 21, 2021

View reviewed changes

k8s-ci-robot requested a review from liggitt March 22, 2021 21:49

liggitt mentioned this pull request Mar 23, 2021

Mark v1beta1 EndpointSlice deprecated in favor of v1 kubernetes/kubernetes#100472

Merged

andrewsykim reviewed Mar 31, 2021

View reviewed changes

reylejano reviewed Mar 31, 2021

View reviewed changes

content/en/docs/concepts/services-networking/service-topology.md Outdated Show resolved Hide resolved

reylejano mentioned this pull request Mar 31, 2021

Add documentation for Service InternalTrafficPolicy #27088

Merged

thockin reviewed Mar 31, 2021

View reviewed changes

k8s-ci-robot assigned thockin Mar 31, 2021

robscott force-pushed the topology-aware-hints branch 2 times, most recently from 1f2f840 to 851610b Compare March 31, 2021 20:59

bowei reviewed Mar 31, 2021

View reviewed changes

content/en/docs/concepts/services-networking/topology-aware-hints.md Outdated Show resolved Hide resolved

Adding documentation for Topology Aware Hints

b9ec368

robscott force-pushed the topology-aware-hints branch from 851610b to b9ec368 Compare March 31, 2021 22:29

thockin reviewed Mar 31, 2021

View reviewed changes

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

robscott mentioned this pull request Mar 31, 2021

Updating Topology Aware Hints to support "Auto" value for annotation kubernetes/kubernetes#100728

Merged

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 31, 2021

k8s-ci-robot merged commit bfa3449 into kubernetes:dev-1.21 Mar 31, 2021

sftim mentioned this pull request Apr 3, 2021

Tidy docs about EndpointSlice & related features #27390

Merged


		## Constraints

		* Topology Aware Hints are not compatible with `internalTrafficPolicy=Local`,

Adding documentation for Topology Aware Hints #27032

Adding documentation for Topology Aware Hints #27032

Conversation

robscott commented Mar 13, 2021

k8sio-netlify-preview-bot commented Mar 13, 2021 • edited Loading

reylejano commented Mar 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sftim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PI-Victor commented Mar 22, 2021

sftim commented Mar 26, 2021

reylejano commented Mar 29, 2021

sftim commented Mar 30, 2021

sftim commented Mar 30, 2021

robscott commented Mar 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reylejano commented Mar 31, 2021

thockin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin commented Mar 31, 2021

robscott commented Mar 31, 2021

thockin left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 31, 2021

reylejano commented Mar 31, 2021

k8s-ci-robot commented Mar 31, 2021

k8sio-netlify-preview-bot commented Mar 13, 2021 •

edited

Loading

reylejano commented Mar 13, 2021 •

edited

Loading