From bc290128630acdf3e5ca55d87c9c38c4a88413d1 Mon Sep 17 00:00:00 2001 From: Nathan Bahr Date: Mon, 24 Feb 2025 20:02:54 +0000 Subject: [PATCH] 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 (cherry picked from commit d840560b74e3a6117aa1e4b1203dcdd8fb254ef6) Fixed merge conflicts for backport --- pimd/pim_autorp.c | 53 ++++++------ pimd/pim_iface.c | 7 +- tests/topotests/pim_autorp/test_pim_autorp.py | 84 +++++++++++++++++++ 3 files changed, 113 insertions(+), 31 deletions(-) diff --git a/pimd/pim_autorp.c b/pimd/pim_autorp.c index e06f48fdc6..d8dc791430 100644 --- 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 19460aa445..697d8df598 100644 --- 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 ad618af29e..a95c9be0a1 100644 --- 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" -- 2.39.5