Refactor ID nomenclature

InvalidID was being used for both networkID and tunnelID. noID was
previously used for just tunnelID and I overloaded it to be used for
networkID as well. This was not a great choice as it causes even more
confusion because noID (value 0) has the same value as DefaultNetworkID.

This commit refactors the variables and moves them into our global
constants file. It changes noID to be noTunnelID and declares
DefaultNetworkID there in a single place. It also creates a noNetworkID
with a value that doesn't collide with DefaultNetworkID. Now logically
the code should be much easier to read.

Also removes a function and unit test that are no longer needed.

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

Author: trozet

go-controller/pkg/clustermanager/clustermanager.go

@@ -25,11 +25,6 @@ import (
 	"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/util"
 )
 
-const (
-	// ID of the default network.
-	defaultNetworkID = 0
-)
-
 // ClusterManager structure is the object which manages the cluster nodes.
 // It creates a default network controller for the default network and a
 // secondary network cluster controller manager to manage the multi networks.

go-controller/pkg/clustermanager/network_cluster_controller.go

@@ -120,7 +120,7 @@ func newDefaultNetworkClusterController(netInfo util.NetInfo, ovnClient *util.OV
 	// defaiult network
 	networkIDAllocator := id.NewIDAllocator(types.DefaultNetworkName, 1)
 	// Reserve the id 0 for the default network.
-	err := networkIDAllocator.ReserveID(types.DefaultNetworkName, defaultNetworkID)
+	err := networkIDAllocator.ReserveID(types.DefaultNetworkName, types.DefaultNetworkID)
 	if err != nil {
 		panic(fmt.Errorf("could not reserve default network ID: %w", err))
 	}
@@ -175,7 +175,7 @@ func (ncc *networkClusterController) init() error {
 	if util.DoesNetworkRequireTunnelIDs(ncc.GetNetInfo()) {
 		ncc.tunnelIDAllocator = id.NewIDAllocator(ncc.GetNetworkName(), types.MaxLogicalPortTunnelKey)
 		// Reserve the id 0. We don't want to assign this id to any of the pods or nodes.
-		if err = ncc.tunnelIDAllocator.ReserveID("zero", util.NoID); err != nil {
+		if err = ncc.tunnelIDAllocator.ReserveID("zero", types.NoTunnelID); err != nil {
 			return err
 		}
 		if util.IsNetworkSegmentationSupportEnabled() && ncc.IsPrimaryNetwork() {
@@ -196,7 +196,7 @@ func (ncc *networkClusterController) init() error {
 							node.Name, ncc.GetNetworkName(), err)
 					}
 				}
-				if tunnelID != util.InvalidID {
+				if tunnelID != types.InvalidID {
 					if err := ncc.tunnelIDAllocator.ReserveID(ncc.GetNetworkName()+"_"+node.Name, tunnelID); err != nil {
 						return fmt.Errorf("unable to reserve id for network %s, node %s: %w", ncc.GetNetworkName(), node.Name, err)
 					}

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

@@ -246,9 +246,14 @@ func (na *NodeAllocator) syncNodeNetworkAnnotations(node *corev1.Node) error {
 	networkName := na.netInfo.GetNetworkName()
 
 	networkID, err := util.ParseNetworkIDAnnotation(node, networkName)
-	if err != nil && !util.IsAnnotationNotSetError(err) {
-		// Log the error and try to allocate new subnets
-		klog.Warningf("Failed to get node %s network id annotations for network %s : %v", node.Name, networkName, err)
+	if err != nil {
+		if !util.IsAnnotationNotSetError(err) {
+			// Log the error and try to allocate new subnets
+			klog.Warningf("Failed to get node %s network id annotations for network %s : %v", node.Name, networkName, err)
+		}
+		// if there is an error we are going to set networkID here to NoNetworkID to prevent
+		// the annotation being updated
+		networkID = types.NoNetworkID
 	}
 
 	updatedSubnetsMap := map[string][]*net.IPNet{}
@@ -319,14 +324,14 @@ func (na *NodeAllocator) syncNodeNetworkAnnotations(node *corev1.Node) error {
 			updatedSubnetsMap[networkName] = validExistingSubnets
 		}
 	}
-	newTunnelID := util.NoID
+	newTunnelID := types.NoTunnelID
 	if util.IsNetworkSegmentationSupportEnabled() && na.netInfo.IsPrimaryNetwork() && util.DoesNetworkRequireTunnelIDs(na.netInfo) {
 		existingTunnelID, err := util.ParseUDNLayer2NodeGRLRPTunnelIDs(node, networkName)
 		if err != nil && !util.IsAnnotationNotSetError(err) {
 			return fmt.Errorf("failed to fetch tunnelID annotation from the node %s for network %s, err: %v",
 				node.Name, networkName, err)
 		}
-		if existingTunnelID == util.InvalidID {
+		if existingTunnelID == types.InvalidID {
 			if newTunnelID, err = na.idAllocator.AllocateID(networkName + "_" + node.Name); err != nil {
 				return fmt.Errorf("failed to assign node %s tunnel id for network %s: %w", node.Name, networkName, err)
 			}
@@ -340,22 +345,24 @@ func (na *NodeAllocator) syncNodeNetworkAnnotations(node *corev1.Node) error {
 		}
 	}
 
-	// only update node annotation with ID if it had it before and does not match
-	// NoID means it will not update
-	// InvalidID means node did not have the annotation
-	annoUpdateID := util.NoID
-	if networkID != util.InvalidID && networkID != na.networkID {
-		annoUpdateID = na.networkID
+	// only update node annotation with ID if it had it before (networkID is not NoNetworkID)
+	// and does not match.
+	// NoNetworkID means do not update the annotation
+	if networkID != types.NoNetworkID && networkID != na.networkID {
+		networkID = na.networkID
+	} else if networkID == na.networkID {
+		// don't need to update if there was no change to the ID
+		networkID = types.NoNetworkID
 	}
 
 	// Also update the node annotation if the networkID doesn't match
-	if len(updatedSubnetsMap) > 0 || annoUpdateID != util.NoID || len(allocatedJoinSubnets) > 0 || newTunnelID != util.NoID {
-		err = na.updateNodeNetworkAnnotationsWithRetry(node.Name, updatedSubnetsMap, annoUpdateID, newTunnelID, allocatedJoinSubnets)
+	if len(updatedSubnetsMap) > 0 || networkID != types.NoNetworkID || len(allocatedJoinSubnets) > 0 || newTunnelID != types.NoTunnelID {
+		err = na.updateNodeNetworkAnnotationsWithRetry(node.Name, updatedSubnetsMap, networkID, newTunnelID, allocatedJoinSubnets)
 		if err != nil {
 			if errR := na.clusterSubnetAllocator.ReleaseNetworks(node.Name, allocatedSubnets...); errR != nil {
 				klog.Warningf("Error releasing node %s subnets: %v", node.Name, errR)
 			}
-			if newTunnelID != util.NoID {
+			if newTunnelID != types.NoTunnelID {
 				na.idAllocator.ReleaseID(networkName + "_" + node.Name)
 				klog.Infof("Releasing node %s tunnelID for network %s since annotation update failed", node.Name, networkName)
 			}
@@ -453,14 +460,14 @@ func (na *NodeAllocator) updateNodeNetworkAnnotationsWithRetry(nodeName string,
 			return fmt.Errorf("failed to update node %q annotation LRPAddrAnnotation %s: %w",
 				node.Name, util.JoinIPNets(joinAddr, ","), err)
 		}
-		if networkId != util.NoID {
+		if networkId != types.NoNetworkID {
 			cnode.Annotations, err = util.UpdateNetworkIDAnnotation(cnode.Annotations, networkName, networkId)
 			if err != nil {
 				return fmt.Errorf("failed to update node %q network id annotation %d for network %s: %w",
 					node.Name, networkId, networkName, err)
 			}
 		}
-		if tunnelID != util.NoID {
+		if tunnelID != types.NoTunnelID {
 			cnode.Annotations, err = util.UpdateUDNLayer2NodeGRLRPTunnelIDs(cnode.Annotations, networkName, tunnelID)
 			if err != nil {
 				return fmt.Errorf("failed to update node %q tunnel id annotation %d for network %s: %w",
@@ -497,7 +504,7 @@ func (na *NodeAllocator) Cleanup() error {
 
 		hostSubnetsMap := map[string][]*net.IPNet{networkName: nil}
 		// passing util.InvalidID deletes the network/tunnel id annotation for the network.
-		err = na.updateNodeNetworkAnnotationsWithRetry(node.Name, hostSubnetsMap, util.InvalidID, util.InvalidID, nil)
+		err = na.updateNodeNetworkAnnotationsWithRetry(node.Name, hostSubnetsMap, types.InvalidID, types.InvalidID, nil)
 		if err != nil {
 			return fmt.Errorf("failed to clear node %q subnet annotation for network %s",
 				node.Name, networkName)

go-controller/pkg/networkmanager/api.go

@@ -13,9 +13,6 @@ import (
 var ErrNetworkControllerTopologyNotManaged = errors.New("no cluster network controller to manage topology")
 
 const (
-	// DefaultNetworkID is the default network.
-	DefaultNetworkID = 0
-
 	// MaxNetworks is the maximum number of networks allowed.
 	MaxNetworks = 4096
 )

go-controller/pkg/networkmanager/nad_controller.go

@@ -104,7 +104,7 @@ func newController(
 	if zone == "" && node == "" {
 		c.networkIDAllocator = id.NewIDAllocator("NetworkIDs", MaxNetworks)
 		// Reserve the ID of the default network
-		err := c.networkIDAllocator.ReserveID(types.DefaultNetworkName, DefaultNetworkID)
+		err := c.networkIDAllocator.ReserveID(types.DefaultNetworkName, types.DefaultNetworkID)
 		if err != nil {
 			return nil, fmt.Errorf("failed to allocate default network ID: %w", err)
 		}
@@ -359,7 +359,7 @@ func (c *nadController) syncNAD(key string, nad *nettypes.NetworkAttachmentDefin
 	// if network ID has not been set and this is not the well known default
 	// network, need to wait until cluster nad controller allocates an ID for
 	// the network
-	if ensureNetwork.GetNetworkID() == util.InvalidID {
+	if ensureNetwork.GetNetworkID() == types.InvalidID {
 		klog.V(4).Infof("%s: will wait for cluster manager to allocate an ID before ensuring network %s", c.name, nadNetworkName)
 		return nil
 	}
@@ -553,7 +553,7 @@ func (c *nadController) handleNetworkID(old util.NetInfo, new util.MutableNetInf
 	}
 
 	var err error
-	id := util.InvalidID
+	id := types.InvalidID
 
 	// check what ID is currently annotated
 	if nad != nil && nad.Annotations[types.OvnNetworkIDAnnotation] != "" {
@@ -585,16 +585,16 @@ func (c *nadController) handleNetworkID(old util.NetInfo, new util.MutableNetInf
 	name := new.GetNetworkName()
 
 	// an ID was annotated, check if it is free to use or stale
-	if id != util.InvalidID {
+	if id != types.InvalidID {
 		err = c.networkIDAllocator.ReserveID(name, id)
 		if err != nil {
 			// already reserved for a different network, allocate a new id
-			id = util.InvalidID
+			id = types.InvalidID
 		}
 	}
 
 	// we don't have an ID, allocate a new one
-	if id == util.InvalidID {
+	if id == types.InvalidID {
 		id, err = c.networkIDAllocator.AllocateID(name)
 		if err != nil {
 			return fmt.Errorf("failed to allocate network ID: %w", err)

go-controller/pkg/networkmanager/nad_controller_test.go

@@ -492,7 +492,7 @@ func TestNADController(t *testing.T) {
 				nadClient:          fakeClient.NetworkAttchDefClient,
 				namespaceLister:    &fakeNamespaceLister{},
 			}
-			err = nadController.networkIDAllocator.ReserveID(types.DefaultNetworkName, DefaultNetworkID)
+			err = nadController.networkIDAllocator.ReserveID(types.DefaultNetworkName, types.DefaultNetworkID)
 			g.Expect(err).ToNot(gomega.HaveOccurred())
 			netController := nadController.networkController
 
@@ -749,7 +749,7 @@ func TestSyncAll(t *testing.T) {
 			for name, network := range expectedNetworks {
 				g.Expect(actualNetworks).To(gomega.HaveKey(name))
 				g.Expect(util.AreNetworksCompatible(actualNetworks[name], network)).To(gomega.BeTrue())
-				if network.GetNetworkID() != util.InvalidID {
+				if network.GetNetworkID() != types.InvalidID {
 					g.Expect(actualNetworks[name].GetNetworkID()).To(gomega.Equal(network.GetNetworkID()))
 				}
 			}

go-controller/pkg/networkmanager/network_controller.go

@@ -532,7 +532,7 @@ func nodeNeedsUpdate(oldNode, newNode *corev1.Node) bool {
 }
 
 func (c *networkController) getRunningNetwork(id int) string {
-	if id == DefaultNetworkID {
+	if id == types.DefaultNetworkID {
 		return types.DefaultNetworkName
 	}
 	for _, state := range c.getAllNetworkStates() {

go-controller/pkg/types/const.go

@@ -264,4 +264,17 @@ const (
 	// entry for the gateway routers. After this time, the entry is removed and
 	// may be refreshed with a new ARP request.
 	GRMACBindingAgeThreshold = "300"
+
+	// InvalidID signifies an invalid ID. Currently used for network and tunnel IDs.
+	InvalidID = -1
+
+	// NoTunnelID signifies an empty/unset ID. Currently used for tunnel ID (reserved as un-usable when the allocator is created)
+	NoTunnelID = 0
+
+	// DefaultNetworkID is reserved for the default network only
+	DefaultNetworkID = 0
+
+	// NoNetworkID is used to signal internally that an ID is empty and should, updates
+	// with this value should be ignored
+	NoNetworkID = -2
 )

go-controller/pkg/util/multi_network.go

@@ -892,7 +892,7 @@ func newLayer3NetConfInfo(netconf *ovncnitypes.NetConf) (MutableNetInfo, error)
 		joinSubnets:    joinSubnets,
 		mtu:            netconf.MTU,
 		mutableNetInfo: mutableNetInfo{
-			id:   InvalidID,
+			id:   types.InvalidID,
 			nads: sets.Set[string]{},
 		},
 	}
@@ -919,7 +919,7 @@ func newLayer2NetConfInfo(netconf *ovncnitypes.NetConf) (MutableNetInfo, error)
 		mtu:                netconf.MTU,
 		allowPersistentIPs: netconf.AllowPersistentIPs,
 		mutableNetInfo: mutableNetInfo{
-			id:   InvalidID,
+			id:   types.InvalidID,
 			nads: sets.Set[string]{},
 		},
 	}
@@ -943,7 +943,7 @@ func newLocalnetNetConfInfo(netconf *ovncnitypes.NetConf) (MutableNetInfo, error
 		allowPersistentIPs:  netconf.AllowPersistentIPs,
 		physicalNetworkName: netconf.PhysicalNetworkName,
 		mutableNetInfo: mutableNetInfo{
-			id:   InvalidID,
+			id:   types.InvalidID,
 			nads: sets.Set[string]{},
 		},
 	}
@@ -1123,13 +1123,13 @@ func ParseNADInfo(nad *nettypes.NetworkAttachmentDefinition) (NetInfo, error) {
 		return nil, err
 	}
 
-	id := InvalidID
+	id := types.InvalidID
 	n, err := newNetInfo(netconf)
 	if err != nil {
 		return nil, err
 	}
 	if n.GetNetworkName() == types.DefaultNetworkName {
-		id = NoID
+		id = types.DefaultNetworkID
 	}
 	if nad.Annotations[types.OvnNetworkIDAnnotation] != "" {
 		annotated := nad.Annotations[types.OvnNetworkIDAnnotation]

go-controller/pkg/util/node_annotations.go

@@ -151,13 +151,6 @@ const (
 	//		"l2-network-b":"10"}
 	// }",
 	ovnUDNLayer2NodeGRLRPTunnelIDs = "k8s.ovn.org/udn-layer2-node-gateway-router-lrp-tunnel-ids"
-
-	// InvalidID signifies an invalid ID. Currently used for network and tunnel IDs.
-	InvalidID = -1
-
-	// NoID signifies an empty/unset ID. Currently used for tunnel ID (reserved as un-usable when the allocator is created)
-	// and network ID (reserved as default network ID).
-	NoID = 0
 )
 
 type L3GatewayConfig struct {
@@ -485,12 +478,12 @@ func HasUDNLayer2NodeGRLRPTunnelID(node *corev1.Node, netName string) bool {
 func ParseUDNLayer2NodeGRLRPTunnelIDs(node *corev1.Node, netName string) (int, error) {
 	tunnelIDsMap, err := parseNetworkMapAnnotation(node.Annotations, ovnUDNLayer2NodeGRLRPTunnelIDs)
 	if err != nil {
-		return InvalidID, err
+		return types.InvalidID, err
 	}
 
 	tunnelID, ok := tunnelIDsMap[netName]
 	if !ok {
-		return InvalidID, newAnnotationNotSetError("node %q has no %q annotation for network %s", node.Name, ovnUDNLayer2NodeGRLRPTunnelIDs, netName)
+		return types.InvalidID, newAnnotationNotSetError("node %q has no %q annotation for network %s", node.Name, ovnUDNLayer2NodeGRLRPTunnelIDs, netName)
 	}
 
 	return strconv.Atoi(tunnelID)
@@ -1363,12 +1356,12 @@ func parseNetworkMapAnnotation(nodeAnnotations map[string]string, annotationName
 func ParseNetworkIDAnnotation(node *corev1.Node, netName string) (int, error) {
 	networkIDsMap, err := parseNetworkMapAnnotation(node.Annotations, ovnNetworkIDs)
 	if err != nil {
-		return InvalidID, err
+		return types.InvalidID, err
 	}
 
 	networkID, ok := networkIDsMap[netName]
 	if !ok {
-		return InvalidID, newAnnotationNotSetError("node %q has no %q annotation for network %s", node.Name, ovnNetworkIDs, netName)
+		return types.InvalidID, newAnnotationNotSetError("node %q has no %q annotation for network %s", node.Name, ovnNetworkIDs, netName)
 	}
 
 	return strconv.Atoi(networkID)
@@ -1393,7 +1386,7 @@ func updateNetworkAnnotation(annotations map[string]string, netName string, id i
 	}
 
 	// add or delete network id of the specified network
-	if id == InvalidID {
+	if id == types.InvalidID {
 		delete(idsMap, netName)
 	} else {
 		idsMap[netName] = strconv.Itoa(id)
@@ -1481,23 +1474,3 @@ func filterIPVersion(cidrs []netip.Prefix, v6 bool) []netip.Prefix {
 	}
 	return validCIDRs
 }
-
-// GetNetworkID will retrieve the network id for the specified network from the
-// first node that contains that network at the network id annotations, it will
-// return at the first ocurrence, rest of nodes will not be parsed.
-func GetNetworkID(nodes []*corev1.Node, nInfo NetInfo) (int, error) {
-	for _, node := range nodes {
-		var err error
-		networkID, err := ParseNetworkIDAnnotation(node, nInfo.GetNetworkName())
-		if err != nil {
-			if IsAnnotationNotSetError(err) {
-				continue
-			}
-			return InvalidID, err
-		}
-		if networkID != InvalidID {
-			return networkID, nil
-		}
-	}
-	return InvalidID, fmt.Errorf("missing network id for network '%s'", nInfo.GetNetworkName())
-}