Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proxier: only get local addresses once per sync loop #85617

Merged
merged 5 commits into from Feb 12, 2020

Conversation

@andrewsykim
Copy link
Member

andrewsykim commented Nov 25, 2019

Signed-off-by: Andrew Sy Kim kiman@vmware.com

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
Currently the iptables and userspace proxier lists all local network interfaces every time it checks if an external IP is local to the system. For most clusters this isn't problematic but for clusters using a lot of external IPs this can impact kube-proxy performance significantly. An optimization can be made where we get all local addresses only once per sync and store it in memory.

The IPVS proxy made this optimization in #84924, this PR makes the same optimization for the iptables proxy. More details in #84906

Which issue(s) this PR fixes:
Fixes #84906

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

iptables/userspace proxy: improve performance by getting local addresses only once per sync loop, instead of for every external IP

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


@andrewsykim

This comment has been minimized.

Copy link
Member Author

andrewsykim commented Nov 25, 2019

/sig network
/assign @danwinship

Copy link
Contributor

danwinship left a comment

Maybe fix the userspace proxy code too and then you can get rid of utilproxy.IsLocalIP?

klog.Errorf("can't determine if IP is local, assuming not: %v", err)
} else if local && (svcInfo.Protocol() != v1.ProtocolSCTP) {
if len(localAddrs) == 0 {
klog.Errorf("couldn't find any local IPs, assuming external IP %s is not local", externalIP)

This comment has been minimized.

Copy link
@danwinship

danwinship Nov 27, 2019

Contributor

You already logged an error about GetLocalAddrs failing above. Do we need to log an error for each external IP too? (Maybe just add "assuming all external IPs are not local" to the earlier message.)
(Likewise in ipvs.)

This comment has been minimized.

Copy link
@andrewsykim

andrewsykim Nov 27, 2019

Author Member

Good catch, thanks

@andrewsykim

This comment has been minimized.

Copy link
Member Author

andrewsykim commented Nov 27, 2019

I was hesitate to do this for userspace proxy because it wasn't scoped to only external IPs, will update it though

@andrewsykim andrewsykim force-pushed the andrewsykim:optimize-external-ips branch 2 times, most recently from da4fa72 to 363299f Nov 27, 2019
@@ -903,10 +908,7 @@ func (proxier *Proxier) closePortal(service proxy.ServicePortName, info *Service

func (proxier *Proxier) closeOnePortal(portal portal, protocol v1.Protocol, proxyIP net.IP, proxyPort int, name proxy.ServicePortName) []error {
el := []error{}

if local, err := utilproxy.IsLocalIP(portal.ip.String()); err != nil {
el = append(el, fmt.Errorf("can't determine if IP %s is local, assuming not: %v", portal.ip, err))

This comment has been minimized.

Copy link
@andrewsykim

andrewsykim Nov 27, 2019

Author Member

This error is no longer being added to el, I think this is fine but not familiar enough with userspace proxy to be sure.

@andrewsykim andrewsykim changed the title iptables proxier: only get local addresses once per sync loop proxier: only get local addresses once per sync loop Nov 27, 2019
@andrewsykim

This comment has been minimized.

Copy link
Member Author

andrewsykim commented Nov 27, 2019

@danwinship comments addressed, PTAL :)

@andrewsykim andrewsykim force-pushed the andrewsykim:optimize-external-ips branch from 363299f to be59693 Nov 27, 2019
@andrewsykim

This comment has been minimized.

Copy link
Member Author

andrewsykim commented Dec 17, 2019

/assign @thockin

Copy link
Member

thockin left a comment

relatively minor points.

pkg/proxy/iptables/proxier.go Show resolved Hide resolved
// on the local system
func IsLocalIP(ip string) (bool, error) {
// GetLocalAddrs returns a list of all network addresses on the local system
func GetLocalAddrs() ([]net.IP, error) {

This comment has been minimized.

Copy link
@thockin

thockin Dec 17, 2019

Member

Why not return a "set" type here. k8s.io/utils/net/ipnet.go defines IPNetSet. We could have the equivalent IPSet type

This comment has been minimized.

Copy link
@andrewsykim

andrewsykim Jan 6, 2020

Author Member

Makes sense to me, I'll open a PR to add IPSet type to k8s.io/utils/net.

This comment has been minimized.

Copy link
@andrewsykim

andrewsykim Jan 7, 2020

Author Member

On second thought, I'm realizing that a set type for net.IP may not work because we can't do comparison checks for net.IP based on their string values since an IPv6 representation of IPv4 addresses are considered equal (see net.IP Equals). We could just ignore this case and assume equality/uniqueness soley based on string values but that would be a behavioral change from what we have right now.

This comment has been minimized.

Copy link
@danwinship

danwinship Jan 8, 2020

Contributor

But String has the same logic. Any two net.IPs that compare as Equal will also String-ify the same. (Specifically, net.ParseIP("::ffff:1.2.3.4").String() → "1.2.3.4")

So as long as you know the IPs are in canonical form, you can compare based on string values.

This comment has been minimized.

Copy link
@andrewsykim

andrewsykim Jan 8, 2020

Author Member

Ah good to know! I'll update k8s.io/utils/net to add a set type for net.IP.

}

// ContainsIP returns true if ip exists in addrs, false otherwise
func ContainsIP(ip net.IP, ips []net.IP) bool {

This comment has been minimized.

Copy link
@thockin

thockin Dec 17, 2019

Member

This is why I think a set type makes sense.

Also, the "receiver" arg should go first if we don't have a type.

@andrewsykim andrewsykim force-pushed the andrewsykim:optimize-external-ips branch from be59693 to 2f5793b Jan 6, 2020
@k8s-ci-robot k8s-ci-robot added size/L and removed size/M labels Jan 6, 2020
@andrewsykim andrewsykim force-pushed the andrewsykim:optimize-external-ips branch from 2f5793b to be072c6 Jan 7, 2020
@andrewsykim andrewsykim force-pushed the andrewsykim:optimize-external-ips branch from be072c6 to 8e32665 Jan 18, 2020
@andrewsykim

This comment has been minimized.

Copy link
Member Author

andrewsykim commented Jan 21, 2020

Blocked on #87408

@andrewsykim andrewsykim force-pushed the andrewsykim:optimize-external-ips branch from 2f36ca0 to 9962181 Feb 7, 2020
andrewsykim added 4 commits Nov 27, 2019
This avoids fetching all local network interfaces everytime we sync an
external IP. For clusters with many external IPs this gets really
expensive. This change caches all local addresses once per sync.

Signed-off-by: Andrew Sy Kim <kiman@vmware.com>
Signed-off-by: Andrew Sy Kim <kiman@vmware.com>
This avoids fetching all local network interfaces everytime we sync an
external IP. For clusters with many external IPs this gets really
expensive. This change caches all local addresses once per sync.

Signed-off-by: Andrew Sy Kim <kiman@vmware.com>
Signed-off-by: andrewsykim <kim.andrewsy@gmail.com>
@andrewsykim andrewsykim force-pushed the andrewsykim:optimize-external-ips branch from 9962181 to 33f503a Feb 11, 2020
This allows the proxier to cache local addresses instead of fetching all
local addresses every time in IsLocalIP.

Signed-off-by: Andrew Sy Kim <kiman@vmware.com>
@andrewsykim andrewsykim force-pushed the andrewsykim:optimize-external-ips branch from 33f503a to 1653476 Feb 11, 2020
@andrewsykim

This comment has been minimized.

Copy link
Member Author

andrewsykim commented Feb 11, 2020

Ready for another review

@andrewsykim

This comment has been minimized.

Copy link
Member Author

andrewsykim commented Feb 11, 2020

/priority important-soon

Copy link
Member

thockin left a comment

Thanks!

/lgtm
/approve

if local, err := utilproxy.IsLocalIP(externalIP); err != nil {
klog.Errorf("can't determine if IP is local, assuming not: %v", err)
} else if local && (svcInfo.Protocol() != v1.ProtocolSCTP) {
if localAddrSet.Len() > 0 && (svcInfo.Protocol() != v1.ProtocolSCTP) && localAddrSet.Has(net.ParseIP(externalIP)) {

This comment has been minimized.

Copy link
@thockin

thockin Feb 11, 2020

Member

nit: Do we really need to check .Len() == 0 ?

This comment has been minimized.

Copy link
@andrewsykim

andrewsykim Feb 12, 2020

Author Member

No this check was here before the set type was added so I think it's redundant now. I'll remove this in a follow-up PR

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 11, 2020
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Feb 11, 2020

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Feb 12, 2020

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

2 similar comments
@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Feb 12, 2020

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Feb 12, 2020

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit ed0d6ee into kubernetes:master Feb 12, 2020
15 of 16 checks passed
15 of 16 checks passed
tide Not mergeable. Retesting: pull-kubernetes-kubemark-e2e-gce-big
Details
cla/linuxfoundation andrewsykim authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-kind Job succeeded.
Details
pull-kubernetes-e2e-kind-ipv6 Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
@k8s-ci-robot k8s-ci-robot added this to the v1.18 milestone Feb 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.