KEP-2086: alpha prod readiness review #2441

andrewsykim · 2021-02-06T18:27:13Z

The PRR for KEP-2086 is already filled out for alpha but it needs to be approved

Signed-off-by: Andrew Sy Kim kim.andrewsy@gmail.com

andrewsykim · 2021-02-07T00:47:53Z

andrewsykim · 2021-02-07T00:48:33Z

keps/prod-readiness/sig-network/2086.yaml

@@ -0,0 +1,3 @@
+kep-number: 2086
+alpha:


PRR is already filled out in the KEP https://github.com/kubernetes/enhancements/blob/699fd3769fd3cd1f6846020675a330cdcee5f5af/keps/sig-network/2086-service-internal-traffic-policy/README.md#production-readiness-review-questionnaire but it needs a review

wojtek-t

/hold

wojtek-t · 2021-02-08T10:34:15Z

keps/prod-readiness/sig-network/2086.yaml

@@ -0,0 +1,3 @@
+kep-number: 2086
+alpha:
+  approver: "@johnbelamaric"


I actually claim we should rethink this proposal with the new approach that we're proceed with decribed in:
#2434

The arguments described in the proposal (EndpointSlice per node) will no longer hold with that (we can easily imagine "forNode" hint too).
So instead of rushing with this one, I would really like to take a step back and ensure that we can't converge this with #2434

@thockin @robscott @aojea @johnbelamaric

the slices have already a hostname field, IIUIC this is a just a matter of signalling kube-proxy to leverage it, it can be an annotation or an api field, no?

// hostname of this endpoint. This field may be used by consumers of // endpoints to distinguish endpoints from each other (e.g. in DNS names). // Multiple endpoints which use the same hostname should be considered // fungible (e.g. multiple A values in DNS). Must pass DNS Label (RFC 1123) // validation. // +optional Hostname *string

So, the problem is that the API field can be contradictory with the new annotations topology.kubernetes.io/topology-aware-routing, right?

Maybe we should add a new value to the annotation to implement local traffic ...

Good point that we don't need any additional hints for that.

Regarding API - given it's explicitly stated there as "local-traffic will require careful handing" (or sth like that), I think we should think if it doesn't make sense to unify (using single annotation is what i was thinking, but I think we should rely on API approvers (@thockin - back to you) to recommend if that makes sense).

Regarding API - given it's explicitly stated there as "local-traffic will require careful handing" (or sth like that), I think we should think if it doesn't make sense to unify (using single annotation is what i was thinking, but I think we should rely on API approvers (@thockin - back to you) to recommend if that makes sense).

There was a lot of back and forth on this and the conclusion IIRC is that "route to node-local" is a distinct and common enough use-case to warrant a different API from topology-aware routing. More on this email thread: https://groups.google.com/g/kubernetes-sig-network/c/wXd1D_fKjqU. But that was also under the assumption that we were going to use EndpointSlice subsetting and a EndpointSlice per-node didn't make sense.

So, the problem is that the API field can be contradictory with the new annotations topology.kubernetes.io/topology-aware-routing, right?

There is an overlap if you consider Local or PreferLocal. The assumption so far has been that this annotation would only be used with Cluster (now All). My personal preference would actually be to have TopologyAware be another possible value of internalTrafficPolicy as opposed to an annotation but I'll leave that feedback in the other KEP PR.

cc @maplain who is working on the initial implementation for internalTrafficPolicy kubernetes/kubernetes#96600

I think it makes sense to try to combine these. There is a lot of overlap. I prefer PreferZone over Topology as it leaves room for expansion in the future if we add other topology algorithms.

In regards to "Auto" meaning "decide for me"- IIUC that means that based on the endpoints available, choosing between Cluster, Local, PreferLocal, and PreferZone. Would that require an additional field or annotation to state what was chosen? PreferZone is the only one that would be detectable because of the the topologyHints. How would kube-proxy differentiate between the others?

I think the easiest thing to understand here might be adding PreferZone

The semantics of PreferNode are "if there is an endpoint on-node, use it. Only
if there is no endpoint should you overflow to another node". I don't think we
want the same for zone (or at least, that's not the heuristic you have
developed and almost certainly not the best for "auto").

potentially RequireZone as options for InternalTrafficPolicy

If and when we have use-cases.

EndpointSlice hints are only generated when internalTrafficPolicy == PreferZone

We could always generate hints, but why bother unless we are in a mode where
they matter? We might want external traffic to have topology, too?

Limiting this to internalTrafficPolicy removes a potential link to LB config.

Actually - is that a reason why topology and local-only might not be the same?
What happens if I set internalTrafficPolicy: Topology and I take external
traffic? Does that not follow topology? Or does topological subsetting apply
to all "whole cluster" policies?

