Merge pull request #18225 from nabahr/autorp-joins

pim: Fix autorp group joins

pimd/pim_autorp.c

@@ -967,6 +967,7 @@ err:
 static bool pim_autorp_socket_enable(struct pim_autorp *autorp)
 {
 	int fd;
+	struct interface *ifp;
 
 	/* Return early if socket is already enabled */
 	if (autorp->sock != -1)
@@ -990,6 +991,11 @@ static bool pim_autorp_socket_enable(struct pim_autorp *autorp)
 
 	autorp->sock = fd;
 
+	/* Join autorp groups on all pim enabled interfaces in the VRF */
+	FOR_ALL_INTERFACES (autorp->pim->vrf, ifp) {
+		pim_autorp_add_ifp(ifp);
+	}
+
 	if (PIM_DEBUG_AUTORP)
 		zlog_debug("%s: AutoRP socket enabled (fd=%u)", __func__, fd);
 
@@ -1002,6 +1008,7 @@ static bool pim_autorp_socket_disable(struct pim_autorp *autorp)
 	if (autorp->sock == -1)
 		return true;
 
+	/* No need to leave the autorp groups explicitly, they are left when the socket is closed */
 	if (close(autorp->sock)) {
 		zlog_warn("Failure closing autorp socket: fd=%d errno=%d: %s", autorp->sock, errno,
 			  safe_strerror(errno));
@@ -1428,10 +1435,10 @@ void pim_autorp_add_ifp(struct interface *ifp)
 {
 	/* Add a new interface for autorp
 	 *   When autorp is enabled, we must join the autorp groups on all
-	 *   pim/multicast interfaces. When autorp first starts, if finds all
+	 *   pim/multicast interfaces. When autorp becomes enabled, it finds all
-	 *   current multicast interfaces and joins on them. If a new interface
+	 *   current pim enabled interfaces and joins the autorp groups on them.
-	 *   comes up or is configured for multicast after autorp is running, then
+	 *   Any new interfaces added after autorp is enabled will use this function
-	 *   this method will add it for autorp->
+	 *   to join the autorp groups
 	 * This is called even when adding a new pim interface that is not yet
 	 * active, so make sure the check, it'll call in again once the interface is up.
 	 */
@@ -1441,7 +1448,8 @@ void pim_autorp_add_ifp(struct interface *ifp)
 	pim_ifp = ifp->info;
 	if (CHECK_FLAG(ifp->status, ZEBRA_INTERFACE_ACTIVE) && pim_ifp && pim_ifp->pim_enable) {
 		pim = pim_ifp->pim;
-		if (pim && pim->autorp && pim->autorp->do_discovery) {
+		if (pim && pim->autorp &&
+		    (pim->autorp->do_discovery || pim->autorp->send_rp_discovery)) {
 			if (PIM_DEBUG_AUTORP)
 				zlog_debug("%s: Adding interface %s to AutoRP, joining AutoRP groups",
 					   __func__, ifp->name);
@@ -1477,44 +1485,37 @@ void pim_autorp_rm_ifp(struct interface *ifp)
 
 void pim_autorp_start_discovery(struct pim_instance *pim)
 {
-	struct interface *ifp;
 	struct pim_autorp *autorp = pim->autorp;
 
+	if (autorp->do_discovery)
+		return;
+
+	autorp->do_discovery = true;
+
 	/* Make sure the socket is open and ready */
 	if (!pim_autorp_socket_enable(autorp)) {
 		zlog_err("%s: AutoRP failed to open socket", __func__);
 		return;
 	}
 
-	if (!autorp->do_discovery) {
+	autorp_read_on(autorp);
-		autorp->do_discovery = true;
-		autorp_read_on(autorp);
 
-		FOR_ALL_INTERFACES (autorp->pim->vrf, ifp) {
+	if (PIM_DEBUG_AUTORP)
-			pim_autorp_add_ifp(ifp);
+		zlog_debug("%s: AutoRP Discovery started", __func__);
-		}
-
-		if (PIM_DEBUG_AUTORP)
-			zlog_debug("%s: AutoRP Discovery started", __func__);
-	}
 }
 
 void pim_autorp_stop_discovery(struct pim_instance *pim)
 {
-	struct interface *ifp;
 	struct pim_autorp *autorp = pim->autorp;
 
-	if (autorp->do_discovery) {
+	if (!autorp->do_discovery)
-		FOR_ALL_INTERFACES (autorp->pim->vrf, ifp) {
+		return;
-			pim_autorp_rm_ifp(ifp);
-		}
 
-		autorp->do_discovery = false;
+	autorp->do_discovery = false;
-		autorp_read_off(autorp);
+	autorp_read_off(autorp);
 
-		if (PIM_DEBUG_AUTORP)
+	if (PIM_DEBUG_AUTORP)
-			zlog_debug("%s: AutoRP Discovery stopped", __func__);
+		zlog_debug("%s: AutoRP Discovery stopped", __func__);
-	}
 
 	/* Close the socket if we need to */
 	if (pim_autorp_should_close(autorp) && !pim_autorp_socket_disable(autorp))

pimd/pim_iface.c

@@ -1901,9 +1901,7 @@ static int pim_ifp_up(struct interface *ifp)
 	}
 
 #if PIM_IPV == 4
-	if (pim->autorp && pim->autorp->do_discovery && pim_ifp &&
+	pim_autorp_add_ifp(ifp);
-	    pim_ifp->pim_enable)
-		pim_autorp_add_ifp(ifp);
 #endif
 
 	pim_cand_addrs_changed();
@@ -2020,8 +2018,7 @@ void pim_pim_interface_delete(struct interface *ifp)
 		return;
 
 #if PIM_IPV == 4
-	if (pim_ifp->pim_enable)
+	pim_autorp_rm_ifp(ifp);
-		pim_autorp_rm_ifp(ifp);
 #endif
 
 	pim_ifp->pim_enable = false;

tests/topotests/pim_autorp/test_pim_autorp.py

@@ -158,6 +158,103 @@ def test_pim_autorp_init(request):
         )
 
 
+def test_pim_autorp_disable_enable(request):
+    "Test PIM AutoRP disable and re-enable works properly"
+    tgen = get_topogen()
+    tc_name = request.node.name
+    write_test_header(tc_name)
+
+    if tgen.routers_have_failure():
+        pytest.skip(tgen.errors)
+
+    step("Ensure AutoRP groups are joined on all routers")
+    for rtr in ["r1", "r2", "r3", "r4"]:
+        expected = {
+            f"{rtr}-eth0": {
+                "name": f"{rtr}-eth0",
+                "224.0.1.39": "*",
+                "224.0.1.40": "*",
+            },
+            f"{rtr}-eth1": {
+                "name": f"{rtr}-eth1",
+                "224.0.1.39": "*",
+                "224.0.1.40": "*",
+            },
+        }
+
+        test_func = partial(
+            topotest.router_json_cmp,
+            tgen.gears[rtr],
+            "show ip igmp sources json",
+            expected,
+        )
+        _, result = topotest.run_and_expect(test_func, None)
+        assert result is None, "{} does not have correct autorp groups joined".format(
+            rtr
+        )
+
+    step("Disable AutoRP on all routers")
+    for rtr in ["r1", "r2", "r3", "r4"]:
+        tgen.routers()[rtr].vtysh_cmd(
+            """
+            conf
+            router pim
+              no autorp discovery
+            """
+        )
+
+    step("Ensure AutoRP groups are no longer joined on all routers")
+    for rtr in ["r1", "r2", "r3", "r4"]:
+        expected = {f"{rtr}-eth0": None, f"{rtr}-eth1": None}
+
+        test_func = partial(
+            topotest.router_json_cmp,
+            tgen.gears[rtr],
+            "show ip igmp sources json",
+            expected,
+        )
+        _, result = topotest.run_and_expect(test_func, None)
+        assert result is None, "{} does not have correct autorp groups joined".format(
+            rtr
+        )
+
+    step("Re-enable AutoRP on all routers")
+    for rtr in ["r1", "r2", "r3", "r4"]:
+        tgen.routers()[rtr].vtysh_cmd(
+            """
+            conf
+            router pim
+              autorp discovery
+            """
+        )
+
+    step("Ensure AutoRP groups are re-joined on all routers")
+    for rtr in ["r1", "r2", "r3", "r4"]:
+        expected = {
+            f"{rtr}-eth0": {
+                "name": f"{rtr}-eth0",
+                "224.0.1.39": "*",
+                "224.0.1.40": "*",
+            },
+            f"{rtr}-eth1": {
+                "name": f"{rtr}-eth1",
+                "224.0.1.39": "*",
+                "224.0.1.40": "*",
+            },
+        }
+
+        test_func = partial(
+            topotest.router_json_cmp,
+            tgen.gears[rtr],
+            "show ip igmp sources json",
+            expected,
+        )
+        _, result = topotest.run_and_expect(test_func, None)
+        assert result is None, "{} does not have correct autorp groups joined".format(
+            rtr
+        )
+
+
 def test_pim_autorp_no_mapping_agent_rp(request):
     "Test PIM AutoRP candidate with no mapping agent"
     tgen = get_topogen()