From 2e23ff52d901ad2c71512b26665a1b002d2f0b02 Mon Sep 17 00:00:00 2001 From: Doug Goldstein Date: Thu, 30 Jul 2026 20:29:11 -0500 Subject: [PATCH] fix(neutron-understack): release dynamic VLAN segment on tenant port delete A baremetal port on a tenant network deleted directly (skipping the update_port_postcommit bound->unbound transition) never released its dynamic VLAN segment, since _tenant_network_port_cleanup only ran on that transition. The leaked segment kept its VLAN unavailable in ml2_vlan_allocations forever, and going stale there let a later rebuild hand the same VLAN to an unrelated network. --- .../neutron_understack_mech.py | 29 +++++-- .../tests/test_neutron_understack_mech.py | 77 +++++++++++++++++++ .../neutron_understack/utils.py | 6 ++ 3 files changed, 104 insertions(+), 8 deletions(-) diff --git a/python/neutron-understack/neutron_understack/neutron_understack_mech.py b/python/neutron-understack/neutron_understack/neutron_understack_mech.py index e3f0dfb60..798bf5b3a 100644 --- a/python/neutron-understack/neutron_understack/neutron_understack_mech.py +++ b/python/neutron-understack/neutron_understack/neutron_understack_mech.py @@ -296,10 +296,9 @@ def _tenant_network_port_cleanup(self, context: PortContext): segment_id = context.original_top_bound_segment["id"] original_binding = context.original[portbindings.PROFILE] - if not utils.ports_bound_to_segment( - segment_id - ) and utils.is_dynamic_network_segment(segment_id): - context.release_dynamic_segment(segment_id) + segment = utils.network_segment_by_id(segment_id) + if segment: + utils.release_segment_if_unused(segment) networks_to_remove = {segment_id} @@ -323,9 +322,6 @@ def delete_port_postcommit(self, context: PortContext) -> None: self._delete_port_baremetal(context) def _delete_port_baremetal(self, context: PortContext) -> None: - # Only clean up provisioning ports. Ports with tenant networks are cleaned - # up in _tenant_network_port_cleanup - port = context.current vlan_group_name = port[portbindings.PROFILE].get("physical_network") @@ -337,10 +333,27 @@ def _delete_port_baremetal(self, context: PortContext) -> None: local_link_info ) - if vlan_group_name and is_provisioning_network(port["network_id"]): + if not vlan_group_name: + return + + if is_provisioning_network(port["network_id"]): # Signals end of the provisioning / cleaning cycle, so we # put the port back to its normal tenant mode: self.undersync.sync(vlan_group_name) + return + + # A tenant network port normally has its dynamic VLAN segment released + # by _tenant_network_port_cleanup, but that only runs on the + # update_port_postcommit bound->unbound transition. A port deleted + # directly (still bound) skips that transition entirely, so without + # this the segment -- and its VLAN -- leaks forever. + segment = utils.network_segment_by_physnet(port["network_id"], vlan_group_name) + if segment: + utils.release_segment_if_unused(segment) + + # Reconcile the switch so the removed port's VLAN config is torn down, + # matching the unbind path in _update_port_baremetal. + self.undersync.sync(vlan_group_name) def bind_port(self, context: PortContext) -> None: """Bind the VXLAN network segment and allocate dynamic VLAN segments. diff --git a/python/neutron-understack/neutron_understack/tests/test_neutron_understack_mech.py b/python/neutron-understack/neutron_understack/tests/test_neutron_understack_mech.py index 5c3d13a6d..9af47ac0b 100644 --- a/python/neutron-understack/neutron_understack/tests/test_neutron_understack_mech.py +++ b/python/neutron-understack/neutron_understack/tests/test_neutron_understack_mech.py @@ -150,6 +150,10 @@ def test_skips_non_baremetal_port(self, understack_driver, port_context): understack_driver.undersync.sync.assert_not_called() +MECH_UTILS = "neutron_understack.neutron_understack_mech.utils" + + +@pytest.mark.usefixtures("_ironic_baremetal_port_physical_network") class TestDeletePortPostCommit: def test_skips_non_baremetal_port(self, understack_driver, port_context): port_context.current[portbindings.VNIC_TYPE] = portbindings.VNIC_NORMAL @@ -158,6 +162,79 @@ def test_skips_non_baremetal_port(self, understack_driver, port_context): understack_driver.undersync.sync.assert_not_called() + def test_releases_unused_dynamic_segment_on_tenant_network( + self, mocker, understack_driver, port_context + ): + """Deleting a still-bound tenant port must release its dynamic segment. + + This is the only way it gets released: unlike an explicit unbind + (update_port_postcommit -> _tenant_network_port_cleanup), a direct + delete never goes through that transition. + """ + segment = mocker.Mock(id="segment-a", is_dynamic=True) + find_segment = mocker.patch( + f"{MECH_UTILS}.network_segment_by_physnet", return_value=segment + ) + mocker.patch(f"{MECH_UTILS}.ports_bound_to_segment", return_value=[]) + release = mocker.patch(f"{MECH_UTILS}.release_dynamic_segment") + + understack_driver.delete_port_postcommit(port_context) + + find_segment.assert_called_once_with( + port_context.current["network_id"], "physnet" + ) + release.assert_called_once_with("segment-a") + # The switch must be reconciled so the removed port's VLAN is torn down. + understack_driver.undersync.sync.assert_called_once_with("physnet") + + def test_keeps_segment_still_bound_to_other_ports( + self, mocker, understack_driver, port_context + ): + segment = mocker.Mock(id="segment-a") + mocker.patch(f"{MECH_UTILS}.network_segment_by_physnet", return_value=segment) + mocker.patch( + f"{MECH_UTILS}.ports_bound_to_segment", return_value=["other-port"] + ) + release = mocker.patch(f"{MECH_UTILS}.release_dynamic_segment") + + understack_driver.delete_port_postcommit(port_context) + + release.assert_not_called() + + def test_keeps_non_dynamic_segment(self, mocker, understack_driver, port_context): + segment = mocker.Mock(id="segment-a", is_dynamic=False) + mocker.patch(f"{MECH_UTILS}.network_segment_by_physnet", return_value=segment) + mocker.patch(f"{MECH_UTILS}.ports_bound_to_segment", return_value=[]) + release = mocker.patch(f"{MECH_UTILS}.release_dynamic_segment") + + understack_driver.delete_port_postcommit(port_context) + + release.assert_not_called() + + def test_skips_release_when_no_segment_found( + self, mocker, understack_driver, port_context + ): + mocker.patch(f"{MECH_UTILS}.network_segment_by_physnet", return_value=None) + release = mocker.patch(f"{MECH_UTILS}.release_dynamic_segment") + + understack_driver.delete_port_postcommit(port_context) + + release.assert_not_called() + + def test_syncs_and_skips_release_on_provisioning_network( + self, mocker, understack_driver, port_context + ): + mocker.patch( + "neutron_understack.neutron_understack_mech.is_provisioning_network", + return_value=True, + ) + release = mocker.patch(f"{MECH_UTILS}.release_dynamic_segment") + + understack_driver.delete_port_postcommit(port_context) + + understack_driver.undersync.sync.assert_called_once_with("physnet") + release.assert_not_called() + @pytest.mark.usefixtures("_ironic_baremetal_port_physical_network") class TestBindPort: diff --git a/python/neutron-understack/neutron_understack/utils.py b/python/neutron-understack/neutron_understack/utils.py index 22334e778..2ce9fadd1 100644 --- a/python/neutron-understack/neutron_understack/utils.py +++ b/python/neutron-understack/neutron_understack/utils.py @@ -466,6 +466,12 @@ def is_dynamic_network_segment(segment_id: str) -> bool: return segment.is_dynamic +def release_segment_if_unused(segment: NetworkSegment) -> None: + """Release a dynamic VLAN segment once no ports remain bound to it.""" + if not ports_bound_to_segment(segment.id) and segment.is_dynamic: + release_dynamic_segment(segment.id) + + def local_link_from_binding_profile(binding_profile: dict) -> dict | None: return binding_profile.get("local_link_information", [None])[0]