-
Notifications
You must be signed in to change notification settings - Fork 463
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
Improve the ipAddrAdd performance #965
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -81,7 +81,7 @@ type ipvsCalls interface { | |
} | ||
|
||
type netlinkCalls interface { | ||
ipAddrAdd(iface netlink.Link, ip string, addRoute bool) error | ||
ipAddrAdd(iface netlink.Link, ip string) error | ||
ipAddrDel(iface netlink.Link, ip string) error | ||
prepareEndpointForDsr(containerID string, endpointIP string, vip string) error | ||
getKubeDummyInterface() (netlink.Link, error) | ||
|
@@ -122,31 +122,14 @@ func (ln *linuxNetworking) ipAddrDel(iface netlink.Link, ip string) error { | |
// utility method to assign an IP to an interface. Mainly used to assign service VIP's | ||
// to kube-dummy-if. Also when DSR is used, used to assign VIP to dummy interface | ||
// inside the container. | ||
func (ln *linuxNetworking) ipAddrAdd(iface netlink.Link, ip string, addRoute bool) error { | ||
func (ln *linuxNetworking) ipAddrAdd(iface netlink.Link, ip string) error { | ||
naddr := &netlink.Addr{IPNet: &net.IPNet{IP: net.ParseIP(ip), Mask: net.IPv4Mask(255, 255, 255, 255)}, Scope: syscall.RT_SCOPE_LINK} | ||
err := netlink.AddrAdd(iface, naddr) | ||
if err != nil && err.Error() != IfaceHasAddr { | ||
glog.Errorf("Failed to assign cluster ip %s to dummy interface: %s", | ||
naddr.IPNet.IP.String(), err.Error()) | ||
return err | ||
} | ||
|
||
// When a service VIP is assigned to a dummy interface and accessed from host, in some of the | ||
// case Linux source IP selection logix selects VIP itself as source leading to problems | ||
// to avoid this an explicit entry is added to use node IP as source IP when accessing | ||
// VIP from the host. Please see https://github.com/cloudnativelabs/kube-router/issues/376 | ||
|
||
if !addRoute { | ||
return nil | ||
} | ||
|
||
// TODO: netlink.RouteReplace which is replacement for below command is not working as expected. Call succeeds but | ||
// route is not replaced. For now do it with command. | ||
iamakulov marked this conversation as resolved.
Show resolved
Hide resolved
|
||
out, err := exec.Command("ip", "route", "replace", "local", ip, "dev", KubeDummyIf, "table", "local", "proto", "kernel", "scope", "host", "src", | ||
NodeIP.String(), "table", "local").CombinedOutput() | ||
if err != nil { | ||
glog.Errorf("Failed to replace route to service VIP %s configured on %s. Error: %v, Output: %s", ip, KubeDummyIf, err, out) | ||
} | ||
return nil | ||
} | ||
|
||
|
@@ -201,23 +184,24 @@ func newLinuxNetworking() (*linuxNetworking, error) { | |
|
||
// NetworkServicesController struct stores information needed by the controller | ||
type NetworkServicesController struct { | ||
nodeIP net.IP | ||
nodeHostName string | ||
syncPeriod time.Duration | ||
mu sync.Mutex | ||
serviceMap serviceInfoMap | ||
endpointsMap endpointsInfoMap | ||
podCidr string | ||
excludedCidrs []net.IPNet | ||
masqueradeAll bool | ||
globalHairpin bool | ||
ipvsPermitAll bool | ||
client kubernetes.Interface | ||
nodeportBindOnAllIP bool | ||
MetricsEnabled bool | ||
ln LinuxNetworking | ||
readyForUpdates bool | ||
ProxyFirewallSetup *sync.Cond | ||
nodeIP net.IP | ||
nodeHostName string | ||
serviceClusterIPRange string | ||
syncPeriod time.Duration | ||
mu sync.Mutex | ||
serviceMap serviceInfoMap | ||
endpointsMap endpointsInfoMap | ||
podCidr string | ||
excludedCidrs []net.IPNet | ||
masqueradeAll bool | ||
globalHairpin bool | ||
ipvsPermitAll bool | ||
client kubernetes.Interface | ||
nodeportBindOnAllIP bool | ||
MetricsEnabled bool | ||
ln LinuxNetworking | ||
readyForUpdates bool | ||
ProxyFirewallSetup *sync.Cond | ||
|
||
// Map of ipsets that we use. | ||
ipsetMap map[string]*utils.Set | ||
|
@@ -347,6 +331,11 @@ func (nsc *NetworkServicesController) Run(healthChan chan<- *healthcheck.Control | |
} | ||
nsc.ProxyFirewallSetup.Broadcast() | ||
|
||
err = nsc.enforceRightServiceInterface() | ||
if err != nil { | ||
glog.Error(err.Error()) | ||
} | ||
|
||
gracefulTicker := time.NewTicker(5 * time.Second) | ||
defer gracefulTicker.Stop() | ||
|
||
|
@@ -1006,7 +995,7 @@ func (ln *linuxNetworking) prepareEndpointForDsr(containerID string, endpointIP | |
} | ||
|
||
// assign VIP to the KUBE_TUNNEL_IF interface | ||
err = ln.ipAddrAdd(tunIf, vip, false) | ||
err = ln.ipAddrAdd(tunIf, vip) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @iamakulov You missed another one of these changes on line 1229: https://github.com/cloudnativelabs/kube-router/blob/master/pkg/controllers/proxy/network_services_controller.go#L1229 |
||
if err != nil && err.Error() != IfaceHasAddr { | ||
err = netns.Set(hostNetworkNamespaceHandle) | ||
if err != nil { | ||
|
@@ -2205,6 +2194,22 @@ func (nsc *NetworkServicesController) handleServiceDelete(obj interface{}) { | |
nsc.OnServiceUpdate(service) | ||
} | ||
|
||
// When a service VIP is assigned to a dummy interface and accessed from host, in some of the | ||
// case Linux source IP selection logix selects VIP itself as source leading to problems | ||
// to avoid this an explicit entry is added to use node IP as source IP when accessing | ||
// VIP from the host. Please see https://github.com/cloudnativelabs/kube-router/issues/376 | ||
func (nsc *NetworkServicesController) enforceRightServiceInterface() error { | ||
// TODO: netlink.RouteReplace which is replacement for below command is not working as expected. Call succeeds but | ||
// route is not replaced. (See https://github.com/cloudnativelabs/kube-router/issues/370#issuecomment-379415278.) | ||
// For now, do it with command. | ||
out, err := exec.Command("ip", "route", "replace", "local", nsc.serviceClusterIPRange, "dev", KubeDummyIf, "table", "local", "proto", "kernel", "scope", "host", "src", | ||
NodeIP.String(), "table", "local").CombinedOutput() | ||
if err != nil { | ||
return fmt.Errorf("Failed to enforce the right interface for service traffic. Error: %v, Output: %s", err, out) | ||
} | ||
return nil | ||
} | ||
|
||
// NewNetworkServicesController returns NetworkServicesController object | ||
func NewNetworkServicesController(clientset kubernetes.Interface, | ||
config *options.KubeRouterConfig, svcInformer cache.SharedIndexInformer, | ||
|
@@ -2241,6 +2246,8 @@ func NewNetworkServicesController(clientset kubernetes.Interface, | |
nsc.gracefulTermination = config.IpvsGracefulTermination | ||
nsc.globalHairpin = config.GlobalHairpinMode | ||
|
||
nsc.serviceClusterIPRange = config.ClusterIPCIDR | ||
|
||
nsc.serviceMap = make(serviceInfoMap) | ||
nsc.endpointsMap = make(endpointsInfoMap) | ||
nsc.client = clientset | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iamakulov I found an additional problem with this logic. Down below, you only account for the ClusterIP range in the
enforceRightServiceInterface()
function. However, we also needs routes inserted for ExternalIP and LoadBalancer VIPs if we remove this section here.ExternalIPs and LoadBalancer VIPs are less likely to be in a cohesive subnet, and I don't think that it would be good to use the same process for those (even though there is a parameter for them) as it would likely exclude certain valid use-cases for users.
I think that we'll likely have to go back to the drawing board here.