address comments

Signed-off-by: Zhecheng Li <[email protected]>

pkg/agent/controller/noderoute/node_route_controller.go

@@ -408,13 +408,15 @@ func (c *Controller) deleteNodeRoute(nodeName string) error {
 }
 
 func (c *Controller) addNodeRoute(nodeName string, node *corev1.Node) error {
+	// It is only for Windows Noencap mode to get Node MAC.
 	peerNodeMAC, err := getNodeMAC(node)
 	if err != nil {
 		return fmt.Errorf("error when retrieving MAC of Node %s: %v", nodeName, err)
 	}
 
 	nrInfo, installed, _ := c.installedNodes.GetByKey(nodeName)
-	if installed && nrInfo != nil && nrInfo.(*nodeRouteInfo).nodeMAC != nil && peerNodeMAC != nil && nrInfo.(*nodeRouteInfo).nodeMAC.String() == peerNodeMAC.String() {
+
+	if installed && nrInfo.(*nodeRouteInfo).nodeMAC.String() == peerNodeMAC.String() {
 		// Route is already added for this Node and Node MAC isn't changed.
 		return nil
 	}
@@ -450,7 +452,8 @@ func (c *Controller) addNodeRoute(nodeName string, node *corev1.Node) error {
 		// event to be processed before proceeding, or the route installation and uninstallation operations may override or
 		// conflict with each other.
 		// For Windows Noencap case, it is possible that nodesHaveSamePodCIDR is the Node itself because the Node
-		// MAC annotation is not set yet.
+		// MAC annotation was not set yet when the Node was initially installed. Then it is processed for the second
+		// time when its MAC annotation is updated.
 		if len(nodesHaveSamePodCIDR) > 0 && (len(nodesHaveSamePodCIDR) != 1 || nodesHaveSamePodCIDR[0] != nodeName) {
 			// Return an error so that the Node will be put back to the workqueue and will be retried later.
 			return fmt.Errorf("skipping addNodeRoute for Node %s because podCIDR %s is duplicate with Node %s, will retry later", nodeName, podCIDR, nodesHaveSamePodCIDR[0])
@@ -666,15 +669,15 @@ func (c *Controller) IPInPodSubnets(ip net.IP) bool {
 	return len(nodeInCluster) > 0 || ipCIDRStr == curNodeCIDRStr
 }
 
-// getNodeMAC gets Node's br-int MAC from its annotation. It is for Windows Noencap mode only.
+// getNodeMAC gets Node's br-int MAC from its annotation. It is only for Windows Noencap mode.
 func getNodeMAC(node *corev1.Node) (net.HardwareAddr, error) {
 	macStr := node.Annotations[types.NodeMACAddressAnnotationKey]
 	if macStr == "" {
 		return nil, nil
 	}
 	mac, err := net.ParseMAC(macStr)
 	if err != nil {
-		return nil, fmt.Errorf("failed to parse MAC `%s`, %v", macStr, err)
+		return nil, fmt.Errorf("failed to parse MAC `%s`: %v", macStr, err)
 	}
 	return mac, nil
 }

pkg/agent/openflow/client.go

@@ -313,7 +313,7 @@ func (c *client) addFlows(cache *flowCategoryCache, flowCacheKey string, flows [
 	return nil
 }
 
-// modifyFlows set the flows of flowCategoryCache be exactly same as the provided slice for the given flowCacheKey.
+// modifyFlows sets the flows of flowCategoryCache be exactly same as the provided slice for the given flowCacheKey.
 func (c *client) modifyFlows(cache *flowCategoryCache, flowCacheKey string, flows []binding.Flow) error {
 	oldFlowCacheI, ok := cache.Load(flowCacheKey)
 	fCache := flowCache{}
@@ -406,6 +406,8 @@ func (c *client) InstallNodeFlows(hostname string,
 		flows = append(flows, c.tunnelClassifierFlow(ipsecTunOFPort, cookie.Node))
 	}
 
+	// For Windows Noencap Mode, the OVS flows for Node need be be exactly same as the provided 'flows' slice because
+	// the Node flows may be processed more than once if the MAC annotation is updated.
 	return c.modifyFlows(c.nodeFlowCache, hostname, flows)
 }
 

pkg/agent/route/route_windows.go

@@ -97,10 +97,6 @@ func (c *Client) Reconcile(podCIDRs []string) error {
 			c.hostRoutes.Store(dst, rt)
 			continue
 		}
-		// If the route is not for uplink interface, ignore it.
-		if c.nodeConfig.UplinkNetConfig != nil && rt.LinkIndex != c.nodeConfig.UplinkNetConfig.Index {
-			continue
-		}
 		err := c.nr.RemoveNetRoute(rt.LinkIndex, rt.DestinationSubnet, rt.GatewayAddress)
 		if err != nil {
 			return err
@@ -195,7 +191,7 @@ func (c *Client) listRoutes() (map[string]*netroute.Route, error) {
 	rtMap := make(map[string]*netroute.Route)
 	for idx := range routes {
 		rt := routes[idx]
-		if rt.LinkIndex != c.nodeConfig.GatewayConfig.LinkIndex && rt.LinkIndex != c.bridgeInfIndex {
+		if rt.LinkIndex != c.nodeConfig.GatewayConfig.LinkIndex {
 			continue
 		}
 		// Only process IPv4 route entries in the loop.