In the context of external traffic, "Local" has a semantic implication (no
SNAT) as well as a perfomance/cost implication. We have to respect that. But
the default is "Cluster" and it seems to me that topology should apply in those
cases (cost/perf win but no semantic change).

This point is weighing on me - thoughts?

In regards to "Auto" meaning "decide for me"- IIUC that means that based on the endpoints available, choosing between Cluster, Local, PreferLocal, and PreferZone. Would that require an additional field or annotation to state what was chosen? PreferZone is the only one that would be detectable because of the the topologyHints. How would kube-proxy differentiate between the others?

I'm not sure it's the right approach, but "Auto" could look at the endpoints
and decide "I have at least one endpoint on every node, so I will set a hint to
use the same node" vs "I don't have enough for node, but I can bound by zone" or
even some mixed mode.

maybe we should all jump on a call? I don't think we need to block the KEP(s), though

Updates from Slack thread:

rename internalTrafficPolicy to trafficPolicy to reflect increased scope

if externalTrafficPolicy=Cluster, fall back to trafficPolicy for external sources

if externalTrafficPolicy=Local -- this rule only takes precedent for external sources, with internal traffic following trafficPolicy

wojtek-t · 2021-02-08T19:42:06Z

@thockin - feel free to hold cancel when you're fine with it; I just wanted to flag the overlap and it seems I succeeded. I will not have time to go back to it by feature freeze, so I'm leaving the resolution to you.

andrewsykim · 2021-02-08T19:50:17Z

@thockin - feel free to hold cancel when you're fine with it; I just wanted to flag the overlap and it seems I succeeded. I will not have time to go back to it by feature freeze, so I'm leaving the resolution to you.

Thanks @wojtek-t, I will also update PRR reviewer to you. I think you still need to approve the PR for changes in keps/prod-readiness?

johnbelamaric · 2021-02-09T00:41:52Z

I'll let @wojtek-t take this one...

Signed-off-by: Andrew Sy Kim <kim.andrewsy@gmail.com>

robscott

Thanks for the work on this! Just a few nits but nothing that should block this PR from getting in.

robscott · 2021-02-09T03:49:04Z

keps/sig-network/2086-service-internal-traffic-policy/README.md

-	InternalTrafficPolicy ServiceInternalTrafficPolicyType `json:"internalTrafficPolicy,omitempty"`
+	// trafficPolicy denotes if the traffic for a Service should route
+	// to cluster-wide endpoints or node-local endpoints. "Cluster" routes traffic
+	// to a Service to all cluster-wide endpoints. "Topology" routes traffic based on


Nit: I'd personally prefer "PreferZone" over "Topology" here but no reason to block this PR.

robscott · 2021-02-09T03:51:06Z

keps/sig-network/2086-service-internal-traffic-policy/kep.yaml

@@ -34,7 +34,7 @@ milestone:
 # The following PRR answers are required at alpha release
 # List the feature gate name and the components for which it must be enabled
 feature-gates:
-  - name: ServiceInternalTrafficPolicy
+  - name: ServiceITrafficPolicy


Suggested change

- name: ServiceITrafficPolicy

- name: ServiceTrafficPolicy

robscott · 2021-02-09T03:51:46Z

keps/sig-network/2086-service-internal-traffic-policy/kep.yaml

@@ -34,7 +34,7 @@ milestone:
 # The following PRR answers are required at alpha release
 # List the feature gate name and the components for which it must be enabled
 feature-gates:
-  - name: ServiceInternalTrafficPolicy
+  - name: ServiceITrafficPolicy
    components:
      - kube-apiserver


Nit: This will also need to be read from kube-controller-manager now that topology is dependent on this feature gate.

thockin

/lgtm

thockin · 2021-02-09T05:06:27Z

keps/sig-network/2086-service-internal-traffic-policy/README.md

 1. Cluster (default): route to all cluster-wide endpoints (or use topology aware subsetting if enabled).
+2. Topology: route to endpoints using topology-aware routing. See Topology Aware Hints KEP for more details.


We can argue names in the PR I need to think about it, but I don't think I like this layout. :) Minor

thockin · 2021-02-09T05:08:39Z

keps/sig-network/2086-service-internal-traffic-policy/README.md

-* when `internalTrafficPolicy=PreferLocal`, route to endpoints in EndpointSlice that matches the local node's topology (topology defined by `kubernetes.io/hostname`),
+* when `trafficPolicy=Cluster`, default to existing behavior today.
+* when `trafficPolicy=Topology`, use topology hints from EndpointSlice API.
+* when `trafficPolicy=PreferLocal`, route to endpoints in EndpointSlice that matches the local node's topology (topology defined by `kubernetes.io/hostname`),
 fall back to "Cluster" behavior if there are no local endpoints.


We'll have to figure out if the fallback is Cluster or Topology or if we need 2 modes. OK for here, the PR will need to explore that.

thockin · 2021-02-09T05:10:01Z

/approve

