Remove stale LRSRs from default and network scopped cluster router #4679

arghosh93 · 2024-08-30T17:57:33Z

This PR removes logical router static routes with stale IPs when join or transit switch IP gets changed as part of day 2 operation.

ip4.dst == 10.244.0.0/24 , nexthop = 100.88.0.2 dst-ip
ip4.dst == 100.64.0.2/16 , nexthop = 100.88.0.2 dst-ip
ip4.dst == 10.244.0.0/16 , nexthop = 100.64.0.4 src-ip
ip4.dst == 100.64.0.4 , nexthop = 100.64.0.4 dst-ip
ip4.dst == 10.244.2.0/24 , nexthop = 100.64.0.4 src-ip

What this PR does and why is it needed

Which issue(s) this PR fixes

Jira: https://issues.redhat.com/browse/OCPBUGS-36177
Fixes #4693 additionally.

Special notes for reviewers

How to verify it

Details to documentation updates

Description for the changelog

Does this PR introduce a user-facing change?

This commit is to effectively filter LRSR created for default cluster router and prevent deletion of any other required routes. Previous predicate was causing 2 LRSRs to be selected while execution of CreateOrReplaceLogicalRouterStaticRouteWithPredicate function and was causing one of the routes to be deleted upon ovnkube-controller restart. 10.244.0.0/16 100.64.0.4 src-ip 10.244.2.0/24 100.64.0.4 src-ip Upstream issue: ovn-kubernetes#4693 Signed-off-by: Arnab Ghosh <[email protected]>

This PR removes logical router static routes with stale IPs when join or transit switch IP gets changed as part of day 2 operation. ip4.dst == 10.244.0.0/24 , nexthop = 100.88.0.2 dst-ip ip4.dst == 100.64.0.2/16 , nexthop = 100.88.0.2 dst-ip ip4.dst == 10.244.0.0/16 , nexthop = 100.64.0.4 src-ip ip4.dst == 100.64.0.4 , nexthop = 100.64.0.4 dst-ip Signed-off-by: Arnab Ghosh <[email protected]>

kyrtapz · 2024-10-07T08:03:40Z

go-controller/pkg/ovn/zone_interconnect/zone_ic_handler.go

