From 5a4f56eede22d9a4484b6b28864306b1e4759f37 Mon Sep 17 00:00:00 2001 From: Zeeshan Lakhani Date: Sat, 28 Feb 2026 06:03:37 +0000 Subject: [PATCH 1/2] [fix] remove ASIC ports before replication cleanup on empty transition When all members are removed from a multicast group, the (true, true) transition path cleaned up replication table entries but skipped process_membership_changes, leaving stale ports in the ASIC groups. Subsequent re-adds failed with "already contains port" (500). This fix calls process_membership_changes before cleanup_empty_group_replication so mc_port_remove runs on every port. Extend the IPv6 empty-then-add integration test to cover the full add -> remove all -> re-add cycle. --- dpd-client/tests/integration_tests/mcast.rs | 43 ++++++++++++++++++++- dpd/src/mcast/mod.rs | 21 +++++++++- 2 files changed, 61 insertions(+), 3 deletions(-) diff --git a/dpd-client/tests/integration_tests/mcast.rs b/dpd-client/tests/integration_tests/mcast.rs index 2b56698..4869dd7 100644 --- a/dpd-client/tests/integration_tests/mcast.rs +++ b/dpd-client/tests/integration_tests/mcast.rs @@ -4571,7 +4571,11 @@ async fn test_multicast_empty_then_add_members_ipv6() -> TestResult { // Update the internal group to add members (2 external, 1 underlay) // Meaning: two decap/port-bitmap members. let update_entry = types::MulticastGroupUpdateUnderlayEntry { - members: vec![external_member1, external_member2, underlay_member], + members: vec![ + external_member1.clone(), + external_member2.clone(), + underlay_member.clone(), + ], }; let ipv6_update = types::UnderlayMulticastIpv6(match internal_group_ip { @@ -4753,6 +4757,43 @@ async fn test_multicast_empty_then_add_members_ipv6() -> TestResult { switch.packet_test(vec![send_final], expected_final)?; + // Re-add members after removing all. This exercises the full + // empty -> members -> empty -> members cycle and verifies that + // ASIC port state is properly cleaned up during the transition + // to empty, so that re-adding does not fail with a duplicate + // port error. + let readd_entry = types::MulticastGroupUpdateUnderlayEntry { + members: vec![external_member1, external_member2, underlay_member], + }; + + switch + .client + .multicast_group_update_underlay( + &ipv6_update, + &make_tag(TEST_TAG), + &readd_entry, + ) + .await + .expect("Should re-add members after removing all"); + + let groups_after_readd = switch + .client + .multicast_groups_list_stream(None) + .try_collect::>() + .await + .expect("Should list groups after re-add"); + + let readd_internal = groups_after_readd + .iter() + .find(|g| get_group_ip(g) == internal_group_ip) + .expect("Should find the internal group"); + + assert_eq!( + get_members(readd_internal).map(|m| m.len()).unwrap_or(0), + 3, + "Internal group should have 3 members after re-add" + ); + cleanup_test_group(&switch, external_group_ip, TEST_TAG).await.unwrap(); cleanup_test_group(&switch, internal_group_ip, TEST_TAG).await } diff --git a/dpd/src/mcast/mod.rs b/dpd/src/mcast/mod.rs index 0535983..7d09971 100644 --- a/dpd/src/mcast/mod.rs +++ b/dpd/src/mcast/mod.rs @@ -826,10 +826,27 @@ pub(crate) fn modify_group_internal( group_entry.replication_info.is_some(), ) { (true, true) => { - // Transition from members to empty - cleanup tables + // Transition from members to empty. + // + // First, remove ports from ASIC groups before cleaning up + // replication table entries, otherwise stale ports cause subsequent + // re-adds to fail with "already contains port". + let repl_info = group_entry.replication_info.clone().unwrap(); + process_membership_changes( + s, + group_ip.into(), + &new_group_info.members, + &mut group_entry, + &repl_info, + ) + .inspect_err(|_e| { + mcast.groups.insert(group_ip.into(), group_entry.clone()); + }) + .map_err(|e| rollback_ctx.rollback_and_restore(e))?; + cleanup_empty_group_replication(s, group_ip.into(), &group_entry) .map_err(|e| rollback_ctx.rollback_and_restore(e))?; - // Immediately clear replication_info to maintain consistency + group_entry.replication_info = None; None } From d049a0b25a92d6d6d5cde956a4519976c137d357 Mon Sep 17 00:00:00 2001 From: Zeeshan Lakhani Date: Tue, 10 Mar 2026 01:42:41 +0000 Subject: [PATCH 2/2] [review] match on replication_info Option directly to remove unwrap --- dpd/src/mcast/mod.rs | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/dpd/src/mcast/mod.rs b/dpd/src/mcast/mod.rs index 7d09971..2186027 100644 --- a/dpd/src/mcast/mod.rs +++ b/dpd/src/mcast/mod.rs @@ -823,21 +823,20 @@ pub(crate) fn modify_group_internal( // Configure replication based on member count transitions let replication_info = match ( new_group_info.members.is_empty(), - group_entry.replication_info.is_some(), + &group_entry.replication_info, ) { - (true, true) => { + (true, Some(repl_info)) => { // Transition from members to empty. // // First, remove ports from ASIC groups before cleaning up // replication table entries, otherwise stale ports cause subsequent // re-adds to fail with "already contains port". - let repl_info = group_entry.replication_info.clone().unwrap(); process_membership_changes( s, group_ip.into(), &new_group_info.members, - &mut group_entry, - &repl_info, + &group_entry, + repl_info, ) .inspect_err(|_e| { mcast.groups.insert(group_ip.into(), group_entry.clone()); @@ -850,15 +849,15 @@ pub(crate) fn modify_group_internal( group_entry.replication_info = None; None } - (false, false) => { + (false, None) => { // Transition from empty to members - configure replication Some(configure_replication(group_entry.external_group_id())) } - (false, true) => { + (false, Some(_)) => { // Already has members and replication - keep existing group_entry.replication_info.clone() } - (true, false) => { + (true, None) => { // Already empty and no replication - keep none None } @@ -881,7 +880,7 @@ pub(crate) fn modify_group_internal( s, group_ip.into(), &new_group_info.members, - &mut group_entry, + &group_entry, repl_info, ) .inspect_err(|_e| { @@ -1495,7 +1494,7 @@ fn process_membership_changes( s: &Switch, group_ip: IpAddr, new_members: &[MulticastGroupMember], - group_entry: &mut MulticastGroup, + group_entry: &MulticastGroup, replication_info: &MulticastReplicationInfo, ) -> DpdResult<(Vec, Vec)> { // First validate that IPv4 doesn't have underlay members