pim: Fix autorp group joins

Group joining got broken when moving the autorp socket to open/close
as needed. This fixes it so autorp group joining is properly handled
as part of opening the socket.

Signed-off-by: Nathan Bahr <nbahr@atcorp.com>
(cherry picked from commit d840560b74e3a6117aa1e4b1203dcdd8fb254ef6)

Fixed merge conflicts for backport

diff --git a/pimd/pim_autorp.c b/pimd/pim_autorp.c
index e06f48fdc64487e57f993348a8a9e251af6bec4e..d8dc791430fc815d5edd14478980805be0be5e19 100644 (file)
--- a/pimd/pim_autorp.c
+++ b/pimd/pim_autorp.c
@@ -495,6 +495,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)
@@ -518,6 +519,11 @@ static bool pim_autorp_socket_enable(struct pim_autorp *autorp)
                }
        }
 
+       /* 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", __func__);
 
@@ -530,6 +536,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));
@@ -926,10 +933,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
-        *   current multicast interfaces and joins on them. If a new interface
-        *   comes up or is configured for multicast after autorp is running, then
-        *   this method will add it for autorp->
+        *   pim/multicast interfaces. When autorp becomes enabled, it finds all
+        *   current pim enabled interfaces and joins the autorp groups on them.
+        *   Any new interfaces added after autorp is enabled will use this function
+        *   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.
         */
@@ -940,7 +947,8 @@ void pim_autorp_add_ifp(struct interface *ifp)
        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)) {
                        if (PIM_DEBUG_AUTORP)
                                zlog_debug("%s: Adding interface %s to AutoRP, joining AutoRP groups",
                                           __func__, ifp->name);
@@ -978,44 +986,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->do_discovery = true;
-               autorp_read_on(autorp);
-
-               FOR_ALL_INTERFACES (autorp->pim->vrf, ifp) {
-                       pim_autorp_add_ifp(ifp);
-               }
+       autorp_read_on(autorp);
 
-               if (PIM_DEBUG_AUTORP)
-                       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) {
-               FOR_ALL_INTERFACES (autorp->pim->vrf, ifp) {
-                       pim_autorp_rm_ifp(ifp);
-               }
+       if (!autorp->do_discovery)
+               return;
 
-               autorp->do_discovery = false;
-               autorp_read_off(autorp);
+       autorp->do_discovery = false;
+       autorp_read_off(autorp);
 
-               if (PIM_DEBUG_AUTORP)
-                       zlog_debug("%s: AutoRP Discovery stopped", __func__);
-       }
+       if (PIM_DEBUG_AUTORP)
+               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))

diff --git a/pimd/pim_iface.c b/pimd/pim_iface.c
index 19460aa445d68b802dcda76074426506f4ed658c..697d8df598353446ae15c95f98154047fdc27989 100644 (file)
--- a/pimd/pim_iface.c
+++ b/pimd/pim_iface.c
@@ -1903,9 +1903,7 @@ static int pim_ifp_up(struct interface *ifp)
        }
 
 #if PIM_IPV == 4
-       if (pim->autorp && pim->autorp->do_discovery && pim_ifp &&
-           pim_ifp->pim_enable)
-               pim_autorp_add_ifp(ifp);
+       pim_autorp_add_ifp(ifp);
 #endif
 
        pim_cand_addrs_changed();
@@ -2022,8 +2020,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;

diff --git a/tests/topotests/pim_autorp/test_pim_autorp.py b/tests/topotests/pim_autorp/test_pim_autorp.py
index ad618af29e3d287640f50f27409a3ad036ffda55..a95c9be0a1095106ebfd3db44fc3d7cdbc05ab82 100644 (file)
--- a/tests/topotests/pim_autorp/test_pim_autorp.py
+++ b/tests/topotests/pim_autorp/test_pim_autorp.py
@@ -132,6 +132,90 @@ def test_pim_autorp_discovery_single_rp(request):
     result = verify_pim_rp_info_is_empty(tgen, "r2")
     assert result is True, "Testcase {} :Failed \n Error: {}".format(tc_name, result)
 
+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"]:
+        expected = {
+            f"{rtr}-eth0": {
+                "name": f"{rtr}-eth0",
+                "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"]:
+        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"]:
+        expected = {f"{rtr}-eth0": 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"]:
+        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"]:
+        expected = {
+            f"{rtr}-eth0": {
+                "name": f"{rtr}-eth0",
+                "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_discovery_multiple_rp(request):
     "Test PIM AutoRP Discovery with multiple RP's"