+
+	nodeTransitSwitchPortIPs, err := util.ParseNodeTransitSwitchPortAddrs(node)
+	if err != nil || len(nodeTransitSwitchPortIPs) == 0 {
+		return fmt.Errorf("failed to get the node transit switch port Ips : %v", err)


I think it would be a better idea to aggregate the errors here instead of doing the early exit. There is still a chance to cleanup the join addr routes.

kyrtapz · 2024-10-07T08:04:56Z

go-controller/pkg/ovn/zone_interconnect/zone_ic_handler.go

+	if util.IsAnnotationNotSetError(err) {
+		var err1 error
+		// this is for backwards compatibility
+		nodeGRPIPs, err1 = util.ParseNodeGatewayRouterLRPAddrs(node)


Are you sure we still need that?

I am not sure. I checked other places where we are getting join IP from annotation and in all those places we are running ParseNodeGatewayRouterLRPAddrs() if IsAnnotationNotSetError() is true.

kyrtapz · 2024-10-07T08:06:35Z

go-controller/pkg/ovn/zone_interconnect/zone_ic_handler.go

+			return lrsr.ExternalIDs["ic-node"] == node.Name && lrsr.ExternalIDs[types.NetworkExternalID] == zic.GetNetworkName() &&
+				(lrsr.Nexthop != v6TransitSwitchIP.IP.String() || (nodeSubnet != nil && lrsr.IPPrefix != nodeSubnet.String() &&
+					lrsr.IPPrefix != v6JoinIP.IP.String()+util.GetIPFullMaskString(v6JoinIP.IP.String())))
+		} else if !utilnet.IsIPv6String(lrsr.Nexthop) {


NIT: Use utilnet.IsIPv4String instead

kyrtapz · 2024-10-07T08:06:56Z

go-controller/pkg/ovn/zone_interconnect/zone_ic_handler.go

+			return lrsr.ExternalIDs["ic-node"] == node.Name && lrsr.ExternalIDs[types.NetworkExternalID] == zic.GetNetworkName() &&
+				(lrsr.Nexthop != v6TransitSwitchIP.IP.String() || (nodeSubnet != nil && lrsr.IPPrefix != nodeSubnet.String() &&
+					lrsr.IPPrefix != v6JoinIP.IP.String()+util.GetIPFullMaskString(v6JoinIP.IP.String())))
+		} else if !utilnet.IsIPv6String(lrsr.Nexthop) {


There is no need for the else if since the first if returns.

kyrtapz · 2024-10-07T08:08:01Z

go-controller/pkg/ovn/zone_interconnect/zone_ic_handler.go

+		return false
+	}
+	err = libovsdbops.DeleteLogicalRouterStaticRoutesWithPredicate(zic.nbClient, zic.networkClusterRouterName, p)
+	if err != nil && !errors.Is(err, libovsdbclient.ErrNotFound) {


I don't think that DeleteLogicalRouterStaticRoutesWithPredicate returns libovsdbclient.ErrNotFound if there was nothing to delete.

yaa, you are right , ErrNotFound is set to false at operationModel.

kyrtapz · 2024-10-07T08:13:38Z

go-controller/pkg/ovn/gateway.go

+			if !config.OVNKubernetesFeature.EnableInterconnect {
+				lrsr.ExternalIDs["nodeName"] = nodeName
+			}
+		} else if !config.OVNKubernetesFeature.EnableInterconnect {


Was it intentional that you override all ExternalIDs in case of non-secondary networks?
Otherwise, You could just extract the if and do it once:

if gw.netInfo.IsSecondary() { ... } ... if !config.OVNKubernetesFeature.EnableInterconnect { }

kyrtapz · 2024-10-07T08:20:13Z

go-controller/pkg/ovn/zone_interconnect/zone_ic_handler.go

@@ -633,7 +636,8 @@ func (zic *ZoneInterconnectHandler) addRemoteNodeStaticRoutes(node *corev1.Node,
 	addRoute := func(prefix, nexthop string) error {
 		logicalRouterStaticRoute := nbdb.LogicalRouterStaticRoute{
 			ExternalIDs: map[string]string{
-				"ic-node": node.Name,
+				"ic-node":               node.Name,
+				types.NetworkExternalID: zic.GetNetworkName(),


IIRC we usually also add the types.TopologyExternalID, it wouldn't hurt to have it here also, especially if we allow same network names on different topologies.

kyrtapz · 2024-10-07T08:21:16Z

go-controller/pkg/ovn/zone_interconnect/zone_ic_handler.go

+	}
+	p := func(lrsr *nbdb.LogicalRouterStaticRoute) bool {
+		if utilnet.IsIPv6String(lrsr.Nexthop) {
+			nodeSubnet, _ := util.MatchFirstIPNetFamily(true, nodeSubnets)


I don't think we should do the the check if err != nil

kyrtapz · 2024-10-07T08:25:25Z

go-controller/pkg/ovn/gateway.go

+		if lrsr.Nexthop != gwLRPIP && utilnet.IPFamilyOfString(lrsr.Nexthop) == utilnet.IPFamilyOfString(gwLRPIP) &&
+			lrsr.Nexthop == lrsr.IPPrefix {
+			networkName, isSecondaryNetwork := lrsr.ExternalIDs[types.NetworkExternalID]
+			if isSecondaryNetwork && networkName == gw.netInfo.GetNetworkName() {


This nested if/elseif block could be simplified.
I wonder if there is any benefit in not including the nodeName when EnableInterconnect is true, if we had it would simplify this check.

kyrtapz · 2024-10-07T08:26:17Z

go-controller/pkg/ovn/gateway.go

+	}
+
+	err := libovsdbops.DeleteLogicalRouterStaticRoutesWithPredicate(gw.nbClient, gw.clusterRouterName, p)
+	if err != nil && !errors.Is(err, libovsdbclient.ErrNotFound) {


I don't think that DeleteLogicalRouterStaticRoutesWithPredicate returns libovsdbclient.ErrNotFound

arghosh93 requested a review from a team as a code owner August 30, 2024 17:57

arghosh93 requested a review from jcaamano August 30, 2024 17:57

github-actions bot added the area/unit-testing Issues related to adding/updating unit tests label Aug 30, 2024

arghosh93 force-pushed the delete_stale_logical_route branch from 3b302b6 to cd269e4 Compare August 30, 2024 18:13

tssurya added the area/core-kubernetes Issues related to core kubernetes like pods, services, endpoints, endpointslices label Aug 31, 2024

arghosh93 force-pushed the delete_stale_logical_route branch from cd269e4 to 661da18 Compare September 2, 2024 15:41

arghosh93 force-pushed the delete_stale_logical_route branch from 661da18 to 263bd30 Compare September 11, 2024 08:54

arghosh93 added 2 commits September 12, 2024 22:50

arghosh93 force-pushed the delete_stale_logical_route branch from 263bd30 to fe9519f Compare September 12, 2024 17:23

tssurya requested review from tssurya and removed request for jcaamano October 1, 2024 15:51

kyrtapz reviewed Oct 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove stale LRSRs from default and network scopped cluster router #4679

Remove stale LRSRs from default and network scopped cluster router #4679

arghosh93 commented Aug 30, 2024 •

edited

Loading

kyrtapz Oct 7, 2024

kyrtapz Oct 7, 2024

arghosh93 Oct 7, 2024

kyrtapz Oct 7, 2024

kyrtapz Oct 7, 2024

kyrtapz Oct 7, 2024

arghosh93 Oct 7, 2024

kyrtapz Oct 7, 2024

kyrtapz Oct 7, 2024

kyrtapz Oct 7, 2024

kyrtapz Oct 7, 2024

kyrtapz Oct 7, 2024

Remove stale LRSRs from default and network scopped cluster router #4679

Are you sure you want to change the base?

Remove stale LRSRs from default and network scopped cluster router #4679

Conversation

arghosh93 commented Aug 30, 2024 • edited Loading

What this PR does and why is it needed

Which issue(s) this PR fixes

Special notes for reviewers

How to verify it

Details to documentation updates

Description for the changelog

Does this PR introduce a user-facing change?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arghosh93 commented Aug 30, 2024 •

edited

Loading