Merge pull request #1109 from dcbw/addLogicalPort-optimizations

Bug 2090843: addLogicalPort() optimization cherry-picks

go-controller/hybrid-overlay/pkg/controller/master.go

@@ -237,8 +237,12 @@ func (m *MasterController) handleOverlayPort(node *kapi.Node, annotator kube.Ann
 		return nil
 	}
 
-	// retrieve port configuration. If port isn't set up, portMAC will be nil
-	portMAC, _, _ = util.GetPortAddresses(portName, m.nbClient)
+	// Retrieve port MAC address; if the port isn't set up, portMAC will be nil
+	lsp := &nbdb.LogicalSwitchPort{Name: portName}
+	lsp, err = libovsdbops.GetLogicalSwitchPort(m.nbClient, lsp)
+	if err == nil {
+		portMAC, _, _ = util.ExtractPortAddresses(lsp)
+	}
 
 	// compare port configuration to annotation MAC, reconcile as needed
 	lspOK := false

go-controller/pkg/ovn/egressgw.go

@@ -588,6 +588,22 @@ func addOrUpdatePerPodGRSNAT(nbClient libovsdbclient.Client, nodeName string, ex
 	return nil
 }
 
+// addOrUpdatePerPodGRSNATReturnOps returns the operation that adds or updates per pod SNAT rules towards the nodeIP that are
+// applied to the GR where the pod resides
+// used when disableSNATMultipleGWs=true
+func (oc *Controller) addOrUpdatePerPodGRSNATReturnOps(nodeName string, extIPs, podIfAddrs []*net.IPNet, ops []ovsdb.Operation) ([]ovsdb.Operation, error) {
+	gr := types.GWRouterPrefix + nodeName
+	router := &nbdb.LogicalRouter{Name: gr}
+	nats, err := buildPerPodGRSNAT(extIPs, podIfAddrs)
+	if err != nil {
+		return nil, err
+	}
+	if ops, err = libovsdbops.CreateOrUpdateNATsOps(oc.nbClient, ops, router, nats...); err != nil {
+		return nil, fmt.Errorf("failed to update SNAT for pods of router: %s, error: %v", gr, err)
+	}
+	return ops, nil
+}
+
 // addHybridRoutePolicyForPod handles adding a higher priority allow policy to allow traffic to be routed normally
 // by ecmp routes
 func (oc *Controller) addHybridRoutePolicyForPod(podIP net.IP, node string) error {

go-controller/pkg/ovn/pods.go

@@ -441,13 +441,16 @@ func (oc *Controller) addLogicalPort(pod *kapi.Pod) (err error) {
 	}
 
 	if needsIP {
-		// try to get the IP from existing port in OVN first
-		podMac, podIfAddrs, err = oc.getPortAddresses(logicalSwitch, portName)
-		if err != nil {
-			return fmt.Errorf("failed to get pod addresses for pod %s on node: %s, err: %v",
-				portName, logicalSwitch, err)
+		if existingLSP != nil {
+			// try to get the MAC and IPs from existing OVN port first
+			podMac, podIfAddrs, err = oc.getPortAddresses(logicalSwitch, existingLSP)
+			if err != nil {
+				return fmt.Errorf("failed to get pod addresses for pod %s on node: %s, err: %v",
+					portName, logicalSwitch, err)
+			}
 		}
 		needsNewAllocation := false
+
 		// ensure we have reserved the IPs found in OVN
 		if len(podIfAddrs) == 0 {
 			needsNewAllocation = true
@@ -553,17 +556,11 @@ func (oc *Controller) addLogicalPort(pod *kapi.Pod) (err error) {
 		// namespace annotations to go through external egress router
 		if extIPs, err := getExternalIPsGRSNAT(oc.watchFactory, pod.Spec.NodeName); err != nil {
 			return err
-		} else if err = addOrUpdatePerPodGRSNAT(oc.nbClient, pod.Spec.NodeName, extIPs, podIfAddrs); err != nil {
+		} else if ops, err = oc.addOrUpdatePerPodGRSNATReturnOps(pod.Spec.NodeName, extIPs, podIfAddrs, ops); err != nil {
 			return err
 		}
 	}
 
-	// check if this pod is serving as an external GW
-	err = oc.addPodExternalGW(pod)
-	if err != nil {
-		return fmt.Errorf("failed to handle external GW check: %v", err)
-	}
-
 	// set addresses on the port
 	// LSP addresses in OVN are a single space-separated value
 	addresses = []string{podMac.String()}
@@ -600,6 +597,12 @@ func (oc *Controller) addLogicalPort(pod *kapi.Pod) (err error) {
 	txOkCallBack()
 	oc.podRecorder.AddLSP(pod.UID)
 
+	// check if this pod is serving as an external GW
+	err = oc.addPodExternalGW(pod)
+	if err != nil {
+		return fmt.Errorf("failed to handle external GW check: %v", err)
+	}
+
 	// if somehow lspUUID is empty, there is a bug here with interpreting OVSDB results
 	if len(lsp.UUID) == 0 {
 		return fmt.Errorf("UUID is empty from LSP: %+v", *lsp)
@@ -643,15 +646,13 @@ func (oc *Controller) assignPodAddresses(nodeName string) (net.HardwareAddr, []*
 	return podMAC, podCIDRs, nil
 }
 
-// Given a pod and the node on which it is scheduled, get all addresses currently assigned
-// to it from the nbdb.
-func (oc *Controller) getPortAddresses(nodeName, portName string) (net.HardwareAddr, []*net.IPNet, error) {
-	podMac, podIPs, err := util.GetPortAddresses(portName, oc.nbClient)
+// Given a logical switch port and the node on which it is scheduled, get all
+// addresses currently assigned to it including subnet masks.
+func (oc *Controller) getPortAddresses(nodeName string, existingLSP *nbdb.LogicalSwitchPort) (net.HardwareAddr, []*net.IPNet, error) {
+	podMac, podIPs, err := util.ExtractPortAddresses(existingLSP)
 	if err != nil {
 		return nil, nil, err
-	}
-
-	if podMac == nil || len(podIPs) == 0 {
+	} else if podMac == nil || len(podIPs) == 0 {
 		return nil, nil, nil
 	}
 

go-controller/pkg/util/net.go

@@ -35,16 +35,8 @@ func intToIP(i *big.Int) net.IP {
 	return net.IP(i.Bytes())
 }
 
-// GetPortAddresses returns the MAC and IPs of the given logical switch port
-func GetPortAddresses(portName string, nbClient client.Client) (net.HardwareAddr, []net.IP, error) {
-	lsp := &nbdb.LogicalSwitchPort{Name: portName}
-	lsp, err := libovsdbops.GetLogicalSwitchPort(nbClient, lsp)
-	if err == client.ErrNotFound {
-		return nil, nil, nil
-	} else if err != nil {
-		return nil, nil, err
-	}
-
+// ExtractPortAddresses returns the MAC and IPs of the given logical switch port
+func ExtractPortAddresses(lsp *nbdb.LogicalSwitchPort) (net.HardwareAddr, []net.IP, error) {
 	var addresses []string
 
 	if lsp.DynamicAddresses == nil {
@@ -63,13 +55,13 @@ func GetPortAddresses(portName string, nbClient client.Client) (net.HardwareAddr
 
 	mac, err := net.ParseMAC(addresses[0])
 	if err != nil {
-		return nil, nil, fmt.Errorf("failed to parse logical switch port %q MAC %q: %v", portName, addresses[0], err)
+		return nil, nil, fmt.Errorf("failed to parse logical switch port %q MAC %q: %v", lsp.Name, addresses[0], err)
 	}
 	var ips []net.IP
 	for _, addr := range addresses[1:] {
 		ip := net.ParseIP(addr)
 		if ip == nil {
-			return nil, nil, fmt.Errorf("failed to parse logical switch port %q IP %q is not a valid ip address", portName, addr)
+			return nil, nil, fmt.Errorf("failed to parse logical switch port %q IP %q is not a valid ip address", lsp.Name, addr)
 		}
 		ips = append(ips, ip)
 	}

go-controller/pkg/util/net_unit_test.go

@@ -8,7 +8,6 @@ import (
 
 	nbdb "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/nbdb"
 	ovntest "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/testing"
-	libovsdbtest "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/testing/libovsdb"
 	mock_k8s_io_utils_exec "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/testing/mocks/k8s.io/utils/exec"
 	"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/util/mocks"
 	"github.com/stretchr/testify/assert"
@@ -73,100 +72,65 @@ const (
 	portName  string = "test-pod"
 )
 
-func TestGetPortAddresses(t *testing.T) {
+func TestExtractPortAddresses(t *testing.T) {
 	tests := []struct {
 		desc       string
-		dbSetup    libovsdbtest.TestSetup
+		lsp        *nbdb.LogicalSwitchPort
 		errMatch   error
 		isNotFound bool
 		hasNoIP    bool
 	}{
-		{
-			desc:       "test path where client.Get(lsp) returns ErrNotFound",
-			dbSetup:    libovsdbtest.TestSetup{},
-			isNotFound: true,
-		},
 		{
 			desc: "test path where lsp.DynamicAddresses is a zero length string and len(addresses)==0",
-			dbSetup: libovsdbtest.TestSetup{
-				NBData: []libovsdbtest.TestData{
-					&nbdb.LogicalSwitchPort{
-						Name:             "test-pod",
-						DynamicAddresses: stringPtr(hwAddr + " " + ipAddr),
-					},
-				},
+			lsp: &nbdb.LogicalSwitchPort{
+				Name:             "test-pod",
+				DynamicAddresses: stringPtr(hwAddr + " " + ipAddr),
 			},
 		},
 		{
 			desc: "test path where lsp.DynamicAddresses is non-zero length string and value of first address in addresses list is set to dynamic",
-			dbSetup: libovsdbtest.TestSetup{
-				NBData: []libovsdbtest.TestData{
-					&nbdb.LogicalSwitchPort{
-						Name:             portName,
-						DynamicAddresses: stringPtr(hwAddr + " " + ipAddr),
-						Addresses:        []string{"dynamic"},
-					},
-				},
+			lsp: &nbdb.LogicalSwitchPort{
+				Name:             portName,
+				DynamicAddresses: stringPtr(hwAddr + " " + ipAddr),
+				Addresses:        []string{"dynamic"},
 			},
 		},
 		{
 			desc: "test code path where port has MAC but no IPs",
-			dbSetup: libovsdbtest.TestSetup{
-				NBData: []libovsdbtest.TestData{
-					&nbdb.LogicalSwitchPort{
-						Name:             "test-pod",
-						DynamicAddresses: stringPtr(hwAddr),
-					},
-				},
+			lsp: &nbdb.LogicalSwitchPort{
+				Name:             "test-pod",
+				DynamicAddresses: stringPtr(hwAddr),
 			},
 			hasNoIP: true,
 		},
 		{
 			desc: "test the code path where ParseMAC fails",
-			dbSetup: libovsdbtest.TestSetup{
-				NBData: []libovsdbtest.TestData{
-					&nbdb.LogicalSwitchPort{
-						Name:             portName,
-						DynamicAddresses: stringPtr(badHWAddr),
-					},
-				},
+			lsp: &nbdb.LogicalSwitchPort{
+				Name:             portName,
+				DynamicAddresses: stringPtr(badHWAddr),
 			},
 			errMatch: fmt.Errorf("failed to parse logical switch port \"%s\" MAC \"%s\": address %s: invalid MAC address", portName, badHWAddr, badHWAddr),
 		},
 		{
 			desc: "test code path where IP address parsing fails",
-			dbSetup: libovsdbtest.TestSetup{
-				NBData: []libovsdbtest.TestData{
-					&nbdb.LogicalSwitchPort{
-						Name:      portName,
-						Addresses: []string{fmt.Sprintf("%s %s", hwAddr, badIPAddr)},
-					},
-				},
+			lsp: &nbdb.LogicalSwitchPort{
+				Name:      portName,
+				Addresses: []string{fmt.Sprintf("%s %s", hwAddr, badIPAddr)},
 			},
 			errMatch: fmt.Errorf("failed to parse logical switch port \"%s\" IP \"%s\" is not a valid ip address", portName, badIPAddr),
 		},
 		{
 			desc: "test success path with len(lsp.Addresses) > 0 and lsp.DynamicAddresses = nil",
-			dbSetup: libovsdbtest.TestSetup{
-				NBData: []libovsdbtest.TestData{
-					&nbdb.LogicalSwitchPort{
-						Name:      portName,
-						Addresses: []string{fmt.Sprintf("%s %s", hwAddr, ipAddr)},
-					},
-				},
+			lsp: &nbdb.LogicalSwitchPort{
+				Name:      portName,
+				Addresses: []string{fmt.Sprintf("%s %s", hwAddr, ipAddr)},
 			},
 		},
 	}
 
 	for i, tc := range tests {
 		t.Run(fmt.Sprintf("%d:%s", i, tc.desc), func(t *testing.T) {
-			nbClient, cleanup, err := libovsdbtest.NewNBTestHarness(tc.dbSetup, nil)
-			if err != nil {
-				t.Fatal(fmt.Errorf("test: \"%s\" failed to create test harness: %v", tc.desc, err))
-			}
-			t.Cleanup(cleanup.Cleanup)
-
-			hardwareAddr, ips, err := GetPortAddresses(portName, nbClient)
+			hardwareAddr, ips, err := ExtractPortAddresses(tc.lsp)
 			if tc.isNotFound {
 				assert.Nil(t, hardwareAddr)
 				assert.Nil(t, ips)