Merge pull request #22117 from Jafaral/pim-bsr-fixes

pimd: BSR/C-RP fixes with expanded topotest coverage

Author: rzalamena

pimd/pim_bsm.c

@@ -300,6 +300,12 @@ void pim_bsm_proc_init(struct pim_instance *pim)
 	cand_rp_groups_init(scope->cand_rp_groups);
 
 	scope->unicast_sock = pim_socket_raw(IPPROTO_PIM);
+	if (scope->unicast_sock < 0) {
+		zlog_warn("%s: Could not create BSR unicast socket: %s",
+			  __func__, safe_strerror(errno));
+		return;
+	}
+
 	set_nonblocking(scope->unicast_sock);
 	sockopt_reuseaddr(scope->unicast_sock);
 
@@ -320,8 +326,12 @@ void pim_bsm_proc_free(struct pim_instance *pim)
 	struct bsgrp_node *bsgrp;
 	struct cand_rp_group *crpgrp;
 
+	pim_crp_db_clear(scope);
+
 	event_cancel(&scope->unicast_read);
-	close(scope->unicast_sock);
+	if (scope->unicast_sock >= 0)
+		close(scope->unicast_sock);
+	scope->unicast_sock = -1;
 
 	pim_bs_timer_stop(scope);
 	pim_bsm_frags_free(scope);

pimd/pim_bsm.h

@@ -395,8 +395,4 @@ int pim_cand_config_write(struct pim_instance *pim, struct vty *vty);
 
 DECLARE_MTYPE(PIM_BSM_FRAG);
 
-DECLARE_MTYPE(PIM_BSM_FRAG);
-
-DECLARE_MTYPE(PIM_BSM_FRAG);
-
 #endif

pimd/pim_bsr_rpdb.c

@@ -466,15 +466,15 @@ int pim_crp_process(struct interface *ifp, pim_sgaddr *src_dst, uint8_t *buf,
 		return -1;
 	}
 
-	//pim_ifp->pim_ifstat_bsm_rx++;
+	pim_ifp->pim_ifstat_bsm_rx++;
 	pim = pim_ifp->pim;
-	//pim->bsm_rcvd++;
+	pim->bsm_rcvd++;
 
 	if (!pim_ifp->bsm_enable) {
 		zlog_warn("%s: BSM not enabled on interface %s", __func__,
 			  ifp->name);
-		//pim_ifp->pim_ifstat_bsm_cfg_miss++;
-		//pim->bsm_dropped++;
+		pim_ifp->pim_ifstat_bsm_cfg_miss++;
+		pim->bsm_dropped++;
 		return -1;
 	}
 
