Merge pull request #18158 from chdxD1/evpn-no-withdraw-for-update

Update EVPN prefix routes properly instead of withdraw/install

bgpd/bgp_route.c

@@ -3968,14 +3968,6 @@ static void bgp_process_main_one(struct bgp *bgp, struct bgp_dest *dest,
 			|| new_select->sub_type == BGP_ROUTE_AGGREGATE
 			|| new_select->sub_type == BGP_ROUTE_IMPORTED)) {
 
-			/* if this is an evpn imported type-5 prefix,
-			 * we need to withdraw the route first to clear
-			 * the nh neigh and the RMAC entry.
-			 */
-			if (old_select &&
-			    is_route_parent_evpn(old_select))
-				bgp_zebra_withdraw_actual(dest, old_select, bgp);
-
 			bgp_zebra_route_install(dest, new_select, bgp, true,
 						NULL, false);
 		} else {

tests/topotests/bgp_evpn_rt5/test_bgp_evpn.py

@@ -9,8 +9,8 @@
 #
 
 """
- test_bgp_evpn.py: Test the FRR BGP daemon with BGP IPv6 interface
- with route advertisements on a separate netns.
+test_bgp_evpn.py: Test the FRR BGP daemon with BGP IPv6 interface
+with route advertisements on a separate netns.
 """
 
 import json
@@ -19,6 +19,7 @@ import os
 import sys
 import pytest
 import platform
+import re
 
 # Save the Current Working Directory to find configuration files.
 CWD = os.path.dirname(os.path.realpath(__file__))
@@ -307,6 +308,40 @@ def test_router_check_ip():
     assert result is None, "ipv6 route check failed"
 
 
+def _test_router_check_evpn_next_hop(expected_paths=1):
+    dut = get_topogen().gears["r2"]
+
+    # Check IPv4
+    expected = {
+        "ip": "192.168.100.21",
+        "refCount": 1,
+        "prefixList": [{"prefix": "192.168.102.21/32", "pathCount": expected_paths}],
+    }
+    test_func = partial(
+        topotest.router_json_cmp,
+        dut,
+        "show evpn next-hops vni 101 ip 192.168.100.21 json",
+        expected,
+    )
+    _, result = topotest.run_and_expect(test_func, None, count=20, wait=1)
+    assert result is None, "evpn ipv4 next-hops check failed"
+
+    # Check IPv6
+    expected = {
+        "ip": "::ffff:192.168.100.21",
+        "refCount": 1,
+        "prefixList": [{"prefix": "fd00::1/128", "pathCount": expected_paths}],
+    }
+    test_func = partial(
+        topotest.router_json_cmp,
+        dut,
+        "show evpn next-hops vni 101 ip ::ffff:192.168.100.21 json",
+        expected,
+    )
+    _, result = topotest.run_and_expect(test_func, None, count=20, wait=1)
+    assert result is None, "evpn ipv6 next-hops check failed"
+
+
 def _test_router_check_evpn_contexts(router, ipv4_only=False):
     """
     Check EVPN nexthops and RMAC number  are correctly configured
@@ -351,6 +386,7 @@ def test_router_check_evpn_contexts():
         pytest.skip(tgen.errors)
 
     _test_router_check_evpn_contexts(tgen.gears["r1"])
+    _test_router_check_evpn_next_hop()
 
 
 def test_evpn_ping():
@@ -452,6 +488,7 @@ def test_router_check_evpn_contexts_again():
         pytest.skip(tgen.errors)
 
     _test_router_check_evpn_contexts(tgen.gears["r1"], ipv4_only=True)
+    _test_router_check_evpn_next_hop()
 
 
 def test_evpn_ping_again():
@@ -465,14 +502,61 @@ def test_evpn_ping_again():
     _test_evpn_ping_router(tgen.gears["r1"], ipv4_only=True)
 
 
-def _test_wait_for_multipath_convergence(router):
+def _get_established_epoch(router, peer):
+    """
+    Get the established epoch for a peer
+    """
+    output = router.vtysh_cmd(f"show bgp neighbor {peer} json", isjson=True)
+    assert peer in output, "peer not found"
+    peer_info = output[peer]
+    assert "bgpState" in peer_info, "peer state not found"
+    assert peer_info["bgpState"] == "Established", "peer not in Established state"
+    assert "bgpTimerUpEstablishedEpoch" in peer_info, "peer epoch not found"
+    return peer_info["bgpTimerUpEstablishedEpoch"]
+
+
+def _check_established_epoch_differ(router, peer, last_established_epoch):
+    """
+    Check that the established epoch has changed
+    """
+    output = router.vtysh_cmd(f"show bgp neighbor {peer} json", isjson=True)
+    assert peer in output, "peer not found"
+    peer_info = output[peer]
+    assert "bgpState" in peer_info, "peer state not found"
+
+    if peer_info["bgpState"] != "Established":
+        return "peer not in Established state"
+
+    assert "bgpTimerUpEstablishedEpoch" in peer_info, "peer epoch not found"
+
+    if peer_info["bgpTimerUpEstablishedEpoch"] == last_established_epoch:
+        return "peer epoch not changed"
+    return None
+
+
+def _test_epoch_after_clear(router, peer, last_established_epoch):
+    """
+    Checking that the established epoch has changed and the peer is in Established state again after clear
+    Without this, the second session is cleared as well on slower systems (like CI)
+    """
+    test_func = partial(
+        _check_established_epoch_differ,
+        router,
+        peer,
+        last_established_epoch,
+    )
+    _, result = topotest.run_and_expect(test_func, None, count=20, wait=1)
+    assert (
+        result is None
+    ), "Established Epoch still the same after clear bgp for peer {}".format(peer)
+
+
+def _test_wait_for_multipath_convergence(router, expected_paths=1):
     """
     Wait for multipath convergence on R2
     """
     expected = {
-        "192.168.102.21/32": [
-            {"nexthops": [{"ip": "192.168.100.21"}, {"ip": "192.168.100.21"}]}
-        ]
+        "192.168.102.21/32": [{"nexthops": [{"ip": "192.168.100.21"}] * expected_paths}]
     }
     # Using router_json_cmp instead of verify_fib_routes, because we need to check for
     # two next-hops with the same IP address.
@@ -485,7 +569,7 @@ def _test_wait_for_multipath_convergence(router):
     _, result = topotest.run_and_expect(test_func, None, count=20, wait=1)
     assert (
         result is None
-    ), "R2 does not have two next-hops for 192.168.102.21/32 JSON output mismatches"
+    ), f"R2 does not have {expected_paths} next-hops for 192.168.102.21/32 JSON output mismatches"
 
 
 def _test_rmac_present(router):
@@ -552,14 +636,67 @@ def test_evpn_multipath():
 
     dut = tgen.gears["r2"]
     dut_peer = tgen.gears["r1"]
-    _test_wait_for_multipath_convergence(dut)
+    _test_wait_for_multipath_convergence(dut, expected_paths=2)
     _test_rmac_present(dut)
 
+    # Enable dataplane logs in FRR
+    dut.vtysh_cmd("configure terminal\ndebug zebra dplane detailed\n")
+
     for i in range(4):
         peer = "192.168.100.41" if i % 2 == 0 else "192.168.99.41"
+        local_peer = "192.168.100.21" if i % 2 == 0 else "192.168.99.21"
+
+        # Retrieving the last established epoch from the DUT to check against
+        last_established_epoch = _get_established_epoch(dut, local_peer)
+        if last_established_epoch is None:
+            assert False, "Failed to retrieve established epoch for peer {}".format(
+                peer
+            )
+
         dut_peer.vtysh_cmd("clear bgp {0}".format(peer))
-        _test_wait_for_multipath_convergence(dut)
+
+        _test_epoch_after_clear(dut, local_peer, last_established_epoch)
+        _test_wait_for_multipath_convergence(dut, expected_paths=2)
         _test_rmac_present(dut)
+        _test_router_check_evpn_next_hop(expected_paths=2)
+
+    # Check for MAC_DELETE or NEIGH_DELETE in zebra log
+    log = dut.net.getLog("log", "zebra")
+    if re.search(r"(MAC_DELETE|NEIGH_DELETE)", log):
+        assert False, "MAC_DELETE or NEIGH_DELETE found in zebra log"
+
+    dut.vtysh_cmd("configure terminal\nno debug zebra dplane detailed\n")
+
+
+def test_shutdown_multipath_check_next_hops():
+    """
+    Deconfigure a second path between R1 and R2, then check that pathCount decreases
+    """
+    tgen = get_topogen()
+    if tgen.routers_have_failure():
+        pytest.skip(tgen.errors)
+
+    shutdown_evpn_multipath = {
+        "r1": {
+            "raw_config": [
+                "router bgp 65000",
+                "neighbor 192.168.99.41 shutdown",
+            ]
+        },
+        "r2": {
+            "raw_config": [
+                "router bgp 65000",
+                "neighbor 192.168.99.21 shutdown",
+            ]
+        },
+    }
+    logger.info("==== Deconfigure second path between R1 and R2")
+    result = apply_raw_config(tgen, shutdown_evpn_multipath)
+    assert (
+        result is True
+    ), "Failed to deconfigure second path between R1 and R2, Error: {} ".format(result)
+    _test_wait_for_multipath_convergence(tgen.gears["r2"])
+    _test_router_check_evpn_next_hop()
 
 
 def test_memory_leak():

zebra/zebra_evpn_mac.h

@@ -18,6 +18,7 @@ struct host_rb_entry {
 	RB_ENTRY(host_rb_entry) hl_entry;
 
 	struct prefix p;
+	uint32_t pathcnt;
 };
 
 RB_HEAD(host_rb_tree_entry, host_rb_entry);

zebra/zebra_evpn_mh.c

@@ -3795,7 +3795,7 @@ void zebra_evpn_proc_remote_nh(ZAPI_HANDLER_ARGS)
 	memset(&dummy_prefix, 0, sizeof(dummy_prefix));
 	dummy_prefix.family = AF_EVPN;
 	dummy_prefix.prefixlen = (sizeof(struct evpn_addr) * 8);
-	dummy_prefix.prefix.route_type = 1; /* XXX - fixup to type-1 def */
+	dummy_prefix.prefix.route_type = BGP_EVPN_AD_ROUTE; /* XXX - fixup to type-1 def */
 	dummy_prefix.prefix.ead_addr.ip.ipa_type = nh.ipa_type;
 
 	if (hdr->command == ZEBRA_EVPN_REMOTE_NH_ADD) {

zebra/zebra_rib.c

@@ -2921,6 +2921,28 @@ static void process_subq_early_route_add(struct zebra_early_route *ere)
 	if (same) {
 		if (dest && same == dest->selected_fib)
 			SET_FLAG(same->status, ROUTE_ENTRY_ROUTE_REPLACING);
+
+		struct nexthop *tmp_nh;
+
+		/* Free up the evpn nhs of the re to be replaced.*/
+		for (ALL_NEXTHOPS(same->nhe->nhg, tmp_nh)) {
+			struct ipaddr vtep_ip;
+
+			if (CHECK_FLAG(tmp_nh->flags, NEXTHOP_FLAG_EVPN)) {
+				memset(&vtep_ip, 0, sizeof(struct ipaddr));
+				if (ere->afi == AFI_IP) {
+					vtep_ip.ipa_type = IPADDR_V4;
+					memcpy(&(vtep_ip.ipaddr_v4), &(tmp_nh->gate.ipv4),
+					       sizeof(struct in_addr));
+				} else {
+					vtep_ip.ipa_type = IPADDR_V6;
+					memcpy(&(vtep_ip.ipaddr_v6), &(tmp_nh->gate.ipv6),
+					       sizeof(struct in6_addr));
+				}
+				zebra_rib_queue_evpn_route_del(same->vrf_id, &vtep_ip, &ere->p);
+			}
+		}
+
 		rib_delnode(rn, same);
 	}
 

zebra/zebra_vxlan.c

@@ -355,6 +355,7 @@ static void zl3vni_print_nh(struct zebra_neigh *n, struct vty *vty,
 	char buf1[ETHER_ADDR_STRLEN];
 	char buf2[INET6_ADDRSTRLEN];
 	json_object *json_hosts = NULL;
+	json_object *json_prefix = NULL;
 	struct host_rb_entry *hle;
 
 	if (!json) {
@@ -370,7 +371,7 @@ static void zl3vni_print_nh(struct zebra_neigh *n, struct vty *vty,
 				rb_host_count(&n->host_rb));
 			vty_out(vty, "  Prefixes:\n");
 			RB_FOREACH (hle, host_rb_tree_entry, &n->host_rb)
-				vty_out(vty, "    %pFX\n", &hle->p);
+				vty_out(vty, "    %pFX (paths: %d)\n", &hle->p, hle->pathcnt);
 		}
 	} else {
 		json_hosts = json_object_new_array();
@@ -385,11 +386,13 @@ static void zl3vni_print_nh(struct zebra_neigh *n, struct vty *vty,
 		else {
 			json_object_int_add(json, "refCount",
 					    rb_host_count(&n->host_rb));
-			RB_FOREACH (hle, host_rb_tree_entry, &n->host_rb)
-				json_object_array_add(
-					json_hosts,
-					json_object_new_string(prefix2str(
-						&hle->p, buf2, sizeof(buf2))));
+			RB_FOREACH (hle, host_rb_tree_entry, &n->host_rb) {
+				json_prefix = json_object_new_object();
+				json_object_string_add(json_prefix, "prefix",
+						       prefix2str(&hle->p, buf2, sizeof(buf2)));
+				json_object_int_add(json_prefix, "pathCount", hle->pathcnt);
+				json_object_array_add(json_hosts, json_prefix);
+			}
 			json_object_object_add(json, "prefixList", json_hosts);
 		}
 	}
@@ -1143,11 +1146,23 @@ static void rb_find_or_add_host(struct host_rb_tree_entry *hrbe,
 	memcpy(&lookup.p, host, sizeof(*host));
 
 	hle = RB_FIND(host_rb_tree_entry, hrbe, &lookup);
-	if (hle)
+	if (hle) {
+		/* never pathcount evpn A-D / MH routes because zebra is not aware
+		 * of specific paths. ADD operations are considered to be upsert
+		 * leading to path count increasing without ever decreasing. A single
+		 * DEL operation should fully remove the prefix from the next-hop.
+		 */
+		if (host->family == AF_EVPN &&
+		    ((const struct prefix_evpn *)host)->prefix.route_type == BGP_EVPN_AD_ROUTE)
+			return;
+
+		hle->pathcnt++;
 		return;
+	}
 
 	hle = XCALLOC(MTYPE_HOST_PREFIX, sizeof(struct host_rb_entry));
 	memcpy(hle, &lookup, sizeof(lookup));
+	hle->pathcnt = 1;
 
 	RB_INSERT(host_rb_tree_entry, hrbe, hle);
 }
@@ -1162,8 +1177,11 @@ static void rb_delete_host(struct host_rb_tree_entry *hrbe, struct prefix *host)
 
 	hle = RB_FIND(host_rb_tree_entry, hrbe, &lookup);
 	if (hle) {
-		RB_REMOVE(host_rb_tree_entry, hrbe, hle);
-		XFREE(MTYPE_HOST_PREFIX, hle);
+		hle->pathcnt--;
+		if (hle->pathcnt == 0) {
+			RB_REMOVE(host_rb_tree_entry, hrbe, hle);
+			XFREE(MTYPE_HOST_PREFIX, hle);
+		}
 	}
 
 	return;