OCPBUGS-77082,OCPBUGS-77096: [release-4.19] combined backport PR for 2 escalations

go-controller/cmd/ovnkube-identity/ovnkubeidentity.go

@@ -329,7 +329,7 @@ func runWebhook(ctx context.Context, restCfg *rest.Config) error {
 	nodeWebhook := admission.WithCustomValidator(
 		scheme.Scheme,
 		&corev1.Node{},
-		ovnwebhook.NewNodeAdmissionWebhook(cliCfg.enableInterconnect, cliCfg.enableHybridOverlay, cliCfg.extraAllowedUsers.Value()...),
+		ovnwebhook.NewNodeAdmissionWebhook(cliCfg.enableHybridOverlay, cliCfg.extraAllowedUsers.Value()...),
 	).WithRecoverPanic(true)
 
 	nodeHandler, err := admission.StandaloneWebhook(

go-controller/pkg/clustermanager/node/node_allocator.go

@@ -343,7 +343,6 @@ func (na *NodeAllocator) syncNodeNetworkAnnotations(node *corev1.Node) error {
 func (na *NodeAllocator) HandleDeleteNode(node *corev1.Node) error {
 	if na.hasHybridOverlayAllocation() {
 		na.releaseHybridOverlayNodeSubnet(node.Name)
-		return nil
 	}
 
 	if na.hasNodeSubnetAllocation() || na.hasHybridOverlayAllocationUnmanaged() {

go-controller/pkg/clustermanager/node/node_allocator_test.go

@@ -12,7 +12,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/kubernetes/fake"
-	"k8s.io/client-go/listers/core/v1"
+	listersv1 "k8s.io/client-go/listers/core/v1"
 	"k8s.io/client-go/tools/cache"
 
 	ovncnitypes "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/cni/types"
@@ -400,12 +400,12 @@ func TestController_allocateNodeSubnets_ReleaseOnError(t *testing.T) {
 	}
 }
 
-func newFakeNodeLister(nodes []*corev1.Node) v1.NodeLister {
+func newFakeNodeLister(nodes []*corev1.Node) listersv1.NodeLister {
 	indexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{})
 	for _, node := range nodes {
 		_ = indexer.Add(node)
 	}
-	return v1.NewNodeLister(indexer)
+	return listersv1.NewNodeLister(indexer)
 }
 
 func TestController_CleanupStaleAnnotation(t *testing.T) {
@@ -448,3 +448,105 @@ func TestController_CleanupStaleAnnotation(t *testing.T) {
 		t.Fatalf("Expected annotation %s to be cleaned up, got %v", util.OVNNodeGRLRPAddrs, nodes.Items[0].Annotations)
 	}
 }
+
+// TestNodeAllocator_HandleDeleteNode verifies that HandleDeleteNode correctly releases
+// both standard cluster subnets and hybrid overlay subnets (if enabled) when a node is deleted.
+func TestNodeAllocator_HandleDeleteNode(t *testing.T) {
+	origHybridEnabled := config.HybridOverlay.Enabled
+	origHybridSubnets := config.HybridOverlay.ClusterSubnets
+	origClusterSubnets := config.Default.ClusterSubnets
+	origNoHostSubnetNodes := config.Kubernetes.NoHostSubnetNodes
+	t.Cleanup(func() {
+		config.HybridOverlay.Enabled = origHybridEnabled
+		config.HybridOverlay.ClusterSubnets = origHybridSubnets
+		config.Default.ClusterSubnets = origClusterSubnets
+		config.Kubernetes.NoHostSubnetNodes = origNoHostSubnetNodes
+	})
+
+	config.HybridOverlay.Enabled = true
+	config.HybridOverlay.ClusterSubnets = []config.CIDRNetworkEntry{
+		{CIDR: ovntest.MustParseIPNet("10.0.0.0/16"), HostSubnetLength: 24},
+	}
+
+	ranges, err := rangesFromStrings([]string{"172.16.0.0/16"}, []int{24})
+	if err != nil {
+		t.Fatal(err)
+	}
+	config.Default.ClusterSubnets = ranges
+
+	netInfo, err := util.NewNetInfo(
+		&ovncnitypes.NetConf{
+			NetConf: cnitypes.NetConf{Name: types.DefaultNetworkName},
+		},
+	)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	na := &NodeAllocator{
+		netInfo:                netInfo,
+		clusterSubnetAllocator: NewSubnetAllocator(),
+		nodeLister:             newFakeNodeLister([]*corev1.Node{}),
+	}
+	if na.hasHybridOverlayAllocation() {
+		na.hybridOverlaySubnetAllocator = NewSubnetAllocator()
+	}
+
+	if !na.hasHybridOverlayAllocation() {
+		t.Fatal("Hybrid overlay allocation should be enabled given the test configuration")
+	}
+
+	if err := na.Init(); err != nil {
+		t.Fatalf("Failed to initialize node allocator: %v", err)
+	}
+
+	nodeName := "node-delete-test"
+	if !na.hasNodeSubnetAllocation() {
+		t.Fatal("Node subnet allocation should be enabled")
+	}
+
+	allocated, _, err := na.allocateNodeSubnets(na.clusterSubnetAllocator, nodeName, nil, true, false)
+	if err != nil {
+		t.Fatalf("Failed to allocate subnet: %v", err)
+	}
+	if len(allocated) == 0 {
+		t.Fatal("No subnet allocated")
+	}
+
+	v4used, _ := na.clusterSubnetAllocator.Usage()
+	if v4used != 1 {
+		t.Fatalf("Expected 1 allocated subnet, got %d", v4used)
+	}
+
+	if na.hasHybridOverlayAllocation() {
+		if _, _, err := na.allocateNodeSubnets(na.hybridOverlaySubnetAllocator, nodeName, nil, true, false); err != nil {
+			t.Fatalf("Failed to allocate hybrid overlay subnet: %v", err)
+		}
+		hoUsed, _ := na.hybridOverlaySubnetAllocator.Usage()
+		if hoUsed != 1 {
+			t.Fatalf("Expected 1 allocated hybrid overlay subnet, got %d", hoUsed)
+		}
+	}
+
+	node := &corev1.Node{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: nodeName,
+		},
+	}
+
+	if err := na.HandleDeleteNode(node); err != nil {
+		t.Fatalf("HandleDeleteNode failed: %v", err)
+	}
+
+	v4usedAfter, _ := na.clusterSubnetAllocator.Usage()
+	if v4usedAfter != 0 {
+		t.Errorf("Subnet leak detected! Expected 0 allocated subnets, got %d", v4usedAfter)
+	}
+
+	if na.hasHybridOverlayAllocation() {
+		hoUsedAfter, _ := na.hybridOverlaySubnetAllocator.Usage()
+		if hoUsedAfter != 0 {
+			t.Errorf("Hybrid overlay subnet leak detected! Expected 0 allocated subnets, got %d", hoUsedAfter)
+		}
+	}
+}

go-controller/pkg/node/default_node_network_controller.go

@@ -799,60 +799,6 @@ func getOVNSBZone() (string, error) {
 	return dbZone, nil
 }
 
-/** HACK BEGIN **/
-// TODO(tssurya): Remove this HACK a few months from now.
-// checkOVNSBNodeLRSR returns true if the logical router static route for the
-// the given nodeSubnet is present in the SBDB
-func checkOVNSBNodeLRSR(nodeSubnet *net.IPNet) bool {
-	var matchv4, matchv6 string
-	v6 := true
-	v4 := true
-	if config.IPv6Mode && utilnet.IsIPv6CIDR(nodeSubnet) {
-		matchv6 = fmt.Sprintf("match=\"reg7 == 0 && ip6.dst == %s\"", nodeSubnet)
-		stdout, stderr, err := util.RunOVNSbctl("--bare", "--columns", "_uuid", "find", "logical_flow", matchv6)
-		klog.Infof("Upgrade Hack: checkOVNSBNodeLRSR for node - %s : match %s : stdout - %s : stderr - %s : err %v",
-			nodeSubnet, matchv6, stdout, stderr, err)
-		v6 = (err == nil && stderr == "" && stdout != "")
-	}
-	if config.IPv4Mode && !utilnet.IsIPv6CIDR(nodeSubnet) {
-		matchv4 = fmt.Sprintf("match=\"reg7 == 0 && ip4.dst == %s\"", nodeSubnet)
-		stdout, stderr, err := util.RunOVNSbctl("--bare", "--columns", "_uuid", "find", "logical_flow", matchv4)
-		klog.Infof("Upgrade Hack: checkOVNSBNodeLRSR for node - %s : match %s : stdout - %s : stderr - %s : err %v",
-			nodeSubnet, matchv4, stdout, stderr, err)
-		v4 = (err == nil && stderr == "" && stdout != "")
-	}
-	return v6 && v4
-}
-
-func fetchLBNames() string {
-	stdout, stderr, err := util.RunOVNSbctl("--bare", "--columns", "name", "find", "Load_Balancer")
-	if err != nil || stderr != "" {
-		klog.Errorf("Upgrade hack: fetchLBNames could not fetch services %v/%v", err, stderr)
-		return stdout // will be empty and we will retry
-	}
-	klog.Infof("Upgrade Hack: fetchLBNames: stdout - %s : stderr - %s : err %v", stdout, stderr, err)
-	return stdout
-}
-
-// lbExists returns true if the OVN load balancer for the corresponding namespace/name
-// was created
-func lbExists(lbNames, namespace, name string) bool {
-	stitchedServiceName := "Service_" + namespace + "/" + name
-	match := strings.Contains(lbNames, stitchedServiceName)
-	klog.Infof("Upgrade Hack: lbExists for service - %s/%s/%s : match - %v",
-		namespace, name, stitchedServiceName, match)
-	return match
-}
-
-func portExists(namespace, name string) bool {
-	lspName := fmt.Sprintf("logical_port=%s", util.GetLogicalPortName(namespace, name))
-	stdout, stderr, err := util.RunOVNSbctl("--bare", "--columns", "_uuid", "find", "Port_Binding", lspName)
-	klog.Infof("Upgrade Hack: portExists for pod - %s/%s : stdout - %s : stderr - %s", namespace, name, stdout, stderr)
-	return err == nil && stderr == "" && stdout != ""
-}
-
-/** HACK END **/
-
 // Init executes the first steps to start the DefaultNodeNetworkController.
 // It is split from Start() and executed before UserDefinedNodeNetworkController (UDNNC)
 // to allow UDNNC to reference the openflow manager created in Init.
@@ -923,12 +869,9 @@ func (nc *DefaultNodeNetworkController) Init(ctx context.Context) error {
 			return fmt.Errorf("timed out waiting for the node zone %s to match the OVN Southbound db zone, err: %v, err1: %v", config.Default.Zone, err, err1)
 		}
 
-		// if its nonIC OR IC=true and if its phase1 OR if its IC to IC upgrades
-		if !config.OVNKubernetesFeature.EnableInterconnect || sbZone == types.OvnDefaultZone || util.HasNodeMigratedZone(node) { // if its nonIC or if its phase1
-			for _, auth := range []config.OvnAuthConfig{config.OvnNorth, config.OvnSouth} {
-				if err := auth.SetDBAuth(); err != nil {
-					return err
-				}
+		for _, auth := range []config.OvnAuthConfig{config.OvnNorth, config.OvnSouth} {
+			if err := auth.SetDBAuth(); err != nil {
+				return err
 			}
 		}
 
@@ -1069,7 +1012,6 @@ func (nc *DefaultNodeNetworkController) Start(ctx context.Context) error {
 	klog.Infof("Starting the default node network controller")
 
 	var err error
-	var node *corev1.Node
 
 	if nc.mgmtPortController == nil {
 		return fmt.Errorf("default node network controller hasn't been pre-started")
@@ -1082,11 +1024,6 @@ func (nc *DefaultNodeNetworkController) Start(ctx context.Context) error {
 		klog.Errorf("Setting klog \"loglevel\" to 5 failed, err: %v", err)
 	}
 
-	if node, err = nc.watchFactory.GetNode(nc.name); err != nil {
-		return fmt.Errorf("error retrieving node %s: %v", nc.name, err)
-	}
-
-	nodeAnnotator := kube.NewNodeAnnotator(nc.Kube, node.Name)
 	waiter := newStartupWaiter()
 
 	// Complete gateway initialization
@@ -1114,125 +1051,6 @@ func (nc *DefaultNodeNetworkController) Start(ctx context.Context) error {
 		}
 	}
 
-	/** HACK BEGIN **/
-	// TODO(tssurya): Remove this HACK a few months from now. This has been added only to
-	// minimize disruption for upgrades when moving to interconnect=true.
-	// We want the legacy ovnkube-master to wait for remote ovnkube-node to
-	// signal it using "k8s.ovn.org/remote-zone-migrated" annotation before
-	// considering a node as remote when we upgrade from "global" (1 zone IC)
-	// zone to multi-zone. This is so that network disruption for the existing workloads
-	// is negligible and until the point where ovnkube-node flips the switch to connect
-	// to the new SBDB, it would continue talking to the legacy RAFT ovnkube-sbdb to ensure
-	// OVN/OVS flows are intact.
-	// STEP1: ovnkube-node start's up in remote zone and sets the "k8s.ovn.org/zone-name" above.
-	// STEP2: We delay the flip of connection for ovnkube-node(ovn-controller) to the new remote SBDB
-	//        until the new remote ovnkube-controller has finished programming all the K8s core objects
-	//        like routes, services and pods. Until then the ovnkube-node will talk to legacy SBDB.
-	// STEP3: Once we get the signal that the new SBDB is ready, we set the "k8s.ovn.org/remote-zone-migrated" annotation
-	// STEP4: We call setDBAuth to now point to new SBDB
-	// STEP5: Legacy ovnkube-master sees "k8s.ovn.org/remote-zone-migrated" annotation on this node and now knows that
-	//        this node has remote-zone-migrated successfully and tears down old setup and creates new IC resource
-	//        plumbing (takes 80ms based on what we saw in CI runs so we might still have that small window of disruption).
-	// NOTE: ovnkube-node in DPU host mode doesn't go through upgrades for OVN-IC and has no SBDB to connect to. Thus this part shall be skipped.
-	var syncNodes, syncServices, syncPods bool
-	if config.OvnKubeNode.Mode != types.NodeModeDPUHost && config.OVNKubernetesFeature.EnableInterconnect && nc.sbZone != types.OvnDefaultZone && !util.HasNodeMigratedZone(node) {
-		klog.Info("Upgrade Hack: Interconnect is enabled")
-		var err1 error
-		start := time.Now()
-		err = wait.PollUntilContextTimeout(ctx, 500*time.Millisecond, 300*time.Second, true, func(_ context.Context) (bool, error) {
-			// we loop through all the nodes in the cluster and ensure ovnkube-controller has finished creating the LRSR required for pod2pod overlay communication
-			if !syncNodes {
-				nodes, err := nc.watchFactory.GetNodes()
-				if err != nil {
-					err1 = fmt.Errorf("upgrade hack: error retrieving node %s: %v", nc.name, err)
-					return false, nil
-				}
-				for _, node := range nodes {
-					node := *node
-					if nc.name != node.Name && util.GetNodeZone(&node) != config.Default.Zone && !util.NoHostSubnet(&node) {
-						nodeSubnets, err := util.ParseNodeHostSubnetAnnotation(&node, types.DefaultNetworkName)
-						if err != nil {
-							if util.IsAnnotationNotSetError(err) {
-								klog.Infof("Skipping node %q. k8s.ovn.org/node-subnets annotation was not found", node.Name)
-								continue
-							}
-							err1 = fmt.Errorf("unable to fetch node-subnet annotation for node %s: err, %v", node.Name, err)
-							return false, nil
-						}
-						for _, nodeSubnet := range nodeSubnets {
-							klog.Infof("Upgrade Hack: node %s, subnet %s", node.Name, nodeSubnet)
-							if !checkOVNSBNodeLRSR(nodeSubnet) {
-								err1 = fmt.Errorf("upgrade hack: unable to find LRSR for node %s", node.Name)
-								return false, nil
-							}
-						}
-					}
-				}
-				klog.Infof("Upgrade Hack: Syncing nodes took %v", time.Since(start))
-				syncNodes = true
-			}
-			// we loop through all existing services in the cluster and ensure ovnkube-controller has finished creating LoadBalancers required for services to work
-			if !syncServices {
-				services, err := nc.watchFactory.GetServices()
-				if err != nil {
-					err1 = fmt.Errorf("upgrade hack: error retrieving the services %v", err)
-					return false, nil
-				}
-				lbNames := fetchLBNames()
-				for _, s := range services {
-					// don't process headless service
-					if !util.ServiceTypeHasClusterIP(s) || !util.IsClusterIPSet(s) {
-						continue
-					}
-					if !lbExists(lbNames, s.Namespace, s.Name) {
-						return false, nil
-					}
-				}
-				klog.Infof("Upgrade Hack: Syncing services took %v", time.Since(start))
-				syncServices = true
-			}
-			if !syncPods {
-				pods, err := nc.watchFactory.GetAllPods()
-				if err != nil {
-					err1 = fmt.Errorf("upgrade hack: error retrieving the services %v", err)
-					return false, nil
-				}
-				for _, p := range pods {
-					if !util.PodScheduled(p) || util.PodCompleted(p) || util.PodWantsHostNetwork(p) {
-						continue
-					}
-					if p.Spec.NodeName != nc.name {
-						// remote pod
-						continue
-					}
-					if !portExists(p.Namespace, p.Name) {
-						return false, nil
-					}
-				}
-				klog.Infof("Upgrade Hack: Syncing pods took %v", time.Since(start))
-				syncPods = true
-			}
-			return true, nil
-		})
-		if err != nil {
-			return fmt.Errorf("upgrade hack: failed while waiting for the remote ovnkube-controller to be ready: %v, %v", err, err1)
-		}
-		if err := util.SetNodeZoneMigrated(nodeAnnotator, nc.sbZone); err != nil {
-			return fmt.Errorf("upgrade hack: failed to set node zone annotation for node %s: %w", nc.name, err)
-		}
-		if err := nodeAnnotator.Run(); err != nil {
-			return fmt.Errorf("upgrade hack: failed to set node %s annotations: %w", nc.name, err)
-		}
-		klog.Infof("ovnkube-node %s finished annotating node with remote-zone-migrated; took: %v", nc.name, time.Since(start))
-		for _, auth := range []config.OvnAuthConfig{config.OvnNorth, config.OvnSouth} {
-			if err := auth.SetDBAuth(); err != nil {
-				return fmt.Errorf("upgrade hack: Unable to set the authentication towards OVN local dbs")
-			}
-		}
-		klog.Infof("Upgrade hack: ovnkube-node %s finished setting DB Auth; took: %v", nc.name, time.Since(start))
-	}
-	/** HACK END **/
-
 	// Wait for management port and gateway resources to be created by the master
 	klog.Infof("Waiting for gateway and management port readiness...")
 	start := time.Now()

go-controller/pkg/ovn/base_network_controller.go

@@ -950,20 +950,6 @@ func (bnc *BaseNetworkController) GetLocalZoneNodes() ([]*corev1.Node, error) {
 
 // isLocalZoneNode returns true if the node is part of the local zone.
 func (bnc *BaseNetworkController) isLocalZoneNode(node *corev1.Node) bool {
-	/** HACK BEGIN **/
-	// TODO(tssurya): Remove this HACK a few months from now. This has been added only to
-	// minimize disruption for upgrades when moving to interconnect=true.
-	// We want the legacy ovnkube-master to wait for remote ovnkube-node to
-	// signal it using "k8s.ovn.org/remote-zone-migrated" annotation before
-	// considering a node as remote when we upgrade from "global" (1 zone IC)
-	// zone to multi-zone. This is so that network disruption for the existing workloads
-	// is negligible and until the point where ovnkube-node flips the switch to connect
-	// to the new SBDB, it would continue talking to the legacy RAFT ovnkube-sbdb to ensure
-	// OVN/OVS flows are intact.
-	if bnc.zone == types.OvnDefaultZone {
-		return !util.HasNodeMigratedZone(node)
-	}
-	/** HACK END **/
 	return util.GetNodeZone(node) == bnc.zone
 }