@@ -510,7 +510,7 @@ int pim_crp_process(struct interface *ifp, pim_sgaddr *src_dst, uint8_t *buf,
 	ngroups = crp_hdr->prefix_cnt;
 	rpaddr = crp_hdr->rp_addr.addr;
 
-	if (remain < ngroups * sizeof(struct pim_encoded_group_ipv4)) {
+	if (remain < ngroups * sizeof(pim_encoded_group)) {
 		if (PIM_DEBUG_BSM)
 			zlog_debug("truncated Candidate-RP advertisement for RP %pPA from %pPA (too short for %zu groups)",
 				   &rpaddr, &src_dst->src, ngroups);

pimd/pim_cmd_common.c

@@ -5791,12 +5791,9 @@ int pim_show_bsr_cand_bsr(const struct vrf *vrf, struct vty *vty, bool uj)
 	}
 
 	if (uj) {
-		char buf[INET_ADDRSTRLEN];
-
 		jsondata = json_object_new_object();
-		inet_ntop(AF_INET, &scope->bsr_addrsel.run_addr, buf,
-			  sizeof(buf));
-		json_object_string_add(jsondata, "address", buf);
+		json_object_string_addf(jsondata, "address", "%pPA",
+					&scope->bsr_addrsel.run_addr);
 		json_object_int_add(jsondata, "priority", scope->cand_bsr_prio);
 		json_object_boolean_add(jsondata, "elected",
 					pim->global_scope.state == BSR_ELECTED);

pimd/pim_nb_config.c

@@ -4332,8 +4332,7 @@ int routing_control_plane_protocols_control_plane_protocol_pim_address_family_rp
 
 		scope->cand_bsr_prio = yang_dnode_get_uint8(args->dnode, NULL);
 
-		/* FIXME: force prio update */
-		candidate_bsr_addrsel(scope, args->dnode);
+		pim_cand_bsr_apply(scope);
 		break;
 	}
 

pimd/pim_pim.c

@@ -528,7 +528,7 @@ static void pim_sock_read(struct event *t)
 
 	pim_ifp = ifp->info;
 
-	if (pim_sock_read_helper(fd, pim_ifp->pim, true) == 0)
+	if (pim_sock_read_helper(fd, pim_ifp->pim, true) != 0)
 		++pim_ifp->pim_ifstat_hello_recvfail;
 
 	pim_sock_read_on(ifp);

tests/topotests/pim_cand_rp_bsr/test_pim_cand_rp_bsr.py

@@ -303,6 +303,50 @@ def test_pim_bsr_rp_info(request):
     assert result is True, "Testcase {} :Failed \n Error: {}".format(tc_name, result)
 
 
+def test_pim_bsr_priority_modify(request):
+    "Test PIM BSR candidate-BSR priority-only change"
+    tgen = get_topogen()
+    tc_name = request.node.name
+    write_test_header(tc_name)
+
+    if tgen.routers_have_failure():
+        pytest.skip("skipped because of router(s) failure")
+
+    r2 = tgen.gears["r2"]
+
+    step("Raise r2 candidate-BSR priority above r1")
+    r2.vtysh_cmd(
+        """
+        configure
+          router pim
+            bsr candidate-bsr priority 250
+        """
+    )
+
+    step("Verify r2 reports the updated candidate-BSR state")
+    expected = {"address": "10.0.0.2", "priority": 250, "elected": True}
+    test_func = partial(
+        topotest.router_json_cmp, r2, "show ip pim bsr candidate-bsr json", expected
+    )
+    _, result = topotest.run_and_expect(test_func, None, count=60, wait=1)
+
+    assertmsg = "r2: candidate bsr priority modify mismatch"
+    assert result is None, assertmsg
+
+    step("Verify r2 is elected as the new BSR")
+    expected = {
+        "bsr": "10.0.0.2",
+        "priority": 250,
+        "state": "BSR_ELECTED",
+    }
+
+    test_func = partial(topotest.router_json_cmp, r2, "show ip pim bsr json", expected)
+    _, result = topotest.run_and_expect(test_func, None, count=180, wait=1)
+
+    assertmsg = "r2: failed to become BSR after priority increase"
+    assert result is None, assertmsg
+
+
 def test_pim_bsr_election_fallback_r2(request):
     "Test PIM BSR Election Backup"
     tgen = get_topogen()
@@ -328,16 +372,16 @@ def test_pim_bsr_election_fallback_r2(request):
     test_func = partial(
         topotest.router_json_cmp, r1, "show ip pim bsr candidate-bsr json", expected
     )
-    _, result = topotest.run_and_expect(test_func, None, count=10, wait=1)
+    _, result = topotest.run_and_expect(test_func, None, count=20, wait=3)
 
     assertmsg = "r1: failed to remove bsr candidate configuration"
     assert result is None, assertmsg
 
     r2 = tgen.gears["r2"]
-    # We should fall back to r2 as the BSR
+    # r2 became BSR earlier after its priority was raised to 250
     expected = {
         "bsr": "10.0.0.2",
-        "priority": 100,
+        "priority": 250,
         "state": "BSR_ELECTED",
     }
 
@@ -429,6 +473,28 @@ def test_pimv6_bsr_election_r1(request):
     assert result is None, assertmsg
 
 
+def test_pimv6_bsr_cand_bsr_r2(request):
+    "Test PIMv6 BSR candidate BSR JSON output"
+    tgen = get_topogen()
+    tc_name = request.node.name
+    write_test_header(tc_name)
+
+    if tgen.routers_have_failure():
+        pytest.skip("skipped because of router(s) failure")
+
+    r2 = tgen.gears["r2"]
+
+    # r2 is a candidate bsr with low priority: elected = False
+    expected = {"address": "fd00::2", "priority": 100, "elected": False}
+    test_func = partial(
+        topotest.router_json_cmp, r2, "show ipv6 pim bsr candidate-bsr json", expected
+    )
+    _, result = topotest.run_and_expect(test_func, None, count=60, wait=1)
+
+    assertmsg = "r2: IPv6 candidate bsr mismatch"
+    assert result is None, assertmsg
+
+
 def test_pimv6_bsr_cand_rp(request):
     "Test PIMv6 BSR candidate RP"
     tgen = get_topogen()