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.

Sign up for GitHub

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

Jump to bottom

optimize proxier duplicate localaddrset #98083

Merged
merged 1 commit into from Jan 29, 2021
Merged

optimize proxier duplicate localaddrset #98083

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 7 additions & 16 deletions pkg/proxy/iptables/proxier.go
View file
Open in desktop
Expand Up @@ -828,21 +828,6 @@ func (proxier *Proxier) syncProxyRules() {
klog.V(2).InfoS("syncProxyRules complete", "elapsed", time.Since(start))
}()

localAddrs, err := utilproxy.GetLocalAddrs()
if err != nil {
klog.ErrorS(err, "Failed to get local addresses during proxy sync, assuming external IPs are not local")
} else if len(localAddrs) == 0 {
klog.InfoS("No local addresses found, assuming all external IPs are not local")
}

localAddrSet := utilnet.IPSet{}
localAddrSet.Insert(localAddrs...)

nodeAddresses, err := utilproxy.GetNodeAddresses(proxier.nodePortAddresses, proxier.networkInterfacer)
if err != nil {
klog.ErrorS(err, "Failed to get node ip address matching nodeport cidrs, services with nodeport may not work as intended", "CIDRs", proxier.nodePortAddresses)
}

// We assume that if this was called, we really want to sync them,
// even if nothing changed in the meantime. In other words, callers are
// responsible for detecting no-op changes and not calling this function.
Expand Down Expand Up @@ -903,7 +888,7 @@ func (proxier *Proxier) syncProxyRules() {
// This will be a map of chain name to chain with rules as stored in iptables-save/iptables-restore
existingFilterChains := make(map[utiliptables.Chain][]byte)
proxier.existingFilterChainsData.Reset()
err = proxier.iptables.SaveInto(utiliptables.TableFilter, proxier.existingFilterChainsData)
err := proxier.iptables.SaveInto(utiliptables.TableFilter, proxier.existingFilterChainsData)
if err != nil { // if we failed to get any rules
klog.ErrorS(err, "Failed to execute iptables-save, syncing all rules")
} else { // otherwise parse the output
Expand Down Expand Up @@ -1007,6 +992,12 @@ func (proxier *Proxier) syncProxyRules() {
proxier.endpointChainsNumber += len(proxier.endpointsMap[svcName])
}

localAddrSet := utilproxy.GetLocalAddrSet()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you put these right next to each other, they seem to overlap a bit. We should clean this up. Later.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which place you think it is overlap? :)

nodeAddresses, err := utilproxy.GetNodeAddresses(proxier.nodePortAddresses, proxier.networkInterfacer)
if err != nil {
klog.ErrorS(err, "Failed to get node ip address matching nodeport cidrs, services with nodeport may not work as intended", "CIDRs", proxier.nodePortAddresses)
}

// Build rules for each service.
for svcName, svc := range proxier.serviceMap {
svcInfo, ok := svc.(*serviceInfo)
Expand Down
14 changes: 3 additions & 11 deletions pkg/proxy/ipvs/proxier.go
View file
Open in desktop
Expand Up @@ -1037,16 +1037,6 @@ func (proxier *Proxier) syncProxyRules() {
klog.V(4).Infof("syncProxyRules took %v", time.Since(start))
}()

localAddrs, err := utilproxy.GetLocalAddrs()
if err != nil {
klog.Errorf("Failed to get local addresses during proxy sync: %v, assuming external IPs are not local", err)
} else if len(localAddrs) == 0 {
klog.Warning("No local addresses found, assuming all external IPs are not local")
}

localAddrSet := utilnet.IPSet{}
localAddrSet.Insert(localAddrs...)

// We assume that if this was called, we really want to sync them,
// even if nothing changed in the meantime. In other words, callers are
// responsible for detecting no-op changes and not calling this function.
Expand Down Expand Up @@ -1083,7 +1073,7 @@ func (proxier *Proxier) syncProxyRules() {
proxier.createAndLinkKubeChain()

// make sure dummy interface exists in the system where ipvs Proxier will bind service address on it
_, err = proxier.netlinkHandle.EnsureDummyDevice(DefaultDummyDevice)
_, err := proxier.netlinkHandle.EnsureDummyDevice(DefaultDummyDevice)
if err != nil {
klog.Errorf("Failed to create dummy interface: %s, error: %v", DefaultDummyDevice, err)
return
Expand Down Expand Up @@ -1159,6 +1149,8 @@ func (proxier *Proxier) syncProxyRules() {
// reset slice to filtered entries
nodeIPs = nodeIPs[:idx]

localAddrSet := utilproxy.GetLocalAddrSet()

// Build IPVS rules for each service.
for svcName, svc := range proxier.serviceMap {
svcInfo, ok := svc.(*serviceInfo)
Expand Down
15 changes: 3 additions & 12 deletions pkg/proxy/userspace/proxier.go
View file
Open in desktop
Expand Up @@ -395,16 +395,7 @@ func (proxier *Proxier) syncProxyRules() {
proxier.unmergeService(change.previous, existingPorts)
}

localAddrs, err := utilproxy.GetLocalAddrs()
if err != nil {
klog.Errorf("Failed to get local addresses during proxy sync: %s, assuming IPs are not local", err)
} else if len(localAddrs) == 0 {
klog.Warning("No local addresses were found, assuming all external IPs are not local")
}

localAddrSet := netutils.IPSet{}
localAddrSet.Insert(localAddrs...)
proxier.localAddrs = localAddrSet
proxier.localAddrs = utilproxy.GetLocalAddrSet()

proxier.ensurePortals()
proxier.cleanupStaleStickySessions()
Expand Down Expand Up @@ -769,7 +760,7 @@ func (proxier *Proxier) openPortal(service proxy.ServicePortName, info *ServiceI
}

func (proxier *Proxier) openOnePortal(portal portal, protocol v1.Protocol, proxyIP net.IP, proxyPort int, name proxy.ServicePortName) error {
if proxier.localAddrs.Len() > 0 && proxier.localAddrs.Has(portal.ip) {
if proxier.localAddrs.Has(portal.ip) {
err := proxier.claimNodePort(portal.ip, portal.port, protocol, name)
if err != nil {
return err
Expand Down Expand Up @@ -945,7 +936,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 proxier.localAddrs.Len() > 0 && proxier.localAddrs.Has(portal.ip) {
if proxier.localAddrs.Has(portal.ip) {
if err := proxier.releaseNodePort(portal.ip, portal.port, protocol, name); err != nil {
el = append(el, err)
}
Expand Down
15 changes: 15 additions & 0 deletions pkg/proxy/util/utils.go
View file
Open in desktop
Expand Up @@ -156,6 +156,21 @@ func GetLocalAddrs() ([]net.IP, error) {
return localAddrs, nil
}

// GetLocalAddrSet return a local IPSet.
// If failed to get local addr, will assume no local ips.
func GetLocalAddrSet() utilnet.IPSet {
localAddrs, err := GetLocalAddrs()
if err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm torn. Part of me feels like this should be logged from the caller, which reduces this function to just a few LOC. I guess it doesn't matter much, its an internal API..

klog.ErrorS(err, "Failed to get local addresses assuming no local IPs", err)
} else if len(localAddrs) == 0 {
klog.InfoS("No local addresses were found")
}

localAddrSet := utilnet.IPSet{}
localAddrSet.Insert(localAddrs...)
return localAddrSet
}

// ShouldSkipService checks if a given service should skip proxying
func ShouldSkipService(service *v1.Service) bool {
// if ClusterIP is "None" or empty, skip proxying
Expand Down