maplain · 2021-02-09T05:22:31Z

keps/sig-network/2086-service-internal-traffic-policy/README.md


 ## Proposal

-Introduce a new field in Service `spec.internalTrafficPolicy`. The field will have 3 codified values:
+Introduce a new field in Service `spec.trafficPolicy`. The field will have 4 codified values:
 1. Cluster (default): route to all cluster-wide endpoints (or use topology aware subsetting if enabled).


should #2166 be rebased or we could take the chance to update Cluster to All
cc @howardjohn

wojtek-t · 2021-02-09T07:14:49Z

/approve PRR

@andrewsykim - please apply the remaining minor comments in the follow up PR

/hold cancel

k8s-ci-robot · 2021-02-09T07:15:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewsykim, thockin, wojtek-t

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:

~~keps/prod-readiness/OWNERS~~ [wojtek-t]
~~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

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 sig/network Categorizes an issue or PR as relevant to SIG Network. labels Feb 6, 2021

k8s-ci-robot requested review from caseydavenport and dcbw February 6, 2021 18:27

k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Feb 6, 2021

andrewsykim changed the title ~~KEP-2086: prod readiness review~~ KEP-2086: alpha prod readiness review Feb 7, 2021

k8s-ci-robot assigned johnbelamaric and thockin Feb 7, 2021

andrewsykim commented Feb 7, 2021

View reviewed changes

wojtek-t reviewed Feb 8, 2021

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 8, 2021

robscott mentioned this pull request Feb 8, 2021

Adding KEP 2433: Topology Aware Hints #2434

Merged

maplain mentioned this pull request Feb 8, 2021

Service Internal Traffic Policy #2086

Closed

KEP-2086: prod readiness review

4b7f9d7

Signed-off-by: Andrew Sy Kim <kim.andrewsy@gmail.com>

andrewsykim force-pushed the prr-2086 branch from 699fd37 to f576397 Compare February 9, 2021 01:48

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 9, 2021

KEP-2086: update to include Topology traffic policy

c0a9f9c

Signed-off-by: Andrew Sy Kim <kim.andrewsy@gmail.com>

andrewsykim force-pushed the prr-2086 branch from f576397 to c0a9f9c Compare February 9, 2021 01:57

robscott reviewed Feb 9, 2021

View reviewed changes

thockin reviewed Feb 9, 2021

View reviewed changes

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

maplain reviewed Feb 9, 2021

View reviewed changes

wojtek-t self-assigned this Feb 9, 2021

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 9, 2021

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

k8s-ci-robot merged commit 01ffe07 into kubernetes:master Feb 9, 2021

k8s-ci-robot added this to the v1.21 milestone Feb 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KEP-2086: alpha prod readiness review #2441

KEP-2086: alpha prod readiness review #2441

andrewsykim commented Feb 6, 2021

andrewsykim commented Feb 7, 2021

andrewsykim Feb 7, 2021

wojtek-t left a comment

wojtek-t Feb 8, 2021

aojea Feb 8, 2021

aojea Feb 8, 2021

wojtek-t Feb 8, 2021

andrewsykim Feb 8, 2021

andrewsykim Feb 8, 2021

swetharepakula Feb 8, 2021 •

edited

thockin Feb 8, 2021 •

edited

thockin Feb 8, 2021

robscott Feb 9, 2021

wojtek-t commented Feb 8, 2021

andrewsykim commented Feb 8, 2021

johnbelamaric commented Feb 9, 2021

robscott left a comment •

edited

robscott Feb 9, 2021

robscott Feb 9, 2021

robscott Feb 9, 2021

thockin left a comment

thockin Feb 9, 2021

thockin Feb 9, 2021

thockin commented Feb 9, 2021

maplain Feb 9, 2021

wojtek-t commented Feb 9, 2021

k8s-ci-robot commented Feb 9, 2021

		1. Cluster (default): route to all cluster-wide endpoints (or use topology aware subsetting if enabled).
		2. Topology: route to endpoints using topology-aware routing. See Topology Aware Hints KEP for more details.

KEP-2086: alpha prod readiness review #2441

KEP-2086: alpha prod readiness review #2441

Conversation

andrewsykim commented Feb 6, 2021

andrewsykim commented Feb 7, 2021

Choose a reason for hiding this comment

wojtek-t 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

swetharepakula Feb 8, 2021 • edited

Choose a reason for hiding this comment

thockin Feb 8, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wojtek-t commented Feb 8, 2021

andrewsykim commented Feb 8, 2021

johnbelamaric commented Feb 9, 2021

robscott left a comment • edited

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin commented Feb 9, 2021

Choose a reason for hiding this comment

wojtek-t commented Feb 9, 2021

k8s-ci-robot commented Feb 9, 2021

swetharepakula Feb 8, 2021 •

edited

thockin Feb 8, 2021 •

edited

robscott left a comment •

edited