From 35e0f44e552f5c997ca572ba5853e8bae8f4ca02 Mon Sep 17 00:00:00 2001 From: Dumitru Ceara Date: Wed, 29 Mar 2023 15:47:39 +0200 Subject: [PATCH] lb_config: Reduce number of required LB backend templates for NodePort. In general, template OVN load balancers used for implementing NodePort services might have completely different sets of backends on different nodes. In particular however, there are quite a few cases in which node port services end up using the same backends on all nodes, e.g., for services that don't have ETP/ITP=local and that select only OVN networked pods as backends. Detect such situations and avoid using templated backends. Use instead a shared list of backend addresses. This reduces NB DB size of the Chassis_Template_Var table, from O(N * S) to O(N), where N is the number of nodes and S is the number of node port services that match the criteria defined above. Signed-off-by: Dumitru Ceara --- .../pkg/ovn/controller/services/lb_config.go | 133 ++++++++++++++---- .../services/services_controller_test.go | 61 ++++---- go-controller/pkg/util/util.go | 7 +- go-controller/pkg/util/util_unit_test.go | 13 +- 4 files changed, 154 insertions(+), 60 deletions(-) diff --git a/go-controller/pkg/ovn/controller/services/lb_config.go b/go-controller/pkg/ovn/controller/services/lb_config.go index 050c614281b..6a7633919f7 100644 --- a/go-controller/pkg/ovn/controller/services/lb_config.go +++ b/go-controller/pkg/ovn/controller/services/lb_config.go @@ -39,8 +39,9 @@ type lbConfig struct { hasNodePort bool } -func (c *lbConfig) makeNodeSwitchTargetIPs(node *nodeInfo, epIPs []string) []string { - targetIPs := epIPs +func (c *lbConfig) makeNodeSwitchTargetIPs(node *nodeInfo, epIPs []string) (targetIPs []string, changed bool) { + targetIPs = epIPs + changed = false if c.externalTrafficLocal { // for ExternalTrafficPolicy=Local, remove non-local endpoints from the router/switch targets @@ -53,11 +54,17 @@ func (c *lbConfig) makeNodeSwitchTargetIPs(node *nodeInfo, epIPs []string) []str targetIPs = util.FilterIPsSlice(targetIPs, node.nodeSubnets(), true) } - return targetIPs + // We potentially only removed stuff from the original slice, so just + // comparing lenghts is enough. + if len(targetIPs) != len(epIPs) { + changed = true + } + return } -func (c *lbConfig) makeNodeRouterTargetIPs(node *nodeInfo, epIPs []string, hostMasqueradeIP string) []string { - targetIPs := epIPs +func (c *lbConfig) makeNodeRouterTargetIPs(node *nodeInfo, epIPs []string, hostMasqueradeIP string) (targetIPs []string, changed bool) { + targetIPs = epIPs + changed = false if c.externalTrafficLocal { // for ExternalTrafficPolicy=Local, remove non-local endpoints from the router/switch targets @@ -67,9 +74,13 @@ func (c *lbConfig) makeNodeRouterTargetIPs(node *nodeInfo, epIPs []string, hostM // any targets local to the node need to have a special // harpin IP added, but only for the router LB - targetIPs = util.UpdateIPsSlice(targetIPs, node.nodeIPsStr(), []string{hostMasqueradeIP}) + targetIPs, updated := util.UpdateIPsSlice(targetIPs, node.nodeIPsStr(), []string{hostMasqueradeIP}) - return targetIPs + // We either only removed stuff from the original slice, or updated some IPs. + if len(targetIPs) != len(epIPs) || updated { + changed = true + } + return } // just used for consistent ordering @@ -347,12 +358,33 @@ func buildTemplateLBs(service *v1.Service, configs []lbConfig, nodes []nodeInfo, for range config.vips { klog.V(5).Infof(" buildTemplateLBs() service %s/%s adding rules", service.Namespace, service.Name) + + // If all targets have exactly the same IPs on all nodes there's + // no need to use a template, just use the same list of explicit + // targets on all nodes. + switchV4TargetNeedsTemplate := false + switchV6TargetNeedsTemplate := false + routerV4TargetNeedsTemplate := false + routerV6TargetNeedsTemplate := false + for _, node := range nodes { - switchV4targetips := config.makeNodeSwitchTargetIPs(&node, config.eps.V4IPs) - switchV6targetips := config.makeNodeSwitchTargetIPs(&node, config.eps.V6IPs) + switchV4targetips, changed := config.makeNodeSwitchTargetIPs(&node, config.eps.V4IPs) + if !switchV4TargetNeedsTemplate && changed { + switchV4TargetNeedsTemplate = true + } + switchV6targetips, changed := config.makeNodeSwitchTargetIPs(&node, config.eps.V6IPs) + if !switchV6TargetNeedsTemplate && changed { + switchV6TargetNeedsTemplate = true + } - routerV4targetips := config.makeNodeRouterTargetIPs(&node, config.eps.V4IPs, types.V4HostMasqueradeIP) - routerV6targetips := config.makeNodeRouterTargetIPs(&node, config.eps.V6IPs, types.V6HostMasqueradeIP) + routerV4targetips, changed := config.makeNodeRouterTargetIPs(&node, config.eps.V4IPs, types.V4HostMasqueradeIP) + if !routerV4TargetNeedsTemplate && changed { + routerV4TargetNeedsTemplate = true + } + routerV6targetips, changed := config.makeNodeRouterTargetIPs(&node, config.eps.V6IPs, types.V6HostMasqueradeIP) + if !routerV6TargetNeedsTemplate && changed { + routerV6TargetNeedsTemplate = true + } switchV4TemplateTarget.Value[node.chassisID] = addrsToString( joinHostsPort(switchV4targetips, config.eps.Port)) @@ -365,23 +397,62 @@ func buildTemplateLBs(service *v1.Service, configs []lbConfig, nodes []nodeInfo, joinHostsPort(routerV6targetips, config.eps.Port)) } - switchV4Rules = append(switchV4Rules, LBRule{ - Source: Addr{Template: nodeIPv4Template, Port: config.inport}, - Targets: []Addr{{Template: switchV4TemplateTarget}}, - }) - switchV6Rules = append(switchV6Rules, LBRule{ - Source: Addr{Template: nodeIPv6Template, Port: config.inport}, - Targets: []Addr{{Template: switchV6TemplateTarget}}, - }) + sharedV4Targets := []Addr{} + sharedV6Targets := []Addr{} + if !switchV4TargetNeedsTemplate || !routerV4TargetNeedsTemplate { + sharedV4Targets = joinHostsPort(config.eps.V4IPs, config.eps.Port) + } + if !switchV6TargetNeedsTemplate || !routerV6TargetNeedsTemplate { + sharedV6Targets = joinHostsPort(config.eps.V6IPs, config.eps.Port) + } - routerV4Rules = append(routerV4Rules, LBRule{ - Source: Addr{Template: nodeIPv4Template, Port: config.inport}, - Targets: []Addr{{Template: routerV4TemplateTarget}}, - }) - routerV6Rules = append(routerV6Rules, LBRule{ - Source: Addr{Template: nodeIPv6Template, Port: config.inport}, - Targets: []Addr{{Template: routerV6TemplateTarget}}, - }) + if switchV4TargetNeedsTemplate { + switchV4Rules = append(switchV4Rules, LBRule{ + Source: Addr{Template: nodeIPv4Template, Port: config.inport}, + Targets: []Addr{{Template: switchV4TemplateTarget}}, + }) + } else { + switchV4Rules = append(switchV4Rules, LBRule{ + Source: Addr{Template: nodeIPv4Template, Port: config.inport}, + Targets: sharedV4Targets, + }) + } + + if switchV6TargetNeedsTemplate { + switchV6Rules = append(switchV6Rules, LBRule{ + Source: Addr{Template: nodeIPv6Template, Port: config.inport}, + Targets: []Addr{{Template: switchV6TemplateTarget}}, + }) + } else { + switchV6Rules = append(switchV6Rules, LBRule{ + Source: Addr{Template: nodeIPv6Template, Port: config.inport}, + Targets: sharedV6Targets, + }) + } + + if routerV4TargetNeedsTemplate { + routerV4Rules = append(routerV4Rules, LBRule{ + Source: Addr{Template: nodeIPv4Template, Port: config.inport}, + Targets: []Addr{{Template: routerV4TemplateTarget}}, + }) + } else { + routerV4Rules = append(routerV4Rules, LBRule{ + Source: Addr{Template: nodeIPv4Template, Port: config.inport}, + Targets: sharedV4Targets, + }) + } + + if routerV6TargetNeedsTemplate { + routerV6Rules = append(routerV6Rules, LBRule{ + Source: Addr{Template: nodeIPv6Template, Port: config.inport}, + Targets: []Addr{{Template: routerV6TemplateTarget}}, + }) + } else { + routerV6Rules = append(routerV6Rules, LBRule{ + Source: Addr{Template: nodeIPv6Template, Port: config.inport}, + Targets: sharedV6Targets, + }) + } } } @@ -485,11 +556,11 @@ func buildPerNodeLBs(service *v1.Service, configs []lbConfig, nodes []nodeInfo) switchRules := make([]LBRule, 0, len(configs)) for _, config := range configs { - switchV4targetips := config.makeNodeSwitchTargetIPs(&node, config.eps.V4IPs) - switchV6targetips := config.makeNodeSwitchTargetIPs(&node, config.eps.V6IPs) + switchV4targetips, _ := config.makeNodeSwitchTargetIPs(&node, config.eps.V4IPs) + switchV6targetips, _ := config.makeNodeSwitchTargetIPs(&node, config.eps.V6IPs) - routerV4targetips := config.makeNodeRouterTargetIPs(&node, config.eps.V4IPs, types.V4HostMasqueradeIP) - routerV6targetips := config.makeNodeRouterTargetIPs(&node, config.eps.V6IPs, types.V6HostMasqueradeIP) + routerV4targetips, _ := config.makeNodeRouterTargetIPs(&node, config.eps.V4IPs, types.V4HostMasqueradeIP) + routerV6targetips, _ := config.makeNodeRouterTargetIPs(&node, config.eps.V6IPs, types.V6HostMasqueradeIP) routerV4targets := joinHostsPort(routerV4targetips, config.eps.Port) routerV6targets := joinHostsPort(routerV6targetips, config.eps.Port) diff --git a/go-controller/pkg/ovn/controller/services/services_controller_test.go b/go-controller/pkg/ovn/controller/services/services_controller_test.go index fa3e93219fa..08f64f0aeb2 100644 --- a/go-controller/pkg/ovn/controller/services/services_controller_test.go +++ b/go-controller/pkg/ovn/controller/services/services_controller_test.go @@ -331,17 +331,16 @@ func TestSyncServices(t *testing.T) { }, ExternalIDs: serviceExternalIDs(namespacedServiceName(ns, serviceName)), }, - nodeSwitchTemplateLoadBalancer(nodePort, serviceName, ns), - nodeRouterTemplateLoadBalancer(nodePort, serviceName, ns), + nodeMergedTemplateLoadBalancer(nodePort, serviceName, ns, outport, nodeAEndpointIP, nodeBEndpointIP), nodeLogicalSwitch(nodeA, initialLsGroups), nodeLogicalSwitch(nodeB, initialLsGroups), nodeLogicalRouter(nodeA, initialLrGroups), nodeLogicalRouter(nodeB, initialLrGroups), lbGroup(types.ClusterLBGroupName, loadBalancerClusterWideTCPServiceName(ns, serviceName)), - lbGroup(types.ClusterSwitchLBGroupName, nodeSwitchTemplateLoadBalancerName(ns, serviceName, v1.IPv4Protocol)), - lbGroup(types.ClusterRouterLBGroupName, nodeRouterTemplateLoadBalancerName(ns, serviceName, v1.IPv4Protocol)), - nodeTemplate(firstNode, nodePort, serviceName, ns, outport, nodeAEndpointIP, nodeBEndpointIP), - nodeTemplate(secondNode, nodePort, serviceName, ns, outport, nodeAEndpointIP, nodeBEndpointIP), + lbGroup(types.ClusterSwitchLBGroupName, nodeMergedTemplateLoadBalancerName(ns, serviceName, v1.IPv4Protocol)), + lbGroup(types.ClusterRouterLBGroupName, nodeMergedTemplateLoadBalancerName(ns, serviceName, v1.IPv4Protocol)), + nodeIPTemplate(firstNode), + nodeIPTemplate(secondNode), }, }, { @@ -414,17 +413,16 @@ func TestSyncServices(t *testing.T) { }, ExternalIDs: serviceExternalIDs(namespacedServiceName(ns, serviceName)), }, - nodeSwitchTemplateLoadBalancer(nodePort, serviceName, ns), - nodeRouterTemplateLoadBalancer(nodePort, serviceName, ns), + nodeMergedTemplateLoadBalancer(nodePort, serviceName, ns, outport, nodeAEndpointIP, nodeBEndpointIP), nodeLogicalSwitch(nodeA, initialLsGroups), nodeLogicalSwitch(nodeB, initialLsGroups), nodeLogicalRouter(nodeA, initialLrGroups), nodeLogicalRouter(nodeB, initialLrGroups), lbGroup(types.ClusterLBGroupName, loadBalancerClusterWideTCPServiceName(ns, serviceName)), - lbGroup(types.ClusterSwitchLBGroupName, nodeSwitchTemplateLoadBalancerName(ns, serviceName, v1.IPv4Protocol)), - lbGroup(types.ClusterRouterLBGroupName, nodeRouterTemplateLoadBalancerName(ns, serviceName, v1.IPv4Protocol)), - nodeTemplate(firstNode, nodePort, serviceName, ns, outport, nodeAEndpointIP, nodeBEndpointIP), - nodeTemplate(secondNode, nodePort, serviceName, ns, outport, nodeAEndpointIP, nodeBEndpointIP), + lbGroup(types.ClusterSwitchLBGroupName, nodeMergedTemplateLoadBalancerName(ns, serviceName, v1.IPv4Protocol)), + lbGroup(types.ClusterRouterLBGroupName, nodeMergedTemplateLoadBalancerName(ns, serviceName, v1.IPv4Protocol)), + nodeIPTemplate(firstNode), + nodeIPTemplate(secondNode), }, nodeToDelete: nodeConfig(nodeA, nodeAHostIP), dbStateAfterDeleting: []libovsdbtest.TestData{ @@ -438,19 +436,16 @@ func TestSyncServices(t *testing.T) { }, ExternalIDs: serviceExternalIDs(namespacedServiceName(ns, serviceName)), }, - nodeSwitchTemplateLoadBalancer(nodePort, serviceName, ns), - nodeRouterTemplateLoadBalancer(nodePort, serviceName, ns), + nodeMergedTemplateLoadBalancer(nodePort, serviceName, ns, outport, nodeAEndpointIP, nodeBEndpointIP), nodeLogicalSwitch(nodeA, initialLsGroups), nodeLogicalSwitch(nodeB, initialLsGroups), nodeLogicalRouter(nodeA, initialLrGroups), nodeLogicalRouter(nodeB, initialLrGroups), lbGroup(types.ClusterLBGroupName, loadBalancerClusterWideTCPServiceName(ns, serviceName)), - lbGroup(types.ClusterSwitchLBGroupName, nodeSwitchTemplateLoadBalancerName(ns, serviceName, v1.IPv4Protocol)), - lbGroup(types.ClusterRouterLBGroupName, nodeRouterTemplateLoadBalancerName(ns, serviceName, v1.IPv4Protocol)), - // node-A chassis template var is cleaned up by the DefaultNetworkController() - // deleteNodeEvent handler. And that doesn't get run in this test. - nodeTemplate(firstNode, nodePort, serviceName, ns, outport, nodeAEndpointIP, nodeBEndpointIP), - nodeTemplate(secondNode, nodePort, serviceName, ns, outport, nodeAEndpointIP, nodeBEndpointIP), + lbGroup(types.ClusterSwitchLBGroupName, nodeMergedTemplateLoadBalancerName(ns, serviceName, v1.IPv4Protocol)), + lbGroup(types.ClusterRouterLBGroupName, nodeMergedTemplateLoadBalancerName(ns, serviceName, v1.IPv4Protocol)), + nodeIPTemplate(firstNode), + nodeIPTemplate(secondNode), }, }, } @@ -568,6 +563,14 @@ func nodeRouterTemplateLoadBalancerName(serviceNamespace string, serviceName str addressFamily) } +func nodeMergedTemplateLoadBalancerName(serviceNamespace string, serviceName string, addressFamily v1.IPFamily) string { + return fmt.Sprintf( + "Service_%s/%s_TCP_node_switch_template_%s_merged", + serviceNamespace, + serviceName, + addressFamily) +} + func servicesOptions() map[string]string { return map[string]string{ "event": "false", @@ -637,12 +640,18 @@ func nodeIPTemplate(node *nodeInfo) *nbdb.ChassisTemplateVar { } } -func nodeTemplate(node *nodeInfo, nodePort int32, serviceName string, serviceNamespace string, outputPort int32, endpointIPs ...string) *nbdb.ChassisTemplateVar { - template := nodeIPTemplate(node) - template.Variables[makeLBNodeIPTemplateName(v1.IPv4Protocol)] = node.nodeIPs[0].String() - template.Variables[makeTarget(serviceName, serviceNamespace, "TCP", nodePort, "node_switch_template", v1.IPv4Protocol)] = computeEndpoints(outputPort, endpointIPs...) - template.Variables[makeTarget(serviceName, serviceNamespace, "TCP", nodePort, "node_router_template", v1.IPv4Protocol)] = computeEndpoints(outputPort, endpointIPs...) - return template +func nodeMergedTemplateLoadBalancer(nodePort int32, serviceName string, serviceNamespace string, outputPort int32, endpointIPs ...string) *nbdb.LoadBalancer { + nodeTemplateIP := makeTemplate(makeLBNodeIPTemplateName(v1.IPv4Protocol)) + return &nbdb.LoadBalancer{ + UUID: nodeMergedTemplateLoadBalancerName(serviceNamespace, serviceName, v1.IPv4Protocol), + Name: nodeMergedTemplateLoadBalancerName(serviceNamespace, serviceName, v1.IPv4Protocol), + Options: templateServicesOptions(), + Protocol: &nbdb.LoadBalancerProtocolTCP, + Vips: map[string]string{ + endpoint(refTemplate(nodeTemplateIP.Name), nodePort): computeEndpoints(outputPort, endpointIPs...), + }, + ExternalIDs: serviceExternalIDs(namespacedServiceName(serviceNamespace, serviceName)), + } } func refTemplate(template string) string { diff --git a/go-controller/pkg/util/util.go b/go-controller/pkg/util/util.go index b002f965d81..b5bd023da43 100644 --- a/go-controller/pkg/util/util.go +++ b/go-controller/pkg/util/util.go @@ -168,9 +168,10 @@ func HashForOVN(s string) string { } // UpdateIPsSlice will search for values of oldIPs in the slice "s" and update it with newIPs values of same IP family -func UpdateIPsSlice(s, oldIPs, newIPs []string) []string { +func UpdateIPsSlice(s, oldIPs, newIPs []string) ([]string, bool) { n := make([]string, len(s)) copy(n, s) + updated := false for i, entry := range s { for _, oldIP := range oldIPs { if entry == oldIP { @@ -178,11 +179,13 @@ func UpdateIPsSlice(s, oldIPs, newIPs []string) []string { if utilnet.IsIPv6(net.ParseIP(oldIP)) { if utilnet.IsIPv6(net.ParseIP(newIP)) { n[i] = newIP + updated = true break } } else { if !utilnet.IsIPv6(net.ParseIP(newIP)) { n[i] = newIP + updated = true break } } @@ -191,7 +194,7 @@ func UpdateIPsSlice(s, oldIPs, newIPs []string) []string { } } } - return n + return n, updated } // FilterIPsSlice will filter a list of IPs by a list of CIDRs. By default, diff --git a/go-controller/pkg/util/util_unit_test.go b/go-controller/pkg/util/util_unit_test.go index 4de5f936a53..4e8437aa7aa 100644 --- a/go-controller/pkg/util/util_unit_test.go +++ b/go-controller/pkg/util/util_unit_test.go @@ -110,6 +110,7 @@ func TestUpdateIPsSlice(t *testing.T) { name string s, oldIPs, newIPs []string want []string + changed bool }{ { "Tests no matching IPs to remove", @@ -117,6 +118,7 @@ func TestUpdateIPsSlice(t *testing.T) { []string{"1.1.1.1"}, []string{"9.9.9.9", "fe99::1"}, []string{"192.168.1.1", "10.0.0.1", "127.0.0.2"}, + false, }, { "Tests some matching IPs to replace", @@ -124,6 +126,7 @@ func TestUpdateIPsSlice(t *testing.T) { []string{"10.0.0.1"}, []string{"9.9.9.9", "fe99::1"}, []string{"192.168.1.1", "9.9.9.9", "127.0.0.2"}, + true, }, { "Tests matching IPv6 to replace", @@ -131,6 +134,7 @@ func TestUpdateIPsSlice(t *testing.T) { []string{"3dfd::99ac"}, []string{"9.9.9.9", "fe99::1"}, []string{"fed9::5", "ab13::1e15", "fe99::1"}, + true, }, { "Tests match but nothing to replace with", @@ -138,6 +142,7 @@ func TestUpdateIPsSlice(t *testing.T) { []string{"3dfd::99ac"}, []string{"9.9.9.9"}, []string{"fed9::5", "ab13::1e15", "3dfd::99ac"}, + false, }, { "Tests with no newIPs", @@ -145,6 +150,7 @@ func TestUpdateIPsSlice(t *testing.T) { []string{"3dfd::99ac"}, []string{}, []string{"fed9::5", "ab13::1e15", "3dfd::99ac"}, + false, }, { "Tests with no newIPs or oldIPs", @@ -152,15 +158,20 @@ func TestUpdateIPsSlice(t *testing.T) { []string{}, []string{}, []string{"fed9::5", "ab13::1e15", "3dfd::99ac"}, + false, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - ans := UpdateIPsSlice(tt.s, tt.oldIPs, tt.newIPs) + ans, changed := UpdateIPsSlice(tt.s, tt.oldIPs, tt.newIPs) if !reflect.DeepEqual(ans, tt.want) { t.Errorf("got %v, want %v", ans, tt.want) } + + if tt.changed != changed { + t.Errorf("got changed %t, want %t", changed, tt.changed) + } }) } }