Introducing Topology Mode Annotation, Deprecating Topology Hints Annotation

[robscott]

What type of PR is this?

/kind feature

What this PR does / why we need it:

This introduces the service.kubernetes.io/topology-mode annotation as an eventual replacement for the service.kubernetes.io/topology-aware-hints annotation. The service.kubernetes.io/topology-aware-hints annotation has been deprecated, but will continue to be supported for at least some period of time.

This introduces the idea of implementation-specific values for the annotation - the idea that someone can bring their own implementation of topology. Although I expect most of these implementations to populate EndpointSlice hints, that is not a requirement.

To enable this concept, kube-proxy now accepts any value for the annotation that is not "disabled" or empty.

Does this PR introduce a user-facing change?

* New `service.kubernetes.io/topology-mode` annotation has been introduced as a replacement for the `service.kubernetes.io/topology-aware-hints` annotation.
* `service.kubernetes.io/topology-aware-hints` annotation has been deprecated.
* kube-proxy now accepts any value that is not "disabled" for these annotations, enabling custom implementation-specific and/or future built-in heuristics to be used. 

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

• KEP: Topology Aware Routing enhancements#2433

/sig network
/cc @aojea @bowei @danwinship @shaneutt
/assign @thockin

[k8s-ci-robot]

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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-triage-robot]

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

[danwinship]

EndpointSlice hints are a typed field so implementation-specific topology modes would not be able to use them, right? Does that put implementation-specific topology modes at a disadvantage relative to official ones?

For that matter, why is this an annotation but the EndpointSlice Hints are a field?

[robscott]

EndpointSlice hints are a typed field so implementation-specific topology modes would not be able to use them, right?

I think this is all working towards a place where people that want to experiment with new hints heuristics can bring their own fork of EPS controller. That would be made easier by kubernetes/enhancements#3686 and could be another reason to invest in a KEP for something like #87488. The conclusion I've personally come to is that it's going to be very hard to find a heuristic that works for everyone. Providing a way for the community to develop others seems like it may be our best path forward. In the future, if there are clearly effective and widely used community heuristics, we may want to merge them into k/k.

For that matter, why is this an annotation but the EndpointSlice Hints are a field?

Probably the most concrete reason is that it's not possible to attach annotations to a specific endpoint in the list. We're also inching closer to a place where the dataplane doesn't need to care why a hint is present, just that it is. (Prior to this PR the annotation needed to be set to "Auto", now it just can't be set to "Disabled").

Open to other ideas here, but at a high level I think it's important that we provide a way for community heuristics to be developed and experimented with because I don't think we can make the correct routing decisions for everyone with a single algorithm and/or the limited time we have to invest in this feature.

[robscott]

cc @s1061123 and @akhilles since this relates to ideas you've worked on

[thockin]

Does that put implementation-specific topology modes at a disadvantage relative to official ones?

I think that, if there was a need and it was reasonable, we could consider additional hints. Unfortunately, our API and our implementation are entangled and we don't have an awesome way to extend existing resources.

In a perfect world, hints would be an extension of the base type, but there's just no way to express that. We could put all hints in a parallel type, but now we have wonky races where the slice is updated but the hints are not, and we 2x the API ops. We could render hints as a per-slice annotation. That's probably the "fairest" approach, but ick.

I expect that other implementations won't use EPSlice anyway, so it's moot

why is this an annotation but the EndpointSlice Hints are a field

This is fair - topologyMode could be a field. As much churn as this has undergone, that would require resetting to alpha. I think I'd just as soon run with annotation and if this pattern sticks, we can eat the tech-debt of converting to a field.

Agree or disagree?

[danwinship]

I think I'd just as soon run with annotation and if this pattern sticks, we can eat the tech-debt of converting to a field.

Agree or disagree?

agree

[s1061123]

@robscott thank you for the mention to me. My interest is delegate endpointSlice management for specific service, by other controller, that is not to create any endpointslices by EPS controller in Kubernetes controller manager. In this case, this topology-mode annotation only controls the topology, so I believe that endpointSlice is created. So it is slightly related but not so related to me.

[robscott]

/retest

[shaneutt]

CI problems from earlier should be set now: kubernetes/test-infra#29003

/test all

[shaneutt]

Looks like we're trying to put something in place sooner (rather than later) to provide some clarifications about the fact that hints have been found to be insufficient and that (for now) we want to provide an open-ended annotation which over the coming months new implementations can coalesce upon. I agree with signaling that we found hints were not by themselves sufficient and I also think that adding just a simple common annotation for this for now (and we can revisit later) is a decent start (annotation technical debt aside).

