kube-proxy/iptables: optimize endpoint map creation by excluding invalid endpoints earlier #42747
Conversation
We don't need the svcPortToInfoMap. Its only purpose was to send "valid" local endpoints (those with valid IP and >0 port) to the health checker. But we shouldn't be sending invalid endpoints to the health checker anyway, because it can't do anything with them. If we exclude invalid endpoints earlier, then we don't need flattenValidEndpoints(). And if we don't need flattenValidEndpoints() it makes no sense to have svcPortToInfoMap store hostPortInfo, since endpointsInfo is the same thing as hostPortInfo except with a combined host:port. And if svcPortToInfoMap now only stores valid endpointsInfos, it is exactly the same thing as newEndpoints.
k8s-ci-robot
added
the
cncf-cla: yes
|
This change is |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED The following people have approved this PR: dcbw Needs approval from an approver in each of these OWNERS Files: We suggest the following people: |
k8s-github-robot
added
size/L
|
@freehan can you test with your setup as well? I don't have the capacity to do that, so I was testing with 250 services, each with 30 endpoints. pperf from this PR: http://people.redhat.com/dcbw/new-steady.svg |
|
@kubernetes/rh-networking @kubernetes/sig-network-misc |
|
Looks like net issues with gcr.io? @k8s-bot cvm gce e2e test this |
|
@bprashanth @matchstick how does this look? this is a blocker for 1.6 |
|
/cc @thockin |
|
Nice cleanup - @dcbw is it really a blocker? What's the measured impact? |
thockin
added
approved
|
NM, I see this is the perf regression. Awesome. |
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
@dcbw: The following test(s) failed:
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
|
Automatic merge from submit-queue (batch tested with PRs 42747, 43030) |
…ts-log-message Automatic merge from submit-queue Restore "Setting endpoints" log message **What this PR does / why we need it**: The "Setting endpoints" message from kube-proxy at high verbosity was lost as part of a larger simplification in #42747. This change brings it back, simply outputting the just-constructed addresses list. I need this message to monitor delays in propagating endpoints changes across nodes. **Release note**: ```release-note NONE ```
We don't need to do as much work as we were doing, if we exclude invalid endpoints earlier in the endpoints processing.
Fixes: #42210
@freehan @liggitt @thockin if you could review this with a fine-toothed comb... I can't immediately think of why invalid endpoints would be useful for the HealthChecker, and this PR prevents the HC from seeing these endpoints.