Fix endpoint selection for externalTrafficPolicy=local #4170
Conversation
ricky-rav
requested review from
trozet,
dcbw,
girishmg,
jcaamano and
tssurya
as code owners
|
|
ricky-rav
force-pushed
the
kep-1669-bis_upstream
branch
2 times, most recently
from
85b0db1
to
6dbc864
Compare
|
I've added some unit tests to |
ricky-rav
force-pushed
the
kep-1669-bis_upstream
branch
from
6dbc864
to
55f7a85
Compare
ricky-rav
force-pushed
the
kep-1669-bis_upstream
branch
6 times, most recently
from
de5f1a5
to
0c4ab97
Compare
ricky-rav
force-pushed
the
kep-1669-bis_upstream
branch
3 times, most recently
from
dde092e
to
96dc373
Compare
|
/retest |
|
Oops, something went wrong: |
ricky-rav
force-pushed
the
kep-1669-bis_upstream
branch
2 times, most recently
from
d395fdf
to
2793a33
Compare
tssurya
added
kind/bug
ricky-rav
force-pushed
the
kep-1669-bis_upstream
branch
2 times, most recently
from
e04df18
to
dc52cb0
Compare
|
I've pushed a new |
ricky-rav
force-pushed
the
kep-1669-bis_upstream
branch
from
dc52cb0
to
e12ac77
Compare
There was a problem hiding this comment.
Good idea! I made
getNodeSwitchTargetIPshandle v4 and v6 at the same time. Does it look ok?
Also, do you prefer a free method because we're not really modifyingc *lbConfig?
I preferred the free method because it didn't make sense to pass as arguments data that is already available through lbConfig. I thought about keeping the receiver and removing the extra arguments but something that I don't remember know didn't make sense as well. So I suggested the free method.
There was a problem hiding this comment.
@jcaamano as discussed, I implemented the new logic in one function,
getEndpointsForService, that gets called once for a service and whose output is then parsed by the rest of the services controller to retrieve cluster-wide and per-node endpoint addresses. PTAL :)
Looks good, added some comments to polish. CI looks bad though, hopefuly something else.
I broke something... let me try to fix it :) |
ricky-rav
force-pushed
the
kep-1669-bis_upstream
branch
4 times, most recently
from
b2891e4
to
bfbfaaf
Compare
There was a problem hiding this comment.
Everything looks good @ricky-rav.
The smal nit, up to you to act on it, and just that failing lane.
Also, woudl you be interested in running this dowsntream to check on the disruption results before merging here?
| } | ||
| klog.V(5).Infof("Cluster endpoints for %s/%s are: %v", service.Namespace, service.Name, portToLBEndpoints) | ||
|
|
||
| if requiresLocalEndpoints { |
There was a problem hiding this comment.
nit: this is a superfluous check, no problem on iterating on an empty map
There was a problem hiding this comment.
right, I'll move the check to the log line further below... no need to print the local endpoints if we don't need them in the first place.
ricky-rav
force-pushed
the
kep-1669-bis_upstream
branch
2 times, most recently
from
61393fe
to
b2e01df
Compare
Yes! Let's see how it goes here: openshift/ovn-kubernetes#2109 |
| } | ||
|
|
||
| if requiresLocalEndpoints { | ||
| klog.V(5).Infof("Local endpoints for %s/%s are: %v", service.Namespace, service.Name, portToNodeToLBEndpoints) |
There was a problem hiding this comment.
nit: double spacing in this log and the one above
ricky-rav
force-pushed
the
kep-1669-bis_upstream
branch
from
b2e01df
to
ac13808
Compare
|
Our downstream CI fails on the
These tests are either not run in our upstream CI for IPv6 jobs or never run at all :( I'm trying to reproduce these failures on KIND and figure out how the tests get skipped... |
ricky-rav
force-pushed
the
kep-1669-bis_upstream
branch
2 times, most recently
from
cba7633
to
f423e57
Compare
|
Downstream CI is back, but I found a bug in |
|
With the second fix, I'll add the code here. |
Fix the case for "all endpoints terminating on a node when traffic policy is local": "When the traffic policy is "Local" and all endpoints are terminating within a single node, then traffic should be routed to any terminating endpoint that is ready on that node." https://github.com/kubernetes/enhancements/blob/master/keps/sig-network/1669-proxy-terminating-endpoints/README.md#example-all-endpoints-terminating-on-a-node-when-traffic-policy-is-local The endpoint selection logic in the services controller is now entirely implemented in getEndpointsForService, which computes for a given service and each service port all its cluster-wide endpoints and per-node local endpoints. We first apply a cluster-wide vs local endpoint selection and only then we apply readiness-based filtering with getEligibleEndpointAddresses. Added unit tests for the new logic. Signed-off-by: Riccardo Ravaioli <rravaiol@redhat.com>
ricky-rav
force-pushed
the
kep-1669-bis_upstream
branch
from
ebd1185
to
8eafec8
Compare
|
Failure in CI is on |
In #4072 the following edge case was not handled correctly in ovnkube-controller code.
Fix the case for
all endpoints terminating on a node when traffic policy is local:https://github.com/kubernetes/enhancements/blob/master/keps/sig-network/1669-proxy-terminating-endpoints/README.md#example-all-endpoints-terminating-on-a-node-when-traffic-policy-is-local
The endpoint selection logic in the services controller is now entirely implemented in
getEndpointsForService, which computes for a given service and each service port all its cluster-wide endpoints and per-node local endpoints. We first apply a cluster-wide vs local endpoint selection and only then we apply readiness-based filtering withgetEligibleEndpointAddresses.Added unit tests for the new logic.