[thockin]

Does that put implementation-specific topology modes at a disadvantage relative to official ones?

I think that, if there was a need and it was reasonable, we could consider additional hints. Unfortunately, our API and our implementation are entangled and we don't have an awesome way to extend existing resources.

In a perfect world, hints would be an extension of the base type, but there's just no way to express that. We could put all hints in a parallel type, but now we have wonky races where the slice is updated but the hints are not, and we 2x the API ops. We could render hints as a per-slice annotation. That's probably the "fairest" approach, but ick.

I expect that other implementations won't use EPSlice anyway, so it's moot

why is this an annotation but the EndpointSlice Hints are a field

This is fair - topologyMode could be a field. As much churn as this has undergone, that would require resetting to alpha. I think I'd just as soon run with annotation and if this pattern sticks, we can eat the tech-debt of converting to a field.

Agree or disagree?

[thockin]

We don't usually allow variant case - should we?

[robscott]

Although we only document "Auto" and "Disabled" at this point, the first implementation unfortunately used "auto" and "disabled" so we were stuck with either inconsistency or forever supporting both.

[thockin]

We should define the syntax clearly. If it were to become a field what would it be?

I guess it would either be like a label ("foo" vs "example.com/foo") or like a condition ("Foo vs example.com/foo")?

Let's just be clear in the definition. We can accept "auto" and "disabled", but "Auto" and "Disabled" are the canonical names, right?

[robscott]

Let's just be clear in the definition. We can accept "auto" and "disabled", but "Auto" and "Disabled" are the canonical names, right?

Yes the documentation on both annotations only specifies "Auto" and "Disabled.

We should define the syntax clearly. If it were to become a field what would it be?
I guess it would either be like a label ("foo" vs "example.com/foo") or like a condition ("Foo vs example.com/foo")?

If we're talking about the allowed values of the field/annotation, I think "like a condition" is the answer here so "Auto", "Disabled", and "example.com/foo" are all valid.

[thockin]

We should define the syntax clearly. If it were a field what would it be? I guess it would either be like a label (foo vs example.com/foo) or like a condition (Foo vs example.com/foo)? Let's be clear.

…

On Mon, Mar 13, 2023, 5:43 PM Rob Scott ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pkg/proxy/topology.go <#116522 (comment)> : > hintsAnnotation := svcInfo.HintsAnnotation() - if hintsAnnotation != "Auto" && hintsAnnotation != "auto" { - if hintsAnnotation != "" && hintsAnnotation != "Disabled" && hintsAnnotation != "disabled" { - klog.InfoS("Skipping topology aware endpoint filtering since Service has unexpected value", "annotationTopologyAwareHints", v1.AnnotationTopologyAwareHints, "hints", hintsAnnotation) - } + if hintsAnnotation == "" || hintsAnnotation == "disabled" || hintsAnnotation == "Disabled" { Although we only document "Auto" and "Disabled" at this point, the first implementation unfortunately used "auto" and "disabled" so we were stuck with either inconsistency or forever supporting both. — Reply to this email directly, view it on GitHub <#116522 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABKWAVDWWECJ5UD7ERR5ARTW365LBANCNFSM6AAAAAAVYUXI4U> . You are receiving this because you were assigned.Message ID: ***@***.***>

[robscott]

We should define the syntax clearly. If it were a field what would it be? I guess it would either be like a label (foo vs example.com/foo) or like a condition (Foo vs example.com/foo)?

Not sure what this is in response to. Unfortunately I think we're stuck with both, although we could choose to not carry over lowercase support to the new annotation, that may result in unexpected migration problems. So I think my perspective is that we should document it as a condition (Foo and example.com/foo are valid values), but I'm not sure we can/should prevent use of "auto" and "disabled" for backwards compat.

[thockin]

Sorry, email responds to threaded comments in conversation, will attach to the variant-case thread.

[thockin]

Should we leave a TODO or something to consider converting this to a field?

[thockin]

Do we want to issue an API warning if both are set and different values? Not super critical, but nice.

[robscott]

That's a great idea, is that something we should just prevent entirely in validation? I'm not familiar with how/where to add API warnings, any tips for the component that should live in if not API validation?

[thockin]

Thanks!

/lgtm
/approve
/hold

unhold if you're happy with definition of constants or want to do anything else as followup.