Some quality of life improvements for layer 3 controllers node handling

During update node events, local and remote addOrUpdate functions are called.
There are a series of sync checks used to know what to configure.
However, in some cases log messages were being printed no matter what,
and hybrid overlay was being processed on every node event.

This cleans things up so that hybrid overlay is only sync'ed when
necessary, and logs are only printed when work is being done to add the
local or remote node.

Also, removes an old test case for hybrid overlay where the node-subnets
annotation of a node was being removed. First introduced here:

ovn-kubernetes/ovn-kubernetes@aef135c#diff-9ab180ea9a39f81dc8334a00ca8ea5e4cd04f9491c27dcfd910b07929c9ddbb5R193

It's not totally clear what the purpose of this test was, but we do not
support clearing OVN configuration when OVNK assigned annotations are
removed by the user. The node-subnets annotation should not be removed,
and if is removed, it should be configured back onto the node by
cluster-manager.

Signed-off-by: Tim Rozet <[email protected]>

Author: trozet

go-controller/pkg/ovn/default_network_controller.go

@@ -15,6 +15,7 @@ import (
 	"k8s.io/client-go/util/workqueue"
 	"k8s.io/klog/v2"
 
+	hotypes "github.com/ovn-org/ovn-kubernetes/go-controller/hybrid-overlay/pkg/types"
 	"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/config"
 	"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/controller"
 	egressfirewall "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/egressfirewall/v1"
@@ -762,6 +763,16 @@ func (h *defaultNetworkControllerEventHandler) AddResource(obj interface{}, from
 		var aggregatedErrors []error
 		if h.oc.isLocalZoneNode(node) {
 			var nodeParams *nodeSyncs
+			hoNeedsCleanup := false
+			if !config.HybridOverlay.Enabled {
+				// check if the node has the stale annotations on it to signal that we need to clean up
+				if _, exists := node.Annotations[hotypes.HybridOverlayDRIP]; exists {
+					hoNeedsCleanup = true
+				}
+				if _, exist := node.Annotations[hotypes.HybridOverlayDRMAC]; exist {
+					hoNeedsCleanup = true
+				}
+			}
 			if fromRetryLoop {
 				_, nodeSync := h.oc.addNodeFailed.Load(node.Name)
 				_, clusterRtrSync := h.oc.nodeClusterRouterPortFailed.Load(node.Name)
@@ -774,18 +785,17 @@ func (h *defaultNetworkControllerEventHandler) AddResource(obj interface{}, from
 					syncClusterRouterPort: clusterRtrSync,
 					syncMgmtPort:          mgmtSync,
 					syncGw:                gwSync,
-					syncHo:                hoSync,
+					syncHo:                hoSync || hoNeedsCleanup,
 					syncZoneIC:            zoneICSync}
 			} else {
 				nodeParams = &nodeSyncs{
 					syncNode:              true,
 					syncClusterRouterPort: true,
 					syncMgmtPort:          true,
 					syncGw:                true,
-					syncHo:                config.HybridOverlay.Enabled,
+					syncHo:                config.HybridOverlay.Enabled || hoNeedsCleanup,
 					syncZoneIC:            config.OVNKubernetesFeature.EnableInterconnect}
 			}
-
 			if err = h.oc.addUpdateLocalNodeEvent(node, nodeParams); err != nil {
 				klog.Infof("Node add failed for %s, will try again later: %v",
 					node.Name, err)
@@ -941,6 +951,16 @@ func (h *defaultNetworkControllerEventHandler) UpdateResource(oldObj, newObj int
 				_, failed = h.oc.gatewaysFailed.Load(newNode.Name)
 				gwSync := failed || gatewayChanged(oldNode, newNode) || nodeSubnetChange ||
 					hostCIDRsChanged(oldNode, newNode) || nodeGatewayMTUSupportChanged(oldNode, newNode)
+				hoNeedsCleanup := false
+				if !config.HybridOverlay.Enabled {
+					// check if the node has the stale annotations on it to signal that we need to clean up
+					if _, exists := newNode.Annotations[hotypes.HybridOverlayDRIP]; exists {
+						hoNeedsCleanup = true
+					}
+					if _, exist := newNode.Annotations[hotypes.HybridOverlayDRMAC]; exist {
+						hoNeedsCleanup = true
+					}
+				}
 				_, hoSync := h.oc.hybridOverlayFailed.Load(newNode.Name)
 				_, syncZoneIC := h.oc.syncZoneICFailed.Load(newNode.Name)
 				syncZoneIC = syncZoneIC || zoneClusterChanged || primaryAddrChanged(oldNode, newNode)
@@ -949,12 +969,12 @@ func (h *defaultNetworkControllerEventHandler) UpdateResource(oldObj, newObj int
 					syncClusterRouterPort: clusterRtrSync,
 					syncMgmtPort:          mgmtSync,
 					syncGw:                gwSync,
-					syncHo:                hoSync,
+					syncHo:                hoSync || hoNeedsCleanup,
 					syncZoneIC:            syncZoneIC,
 				}
 			} else {
-				klog.Infof("Node %s moved from the remote zone %s to local zone %s.",
-					newNode.Name, util.GetNodeZone(oldNode), util.GetNodeZone(newNode))
+				klog.Infof("Node %s moved from the remote zone %s to local zone %s, in network: %q",
+					newNode.Name, util.GetNodeZone(oldNode), util.GetNodeZone(newNode), h.oc.GetNetworkName())
 				// The node is now a local zone node.  Trigger a full node sync.
 				nodeSyncsParam = &nodeSyncs{
 					syncNode:              true,
@@ -964,7 +984,6 @@ func (h *defaultNetworkControllerEventHandler) UpdateResource(oldObj, newObj int
 					syncHo:                true,
 					syncZoneIC:            config.OVNKubernetesFeature.EnableInterconnect}
 			}
-
 			if err := h.oc.addUpdateLocalNodeEvent(newNode, nodeSyncsParam); err != nil {
 				aggregatedErrors = append(aggregatedErrors, err)
 			}
@@ -977,8 +996,8 @@ func (h *defaultNetworkControllerEventHandler) UpdateResource(oldObj, newObj int
 			syncZoneIC = syncZoneIC || h.oc.isLocalZoneNode(oldNode) || nodeSubnetChange || zoneClusterChanged ||
 				switchToOvnNode || nodeEncapIPsChanged
 			if syncZoneIC {
-				klog.Infof("Node %s in remote zone %s needs interconnect zone sync up. Zone cluster changed: %v",
-					newNode.Name, util.GetNodeZone(newNode), zoneClusterChanged)
+				klog.Infof("Node %q in remote zone %q, network %q, needs interconnect zone sync up. Zone cluster changed: %v",
+					newNode.Name, util.GetNodeZone(newNode), h.oc.GetNetworkName(), zoneClusterChanged)
 			}
 			if err := h.oc.addUpdateRemoteNodeEvent(newNode, syncZoneIC); err != nil {
 				aggregatedErrors = append(aggregatedErrors, err)

go-controller/pkg/ovn/hybrid.go

@@ -137,12 +137,18 @@ func (oc *DefaultNetworkController) handleHybridOverlayPort(node *corev1.Node, a
 }
 
 func (oc *DefaultNetworkController) deleteHybridOverlayPort(node *corev1.Node) error {
-	klog.Infof("Removing node %s hybrid overlay port", node.Name)
 	portName := util.GetHybridOverlayPortName(node.Name)
 	lsp := nbdb.LogicalSwitchPort{Name: portName}
-	sw := nbdb.LogicalSwitch{Name: oc.GetNetworkScopedSwitchName(node.Name)}
-	if err := libovsdbops.DeleteLogicalSwitchPorts(oc.nbClient, &sw, &lsp); err != nil {
-		return err
+	if _, err := libovsdbops.GetLogicalSwitchPort(oc.nbClient, &lsp); err != nil {
+		if !errors.Is(err, libovsdbclient.ErrNotFound) {
+			return fmt.Errorf("failed to get logical switch port for hybrid overlay port %s, err: %v", portName, err)
+		}
+	} else {
+		sw := nbdb.LogicalSwitch{Name: oc.GetNetworkScopedSwitchName(node.Name)}
+		klog.Infof("Removing node %s hybrid overlay port", node.Name)
+		if err := libovsdbops.DeleteLogicalSwitchPorts(oc.nbClient, &sw, &lsp); err != nil {
+			return err
+		}
 	}
 	if err := oc.removeHybridLRPolicySharedGW(node); err != nil {
 		return err

go-controller/pkg/ovn/hybrid_test.go

@@ -1475,229 +1475,6 @@ var _ = ginkgo.Describe("Hybrid SDN Master Operations", func() {
 		gomega.Expect(err).NotTo(gomega.HaveOccurred())
 	})
 
-	ginkgo.It("cleans up a Linux node when the OVN hostsubnet annotation is removed", func() {
-		app.Action = func(ctx *cli.Context) error {
-			const (
-				nodeHOMAC string = "0a:58:0a:01:01:03"
-				hoSubnet  string = "11.1.0.0/16"
-				nodeHOIP  string = "10.1.1.3"
-			)
-			node1 := tNode{
-				Name:                 "node1",
-				NodeIP:               "1.2.3.4",
-				NodeLRPMAC:           "0a:58:0a:01:01:01",
-				LrpIP:                "100.64.0.2",
-				DrLrpIP:              "100.64.0.1",
-				PhysicalBridgeMAC:    "11:22:33:44:55:66",
-				SystemID:             "cb9ec8fa-b409-4ef3-9f42-d9283c47aac6",
-				NodeSubnet:           "10.1.1.0/24",
-				GWRouter:             types.GWRouterPrefix + "node1",
-				GatewayRouterIPMask:  "172.16.16.2/24",
-				GatewayRouterIP:      "172.16.16.2",
-				GatewayRouterNextHop: "172.16.16.1",
-				PhysicalBridgeName:   "br-eth0",
-				NodeGWIP:             "10.1.1.1/24",
-				NodeMgmtPortIP:       "10.1.1.2",
-				//NodeMgmtPortMAC:      "0a:58:0a:01:01:02",
-				NodeMgmtPortMAC: "0a:58:64:40:00:03",
-				DnatSnatIP:      "169.254.0.1",
-			}
-			testNode := node1.k8sNode("2")
-
-			kubeFakeClient := fake.NewSimpleClientset(&corev1.NodeList{
-				Items: []corev1.Node{testNode},
-			})
-			egressFirewallFakeClient := &egressfirewallfake.Clientset{}
-			egressIPFakeClient := &egressipfake.Clientset{}
-			egressQoSFakeClient := &egressqosfake.Clientset{}
-			egressServiceFakeClient := &egressservicefake.Clientset{}
-			fakeClient := &util.OVNMasterClientset{
-				KubeClient:           kubeFakeClient,
-				EgressIPClient:       egressIPFakeClient,
-				EgressFirewallClient: egressFirewallFakeClient,
-				EgressQoSClient:      egressQoSFakeClient,
-				EgressServiceClient:  egressServiceFakeClient,
-			}
-
-			vlanID := 1024
-			_, err := config.InitConfig(ctx, nil, nil)
-			gomega.Expect(err).NotTo(gomega.HaveOccurred())
-			config.Kubernetes.HostNetworkNamespace = ""
-			nodeAnnotator := kube.NewNodeAnnotator(&kube.Kube{KClient: kubeFakeClient}, testNode.Name)
-			l3Config := node1.gatewayConfig(config.GatewayModeShared, uint(vlanID))
-			err = util.SetL3GatewayConfig(nodeAnnotator, l3Config)
-			gomega.Expect(err).NotTo(gomega.HaveOccurred())
-			err = util.UpdateNodeManagementPortMACAddresses(&testNode, nodeAnnotator,
-				ovntest.MustParseMAC(node1.NodeMgmtPortMAC), types.DefaultNetworkName)
-			gomega.Expect(err).NotTo(gomega.HaveOccurred())
-			err = util.SetNodeHostSubnetAnnotation(nodeAnnotator, ovntest.MustParseIPNets(node1.NodeSubnet))
-			gomega.Expect(err).NotTo(gomega.HaveOccurred())
-			err = util.SetNodeHostCIDRs(nodeAnnotator, sets.New(fmt.Sprintf("%s/24", node1.NodeIP)))
-			gomega.Expect(err).NotTo(gomega.HaveOccurred())
-			err = nodeAnnotator.Run()
-			gomega.Expect(err).NotTo(gomega.HaveOccurred())
-
-			updatedNode, err := fakeClient.KubeClient.CoreV1().Nodes().Get(context.TODO(), testNode.Name, metav1.GetOptions{})
-			gomega.Expect(err).NotTo(gomega.HaveOccurred())
-			l3GatewayConfig, err := util.ParseNodeL3GatewayAnnotation(updatedNode)
-			gomega.Expect(err).NotTo(gomega.HaveOccurred())
-			hostAddrs, err := util.ParseNodeHostCIDRsDropNetMask(updatedNode)
-			gomega.Expect(err).NotTo(gomega.HaveOccurred())
-
-			f, err = factory.NewMasterWatchFactory(fakeClient)
-			gomega.Expect(err).NotTo(gomega.HaveOccurred())
-			err = f.Start()
-			gomega.Expect(err).NotTo(gomega.HaveOccurred())
-
-			expectedClusterLBGroup := newLoadBalancerGroup(types.ClusterLBGroupName)
-			expectedSwitchLBGroup := newLoadBalancerGroup(types.ClusterSwitchLBGroupName)
-			expectedRouterLBGroup := newLoadBalancerGroup(types.ClusterRouterLBGroupName)
-			expectedOVNClusterRouter := newOVNClusterRouter()
-			ovnClusterRouterLRP := &nbdb.LogicalRouterPort{
-				Name:     types.GWRouterToJoinSwitchPrefix + types.OVNClusterRouter,
-				Networks: []string{"100.64.0.1/16"},
-				UUID:     types.GWRouterToJoinSwitchPrefix + types.OVNClusterRouter + "-UUID",
-			}
-			expectedOVNClusterRouter.Ports = []string{ovnClusterRouterLRP.UUID}
-			expectedNodeSwitch := node1.logicalSwitch([]string{expectedClusterLBGroup.UUID, expectedSwitchLBGroup.UUID})
-			expectedClusterRouterPortGroup := newRouterPortGroup()
-			expectedClusterPortGroup := newClusterPortGroup()
-
-			dbSetup := libovsdbtest.TestSetup{
-				NBData: []libovsdbtest.TestData{
-					newClusterJoinSwitch(),
-					expectedNodeSwitch,
-					ovnClusterRouterLRP,
-					expectedOVNClusterRouter,
-					expectedClusterRouterPortGroup,
-					expectedClusterPortGroup,
-					expectedClusterLBGroup,
-					expectedSwitchLBGroup,
-					expectedRouterLBGroup,
-				},
-			}
-			var libovsdbOvnNBClient, libovsdbOvnSBClient libovsdbclient.Client
-			libovsdbOvnNBClient, libovsdbOvnSBClient, libovsdbCleanup, err = libovsdbtest.NewNBSBTestHarness(dbSetup)
-			gomega.Expect(err).NotTo(gomega.HaveOccurred())
-
-			expectedDatabaseState := []libovsdbtest.TestData{ovnClusterRouterLRP}
-			expectedDatabaseState = addNodeLogicalFlows(expectedDatabaseState, expectedOVNClusterRouter, expectedNodeSwitch, expectedClusterRouterPortGroup, expectedClusterPortGroup, &node1)
-
-			clusterController, err := NewOvnController(
-				fakeClient,
-				f,
-				stopChan,
-				nil,
-				networkmanager.Default().Interface(),
-				libovsdbOvnNBClient,
-				libovsdbOvnSBClient,
-				record.NewFakeRecorder(10),
-				wg,
-				nil,
-				NewPortCache(stopChan),
-			)
-			gomega.Expect(err).NotTo(gomega.HaveOccurred())
-
-			setupCOPP := true
-			setupClusterController(clusterController, setupCOPP)
-
-			//assuming all the pods have finished processing
-			atomic.StoreUint32(&clusterController.allInitialPodsProcessed, 1)
-			// Let the real code run and ensure OVN database sync
-			gomega.Expect(clusterController.WatchNodes()).To(gomega.Succeed())
-
-			gomega.Eventually(func() (map[string]string, error) {
-				updatedNode, err := fakeClient.KubeClient.CoreV1().Nodes().Get(context.TODO(), testNode.Name, metav1.GetOptions{})
-				if err != nil {
-					return nil, err
-				}
-				return updatedNode.Annotations, nil
-			}, 2).Should(gomega.HaveKeyWithValue(hotypes.HybridOverlayDRMAC, nodeHOMAC))
-
-			subnet := ovntest.MustParseIPNet(node1.NodeSubnet)
-			err = clusterController.syncDefaultGatewayLogicalNetwork(updatedNode, l3GatewayConfig, []*net.IPNet{subnet}, hostAddrs.UnsortedList())
-			gomega.Expect(err).NotTo(gomega.HaveOccurred())
-
-			var clusterSubnets []*net.IPNet
-			for _, clusterSubnet := range config.Default.ClusterSubnets {
-				clusterSubnets = append(clusterSubnets, clusterSubnet.CIDR)
-			}
-
-			skipSnat := false
-			expectedDatabaseState = generateGatewayInitExpectedNB(expectedDatabaseState, expectedOVNClusterRouter,
-				expectedNodeSwitch, node1.Name, clusterSubnets, []*net.IPNet{subnet}, l3Config,
-				[]*net.IPNet{classBIPAddress(node1.LrpIP)}, []*net.IPNet{classBIPAddress(node1.DrLrpIP)}, skipSnat,
-				node1.NodeMgmtPortIP, "1400")
-
-			hybridSubnetStaticRoute1, hybridLogicalRouterStaticRoute, hybridSubnetLRP1, hybridSubnetLRP2, hybridLogicalSwitchPort := setupHybridOverlayOVNObjects(node1, "", hoSubnet, nodeHOIP, nodeHOMAC)
-
-			var node1LogicalRouter *nbdb.LogicalRouter
-			var basicNode1StaticRoutes []string
-
-			for _, obj := range expectedDatabaseState {
-				if logicalRouter, ok := obj.(*nbdb.LogicalRouter); ok {
-					if logicalRouter.Name == "GR_node1" {
-						// keep a referance so that we can edit this object
-						node1LogicalRouter = logicalRouter
-						basicNode1StaticRoutes = logicalRouter.StaticRoutes
-						logicalRouter.StaticRoutes = append(logicalRouter.StaticRoutes, hybridLogicalRouterStaticRoute.UUID)
-					}
-				}
-			}
-
-			// keep copies of these before appending hybrid overlay elements
-			basicExpectedNodeSwitchPorts := expectedNodeSwitch.Ports
-			basicExpectedOVNClusterRouterPolicies := expectedOVNClusterRouter.Policies
-			basicExpectedOVNClusterStaticRoutes := expectedOVNClusterRouter.StaticRoutes
-
-			expectedNodeSwitch.Ports = append(expectedNodeSwitch.Ports, hybridLogicalSwitchPort.UUID)
-			expectedOVNClusterRouter.Policies = append(expectedOVNClusterRouter.Policies, hybridSubnetLRP1.UUID, hybridSubnetLRP2.UUID)
-			expectedOVNClusterRouter.StaticRoutes = append(expectedOVNClusterRouter.StaticRoutes, hybridSubnetStaticRoute1.UUID)
-
-			expectedDatabaseStateWithHybridNode := append([]libovsdbtest.TestData{hybridSubnetStaticRoute1, hybridSubnetLRP2, hybridSubnetLRP1, hybridLogicalSwitchPort, hybridLogicalRouterStaticRoute}, expectedDatabaseState...)
-			expectedStaticMACBinding := &nbdb.StaticMACBinding{
-				UUID:               "MAC-binding-HO-UUID",
-				IP:                 nodeHOIP,
-				LogicalPort:        "rtos-node1",
-				MAC:                nodeHOMAC,
-				OverrideDynamicMAC: true,
-			}
-			expectedDatabaseStateWithHybridNode = append(expectedDatabaseStateWithHybridNode, expectedStaticMACBinding)
-			gomega.Eventually(libovsdbOvnNBClient).Should(libovsdbtest.HaveData(expectedDatabaseStateWithHybridNode))
-
-			nodeAnnotator = kube.NewNodeAnnotator(&kube.Kube{KClient: kubeFakeClient}, testNode.Name)
-			util.DeleteNodeHostSubnetAnnotation(nodeAnnotator)
-			err = nodeAnnotator.Run()
-			gomega.Expect(err).NotTo(gomega.HaveOccurred())
-
-			gomega.Eventually(func() (map[string]string, error) {
-				updatedNode, err := fakeClient.KubeClient.CoreV1().Nodes().Get(context.TODO(), testNode.Name, metav1.GetOptions{})
-				if err != nil {
-					return nil, err
-				}
-				return updatedNode.Annotations, nil
-			}, 5).ShouldNot(gomega.HaveKey(hotypes.HybridOverlayDRMAC))
-
-			// restore values from the non-hybrid versions
-			expectedNodeSwitch.Ports = basicExpectedNodeSwitchPorts
-			expectedOVNClusterRouter.Policies = basicExpectedOVNClusterRouterPolicies
-			expectedOVNClusterRouter.StaticRoutes = basicExpectedOVNClusterStaticRoutes
-			node1LogicalRouter.StaticRoutes = basicNode1StaticRoutes
-
-			gomega.Eventually(libovsdbOvnNBClient).Should(libovsdbtest.HaveData(expectedDatabaseState))
-
-			return nil
-		}
-		err := app.Run([]string{
-			app.Name,
-			"-cluster-subnets=" + clusterCIDR,
-			"-gateway-mode=shared",
-			"-enable-hybrid-overlay",
-			"-hybrid-overlay-cluster-subnets=" + hybridOverlayClusterCIDR,
-		})
-		gomega.Expect(err).NotTo(gomega.HaveOccurred())
-	})
-
 	ginkgo.It("cleans up a Linux node that has hybridOverlay annotations and database objects when hybrid overlay is disabled", func() {
 		app.Action = func(ctx *cli.Context) error {
 			const (