diff --git a/changelog.d/20260817_152209_doug.goldstein_remove_physical_network_fallback.md b/changelog.d/20260817_152209_doug.goldstein_remove_physical_network_fallback.md new file mode 100644 index 000000000..19f053f6a --- /dev/null +++ b/changelog.d/20260817_152209_doug.goldstein_remove_physical_network_fallback.md @@ -0,0 +1,28 @@ +### Action required + +Neutron no longer falls back to querying Ironic for a baremetal port's +`physical_network` when it is absent from the port's `binding_profile`. It is +now a hard requirement: without it, binding a baremetal port is refused and +trunk create and delete operations are rejected with a `BadRequest`. + +Before upgrading, confirm no bound baremetal ports are missing +`physical_network`, and backfill any that are: + +1. Take a report-only inventory of affected ports in each cloud: + + ```bash + ./scripts/audit_ports_missing_physical_network.py --os-cloud $CLOUD + ``` + +2. If the report lists any ports, backfill them from their Ironic baremetal + port: + + ```bash + ./scripts/audit_ports_missing_physical_network.py --os-cloud $CLOUD --execute + ``` + +3. Re-run step 1 and confirm it reports no matches. + +A port whose trunk parent is still bound but has no `physical_network` cannot +have its trunk deleted until the profile is corrected. Trunks whose parent port +is unbound are unaffected and delete as before. diff --git a/python/neutron-understack/neutron_understack/ironic.py b/python/neutron-understack/neutron_understack/ironic.py index 3dbf8e131..096e8f6ce 100644 --- a/python/neutron-understack/neutron_understack/ironic.py +++ b/python/neutron-understack/neutron_understack/ironic.py @@ -4,7 +4,6 @@ from openstack import connection from openstack.baremetal.baremetal_service import BaremetalService from openstack.baremetal.v1.node import Node as BaremetalNode -from openstack.baremetal.v1.port import Port as BaremetalPort from oslo_config import cfg from neutron_understack import config @@ -61,18 +60,6 @@ def _get_ironic_client(self) -> BaremetalService: app_version=version, ).baremetal - def baremetal_port_physical_network(self, local_link_info: dict) -> str | None: - port = self._port_by_local_link(local_link_info) - return port.physical_network if port else None - - def _port_by_local_link(self, local_link_info: dict) -> BaremetalPort | None: - try: - return next( - self.irclient.ports(details=True, local_link_connection=local_link_info) - ) - except StopIteration: - return None - def baremetal_node_name(self, node_uuid: str) -> str | None: try: node = self.irclient.get_node(node_uuid) diff --git a/python/neutron-understack/neutron_understack/neutron_understack_mech.py b/python/neutron-understack/neutron_understack/neutron_understack_mech.py index 798bf5b3a..4d9379c27 100644 --- a/python/neutron-understack/neutron_understack/neutron_understack_mech.py +++ b/python/neutron-understack/neutron_understack/neutron_understack_mech.py @@ -14,7 +14,6 @@ from neutron_understack import config from neutron_understack import routers from neutron_understack import utils -from neutron_understack.ironic import IronicClient from neutron_understack.l3_router import svi as svi_router from neutron_understack.trunk import UnderStackTrunkDriver from neutron_understack.undersync import Undersync @@ -49,7 +48,6 @@ def initialize(self): conf = cfg.CONF.ml2_understack self.undersync = Undersync(conf.undersync_url) - self.ironic_client = IronicClient() self.trunk_driver = UnderStackTrunkDriver.create(self) self.subscribe() @@ -266,13 +264,6 @@ def _update_port_baremetal(self, context: PortContext) -> None: port = context.current vlan_group_name = port[portbindings.PROFILE].get("physical_network") - if vlan_group_name is None: - local_link_info = utils.local_link_from_binding_profile( - port[portbindings.PROFILE] - ) - vlan_group_name = self.ironic_client.baremetal_port_physical_network( - local_link_info - ) if current_vif_unbound and original_vif_other: self._tenant_network_port_cleanup(context) @@ -325,13 +316,6 @@ def _delete_port_baremetal(self, context: PortContext) -> None: port = context.current vlan_group_name = port[portbindings.PROFILE].get("physical_network") - if vlan_group_name is None: - local_link_info = utils.local_link_from_binding_profile( - port[portbindings.PROFILE] - ) - vlan_group_name = self.ironic_client.baremetal_port_physical_network( - local_link_info - ) if not vlan_group_name: return @@ -400,19 +384,17 @@ def _bind_port_segment(self, context: PortContext, segment): port = context.current vlan_group_name = port[portbindings.PROFILE].get("physical_network") - if vlan_group_name is None: - local_link_info = utils.local_link_from_binding_profile( - port[portbindings.PROFILE] - ) - vlan_group_name = self.ironic_client.baremetal_port_physical_network( - local_link_info - ) if not vlan_group_name: LOG.error( - "bind_port_segment: no physical_network found for baremetal " - "port with mac address: %(mac)s", - {"mac": mac_address}, + "bind_port_segment: physical_network is required in the " + "binding_profile for baremetal port binding, but was not found. " + "port_id=%(port_id)s mac_address=%(mac)s network_id=%(network_id)s.", + { + "port_id": port["id"], + "mac": mac_address, + "network_id": network_id, + }, ) return diff --git a/python/neutron-understack/neutron_understack/tests/conftest.py b/python/neutron-understack/neutron_understack/tests/conftest.py index cfb5b4256..a9b39c6da 100644 --- a/python/neutron-understack/neutron_understack/tests/conftest.py +++ b/python/neutron-understack/neutron_understack/tests/conftest.py @@ -26,7 +26,6 @@ from oslo_config import fixture as config_fixture from neutron_understack import config as understack_config -from neutron_understack.ironic import IronicClient from neutron_understack.neutron_understack_mech import UnderstackDriver from neutron_understack.tests.helpers import Ml2PluginNoInit from neutron_understack.tests.helpers import extend_network_dict @@ -196,18 +195,24 @@ def subnet_context(ml2_plugin, subnet_dict) -> SubnetContext: @pytest.fixture def binding_profile(request, port_id) -> str: + """A baremetal port binding profile. + + Pass ``{"physical_network": None}`` to omit that key entirely. + """ req = getattr(request, "param", {}) - return json.dumps( - { - "local_link_information": [ - { - "port_id": req.get("port_id", str(port_id)), - "switch_id": "11:22:33:44:55:66", - "switch_info": "a1-1-1.iad3.rackspace.net", - } - ] - } - ) + profile = { + "local_link_information": [ + { + "port_id": req.get("port_id", str(port_id)), + "switch_id": "11:22:33:44:55:66", + "switch_info": "a1-1-1.iad3.rackspace.net", + } + ], + } + physical_network = req.get("physical_network", "physnet") + if physical_network is not None: + profile["physical_network"] = physical_network + return json.dumps(profile) @pytest.fixture @@ -274,15 +279,9 @@ def port_context(network_context, port_dict, port_binding, ml2_plugin) -> PortCo @pytest.fixture -def ironic_client(mocker) -> IronicClient: - return mocker.MagicMock(spec_set=IronicClient) - - -@pytest.fixture -def understack_driver(oslo_config, ironic_client) -> UnderstackDriver: +def understack_driver(oslo_config) -> UnderstackDriver: driver = UnderstackDriver() driver.undersync = MagicMock(spec_set=Undersync) - driver.ironic_client = ironic_client return driver @@ -291,15 +290,6 @@ def understack_trunk_driver(understack_driver) -> UnderStackTrunkDriver: return UnderStackTrunkDriver.create(understack_driver) -@pytest.fixture -def _ironic_baremetal_port_physical_network(mocker, understack_driver) -> None: - mocker.patch.object( - understack_driver.ironic_client, - "baremetal_port_physical_network", - return_value="physnet", - ) - - @pytest.fixture def _utils_fetch_subport_network_id_patch(mocker, network_id) -> None: mocker.patch( 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 9af47ac0b..edbc97fb4 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 @@ -1,3 +1,4 @@ +import logging from dataclasses import dataclass from types import SimpleNamespace @@ -6,6 +7,7 @@ from neutron_lib.api.definitions import portbindings from neutron_lib.plugins.ml2 import api +from neutron_understack import neutron_understack_mech from neutron_understack.neutron_understack_mech import UnderstackDriver @@ -153,7 +155,6 @@ def test_skips_non_baremetal_port(self, understack_driver, port_context): 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 @@ -236,7 +237,6 @@ def test_syncs_and_skips_release_on_provisioning_network( release.assert_not_called() -@pytest.mark.usefixtures("_ironic_baremetal_port_physical_network") class TestBindPort: def test_does_not_bind_vlan_only_segments( self, @@ -327,19 +327,26 @@ def test_refuses_unsupported_vnic_type( port_context.continue_binding.assert_not_called() - @pytest.mark.usefixtures("_ironic_baremetal_port_physical_network") - def test_does_not_bind_when_physical_network_not_found( - self, mocker, port_context, understack_driver + @pytest.mark.parametrize( + "binding_profile", [{"physical_network": None}], indirect=True + ) + def test_does_not_bind_when_physical_network_missing( + self, mocker, caplog, port_context, understack_driver ): - understack_driver.ironic_client.baremetal_port_physical_network.return_value = ( - None - ) + """physical_network is required: there is no fallback lookup any more. + + Without it we cannot tell which VLAN group to allocate a segment in, so + binding is refused and the reason is logged rather than failing silently. + """ mocker.patch.object(port_context, "continue_binding") port_context._prepare_to_bind(port_context.network.network_segments) + caplog.set_level(logging.ERROR, logger=neutron_understack_mech.LOG.name) understack_driver.bind_port(port_context) port_context.continue_binding.assert_not_called() + assert "physical_network is required" in caplog.text + assert port_context.current["id"] in caplog.text @pytest.mark.parametrize("port_dict", [{"trunk": True}], indirect=True) def test_with_trunk_details( @@ -420,8 +427,6 @@ def test_initialize_with_keystone_auth(self, mocker): "neutron_understack.config.get_session", return_value=mock_session_instance, ) - mocker.patch("neutron_understack.neutron_understack_mech.IronicClient") - driver = UnderstackDriver() driver.initialize() diff --git a/python/neutron-understack/neutron_understack/tests/test_trunk.py b/python/neutron-understack/neutron_understack/tests/test_trunk.py index 1d0b80110..dacfc298c 100644 --- a/python/neutron-understack/neutron_understack/tests/test_trunk.py +++ b/python/neutron-understack/neutron_understack/tests/test_trunk.py @@ -1,6 +1,10 @@ +import logging + import pytest from neutron.plugins.ml2.driver_context import portbindings +from neutron_lib import exceptions as exc +from neutron_understack import trunk as trunk_module from neutron_understack import utils from neutron_understack.trunk import SubportSegmentationIDError @@ -51,7 +55,6 @@ def test_when_subports_are_not_present( ) -@pytest.mark.usefixtures("_ironic_baremetal_port_physical_network") @pytest.mark.usefixtures("_utils_fetch_subport_network_id_patch") class Test_HandleTenantVlanIDAndSwitchportConfig: def test_that_check_subports_segmentation_id_is_called( @@ -182,7 +185,6 @@ def test_when_subports_are_not_present( ) -@pytest.mark.usefixtures("_ironic_baremetal_port_physical_network") @pytest.mark.usefixtures("_utils_fetch_subport_network_id_patch") class Test_CleanParentPortSwitchportConfig: def test_when_parent_port_is_bound( @@ -390,3 +392,100 @@ def test_when_segmentation_id_is_not_in_allowed_range( subport.segmentation_id = 1600 with pytest.raises(SubportSegmentationIDError): understack_trunk_driver._check_subports_segmentation_id([subport], trunk_id) + + +@pytest.mark.parametrize("binding_profile", [{"physical_network": None}], indirect=True) +class TestMissingPhysicalNetwork: + """physical_network is mandatory: there is no fallback lookup. + + It is enforced on the precommit hooks, where raising aborts the + transaction. The postcommit hooks cannot roll anything back, so they log + and degrade instead of raising. + """ + + @pytest.fixture + def _bound_parent_port(self, mocker, port_object) -> None: + mocker.patch( + "neutron_understack.utils.fetch_port_object", return_value=port_object + ) + mocker.patch("neutron_understack.utils.parent_port_is_bound", return_value=True) + + def test_precommit_create_rejects_the_request( + self, understack_trunk_driver, port_object, subport + ): + with pytest.raises(exc.BadRequest, match="physical_network is required"): + understack_trunk_driver._add_subports_networks_to_parent_port_switchport( + port_object, [subport] + ) + + @pytest.mark.usefixtures("_bound_parent_port") + def test_subports_deleted_precommit_rejects_the_request( + self, mocker, understack_trunk_driver, trunk + ): + with pytest.raises(exc.BadRequest, match="physical_network is required"): + understack_trunk_driver.subports_deleted_precommit( + None, None, None, mocker.Mock(states=[trunk]) + ) + + @pytest.mark.usefixtures("_bound_parent_port") + def test_trunk_deleted_precommit_rejects_the_request( + self, mocker, understack_trunk_driver, trunk + ): + with pytest.raises(exc.BadRequest, match="physical_network is required"): + understack_trunk_driver.trunk_deleted_precommit( + None, None, None, mocker.Mock(states=[trunk]) + ) + + def test_precommit_delete_ignores_unbound_parent_port( + self, mocker, understack_trunk_driver, trunk, port_object + ): + """An unbound parent port has no switchport config to tear down. + + There is nothing to validate, so a missing physnet must not block the + delete. + """ + mocker.patch( + "neutron_understack.utils.fetch_port_object", return_value=port_object + ) + mocker.patch( + "neutron_understack.utils.parent_port_is_bound", return_value=False + ) + + understack_trunk_driver.subports_deleted_precommit( + None, None, None, mocker.Mock(states=[trunk]) + ) + + @pytest.mark.usefixtures("_bound_parent_port") + def test_subports_added_post_logs_instead_of_raising( + self, mocker, caplog, understack_trunk_driver, trunk + ): + caplog.set_level(logging.ERROR, logger=trunk_module.LOG.name) + + understack_trunk_driver.subports_added_post( + None, None, None, mocker.Mock(states=[trunk]) + ) + + assert "physical_network is required" in caplog.text + understack_trunk_driver.undersync.sync.assert_not_called() + + @pytest.mark.usefixtures( + "_bound_parent_port", "_utils_fetch_subport_network_id_patch" + ) + def test_clean_parent_port_releases_segments_without_syncing( + self, mocker, caplog, understack_trunk_driver, trunk, subport + ): + """Teardown still runs postcommit so the subports' VLANs do not leak. + + Only the undersync call is skipped, since there is no vlan group to + reconcile. + """ + deallocate = mocker.patch.object( + understack_trunk_driver, "_handle_segment_deallocation" + ) + caplog.set_level(logging.ERROR, logger=trunk_module.LOG.name) + + understack_trunk_driver._clean_parent_port_switchport_config(trunk, [subport]) + + assert "physical_network is required" in caplog.text + deallocate.assert_called_once() + understack_trunk_driver.undersync.sync.assert_not_called() diff --git a/python/neutron-understack/neutron_understack/trunk.py b/python/neutron-understack/neutron_understack/trunk.py index 6d3f07ae7..96131eecf 100644 --- a/python/neutron-understack/neutron_understack/trunk.py +++ b/python/neutron-understack/neutron_understack/trunk.py @@ -29,6 +29,18 @@ class SubportSegmentationIDError(exc.NeutronException): ) +def _missing_physnet_msg(port_id: str) -> str: + """Explain that a parent port cannot be configured without a physnet. + + physical_network names the VLAN group that undersync configures, so + without it there is no switch to push the trunk's subport VLANs to. + """ + return ( + "physical_network is required in the binding_profile for baremetal port " + f"trunk configuration, but port {port_id} does not have one." + ) + + class UnderStackTrunkDriver(trunk_base.DriverBase): def __init__( self, @@ -46,7 +58,6 @@ def __init__( can_trunk_bound_port=can_trunk_bound_port, ) self.undersync = self.plugin_driver.undersync - self.ironic_client = self.plugin_driver.ironic_client @property def is_loaded(self): @@ -82,6 +93,12 @@ def register(self, resource, event, trigger, payload=None): events.AFTER_CREATE, cancellable=True, ) + registry.subscribe( + self.subports_deleted_precommit, + resources.SUBPORTS, + events.PRECOMMIT_DELETE, + cancellable=True, + ) registry.subscribe( self.subports_deleted, resources.SUBPORTS, @@ -94,6 +111,12 @@ def register(self, resource, event, trigger, payload=None): events.PRECOMMIT_CREATE, cancellable=True, ) + registry.subscribe( + self.trunk_deleted_precommit, + resources.TRUNK, + events.PRECOMMIT_DELETE, + cancellable=True, + ) registry.subscribe( self.trunk_deleted, resources.TRUNK, @@ -181,10 +204,11 @@ def _add_subports_networks_to_parent_port_switchport( binding_host = parent_port.bindings[0].host vlan_group_name = binding_profile.get("physical_network") - if vlan_group_name is None: - local_link_info = utils.local_link_from_binding_profile(binding_profile) - vlan_group_name = self.ironic_client.baremetal_port_physical_network( - local_link_info + if not vlan_group_name: + # Reached from the PRECOMMIT_CREATE handlers, so raising here aborts + # the transaction and surfaces the error to the API caller. + raise exc.BadRequest( + resource="port", msg=_missing_physnet_msg(parent_port.id) ) self._handle_segment_allocation(subports, vlan_group_name, binding_host) @@ -210,11 +234,14 @@ def _clean_parent_port_switchport_config( binding_host = parent_port_obj.bindings[0].host vlan_group_name = binding_profile.get("physical_network") - if vlan_group_name is None: - local_link_info = utils.local_link_from_binding_profile(binding_profile) - vlan_group_name = self.ironic_client.baremetal_port_physical_network( - local_link_info - ) + if not vlan_group_name: + # This runs postcommit: the subports are already gone from the DB, + # so raising cannot roll anything back. The PRECOMMIT_DELETE + # handlers reject this case while it is still abortable; getting + # here means the binding profile changed underneath us. Log it and + # still release the segments -- otherwise their VLANs leak -- but + # skip the undersync call, since there is no vlan group to sync. + LOG.error(_missing_physnet_msg(parent_port_obj.id)) self._handle_subports_removal( binding_profile=binding_profile, binding_host=binding_host, @@ -263,14 +290,34 @@ def subports_added_post(self, resource, event, trunk_plugin, payload): if utils.parent_port_is_bound(parent_port): binding_profile = parent_port.bindings[0].profile vlan_group_name = binding_profile.get("physical_network") - if vlan_group_name is None: - local_link_info = utils.local_link_from_binding_profile(binding_profile) - vlan_group_name = self.ironic_client.baremetal_port_physical_network( - local_link_info - ) + if not vlan_group_name: + # subports_added validates the same parent port on + # PRECOMMIT_CREATE, so normally we never get here. This runs + # postcommit, where raising cannot undo the subport creation, + # and there is no vlan group to sync. + LOG.error(_missing_physnet_msg(parent_port.id)) + return LOG.debug("subports_added_post found vlan_group_name=%s", vlan_group_name) self.undersync.sync(vlan_group_name) + def _validate_parent_port_physnet(self, trunk: Trunk) -> None: + """Reject a teardown whose parent port has no physical_network. + + The switchport teardown itself runs on AFTER_DELETE, where the rows are + already committed and an exception cannot roll them back, so the check + has to happen here while the transaction can still be aborted. + """ + parent_port_obj = utils.fetch_port_object(trunk.port_id) + if not utils.parent_port_is_bound(parent_port_obj): + return + if not parent_port_obj.bindings[0].profile.get("physical_network"): + raise exc.BadRequest( + resource="port", msg=_missing_physnet_msg(parent_port_obj.id) + ) + + def subports_deleted_precommit(self, resource, event, trunk_plugin, payload): + self._validate_parent_port_physnet(payload.states[0]) + def subports_deleted(self, resource, event, trunk_plugin, payload): trunk = payload.states[0] subports = payload.metadata["subports"] @@ -282,6 +329,11 @@ def trunk_created(self, resource, event, trunk_plugin, payload): if subports: self._handle_tenant_vlan_id_and_switchport_config(subports, trunk) + def trunk_deleted_precommit(self, resource, event, trunk_plugin, payload): + trunk = payload.states[0] + if trunk.sub_ports: + self._validate_parent_port_physnet(trunk) + def trunk_deleted(self, resource, event, trunk_plugin, payload): trunk = payload.states[0] subports = trunk.sub_ports diff --git a/python/neutron-understack/neutron_understack/utils.py b/python/neutron-understack/neutron_understack/utils.py index 2ce9fadd1..0a638a090 100644 --- a/python/neutron-understack/neutron_understack/utils.py +++ b/python/neutron-understack/neutron_understack/utils.py @@ -472,10 +472,6 @@ def release_segment_if_unused(segment: NetworkSegment) -> None: 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] - - def parent_port_is_bound(port: port_obj.Port) -> bool: port_binding = port.bindings[0] return bool(