From 8e966e1fe44cdbace9f6611108c9e4daa3075bfc Mon Sep 17 00:00:00 2001 From: Nidhi Rai Date: Tue, 13 Oct 2026 18:18:02 +0530 Subject: [PATCH 1/2] refactor(neutron-understack): move Palo Alto wiring into its own module Move the wiring into PaloAltoWiring so the wiring reconciler can reuse it. --- .../neutron_understack/l3_router/palo_alto.py | 727 ++----------- .../l3_router/palo_alto_wiring.py | 673 ++++++++++++ .../tests/test_palo_alto_provider.py | 967 ++++++------------ .../tests/test_palo_alto_wiring.py | 877 ++++++++++++++++ 4 files changed, 1939 insertions(+), 1305 deletions(-) create mode 100644 python/neutron-understack/neutron_understack/l3_router/palo_alto_wiring.py create mode 100644 python/neutron-understack/neutron_understack/tests/test_palo_alto_wiring.py diff --git a/python/neutron-understack/neutron_understack/l3_router/palo_alto.py b/python/neutron-understack/neutron_understack/l3_router/palo_alto.py index bd11998d6..e50030dcf 100644 --- a/python/neutron-understack/neutron_understack/l3_router/palo_alto.py +++ b/python/neutron-understack/neutron_understack/l3_router/palo_alto.py @@ -1,14 +1,11 @@ import json import logging import weakref -from dataclasses import dataclass from neutron.objects import router as l3_obj from neutron.services.l3_router.service_providers import base from neutron_lib import constants as const -from neutron_lib import context as n_context from neutron_lib import exceptions as n_exc -from neutron_lib.api.definitions import portbindings from neutron_lib.callbacks import events from neutron_lib.callbacks import priority_group from neutron_lib.callbacks import registry @@ -16,36 +13,11 @@ from neutron_lib.plugins import constants as plugin_constants from neutron_lib.plugins import directory -from neutron_understack import utils from neutron_understack.ironic import IronicClient +from neutron_understack.l3_router.palo_alto_wiring import PaloAltoWiring LOG = logging.getLogger(__name__) -# Single shared sentinel network owned by the router flavor code. -ANCHOR_NETWORK_NAME = "palo_alto_router_anchor_network" - -# Deterministic per-router names. Deriving the parent port and trunk names from -# the router id lets the gateway teardown find them by name without depending on -# catching a specific delete event with the gateway port still visible. -ANCHOR_PARENT_PORT_NAME_PREFIX = "palo-alto-router-anchor" -TRUNK_NAME_PREFIX = "palo-alto-router-trunk" - -# Temporary fixed trunk subport tag. This keeps the subport segmentation_id in -# the existing allowed/gap VLAN validation path. Replace with a configured or -# allocated model once the trunk-tag semantics are revisited. -GATEWAY_SUBPORT_VLAN = 200 -INTERFACE_SUBPORT_VLAN_START = GATEWAY_SUBPORT_VLAN + 1 - - -@dataclass(frozen=True) -class _InterfaceAttachment: - router_id: str - port_id: str - parent_id: str | None - trunk_id: str | None - subport_present: bool - parent_vif_attached: bool - # Conflict -> HTTP 409: the request cannot be satisfied because the hardware # pool is exhausted. @@ -64,10 +36,19 @@ class PaloAltoFlavorMisconfigured(n_exc.BadRequest): ) -class NoPaloAltoSubportVlanAvailable(n_exc.Conflict): +# BadRequest -> HTTP 400: the gateway event fired without a gateway port. +class PaloAltoGatewayPortNotFound(n_exc.BadRequest): message = ( - "No Palo Alto trunk subport VLAN is available for router %(router_id)s " - "on trunk %(trunk_id)s. Allowed ranges: %(network_segment_ranges)s." + "Palo Alto router %(router_id)s gateway was created but no router " + "gateway port was found." + ) + + +# BadRequest -> HTTP 400: the interface event carried no port. +class PaloAltoInterfacePortMissing(n_exc.BadRequest): + message = ( + "Palo Alto router %(router_id)s interface attachment has no router " + "interface port." ) @@ -75,10 +56,8 @@ def _parse_metainfo(raw) -> dict: """Service-profile metainfo is stored as a JSON string.""" if not raw: return {} - # already a dict return as-is if isinstance(raw, dict): return raw - # parse the string; malformed JSON try: parsed = json.loads(raw) except (TypeError, ValueError): @@ -95,7 +74,8 @@ class PaloAlto(base.L3ServiceProvider): declared in the flavor's service profile metainfo), binds it to the owning project/router, and ensures the shared sentinel anchor network exists. On delete it returns the node to the available pool. Routers of this flavor are - detected via their flavor's service profile driver. + detected via their flavor's service profile driver. Gateway and interface + wiring is delegated to PaloAltoWiring. """ ha_support = base.OPTIONAL @@ -103,7 +83,8 @@ class PaloAlto(base.L3ServiceProvider): def __init__(self, l3_plugin): super().__init__(l3_plugin) self._palo_alto_provider = f"{__name__}.{self.__class__.__name__}" - self._interface_attachments = weakref.WeakKeyDictionary() + self._interface_snapshots = weakref.WeakKeyDictionary() + self._wiring = PaloAltoWiring(ironic=lambda: self._ironic) # Gateway attach must run on AFTER_CREATE (the gateway port does not # exist earlier) and must be cancellable so a wiring failure returns a # real API error instead of a swallowed 200. @registry.receives cannot @@ -168,24 +149,15 @@ def _ironic(self) -> IronicClient: self._ironic_ref = IronicClient() return self._ironic_ref - def _is_palo_alto_provider(self, context, router): + def _is_palo_alto_provider(self, context, router) -> bool: flavor_id = router.get("flavor_id") - if flavor_id is None or flavor_id is const.ATTR_NOT_SPECIFIED: - LOG.debug( - "Palo Alto flavor check skipped: router=%s name=%s project=%s " - "flavor=%s request_id=%s", - router.get("id"), - router.get("name"), - router.get("project_id"), - flavor_id, - getattr(context, "request_id", None), + actual_driver = None + if flavor_id is not None and flavor_id is not const.ATTR_NOT_SPECIFIED: + flavor = self._flavor_plugin.get_flavor(context, flavor_id) + providers = self._flavor_plugin.get_flavor_next_provider( + context, flavor["id"] ) - return False - flavor = self._flavor_plugin.get_flavor(context, flavor_id) - provider = self._flavor_plugin.get_flavor_next_provider(context, flavor["id"])[ - 0 - ] - actual_driver = str(provider["driver"]) + actual_driver = str(providers[0]["driver"]) matched = actual_driver == self._palo_alto_provider LOG.debug( "Palo Alto flavor check: router=%s name=%s project=%s flavor=%s " @@ -201,6 +173,13 @@ def _is_palo_alto_provider(self, context, router): ) return matched + def _palo_alto_router(self, context, router_id: str) -> dict | None: + """Return the router if it uses the Palo Alto flavor, else None.""" + router = self.l3plugin.get_router(context, router_id) + if not self._is_palo_alto_provider(context, router): + return None + return router + def _resource_class_for_router(self, context, router) -> str: """Read the target resource_class from the flavor's profile metainfo. @@ -220,532 +199,6 @@ def _resource_class_for_router(self, context, router) -> str: router_id=router["id"], flavor_id=router["flavor_id"] ) - def _ensure_anchor_network(self): - """Create the shared sentinel anchor network if it does not exist.""" - core_plugin = directory.get_plugin() - admin_context = n_context.get_admin_context() - existing = core_plugin.get_networks( - admin_context, filters={"name": [ANCHOR_NETWORK_NAME]} - ) - if existing: - LOG.debug( - "Reusing existing anchor network %s (id=%s)", - ANCHOR_NETWORK_NAME, - existing[0]["id"], - ) - return existing[0] - LOG.info("Creating shared anchor network %s", ANCHOR_NETWORK_NAME) - # Calling the core plugin directly (not via the REST API) skips the - # API layer that fills in extension-attribute defaults, so we must - # supply them ourselves. project_id (ownership) and router:external - # (read by the auto_allocate NETWORK-create callback) are required; - # without router:external the create fails with KeyError 'router:external'. - return core_plugin.create_network( - admin_context, - { - "network": { - "name": ANCHOR_NETWORK_NAME, - "admin_state_up": True, - "shared": False, - "router:external": False, - "project_id": admin_context.project_id or "", - } - }, - ) - - # --- gateway attachment: names + lookups (read-only) --- - - @property - def _trunk_plugin(self): - return utils.fetch_trunk_plugin() - - def _parent_port_name(self, router_id: str) -> str: - """Deterministic name for the router's anchor-network parent port.""" - return f"{ANCHOR_PARENT_PORT_NAME_PREFIX}-{router_id}" - - def _trunk_name(self, router_id: str) -> str: - """Deterministic name for the router's trunk.""" - return f"{TRUNK_NAME_PREFIX}-{router_id}" - - def _gateway_port_for_router(self, router_id: str) -> dict | None: - """Return the router's Neutron external-gateway port, or None. - - The gateway port is owned by the router (``device_id == router_id``) with - ``device_owner == network:router_gateway``. - """ - core_plugin = directory.get_plugin() - admin_context = n_context.get_admin_context() - ports = core_plugin.get_ports( - admin_context, - filters={ - "device_id": [router_id], - "device_owner": [const.DEVICE_OWNER_ROUTER_GW], - }, - ) - if not ports: - LOG.debug("No gateway port found for Palo Alto router %s", router_id) - return None - if len(ports) > 1: - LOG.warning( - "Expected one gateway port for Palo Alto router %s, found %d; using %s", - router_id, - len(ports), - ports[0]["id"], - ) - return ports[0] - - def _parent_port_for_router(self, router_id: str) -> dict | None: - """Return the router's existing anchor-network parent port, or None.""" - core_plugin = directory.get_plugin() - admin_context = n_context.get_admin_context() - anchor_network = self._ensure_anchor_network() - ports = core_plugin.get_ports( - admin_context, - filters={ - "name": [self._parent_port_name(router_id)], - "network_id": [anchor_network["id"]], - }, - ) - return ports[0] if ports else None - - def _create_parent_port(self, router: dict) -> dict: - """Create the router's parent port on the anchor network. - - vnic_type=baremetal so Ironic can VIF-attach it to the adopted node. - Direct core-plugin call (server-side), so extension-default fields are - supplied explicitly, matching the codebase's other direct port creates. - """ - core_plugin = directory.get_plugin() - admin_context = n_context.get_admin_context() - anchor_network = self._ensure_anchor_network() - port_name = self._parent_port_name(router["id"]) - LOG.info( - "Creating Palo Alto anchor parent port %s for router %s", - port_name, - router["id"], - ) - return core_plugin.create_port( - admin_context, - { - "port": { - "name": port_name, - "network_id": anchor_network["id"], - "admin_state_up": True, - "device_owner": "", - "device_id": router["id"], - "mac_address": "", - "fixed_ips": [], - "project_id": admin_context.project_id or "", - portbindings.VNIC_TYPE: portbindings.VNIC_BAREMETAL, - } - }, - ) - - def _ensure_parent_port(self, router: dict) -> dict: - """Find-or-create the router's anchor-network parent port (idempotent).""" - existing = self._parent_port_for_router(router["id"]) - if existing is not None: - LOG.debug( - "Reusing Palo Alto anchor parent port %s for router %s", - existing["id"], - router["id"], - ) - return existing - return self._create_parent_port(router) - - def _fresh_port(self, port_id: str) -> dict: - """Re-read a port so callers see its current binding profile.""" - core_plugin = directory.get_plugin() - admin_context = n_context.get_admin_context() - return core_plugin.get_port(admin_context, port_id) - - def _confirm_parent_vif_attach_after_error( - self, router_id: str, node, parent_port_id: str - ) -> dict | None: - """Return the fresh parent port if an errored attach actually landed. - - Ironic VIF attach can time out waiting for the conductor/RPC reply even - after the conductor has applied the binding. - """ - try: - vif_ids = self._ironic.node_vif_ids(node) - except Exception: - LOG.debug( - "Could not re-check VIF attachments for Palo Alto router %s " - "after an attach error", - router_id, - exc_info=True, - ) - return None - if parent_port_id not in vif_ids: - return None - - fresh = self._fresh_port(parent_port_id) - self._verify_parent_annotated(router_id, fresh) - return fresh - - def _ensure_parent_vif_attached(self, router: dict, parent_port: dict) -> dict: - """VIF-attach the parent port to the router's node (idempotent). - - Attaching a single VIF; Ironic binds it to a free baremetal port on the - node and annotates the Neutron port with local_link_information + - physical_network and host_id . - - Returns a fresh copy of the parent port reflecting the new binding. - """ - router_id = router["id"] - node = self._ironic.node_by_instance_uuid(router_id) - if node is None: - raise n_exc.BadRequest( - resource="router", - msg=( - f"Palo Alto router {router_id} has no adopted Ironic node to " - "attach the gateway uplink to." - ), - ) - - parent_port_id = parent_port["id"] - if parent_port_id in self._ironic.node_vif_ids(node): - LOG.debug( - "Parent port %s already VIF-attached to node %s", - parent_port_id, - node.id, - ) - else: - try: - self._ironic.attach_vif_to_node(node, parent_port_id) - except Exception: - fresh = self._confirm_parent_vif_attach_after_error( - router_id, node, parent_port_id - ) - if fresh is None: - raise - LOG.warning( - "Ironic VIF attach for Palo Alto router %s parent port %s " - "on node %s raised, but the VIF is attached and annotated; " - "continuing", - router_id, - parent_port_id, - getattr(node, "id", node), - exc_info=True, - ) - return fresh - - fresh = self._fresh_port(parent_port_id) - self._verify_parent_annotated(router_id, fresh) - return fresh - - def _verify_parent_annotated(self, router_id: str, parent_port: dict) -> None: - """Fail fast if Ironic did not annotate the parent port. - - The trunk + undersync need host_id, physical_network and - local_link_information on the binding profile. If they are missing the - node's baremetal port most likely has no physical_network (enroll side); - surface a clear error here instead of a silent no-op at undersync. - """ - profile = parent_port.get(portbindings.PROFILE) or {} - missing = [ - name - for name, value in ( - ("binding:host_id", parent_port.get(portbindings.HOST_ID)), - ("physical_network", profile.get("physical_network")), - ("local_link_information", profile.get("local_link_information")), - ) - if not value - ] - if missing: - raise n_exc.BadRequest( - resource="router", - msg=( - f"Palo Alto router {router_id} parent port {parent_port['id']} " - f"was not annotated by Ironic (missing {', '.join(missing)}); " - "check the node's baremetal port has physical_network." - ), - ) - - def _trunk_for_router(self, router_id: str) -> dict | None: - """Return the router's existing trunk (by deterministic name), or None.""" - admin_context = n_context.get_admin_context() - trunks = self._trunk_plugin.get_trunks( - admin_context, filters={"name": [self._trunk_name(router_id)]} - ) - return trunks[0] if trunks else None - - def _create_trunk(self, router: dict, parent_port: dict) -> dict: - """Create the router's trunk with the parent port as its trunk parent.""" - admin_context = n_context.get_admin_context() - trunk_name = self._trunk_name(router["id"]) - LOG.info( - "Creating Palo Alto trunk %s on parent port %s for router %s", - trunk_name, - parent_port["id"], - router["id"], - ) - return self._trunk_plugin.create_trunk( - admin_context, - { - "trunk": { - "name": trunk_name, - "port_id": parent_port["id"], - "admin_state_up": True, - "project_id": admin_context.project_id or "", - "sub_ports": [], - } - }, - ) - - def _ensure_trunk(self, router: dict, parent_port: dict) -> dict: - """Find-or-create the router's trunk (idempotent).""" - existing = self._trunk_for_router(router["id"]) - if existing is not None: - LOG.debug( - "Reusing Palo Alto trunk %s for router %s", - existing["id"], - router["id"], - ) - return existing - return self._create_trunk(router, parent_port) - - def _used_subport_vlans(self, trunk: dict) -> set[int]: - """Return VLAN segmentation IDs already used on a router trunk.""" - return { - sp["segmentation_id"] - for sp in trunk.get("sub_ports", []) - if sp.get("segmentation_type") == "vlan" - and sp.get("segmentation_id") is not None - } - - def _next_available_subport_vlan( - self, router_id: str, trunk: dict, start_vlan: int - ) -> int: - """Pick the first allowed, unused Palo Alto subport VLAN from start.""" - used = self._used_subport_vlans(trunk) - ranges = sorted(utils.allowed_tenant_vlan_id_ranges()) - for start, end in ranges: - for vlan in range(max(start, start_vlan), end + 1): - if vlan not in used: - return vlan - raise NoPaloAltoSubportVlanAvailable( - router_id=router_id, - trunk_id=trunk["id"], - network_segment_ranges=utils.printable_ranges(ranges), - ) - - def _add_router_port_subport( - self, - router: dict, - trunk: dict, - port: dict, - segmentation_id: int, - label: str, - ): - """Add a router-owned port to the trunk as a VLAN subport. - - Adding the subport fires the understack trunk driver (SUBPORTS events), - which allocates the fabric segment, binds it, and calls undersync to - program the switch. No-ops if the port is already a subport. - """ - port_id = port["id"] - existing = {sp["port_id"] for sp in trunk.get("sub_ports", [])} - if port_id in existing: - LOG.debug( - "Palo Alto %s port %s already a subport on trunk %s", - label, - port_id, - trunk["id"], - ) - return trunk - - admin_context = n_context.get_admin_context() - LOG.info( - "Adding Palo Alto %s port %s to trunk %s as VLAN %s subport for router %s", - label, - port_id, - trunk["id"], - segmentation_id, - router["id"], - ) - # The trunk subport validator rejects a port that has device_id set - # (rules.py check_not_in_use). Router-owned ports have - # device_id=router_id, so clear it for the add and restore it - # afterwards so the router keeps its port association. - original_device_id = port["device_id"] - original_device_owner = port["device_owner"] - utils.clear_device_id_for_port(port_id) - try: - return self._trunk_plugin.add_subports( - admin_context, - trunk["id"], - { - "sub_ports": [ - { - "port_id": port_id, - "segmentation_type": "vlan", - "segmentation_id": segmentation_id, - } - ] - }, - ) - finally: - utils.set_device_id_and_owner_for_port( - port_id, original_device_id, original_device_owner - ) - - def _add_gateway_subport(self, router: dict, trunk: dict, gateway_port: dict): - """Add the gateway port to the trunk as a VLAN subport (idempotent).""" - port_id = gateway_port["id"] - if port_id in {sp["port_id"] for sp in trunk.get("sub_ports", [])}: - LOG.debug( - "Palo Alto gateway port %s already a subport on trunk %s", - port_id, - trunk["id"], - ) - return trunk - return self._add_router_port_subport( - router, - trunk, - gateway_port, - GATEWAY_SUBPORT_VLAN, - "gateway", - ) - - def _add_interface_subport(self, router: dict, trunk: dict, interface_port: dict): - """Add a router-interface port to the trunk as a VLAN subport.""" - port_id = interface_port["id"] - if port_id in {sp["port_id"] for sp in trunk.get("sub_ports", [])}: - LOG.debug( - "Palo Alto interface port %s already a subport on trunk %s", - port_id, - trunk["id"], - ) - return trunk - return self._add_router_port_subport( - router, - trunk, - interface_port, - self._next_available_subport_vlan( - router["id"], trunk, INTERFACE_SUBPORT_VLAN_START - ), - "interface", - ) - - def _remove_router_port_subport(self, trunk: dict, port_id: str, label: str): - """Remove a router-owned port from the trunk (idempotent). - - Fires the trunk driver's SUBPORTS delete events, which release the - fabric segment and update the switchport. No-ops if it is not a subport. - """ - existing = {sp["port_id"] for sp in trunk.get("sub_ports", [])} - if port_id not in existing: - LOG.debug( - "Palo Alto %s port %s is not a subport on trunk %s; skip removal", - label, - port_id, - trunk["id"], - ) - return trunk - admin_context = n_context.get_admin_context() - LOG.info( - "Removing Palo Alto %s subport %s from trunk %s", - label, - port_id, - trunk["id"], - ) - return self._trunk_plugin.remove_subports( - admin_context, - trunk["id"], - {"sub_ports": [{"port_id": port_id}]}, - ) - - def _remove_gateway_subport(self, trunk: dict, gateway_port_id: str): - """Remove the gateway port from the trunk (idempotent).""" - return self._remove_router_port_subport(trunk, gateway_port_id, "gateway") - - def _remove_interface_subport(self, trunk: dict, interface_port_id: str): - """Remove a router-interface port from the trunk (idempotent).""" - return self._remove_router_port_subport(trunk, interface_port_id, "interface") - - def _detach_and_delete_parent(self, router_id: str, parent_id: str) -> None: - """Detach the parent VIF from the node and delete the parent port.""" - node = self._ironic.node_by_instance_uuid(router_id) - if node is not None: - self._ironic.detach_vif_from_node(node, parent_id) - else: - LOG.warning( - "No node found for router %s while detaching parent %s", - router_id, - parent_id, - ) - core_plugin = directory.get_plugin() - admin_context = n_context.get_admin_context() - LOG.info("Deleting Palo Alto anchor parent port %s", parent_id) - core_plugin.delete_port(admin_context, parent_id) - - def _delete_parent_stack_if_unused(self, router_id: str, trunk: dict) -> None: - """Delete the trunk + parent port only if no subports remain. - - Re-reads the trunk so a subport removed just before this is reflected. If - other subports are still present (e.g. tenant subnets), leave the trunk - and parent for them to share. - """ - admin_context = n_context.get_admin_context() - fresh = self._trunk_plugin.get_trunk(admin_context, trunk["id"]) - if fresh.get("sub_ports"): - LOG.debug( - "Trunk %s still has subports; leaving parent stack for router %s", - trunk["id"], - router_id, - ) - return - parent_id = fresh["port_id"] - LOG.info( - "Deleting Palo Alto trunk %s (no subports left) for router %s", - trunk["id"], - router_id, - ) - self._trunk_plugin.delete_trunk(admin_context, trunk["id"]) - self._detach_and_delete_parent(router_id, parent_id) - - def _cleanup_router_port_attachment( - self, - router: dict, - port_id: str, - label: str, - ) -> None: - """Reverse of the add: remove subport, then tear down the parent stack. - - Handles a partially-built attach too: if a prior add failed after the - parent port was created/VIF-attached but before the trunk existed, there - is no trunk to key off, so find and tear down the orphan parent directly. - """ - router_id = router["id"] - trunk = self._trunk_for_router(router_id) - if trunk is None: - parent = self._parent_port_for_router(router_id) - if parent is not None: - LOG.info( - "No trunk for Palo Alto router %s; deleting orphan parent %s", - router_id, - parent["id"], - ) - self._detach_and_delete_parent(router_id, parent["id"]) - else: - LOG.debug( - "No trunk or parent for Palo Alto router %s; nothing to clean up", - router_id, - ) - return - self._remove_router_port_subport(trunk, port_id, label) - self._delete_parent_stack_if_unused(router_id, trunk) - - def _cleanup_gateway_attachment(self, router: dict, gateway_port: dict) -> None: - """Clean up a gateway port's Palo Alto trunk attachment.""" - self._cleanup_router_port_attachment(router, gateway_port["id"], "gateway") - - def _cleanup_interface_attachment(self, router: dict, interface_port: dict) -> None: - """Clean up a router-interface port's Palo Alto trunk attachment.""" - self._cleanup_router_port_attachment(router, interface_port["id"], "interface") - @registry.receives(resources.ROUTER, [events.BEFORE_CREATE]) def _process_router_create(self, resource, event, trigger, payload=None): """Realize the router on hardware, before the router row is created. @@ -771,7 +224,7 @@ def _process_router_create(self, resource, event, trigger, payload=None): # Ensure the shared anchor network first: it is idempotent and meant to # persist, so creating it before adoption never strands an adopted node. - self._ensure_anchor_network() + self._wiring.ensure_anchor_network() self._ironic.adopt_node_for_router( node, project_id=router.get("project_id"), @@ -828,33 +281,23 @@ def _process_gateway_create(self, resource, event, trigger, payload=None): subport is added -- the trunk driver only programs the switchport once the parent is bound. """ - context = payload.context router_id = payload.resource_id - router = self.l3plugin.get_router(context, router_id) - if not self._is_palo_alto_provider(context, router): + if self._palo_alto_router(payload.context, router_id) is None: return - gateway_port = self._gateway_port_for_router(router_id) + gateway_port = self._wiring.gateway_port_for_router(router_id) if gateway_port is None: - raise n_exc.BadRequest( - resource="router", - msg=( - f"Palo Alto router {router_id} gateway was created but no " - "router gateway port was found." - ), - ) + raise PaloAltoGatewayPortNotFound(router_id=router_id) - parent = self._ensure_parent_port(router) - parent = self._ensure_parent_vif_attached(router, parent) - trunk = self._ensure_trunk(router, parent) - self._add_gateway_subport(router, trunk, gateway_port) + stack = self._wiring.ensure_stack(router_id) + self._wiring.add_gateway_subport(router_id, stack.trunk, gateway_port) LOG.info( "Attached Palo Alto router %s gateway port %s via parent %s trunk %s", router_id, gateway_port["id"], - parent["id"], - trunk["id"], + stack.parent["id"], + stack.trunk["id"], ) def _process_gateway_delete(self, resource, event, trigger, payload=None): @@ -863,13 +306,11 @@ def _process_gateway_delete(self, resource, event, trigger, payload=None): The gateway port still exists at this point, so we can find it and remove its subport before Neutron deletes it. """ - context = payload.context router_id = payload.resource_id - router = self.l3plugin.get_router(context, router_id) - if not self._is_palo_alto_provider(context, router): + if self._palo_alto_router(payload.context, router_id) is None: return - gateway_port = self._gateway_port_for_router(router_id) + gateway_port = self._wiring.gateway_port_for_router(router_id) if gateway_port is None: LOG.debug( "Palo Alto router %s gateway cleanup skipped; gateway port not found", @@ -877,7 +318,7 @@ def _process_gateway_delete(self, resource, event, trigger, payload=None): ) return - self._cleanup_gateway_attachment(router, gateway_port) + self._wiring.cleanup_attachment(router_id, gateway_port["id"], "gateway") LOG.info( "Cleaned Palo Alto router %s gateway attachment (port %s)", router_id, @@ -886,48 +327,22 @@ def _process_gateway_delete(self, resource, event, trigger, payload=None): def _process_router_interface_create(self, resource, event, trigger, payload=None): """Wire the interface before Neutron commits its RouterPort association.""" - context = payload.context router_id = payload.resource_id - router = self.l3plugin.get_router(context, router_id) - if not self._is_palo_alto_provider(context, router): + if self._palo_alto_router(payload.context, router_id) is None: return interface_port = payload.metadata.get("port") if not interface_port: - raise n_exc.BadRequest( - resource="router", - msg=( - f"Palo Alto router {router_id} interface attachment has no " - "router interface port." - ), - ) + raise PaloAltoInterfacePortMissing(router_id=router_id) if interface_port.get("device_owner") not in const.ROUTER_INTERFACE_OWNERS: return - previous_parent = self._parent_port_for_router(router_id) - previous_trunk = self._trunk_for_router(router_id) - parent_vif_attached = False - if previous_parent is not None: - node = self._ironic.node_by_instance_uuid(router_id) - if node is not None: - vif_ids = self._ironic.node_vif_ids(node) - parent_vif_attached = previous_parent["id"] in vif_ids - self._interface_attachments[payload] = _InterfaceAttachment( - router_id=router_id, - port_id=interface_port["id"], - parent_id=previous_parent["id"] if previous_parent else None, - trunk_id=previous_trunk["id"] if previous_trunk else None, - subport_present=any( - sp["port_id"] == interface_port["id"] - for sp in (previous_trunk or {}).get("sub_ports", []) - ), - parent_vif_attached=parent_vif_attached, + self._interface_snapshots[payload] = self._wiring.snapshot_interface_attachment( + router_id, interface_port["id"] ) try: - parent = self._ensure_parent_port(router) - parent = self._ensure_parent_vif_attached(router, parent) - trunk = self._ensure_trunk(router, parent) - self._add_interface_subport(router, trunk, interface_port) + stack = self._wiring.ensure_stack(router_id) + self._wiring.add_interface_subport(router_id, stack.trunk, interface_port) except Exception: # Attach-by-port is reverted with a port update by Neutron, so it # cannot rely on PORT/BEFORE_DELETE to undo partial realization. @@ -938,47 +353,28 @@ def _process_router_interface_create(self, resource, event, trigger, payload=Non "Attached Palo Alto router %s interface port %s via parent %s trunk %s", router_id, interface_port["id"], - parent["id"], - trunk["id"], + stack.parent["id"], + stack.trunk["id"], ) - def _rollback_interface_attachment(self, payload): + def _rollback_interface_attachment(self, payload) -> None: """Undo this request's changes, including calls that failed postcommit.""" - attachment = self._interface_attachments.get(payload) - if attachment is None: + snapshot = self._interface_snapshots.get(payload) + if snapshot is None: return try: - trunk = self._trunk_for_router(attachment.router_id) - if trunk is not None: - if not attachment.subport_present: - self._remove_interface_subport(trunk, attachment.port_id) - admin_context = n_context.get_admin_context() - trunk = self._trunk_plugin.get_trunk(admin_context, trunk["id"]) - if trunk.get("sub_ports"): - # Other interfaces still need the shared parent and VIF. - self._interface_attachments.pop(payload, None) - return - if trunk["id"] != attachment.trunk_id: - self._trunk_plugin.delete_trunk(admin_context, trunk["id"]) - - parent = self._parent_port_for_router(attachment.router_id) - if parent is not None: - if parent["id"] != attachment.parent_id: - self._detach_and_delete_parent(attachment.router_id, parent["id"]) - elif not attachment.parent_vif_attached: - node = self._ironic.node_by_instance_uuid(attachment.router_id) - if node is not None: - self._ironic.detach_vif_from_node(node, parent["id"]) - self._interface_attachments.pop(payload, None) + self._wiring.undo_interface_attachment(snapshot) except Exception: # Preserve the original attach error. Keep the snapshot so the # subsequent ABORT_CREATE can retry compensation if it failed here. LOG.exception( "Failed to roll back Palo Alto router %s interface port %s; " "attachment cleanup is incomplete", - attachment.router_id, - attachment.port_id, + snapshot.router_id, + snapshot.port_id, ) + return + self._interface_snapshots.pop(payload, None) def _process_router_interface_abort(self, resource, event, trigger, payload=None): # The registry continues invoking callbacks after validation errors. An @@ -1007,8 +403,7 @@ def _process_router_interface_port_delete( return try: - router = self.l3plugin.get_router(context, router_id) - is_palo_alto = self._is_palo_alto_provider(context, router) + router = self._palo_alto_router(context, router_id) except Exception: LOG.debug( "Skipping Palo Alto interface cleanup for port %s; router %s " @@ -1019,7 +414,7 @@ def _process_router_interface_port_delete( ) return - if not is_palo_alto: + if router is None: return # Neutron also deletes newly-created ports when attachment aborts. That @@ -1030,7 +425,7 @@ def _process_router_interface_port_delete( ): return - self._cleanup_interface_attachment(router, port) + self._wiring.cleanup_attachment(router_id, port["id"], "interface") LOG.info( "Cleaned Palo Alto router %s interface attachment (port %s)", router_id, diff --git a/python/neutron-understack/neutron_understack/l3_router/palo_alto_wiring.py b/python/neutron-understack/neutron_understack/l3_router/palo_alto_wiring.py new file mode 100644 index 000000000..30120498b --- /dev/null +++ b/python/neutron-understack/neutron_understack/l3_router/palo_alto_wiring.py @@ -0,0 +1,673 @@ +"""Neutron and Ironic wiring for the Palo Alto router flavor. + +A router's gateway and interfaces reach its adopted Ironic node through one +stack: a parent port on the shared anchor network, VIF-attached to the node, +with a trunk whose VLAN subports are the router's own ports. This module holds +that wiring logic without any Neutron callback subscriptions. +""" + +import logging +from collections.abc import Callable +from collections.abc import Generator +from contextlib import contextmanager +from dataclasses import dataclass +from typing import Literal + +from neutron_lib import constants as const +from neutron_lib import context as n_context +from neutron_lib import exceptions as n_exc +from neutron_lib.api.definitions import portbindings +from neutron_lib.plugins import directory +from neutron_lib.services.trunk import constants as trunk_consts + +from neutron_understack import utils +from neutron_understack.ironic import IronicClient + +LOG = logging.getLogger(__name__) + +# Single shared sentinel network owned by the router flavor code. +ANCHOR_NETWORK_NAME = "palo_alto_router_anchor_network" + +# Deterministic per-router names. Deriving the parent port and trunk names from +# the router id lets the gateway teardown find them by name without depending on +# catching a specific delete event with the gateway port still visible. +ANCHOR_PARENT_PORT_NAME_PREFIX = "palo-alto-router-anchor" +TRUNK_NAME_PREFIX = "palo-alto-router-trunk" + +# Temporary fixed trunk subport tag. This keeps the subport segmentation_id in +# the existing allowed/gap VLAN validation path. Replace with a configured or +# allocated model once the trunk-tag semantics are revisited. +GATEWAY_SUBPORT_VLAN = 200 +INTERFACE_SUBPORT_VLAN_START = GATEWAY_SUBPORT_VLAN + 1 + +# Which kind of router port a trunk subport carries; used in log messages. +PortLabel = Literal["gateway", "interface"] + + +@dataclass(frozen=True) +class AttachmentSnapshot: + """Router wiring that existed before an interface attach request. + + Rollback compares current state against this, so it removes only what the + request added and leaves pre-existing parent, trunk and VIF in place. + """ + + router_id: str + port_id: str + parent_id: str | None + trunk_id: str | None + subport_present: bool + parent_vif_attached: bool + + +@dataclass(frozen=True) +class AttachmentStack: + """A router's VIF-bound anchor parent port and the trunk built on it.""" + + parent: dict + trunk: dict + + +# Conflict -> HTTP 409: every allowed VLAN on the router's trunk is in use. +class NoPaloAltoSubportVlanAvailable(n_exc.Conflict): + message = ( + "No Palo Alto trunk subport VLAN is available for router %(router_id)s " + "on trunk %(trunk_id)s. Allowed ranges: %(network_segment_ranges)s." + ) + + +# BadRequest -> HTTP 400: the router has no adopted node to wire. +class PaloAltoNodeNotAdopted(n_exc.BadRequest): + message = ( + "Palo Alto router %(router_id)s has no adopted Ironic node to attach " + "its anchor parent port to." + ) + + +# BadRequest -> HTTP 400: the node's baremetal port is missing enrollment data. +class PaloAltoParentNotAnnotated(n_exc.BadRequest): + message = ( + "Palo Alto router %(router_id)s parent port %(port_id)s was not " + "annotated by Ironic (missing %(missing)s); check the node's baremetal " + "port has physical_network." + ) + + +def _parent_port_name(router_id: str) -> str: + """Deterministic name for the router's anchor-network parent port.""" + return f"{ANCHOR_PARENT_PORT_NAME_PREFIX}-{router_id}" + + +def _trunk_name(router_id: str) -> str: + """Deterministic name for the router's trunk.""" + return f"{TRUNK_NAME_PREFIX}-{router_id}" + + +def _has_subport(trunk: dict, port_id: str) -> bool: + """Return True if the port is already a subport on the trunk.""" + return any(sp["port_id"] == port_id for sp in trunk.get("sub_ports", [])) + + +def _used_subport_vlans(trunk: dict) -> set[int]: + """Return VLAN segmentation IDs already used on a router trunk.""" + return { + sp["segmentation_id"] + for sp in trunk.get("sub_ports", []) + if sp.get("segmentation_type") == trunk_consts.SEGMENTATION_TYPE_VLAN + and sp.get("segmentation_id") is not None + } + + +def _first_free_vlan( + ranges: list[tuple[int, int]], used: set[int], start: int +) -> int | None: + """Return the lowest VLAN >= start that is in ranges and not used, or None.""" + for low, high in sorted(ranges): + for vlan in range(max(low, start), high + 1): + if vlan not in used: + return vlan + return None + + +@contextmanager +def _device_id_cleared(port: dict) -> Generator[None, None, None]: + """Clear the port's device_id for the block, then restore device_id + owner.""" + # Note: The trunk subport validator rejects a port that has device_id set + # (rules.py check_not_in_use). Router-owned ports have + # device_id=router_id, so clear it for the add and restore it + # afterwards so the router keeps its port association. + original_device_id = port["device_id"] + original_device_owner = port["device_owner"] + utils.clear_device_id_for_port(port["id"]) + try: + yield + finally: + utils.set_device_id_and_owner_for_port( + port["id"], original_device_id, original_device_owner + ) + + +def _missing_binding_fields(port: dict) -> list[str]: + """Return the binding fields Ironic should have set on the port but did not.""" + profile = port.get(portbindings.PROFILE) or {} + return [ + name + for name, value in ( + (portbindings.HOST_ID, port.get(portbindings.HOST_ID)), + ("physical_network", profile.get("physical_network")), + ("local_link_information", profile.get("local_link_information")), + ) + if not value + ] + + +class PaloAltoWiring: + """Build and tear down a Palo Alto router's parent, VIF and trunk stack. + + Holds no callback subscriptions, so code outside the L3 service provider + can use it too. Plugin and Ironic lookups are plain methods, not + properties, so inspecting this object (inspect.getmembers() evaluates + properties) never contacts Neutron or Ironic. + """ + + def __init__(self, ironic: Callable[[], IronicClient]): + # A getter rather than a client: the client is still created lazily, + # and the provider and the wiring share one instance. + self._get_ironic = ironic + + def _ironic(self) -> IronicClient: + return self._get_ironic() + + def _trunk_plugin(self): + return utils.fetch_trunk_plugin() + + def ensure_anchor_network(self) -> dict: + """Create the shared sentinel anchor network if it does not exist.""" + core_plugin = directory.get_plugin() + admin_context = n_context.get_admin_context() + existing = core_plugin.get_networks( + admin_context, filters={"name": [ANCHOR_NETWORK_NAME]} + ) + if existing: + LOG.debug( + "Reusing existing anchor network %s (id=%s)", + ANCHOR_NETWORK_NAME, + existing[0]["id"], + ) + return existing[0] + LOG.info("Creating shared anchor network %s", ANCHOR_NETWORK_NAME) + # Calling the core plugin directly (not via the REST API) skips the + # API layer that fills in extension-attribute defaults, so we must + # supply them ourselves. project_id (ownership) and router:external + # (read by the auto_allocate NETWORK-create callback) are required; + # without router:external the create fails with KeyError 'router:external'. + return core_plugin.create_network( + admin_context, + { + "network": { + "name": ANCHOR_NETWORK_NAME, + "admin_state_up": True, + "shared": False, + "router:external": False, + "project_id": admin_context.project_id or "", + } + }, + ) + + def gateway_port_for_router(self, router_id: str) -> dict | None: + """Return the router's Neutron external-gateway port, or None. + + The gateway port is owned by the router (``device_id == router_id``) with + ``device_owner == network:router_gateway``. + """ + core_plugin = directory.get_plugin() + admin_context = n_context.get_admin_context() + ports = core_plugin.get_ports( + admin_context, + filters={ + "device_id": [router_id], + "device_owner": [const.DEVICE_OWNER_ROUTER_GW], + }, + ) + if not ports: + LOG.debug("No gateway port found for Palo Alto router %s", router_id) + return None + if len(ports) > 1: + LOG.warning( + "Expected one gateway port for Palo Alto router %s, found %d; using %s", + router_id, + len(ports), + ports[0]["id"], + ) + return ports[0] + + def _parent_port_for_router(self, router_id: str) -> dict | None: + """Return the router's existing anchor-network parent port, or None.""" + core_plugin = directory.get_plugin() + admin_context = n_context.get_admin_context() + anchor_network = self.ensure_anchor_network() + ports = core_plugin.get_ports( + admin_context, + filters={ + "name": [_parent_port_name(router_id)], + "network_id": [anchor_network["id"]], + }, + ) + return ports[0] if ports else None + + def _create_parent_port(self, router_id: str) -> dict: + """Create the router's parent port on the anchor network. + + vnic_type=baremetal so Ironic can VIF-attach it to the adopted node. + Direct core-plugin call (server-side), so extension-default fields are + supplied explicitly, matching the codebase's other direct port creates. + """ + core_plugin = directory.get_plugin() + admin_context = n_context.get_admin_context() + anchor_network = self.ensure_anchor_network() + port_name = _parent_port_name(router_id) + LOG.info( + "Creating Palo Alto anchor parent port %s for router %s", + port_name, + router_id, + ) + return core_plugin.create_port( + admin_context, + { + "port": { + "name": port_name, + "network_id": anchor_network["id"], + "admin_state_up": True, + "device_owner": "", + "device_id": router_id, + "mac_address": "", + "fixed_ips": [], + "project_id": admin_context.project_id or "", + portbindings.VNIC_TYPE: portbindings.VNIC_BAREMETAL, + } + }, + ) + + def _ensure_parent_port(self, router_id: str) -> dict: + """Find-or-create the router's anchor-network parent port (idempotent).""" + existing = self._parent_port_for_router(router_id) + if existing is not None: + LOG.debug( + "Reusing Palo Alto anchor parent port %s for router %s", + existing["id"], + router_id, + ) + return existing + return self._create_parent_port(router_id) + + def _fresh_port(self, port_id: str) -> dict: + """Re-read a port so callers see its current binding profile.""" + core_plugin = directory.get_plugin() + admin_context = n_context.get_admin_context() + return core_plugin.get_port(admin_context, port_id) + + def _vif_landed_after_error( + self, router_id: str, node, parent_port_id: str + ) -> bool: + """Return True if an errored attach actually attached the VIF. + + Ironic VIF attach can time out waiting for the conductor/RPC reply even + after the conductor has applied the binding. + """ + try: + vif_ids = self._ironic().node_vif_ids(node) + except Exception: + LOG.debug( + "Could not re-check VIF attachments for Palo Alto router %s " + "after an attach error", + router_id, + exc_info=True, + ) + return False + return parent_port_id in vif_ids + + def _ensure_parent_vif_attached(self, router_id: str, parent_port: dict) -> dict: + """VIF-attach the parent port to the router's node (idempotent). + + Attaching a single VIF; Ironic binds it to a free baremetal port on the + node and annotates the Neutron port with local_link_information + + physical_network and host_id . + + Returns a fresh copy of the parent port reflecting the new binding. + """ + node = self._ironic().node_by_instance_uuid(router_id) + if node is None: + raise PaloAltoNodeNotAdopted(router_id=router_id) + + parent_port_id = parent_port["id"] + if parent_port_id in self._ironic().node_vif_ids(node): + LOG.debug( + "Parent port %s already VIF-attached to node %s", + parent_port_id, + node.id, + ) + else: + try: + self._ironic().attach_vif_to_node(node, parent_port_id) + except Exception: + if not self._vif_landed_after_error(router_id, node, parent_port_id): + raise + LOG.warning( + "Ironic VIF attach for Palo Alto router %s parent port %s " + "on node %s raised, but the VIF is attached; continuing", + router_id, + parent_port_id, + node.id, + exc_info=True, + ) + + fresh = self._fresh_port(parent_port_id) + self._verify_parent_annotated(router_id, fresh) + return fresh + + def _verify_parent_annotated(self, router_id: str, parent_port: dict) -> None: + """Fail fast if Ironic did not annotate the parent port. + + The trunk + undersync need host_id, physical_network and + local_link_information on the binding profile. If they are missing the + node's baremetal port most likely has no physical_network (enroll side); + surface a clear error here instead of a silent no-op at undersync. + """ + missing = _missing_binding_fields(parent_port) + if missing: + raise PaloAltoParentNotAnnotated( + router_id=router_id, + port_id=parent_port["id"], + missing=", ".join(missing), + ) + + def _trunk_for_router(self, router_id: str) -> dict | None: + """Return the router's existing trunk (by deterministic name), or None.""" + admin_context = n_context.get_admin_context() + trunks = self._trunk_plugin().get_trunks( + admin_context, filters={"name": [_trunk_name(router_id)]} + ) + return trunks[0] if trunks else None + + def _create_trunk(self, router_id: str, parent_port: dict) -> dict: + """Create the router's trunk with the parent port as its trunk parent.""" + admin_context = n_context.get_admin_context() + trunk_name = _trunk_name(router_id) + LOG.info( + "Creating Palo Alto trunk %s on parent port %s for router %s", + trunk_name, + parent_port["id"], + router_id, + ) + return self._trunk_plugin().create_trunk( + admin_context, + { + "trunk": { + "name": trunk_name, + "port_id": parent_port["id"], + "admin_state_up": True, + "project_id": admin_context.project_id or "", + "sub_ports": [], + } + }, + ) + + def _ensure_trunk(self, router_id: str, parent_port: dict) -> dict: + """Find-or-create the router's trunk (idempotent).""" + existing = self._trunk_for_router(router_id) + if existing is not None: + LOG.debug( + "Reusing Palo Alto trunk %s for router %s", + existing["id"], + router_id, + ) + return existing + return self._create_trunk(router_id, parent_port) + + def ensure_stack(self, router_id: str) -> AttachmentStack: + """Find or create the router's parent port, VIF attachment and trunk. + + Builds them in this order so the parent is VIF-bound before any subport + is added: the trunk driver only programs the switchport once the parent + is bound. + """ + parent = self._ensure_parent_port(router_id) + parent = self._ensure_parent_vif_attached(router_id, parent) + trunk = self._ensure_trunk(router_id, parent) + return AttachmentStack(parent=parent, trunk=trunk) + + def _next_available_subport_vlan( + self, router_id: str, trunk: dict, start_vlan: int + ) -> int: + """Pick the first allowed, unused Palo Alto subport VLAN from start.""" + ranges = utils.allowed_tenant_vlan_id_ranges() + vlan = _first_free_vlan(ranges, _used_subport_vlans(trunk), start_vlan) + if vlan is None: + raise NoPaloAltoSubportVlanAvailable( + router_id=router_id, + trunk_id=trunk["id"], + network_segment_ranges=utils.printable_ranges(sorted(ranges)), + ) + return vlan + + def _add_router_port_subport( + self, + router_id: str, + trunk: dict, + port: dict, + label: PortLabel, + pick_vlan: Callable[[], int], + ) -> dict: + """Add a router-owned port to the trunk as a VLAN subport. + + Adding the subport fires the understack trunk driver (SUBPORTS events), + which allocates the fabric segment, binds it, and calls undersync to + program the switch. No-ops if the port is already a subport. + + pick_vlan is called only when the port is actually added, so an + exhausted VLAN pool cannot fail an already-attached port. + """ + port_id = port["id"] + if _has_subport(trunk, port_id): + LOG.debug( + "Palo Alto %s port %s already a subport on trunk %s", + label, + port_id, + trunk["id"], + ) + return trunk + + segmentation_id = pick_vlan() + admin_context = n_context.get_admin_context() + LOG.info( + "Adding Palo Alto %s port %s to trunk %s as VLAN %s subport for router %s", + label, + port_id, + trunk["id"], + segmentation_id, + router_id, + ) + with _device_id_cleared(port): + return self._trunk_plugin().add_subports( + admin_context, + trunk["id"], + { + "sub_ports": [ + { + "port_id": port_id, + "segmentation_type": trunk_consts.SEGMENTATION_TYPE_VLAN, + "segmentation_id": segmentation_id, + } + ] + }, + ) + + def add_gateway_subport( + self, router_id: str, trunk: dict, gateway_port: dict + ) -> dict: + """Add the gateway port to the trunk as a VLAN subport (idempotent).""" + return self._add_router_port_subport( + router_id, trunk, gateway_port, "gateway", lambda: GATEWAY_SUBPORT_VLAN + ) + + def add_interface_subport( + self, router_id: str, trunk: dict, interface_port: dict + ) -> dict: + """Add a router-interface port to the trunk as a VLAN subport.""" + return self._add_router_port_subport( + router_id, + trunk, + interface_port, + "interface", + lambda: self._next_available_subport_vlan( + router_id, trunk, INTERFACE_SUBPORT_VLAN_START + ), + ) + + def _remove_router_port_subport( + self, trunk: dict, port_id: str, label: PortLabel + ) -> dict: + """Remove a router-owned port from the trunk (idempotent). + + Fires the trunk driver's SUBPORTS delete events, which release the + fabric segment and update the switchport. No-ops if it is not a subport. + """ + if not _has_subport(trunk, port_id): + LOG.debug( + "Palo Alto %s port %s is not a subport on trunk %s; skip removal", + label, + port_id, + trunk["id"], + ) + return trunk + admin_context = n_context.get_admin_context() + LOG.info( + "Removing Palo Alto %s subport %s from trunk %s", + label, + port_id, + trunk["id"], + ) + return self._trunk_plugin().remove_subports( + admin_context, + trunk["id"], + {"sub_ports": [{"port_id": port_id}]}, + ) + + def _detach_and_delete_parent(self, router_id: str, parent_id: str) -> None: + """Detach the parent VIF from the node and delete the parent port.""" + node = self._ironic().node_by_instance_uuid(router_id) + if node is not None: + self._ironic().detach_vif_from_node(node, parent_id) + else: + LOG.warning( + "No node found for router %s while detaching parent %s", + router_id, + parent_id, + ) + core_plugin = directory.get_plugin() + admin_context = n_context.get_admin_context() + LOG.info("Deleting Palo Alto anchor parent port %s", parent_id) + core_plugin.delete_port(admin_context, parent_id) + + def _delete_parent_stack_if_unused(self, router_id: str, trunk: dict) -> None: + """Delete the trunk + parent port only if no subports remain. + + Re-reads the trunk so a subport removed just before this is reflected. If + other subports are still present (e.g. tenant subnets), leave the trunk + and parent for them to share. + """ + admin_context = n_context.get_admin_context() + fresh = self._trunk_plugin().get_trunk(admin_context, trunk["id"]) + if fresh.get("sub_ports"): + LOG.debug( + "Trunk %s still has subports; leaving parent stack for router %s", + trunk["id"], + router_id, + ) + return + parent_id = fresh["port_id"] + LOG.info( + "Deleting Palo Alto trunk %s (no subports left) for router %s", + trunk["id"], + router_id, + ) + self._trunk_plugin().delete_trunk(admin_context, trunk["id"]) + self._detach_and_delete_parent(router_id, parent_id) + + def cleanup_attachment( + self, + router_id: str, + port_id: str, + label: PortLabel, + ) -> None: + """Reverse of the add: remove subport, then tear down the parent stack. + + Handles a partially-built attach too: if a prior add failed after the + parent port was created/VIF-attached but before the trunk existed, there + is no trunk to key off, so find and tear down the orphan parent directly. + """ + trunk = self._trunk_for_router(router_id) + if trunk is None: + parent = self._parent_port_for_router(router_id) + if parent is not None: + LOG.info( + "No trunk for Palo Alto router %s; deleting orphan parent %s", + router_id, + parent["id"], + ) + self._detach_and_delete_parent(router_id, parent["id"]) + else: + LOG.debug( + "No trunk or parent for Palo Alto router %s; nothing to clean up", + router_id, + ) + return + self._remove_router_port_subport(trunk, port_id, label) + self._delete_parent_stack_if_unused(router_id, trunk) + + def snapshot_interface_attachment( + self, router_id: str, port_id: str + ) -> AttachmentSnapshot: + """Record which wiring already exists before an interface attach.""" + parent = self._parent_port_for_router(router_id) + trunk = self._trunk_for_router(router_id) + parent_vif_attached = False + if parent is not None: + node = self._ironic().node_by_instance_uuid(router_id) + if node is not None: + parent_vif_attached = parent["id"] in self._ironic().node_vif_ids(node) + return AttachmentSnapshot( + router_id=router_id, + port_id=port_id, + parent_id=parent["id"] if parent else None, + trunk_id=trunk["id"] if trunk else None, + subport_present=trunk is not None and _has_subport(trunk, port_id), + parent_vif_attached=parent_vif_attached, + ) + + def undo_interface_attachment(self, snapshot: AttachmentSnapshot) -> None: + """Return the router's wiring to the state recorded in the snapshot. + + Re-reads current state rather than replaying the calls that succeeded, + because a remote call can apply its change and still raise. + """ + trunk = self._trunk_for_router(snapshot.router_id) + if trunk is not None: + if not snapshot.subport_present: + self._remove_router_port_subport(trunk, snapshot.port_id, "interface") + admin_context = n_context.get_admin_context() + trunk = self._trunk_plugin().get_trunk(admin_context, trunk["id"]) + if trunk.get("sub_ports"): + # Other interfaces still need the shared parent and VIF. + return + if trunk["id"] != snapshot.trunk_id: + self._trunk_plugin().delete_trunk(admin_context, trunk["id"]) + + parent = self._parent_port_for_router(snapshot.router_id) + if parent is not None: + if parent["id"] != snapshot.parent_id: + self._detach_and_delete_parent(snapshot.router_id, parent["id"]) + elif not snapshot.parent_vif_attached: + node = self._ironic().node_by_instance_uuid(snapshot.router_id) + if node is not None: + self._ironic().detach_vif_from_node(node, parent["id"]) diff --git a/python/neutron-understack/neutron_understack/tests/test_palo_alto_provider.py b/python/neutron-understack/neutron_understack/tests/test_palo_alto_provider.py index 1a5a99802..a5b382e19 100644 --- a/python/neutron-understack/neutron_understack/tests/test_palo_alto_provider.py +++ b/python/neutron-understack/neutron_understack/tests/test_palo_alto_provider.py @@ -18,6 +18,7 @@ from neutron_understack.ironic import NodeReleaseResult from neutron_understack.l3_router import palo_alto +from neutron_understack.l3_router import palo_alto_wiring @pytest.fixture(autouse=True) @@ -61,7 +62,7 @@ def _make_provider(mocker, flavor_plugin, ironic=None, core_plugin=None): side_effect=lambda *a: flavor_plugin if a else core_plugin, ) # get_admin_context() would init oslo policy; not needed for these tests. - mocker.patch.object(palo_alto.n_context, "get_admin_context") + mocker.patch.object(palo_alto_wiring.n_context, "get_admin_context") provider = palo_alto.PaloAlto(mocker.Mock()) provider._flavor_plugin_ref = flavor_plugin if ironic is not None: @@ -69,6 +70,35 @@ def _make_provider(mocker, flavor_plugin, ironic=None, core_plugin=None): return provider +# Driver of a flavor that is not Palo Alto. +_OTHER_DRIVER = "neutron_understack.l3_router.vrf.Vrf" + + +@pytest.fixture +def provider(mocker): + """A provider whose router r1 uses the Palo Alto flavor.""" + provider = _make_provider(mocker, FakeFlavorPlugin(_palo_alto_driver())) + provider.l3plugin.get_router.return_value = {"id": "r1", "flavor_id": "f1"} + return provider + + +@pytest.fixture +def other_provider(mocker): + """A provider whose router r1 uses another flavor.""" + provider = _make_provider(mocker, FakeFlavorPlugin(_OTHER_DRIVER)) + provider.l3plugin.get_router.return_value = {"id": "r1", "flavor_id": "f1"} + return provider + + +def _payload(mocker, resource_id="r1", **metadata): + """A handler event payload; metadata is what Neutron passes (port, port_check).""" + payload = mocker.Mock() + payload.context = "ctx" + payload.resource_id = resource_id + payload.metadata = metadata + return payload + + class TestMetainfoParsing: def test_parses_json_string(self): assert _parse('{"resource_class": "BLAH"}') == {"resource_class": "BLAH"} @@ -85,7 +115,7 @@ def _parse(raw): return palo_alto._parse_metainfo(raw) -class TestPaloAltoProvider: +class TestFlavorCheck: def test_flavor_plugin_is_cached(self, mocker): plugin = FakeFlavorPlugin(_palo_alto_driver()) get_plugin = mocker.patch.object( @@ -125,7 +155,7 @@ def test_is_palo_alto_provider_returns_true_for_palo_alto_driver(self, mocker): ) def test_is_palo_alto_provider_returns_false_for_different_driver(self, mocker): - plugin = FakeFlavorPlugin("neutron_understack.l3_router.vrf.Vrf") + plugin = FakeFlavorPlugin(_OTHER_DRIVER) mocker.patch.object(palo_alto.directory, "get_plugin", return_value=plugin) provider = palo_alto.PaloAlto(None) @@ -137,6 +167,49 @@ def test_is_palo_alto_provider_returns_false_for_different_driver(self, mocker): is False ) + def test_palo_alto_router_returns_router_for_palo_alto_flavor(self, mocker): + provider = _make_provider(mocker, FakeFlavorPlugin(_palo_alto_driver())) + router = {"id": "r1", "flavor_id": "f1"} + provider.l3plugin.get_router.return_value = router + + assert provider._palo_alto_router("ctx", "r1") is router + provider.l3plugin.get_router.assert_called_once_with("ctx", "r1") + + @pytest.mark.parametrize( + ("driver", "router"), + [ + (_OTHER_DRIVER, {"id": "r1", "flavor_id": "f1"}), + (_palo_alto_driver(), {"id": "r1"}), + ], + ids=["other-flavor", "no-flavor"], + ) + def test_palo_alto_router_returns_none_otherwise(self, mocker, driver, router): + provider = _make_provider(mocker, FakeFlavorPlugin(driver)) + provider.l3plugin.get_router.return_value = router + + assert provider._palo_alto_router("ctx", "r1") is None + + def test_palo_alto_router_propagates_lookup_errors(self, mocker): + # Callers decide: most handlers let it raise, port delete skips cleanup. + provider = _make_provider(mocker, FakeFlavorPlugin(_palo_alto_driver())) + provider.l3plugin.get_router.side_effect = RuntimeError("db hiccup") + + with pytest.raises(RuntimeError, match="db hiccup"): + provider._palo_alto_router("ctx", "r1") + + +class TestIronicClient: + def test_created_lazily_once_and_shared_with_wiring(self, mocker): + ironic_client = mocker.patch.object(palo_alto, "IronicClient") + + provider = palo_alto.PaloAlto(mocker.Mock()) + + # Loading the provider must not need Ironic credentials. + ironic_client.assert_not_called() + assert provider._ironic is provider._ironic + assert provider._wiring._ironic() is provider._ironic + ironic_client.assert_called_once_with() + class TestExceptionHttpCodes: """Exceptions must map to real HTTP codes, not 500. @@ -151,8 +224,15 @@ def test_no_node_available_is_conflict(self): def test_flavor_misconfigured_is_bad_request(self): assert issubclass(palo_alto.PaloAltoFlavorMisconfigured, n_exc.BadRequest) - def test_no_subport_vlan_available_is_conflict(self): - assert issubclass(palo_alto.NoPaloAltoSubportVlanAvailable, n_exc.Conflict) + @pytest.mark.parametrize( + "exc", + [ + palo_alto.PaloAltoGatewayPortNotFound, + palo_alto.PaloAltoInterfacePortMissing, + ], + ) + def test_handler_errors_are_bad_request(self, exc): + assert issubclass(exc, n_exc.BadRequest) class TestResourceClassLookup: @@ -297,7 +377,7 @@ def test_propagates_adopt_failure(self, mocker): def test_skips_non_palo_alto_router(self, mocker): ironic = mocker.Mock() - plugin = FakeFlavorPlugin("neutron_understack.l3_router.vrf.Vrf") + plugin = FakeFlavorPlugin(_OTHER_DRIVER) provider = _make_provider(mocker, plugin, ironic=ironic) provider._process_router_create( @@ -357,7 +437,7 @@ def test_warns_when_release_is_pending_reconciliation(self, mocker, caplog): def test_skips_non_palo_alto_router(self, mocker): ironic = mocker.Mock() - plugin = FakeFlavorPlugin("neutron_understack.l3_router.vrf.Vrf") + plugin = FakeFlavorPlugin(_OTHER_DRIVER) provider = _make_provider(mocker, plugin, ironic=ironic) provider._process_router_delete( @@ -366,185 +446,127 @@ def test_skips_non_palo_alto_router(self, mocker): ironic.release_node_for_router.assert_not_called() -class TestGatewayLookups: - def test_names_are_deterministic(self, mocker): - provider = _make_provider(mocker, FakeFlavorPlugin(_palo_alto_driver())) - assert provider._parent_port_name("r1") == "palo-alto-router-anchor-r1" - assert provider._trunk_name("r1") == "palo-alto-router-trunk-r1" - - def test_trunk_plugin_delegates_to_utils(self, mocker): - tp = mocker.Mock() - mocker.patch.object(palo_alto.utils, "fetch_trunk_plugin", return_value=tp) - provider = _make_provider(mocker, FakeFlavorPlugin(_palo_alto_driver())) - assert provider._trunk_plugin is tp - - def test_gateway_port_found_filters_by_owner_and_router(self, mocker): - core_plugin = mocker.Mock() - core_plugin.get_ports.return_value = [{"id": "gw-1"}] - provider = _make_provider( - mocker, FakeFlavorPlugin(_palo_alto_driver()), core_plugin=core_plugin - ) +class TestEventSubscriptions: + """Publish through the real registry and check what Neutron's caller sees. - assert provider._gateway_port_for_router("r1") == {"id": "gw-1"} - _args, kwargs = core_plugin.get_ports.call_args - assert kwargs["filters"]["device_id"] == ["r1"] - assert kwargs["filters"]["device_owner"] == ["network:router_gateway"] + These pin the event phase and cancellable flag of each subscription, which + calling a handler directly cannot. They mock only the plugins and Ironic, + never provider internals, so they hold across internal refactors. + """ - def test_gateway_port_none_when_absent(self, mocker): - core_plugin = mocker.Mock() - core_plugin.get_ports.return_value = [] - provider = _make_provider( - mocker, FakeFlavorPlugin(_palo_alto_driver()), core_plugin=core_plugin - ) - assert provider._gateway_port_for_router("r1") is None + _ROUTER = {"id": "r1", "name": "pa-router", "project_id": "p1", "flavor_id": "f1"} + def _payload(self, **kwargs): + return events.DBEventPayload("ctx", resource_id="r1", **kwargs) -class TestParentPort: - def _core_plugin(self, mocker, ports): - core = mocker.Mock() - core.get_networks.return_value = [{"id": "anchor-net"}] # anchor exists - core.get_ports.return_value = ports - core.create_port.return_value = {"id": "parent-new"} - return core + def test_router_create_failure_aborts_the_create(self, mocker): + ironic = mocker.Mock() + ironic.available_node_for_resource_class.return_value = None + _make_provider(mocker, _adopting_plugin(), ironic=ironic) + + with pytest.raises(callback_exc.CallbackFailure) as failure: + registry.publish( + resources.ROUTER, + events.BEFORE_CREATE, + self, + payload=self._payload(states=(dict(self._ROUTER),)), + ) - def test_creates_parent_when_absent(self, mocker): - core = self._core_plugin(mocker, ports=[]) - provider = _make_provider( - mocker, FakeFlavorPlugin(_palo_alto_driver()), core_plugin=core + assert any( + isinstance(exc, palo_alto.NoNetdevNodeAvailable) + for exc in failure.value.inner_exceptions ) - port = provider._ensure_parent_port({"id": "r1"}) - - assert port == {"id": "parent-new"} - core.create_port.assert_called_once() - _ctx, body = core.create_port.call_args[0] - net = body["port"] - assert net["name"] == "palo-alto-router-anchor-r1" - assert net["network_id"] == "anchor-net" - assert net["binding:vnic_type"] == "baremetal" - assert net["device_id"] == "r1" - - def test_reuses_existing_parent(self, mocker): - core = self._core_plugin(mocker, ports=[{"id": "parent-existing"}]) - provider = _make_provider( - mocker, FakeFlavorPlugin(_palo_alto_driver()), core_plugin=core + def test_router_node_released_after_delete_not_before(self, mocker): + # Releasing on BEFORE_DELETE would free the node even when Neutron + # then rejects the delete (for example, router still in use). + ironic = mocker.Mock() + ironic.release_node_for_router.return_value = NodeReleaseResult( + node=mocker.Mock(id="node-1"), released=True ) + _make_provider(mocker, FakeFlavorPlugin(_palo_alto_driver()), ironic=ironic) + payload = self._payload(states=(dict(self._ROUTER),)) - port = provider._ensure_parent_port({"id": "r1"}) + registry.publish(resources.ROUTER, events.BEFORE_DELETE, self, payload=payload) + ironic.release_node_for_router.assert_not_called() - assert port == {"id": "parent-existing"} - core.create_port.assert_not_called() + registry.publish(resources.ROUTER, events.AFTER_DELETE, self, payload=payload) + ironic.release_node_for_router.assert_called_once_with("r1") + def test_router_release_error_does_not_fail_the_delete(self, mocker): + # The router row is already gone; reconciliation retries the release. + ironic = mocker.Mock() + ironic.release_node_for_router.side_effect = RuntimeError("ironic down") + _make_provider(mocker, FakeFlavorPlugin(_palo_alto_driver()), ironic=ironic) -_ANNOTATED_PARENT = { - "id": "parent-1", - "binding:host_id": "node-1", - "binding:profile": { - "physical_network": "n11-22-network", - "local_link_information": [{"switch_id": "aa", "port_id": "Eth1/1"}], - }, -} + registry.publish( + resources.ROUTER, + events.AFTER_DELETE, + self, + payload=self._payload(states=(dict(self._ROUTER),)), + ) + ironic.release_node_for_router.assert_called_once_with("r1") -class TestParentVifAttach: - def _provider(self, mocker, node, vif_ids, fresh_port=None): - ironic = mocker.Mock() - ironic.node_by_instance_uuid.return_value = node - ironic.node_vif_ids.return_value = vif_ids + def test_gateway_wiring_error_reaches_the_caller(self, mocker): + # ROUTER_GATEWAY/AFTER_CREATE only re-raises for a cancellable + # subscriber; otherwise the API would return success for a gateway + # that was never wired. core = mocker.Mock() - core.get_port.return_value = fresh_port or _ANNOTATED_PARENT + core.get_ports.return_value = [] # gateway port not found provider = _make_provider( - mocker, - FakeFlavorPlugin(_palo_alto_driver()), - ironic=ironic, - core_plugin=core, + mocker, FakeFlavorPlugin(_palo_alto_driver()), core_plugin=core ) - return provider, ironic - - def test_attaches_when_not_already(self, mocker): - node = mocker.Mock(id="node-1") - provider, ironic = self._provider(mocker, node, vif_ids=[]) - - result = provider._ensure_parent_vif_attached({"id": "r1"}, {"id": "parent-1"}) - - ironic.attach_vif_to_node.assert_called_once_with(node, "parent-1") - assert result == _ANNOTATED_PARENT # fresh, annotated copy returned - - def test_recovers_when_attach_raises_after_vif_lands(self, mocker): - node = mocker.Mock(id="node-1") - provider, ironic = self._provider(mocker, node, vif_ids=[]) - ironic.node_vif_ids.side_effect = [[], ["parent-1"]] - ironic.attach_vif_to_node.side_effect = RuntimeError("rpc timeout") - - result = provider._ensure_parent_vif_attached({"id": "r1"}, {"id": "parent-1"}) - - ironic.attach_vif_to_node.assert_called_once_with(node, "parent-1") - assert result == _ANNOTATED_PARENT - - def test_reraises_attach_error_when_vif_did_not_land(self, mocker): - node = mocker.Mock(id="node-1") - provider, ironic = self._provider(mocker, node, vif_ids=[]) - ironic.node_vif_ids.side_effect = [[], []] - ironic.attach_vif_to_node.side_effect = RuntimeError("rpc timeout") - - with pytest.raises(RuntimeError, match="rpc timeout"): - provider._ensure_parent_vif_attached({"id": "r1"}, {"id": "parent-1"}) - - def test_skips_attach_when_already_attached(self, mocker): - node = mocker.Mock(id="node-1") - provider, ironic = self._provider(mocker, node, vif_ids=["parent-1"]) - - provider._ensure_parent_vif_attached({"id": "r1"}, {"id": "parent-1"}) - - ironic.attach_vif_to_node.assert_not_called() + provider.l3plugin.get_router.return_value = dict(self._ROUTER) - def test_raises_when_no_adopted_node(self, mocker): - provider, ironic = self._provider(mocker, node=None, vif_ids=[]) - - with pytest.raises(n_exc.BadRequest): - provider._ensure_parent_vif_attached({"id": "r1"}, {"id": "parent-1"}) - ironic.attach_vif_to_node.assert_not_called() - - def test_raises_when_parent_not_annotated(self, mocker): - # e.g. the enrolled baremetal port had no physical_network - node = mocker.Mock(id="node-1") - unannotated = {"id": "parent-1", "binding:host_id": "", "binding:profile": {}} - provider, _ = self._provider(mocker, node, vif_ids=[], fresh_port=unannotated) - - with pytest.raises(n_exc.BadRequest): - provider._ensure_parent_vif_attached({"id": "r1"}, {"id": "parent-1"}) - - -class TestTrunk: - def _provider_with_trunk(self, mocker, existing_trunks): - tp = mocker.Mock() - tp.get_trunks.return_value = existing_trunks - tp.create_trunk.return_value = {"id": "trunk-new"} - mocker.patch.object(palo_alto.utils, "fetch_trunk_plugin", return_value=tp) - provider = _make_provider(mocker, FakeFlavorPlugin(_palo_alto_driver())) - return provider, tp - - def test_creates_trunk_when_absent(self, mocker): - provider, tp = self._provider_with_trunk(mocker, existing_trunks=[]) - - trunk = provider._ensure_trunk({"id": "r1"}, {"id": "parent-1"}) + with pytest.raises(callback_exc.CallbackFailure) as failure: + registry.publish( + resources.ROUTER_GATEWAY, + events.AFTER_CREATE, + self, + payload=self._payload(), + ) - assert trunk == {"id": "trunk-new"} - tp.create_trunk.assert_called_once() - _ctx, body = tp.create_trunk.call_args[0] - assert body["trunk"]["name"] == "palo-alto-router-trunk-r1" - assert body["trunk"]["port_id"] == "parent-1" - assert body["trunk"]["sub_ports"] == [] + assert any( + isinstance(exc, palo_alto.PaloAltoGatewayPortNotFound) + for exc in failure.value.inner_exceptions + ) - def test_reuses_existing_trunk(self, mocker): - provider, tp = self._provider_with_trunk( - mocker, existing_trunks=[{"id": "trunk-existing"}] + def test_gateway_teardown_runs_before_gateway_port_delete(self, mocker): + # BEFORE_DELETE is the last point where the gateway port still exists. + core = mocker.Mock() + core.get_ports.return_value = [dict(_GATEWAY_PORT)] + trunk_plugin = mocker.Mock() + trunk_plugin.get_trunks.return_value = [ + { + "id": "trunk-1", + "port_id": "parent-1", + "sub_ports": [{"port_id": "gw-1"}, {"port_id": "intf-1"}], + } + ] + trunk_plugin.get_trunk.return_value = { + "id": "trunk-1", + "port_id": "parent-1", + "sub_ports": [{"port_id": "intf-1"}], + } + mocker.patch( + "neutron_understack.utils.fetch_trunk_plugin", return_value=trunk_plugin ) + provider = _make_provider( + mocker, FakeFlavorPlugin(_palo_alto_driver()), core_plugin=core + ) + provider.l3plugin.get_router.return_value = dict(self._ROUTER) - trunk = provider._ensure_trunk({"id": "r1"}, {"id": "parent-1"}) + registry.publish( + resources.ROUTER_GATEWAY, + events.BEFORE_DELETE, + self, + payload=self._payload(), + ) - assert trunk == {"id": "trunk-existing"} - tp.create_trunk.assert_not_called() + trunk_plugin.remove_subports.assert_called_once() + _ctx, trunk_id, body = trunk_plugin.remove_subports.call_args[0] + assert (trunk_id, body) == ("trunk-1", {"sub_ports": [{"port_id": "gw-1"}]}) _GATEWAY_PORT = { @@ -560,458 +582,170 @@ def test_reuses_existing_trunk(self, mocker): } -class TestGatewaySubport: - def _provider(self, mocker): - tp = mocker.Mock() - tp.add_subports.return_value = {"id": "trunk-1", "updated": True} - mocker.patch.object(palo_alto.utils, "fetch_trunk_plugin", return_value=tp) - self.clear = mocker.patch.object(palo_alto.utils, "clear_device_id_for_port") - self.restore = mocker.patch.object( - palo_alto.utils, "set_device_id_and_owner_for_port" - ) - provider = _make_provider(mocker, FakeFlavorPlugin(_palo_alto_driver())) - return provider, tp - - def test_adds_subport_with_fixed_vlan(self, mocker): - provider, tp = self._provider(mocker) - trunk = {"id": "trunk-1", "sub_ports": []} - - provider._add_gateway_subport({"id": "r1"}, trunk, dict(_GATEWAY_PORT)) - - tp.add_subports.assert_called_once() - _ctx, trunk_id, body = tp.add_subports.call_args[0] - assert trunk_id == "trunk-1" - sub = body["sub_ports"][0] - assert sub["port_id"] == "gw-1" - assert sub["segmentation_type"] == "vlan" - assert sub["segmentation_id"] == palo_alto.GATEWAY_SUBPORT_VLAN - # device_id cleared for the add (trunk validator rejects it) and restored - self.clear.assert_called_once_with("gw-1") - self.restore.assert_called_once_with("gw-1", "r1", "network:router_gateway") - - def test_add_subport_is_idempotent(self, mocker): - provider, tp = self._provider(mocker) - trunk = { - "id": "trunk-1", - "sub_ports": [ - { - "port_id": "gw-1", - "segmentation_id": palo_alto.GATEWAY_SUBPORT_VLAN, - } - ], - } - - provider._add_gateway_subport({"id": "r1"}, trunk, dict(_GATEWAY_PORT)) - - tp.add_subports.assert_not_called() - self.clear.assert_not_called() - - -class TestSubportVlanAllocation: - def _provider(self, mocker, ranges): +class TestGatewayCreateHandler: + def test_orchestrates_in_order(self, mocker, provider): mocker.patch.object( - palo_alto.utils, - "allowed_tenant_vlan_id_ranges", - return_value=ranges, + provider._wiring, "gateway_port_for_router", return_value={"id": "gw-1"} ) - return _make_provider(mocker, FakeFlavorPlugin(_palo_alto_driver())) - - def test_starts_at_requested_vlan(self, mocker): - provider = self._provider(mocker, ranges=[(1, 199), (200, 202)]) - trunk = {"id": "trunk-1", "sub_ports": []} - - assert ( - provider._next_available_subport_vlan( - "r1", trunk, palo_alto.INTERFACE_SUBPORT_VLAN_START - ) - == 201 + stack = palo_alto_wiring.AttachmentStack( + parent={"id": "parent-1"}, trunk={"id": "trunk-1"} ) - - def test_skips_used_vlans(self, mocker): - provider = self._provider(mocker, ranges=[(200, 202)]) - trunk = { - "id": "trunk-1", - "sub_ports": [ - { - "port_id": "gw-1", - "segmentation_type": "vlan", - "segmentation_id": 200, - }, - { - "port_id": "intf-1", - "segmentation_type": "vlan", - "segmentation_id": 201, - }, - ], - } - - assert ( - provider._next_available_subport_vlan( - "r1", trunk, palo_alto.INTERFACE_SUBPORT_VLAN_START - ) - == 202 + calls = mocker.Mock() + calls.ensure_stack.return_value = stack + mocker.patch.object(provider._wiring, "ensure_stack", calls.ensure_stack) + mocker.patch.object( + provider._wiring, "add_gateway_subport", calls.add_gateway_subport ) - def test_raises_when_no_vlan_available(self, mocker): - provider = self._provider(mocker, ranges=[(200, 201)]) - trunk = { - "id": "trunk-1", - "sub_ports": [ - { - "port_id": "gw-1", - "segmentation_type": "vlan", - "segmentation_id": 200, - }, - { - "port_id": "intf-1", - "segmentation_type": "vlan", - "segmentation_id": 201, - }, - ], - } - - with pytest.raises(palo_alto.NoPaloAltoSubportVlanAvailable): - provider._next_available_subport_vlan( - "r1", trunk, palo_alto.INTERFACE_SUBPORT_VLAN_START - ) + provider._process_gateway_create("r", "e", "t", _payload(mocker)) + # The VIF-bound stack exists before the subport is added to its trunk. + assert calls.mock_calls == [ + mocker.call.ensure_stack("r1"), + mocker.call.add_gateway_subport("r1", stack.trunk, {"id": "gw-1"}), + ] -class TestInterfaceSubport: - def _provider(self, mocker): - tp = mocker.Mock() - tp.add_subports.return_value = {"id": "trunk-1", "updated": True} - mocker.patch.object(palo_alto.utils, "fetch_trunk_plugin", return_value=tp) - self.clear = mocker.patch.object(palo_alto.utils, "clear_device_id_for_port") - self.restore = mocker.patch.object( - palo_alto.utils, "set_device_id_and_owner_for_port" - ) - provider = _make_provider(mocker, FakeFlavorPlugin(_palo_alto_driver())) - return provider, tp + def test_skips_non_palo_alto_router(self, mocker, other_provider): + m_parent = mocker.patch.object(other_provider._wiring, "_ensure_parent_port") - def test_adds_subport_with_next_available_vlan(self, mocker): - provider, tp = self._provider(mocker) - next_vlan = mocker.patch.object( - provider, "_next_available_subport_vlan", return_value=201 - ) - trunk = { - "id": "trunk-1", - "sub_ports": [ - { - "port_id": "gw-1", - "segmentation_type": "vlan", - "segmentation_id": palo_alto.GATEWAY_SUBPORT_VLAN, - } - ], - } + other_provider._process_gateway_create("r", "e", "t", _payload(mocker)) - provider._add_interface_subport({"id": "r1"}, trunk, dict(_INTERFACE_PORT)) + m_parent.assert_not_called() - next_vlan.assert_called_once_with( - "r1", trunk, palo_alto.INTERFACE_SUBPORT_VLAN_START + def test_raises_when_gateway_port_missing(self, mocker, provider): + mocker.patch.object( + provider._wiring, "gateway_port_for_router", return_value=None ) - tp.add_subports.assert_called_once() - _ctx, trunk_id, body = tp.add_subports.call_args[0] - assert trunk_id == "trunk-1" - sub = body["sub_ports"][0] - assert sub["port_id"] == "intf-1" - assert sub["segmentation_type"] == "vlan" - assert sub["segmentation_id"] == 201 - self.clear.assert_called_once_with("intf-1") - self.restore.assert_called_once_with("intf-1", "r1", "network:router_interface") - - def test_add_subport_is_idempotent(self, mocker): - provider, tp = self._provider(mocker) - next_vlan = mocker.patch.object(provider, "_next_available_subport_vlan") - trunk = { - "id": "trunk-1", - "sub_ports": [ - { - "port_id": "intf-1", - "segmentation_type": "vlan", - "segmentation_id": 201, - } - ], - } - - provider._add_interface_subport({"id": "r1"}, trunk, dict(_INTERFACE_PORT)) - - next_vlan.assert_not_called() - tp.add_subports.assert_not_called() - self.clear.assert_not_called() + with pytest.raises(palo_alto.PaloAltoGatewayPortNotFound, match="router r1 "): + provider._process_gateway_create("r", "e", "t", _payload(mocker)) -class TestGatewayCreateHandler: - def _payload(self, mocker, router_id="r1"): - payload = mocker.Mock() - payload.context = "ctx" - payload.resource_id = router_id - return payload - def test_orchestrates_in_order(self, mocker): - provider = _make_provider(mocker, FakeFlavorPlugin(_palo_alto_driver())) - router = {"id": "r1", "flavor_id": "f1"} - provider.l3plugin.get_router.return_value = router - mocker.patch.object(provider, "_is_palo_alto_provider", return_value=True) +class TestRouterInterfaceCreateHandler: + def test_orchestrates_in_order(self, mocker, provider): mocker.patch.object( - provider, "_gateway_port_for_router", return_value={"id": "gw-1"} - ) - parent = {"id": "parent-1"} - bound = {"id": "parent-1", "bound": True} - trunk = {"id": "trunk-1"} - m_parent = mocker.patch.object( - provider, "_ensure_parent_port", return_value=parent + provider._wiring, "_parent_port_for_router", return_value=None ) - m_vif = mocker.patch.object( - provider, "_ensure_parent_vif_attached", return_value=bound + mocker.patch.object(provider._wiring, "_trunk_for_router", return_value=None) + stack = palo_alto_wiring.AttachmentStack( + parent={"id": "parent-1"}, trunk={"id": "trunk-1"} ) - m_trunk = mocker.patch.object(provider, "_ensure_trunk", return_value=trunk) - m_sub = mocker.patch.object(provider, "_add_gateway_subport") - - provider._process_gateway_create("r", "e", "t", self._payload(mocker)) - - m_parent.assert_called_once_with(router) - # VIF-attach runs on the parent BEFORE the trunk/subport - m_vif.assert_called_once_with(router, parent) - # trunk + subport use the BOUND parent - m_trunk.assert_called_once_with(router, bound) - m_sub.assert_called_once_with(router, trunk, {"id": "gw-1"}) - - def test_skips_non_palo_alto_router(self, mocker): - provider = _make_provider( - mocker, FakeFlavorPlugin("neutron_understack.l3_router.vrf.Vrf") + calls = mocker.Mock() + calls.ensure_stack.return_value = stack + mocker.patch.object(provider._wiring, "ensure_stack", calls.ensure_stack) + mocker.patch.object( + provider._wiring, "add_interface_subport", calls.add_interface_subport ) - provider.l3plugin.get_router.return_value = {"id": "r1", "flavor_id": "f1"} - m_parent = mocker.patch.object(provider, "_ensure_parent_port") - - provider._process_gateway_create("r", "e", "t", self._payload(mocker)) - - m_parent.assert_not_called() - - def test_raises_when_gateway_port_missing(self, mocker): - provider = _make_provider(mocker, FakeFlavorPlugin(_palo_alto_driver())) - provider.l3plugin.get_router.return_value = {"id": "r1", "flavor_id": "f1"} - mocker.patch.object(provider, "_is_palo_alto_provider", return_value=True) - mocker.patch.object(provider, "_gateway_port_for_router", return_value=None) - - with pytest.raises(n_exc.BadRequest): - provider._process_gateway_create("r", "e", "t", self._payload(mocker)) - -class TestRouterInterfaceCreateHandler: - def _payload(self, mocker, router_id="r1", port=None): - payload = mocker.Mock() - payload.context = "ctx" - payload.resource_id = router_id - payload.metadata = {"port": port if port is not None else dict(_INTERFACE_PORT)} - return payload - - def test_orchestrates_in_order(self, mocker): - provider = _make_provider(mocker, FakeFlavorPlugin(_palo_alto_driver())) - router = {"id": "r1", "flavor_id": "f1"} - provider.l3plugin.get_router.return_value = router - mocker.patch.object(provider, "_is_palo_alto_provider", return_value=True) - parent = {"id": "parent-1"} - bound = {"id": "parent-1", "bound": True} - trunk = {"id": "trunk-1"} - m_parent = mocker.patch.object( - provider, "_ensure_parent_port", return_value=parent - ) - m_vif = mocker.patch.object( - provider, "_ensure_parent_vif_attached", return_value=bound + provider._process_router_interface_create( + "r", "e", "t", _payload(mocker, port=dict(_INTERFACE_PORT)) ) - m_trunk = mocker.patch.object(provider, "_ensure_trunk", return_value=trunk) - m_sub = mocker.patch.object(provider, "_add_interface_subport") - mocker.patch.object(provider, "_parent_port_for_router", return_value=None) - mocker.patch.object(provider, "_trunk_for_router", return_value=None) - provider._process_router_interface_create("r", "e", "t", self._payload(mocker)) + # The VIF-bound stack exists before the subport is added to its trunk. + assert calls.mock_calls == [ + mocker.call.ensure_stack("r1"), + mocker.call.add_interface_subport("r1", stack.trunk, dict(_INTERFACE_PORT)), + ] - m_parent.assert_called_once_with(router) - m_vif.assert_called_once_with(router, parent) - m_trunk.assert_called_once_with(router, bound) - m_sub.assert_called_once_with(router, trunk, dict(_INTERFACE_PORT)) + def test_skips_non_palo_alto_router(self, mocker, other_provider): + m_parent = mocker.patch.object(other_provider._wiring, "_ensure_parent_port") - def test_skips_non_palo_alto_router(self, mocker): - provider = _make_provider( - mocker, FakeFlavorPlugin("neutron_understack.l3_router.vrf.Vrf") + other_provider._process_router_interface_create( + "r", "e", "t", _payload(mocker, port=dict(_INTERFACE_PORT)) ) - provider.l3plugin.get_router.return_value = {"id": "r1", "flavor_id": "f1"} - m_parent = mocker.patch.object(provider, "_ensure_parent_port") - - provider._process_router_interface_create("r", "e", "t", self._payload(mocker)) m_parent.assert_not_called() - def test_skips_non_router_interface_port(self, mocker): - provider = _make_provider(mocker, FakeFlavorPlugin(_palo_alto_driver())) - provider.l3plugin.get_router.return_value = {"id": "r1", "flavor_id": "f1"} - mocker.patch.object(provider, "_is_palo_alto_provider", return_value=True) - m_parent = mocker.patch.object(provider, "_ensure_parent_port") + def test_skips_non_router_interface_port(self, mocker, provider): + m_parent = mocker.patch.object(provider._wiring, "_ensure_parent_port") port = {**_INTERFACE_PORT, "device_owner": "network:dhcp"} provider._process_router_interface_create( - "r", "e", "t", self._payload(mocker, port=port) + "r", "e", "t", _payload(mocker, port=port) ) m_parent.assert_not_called() - def test_raises_when_interface_port_missing(self, mocker): - provider = _make_provider(mocker, FakeFlavorPlugin(_palo_alto_driver())) - provider.l3plugin.get_router.return_value = {"id": "r1", "flavor_id": "f1"} - mocker.patch.object(provider, "_is_palo_alto_provider", return_value=True) - payload = self._payload(mocker, port=None) - payload.metadata = {} - - with pytest.raises(n_exc.BadRequest): - provider._process_router_interface_create("r", "e", "t", payload) - + def test_raises_when_interface_port_missing(self, mocker, provider): + with pytest.raises(palo_alto.PaloAltoInterfacePortMissing, match="router r1 "): + provider._process_router_interface_create("r", "e", "t", _payload(mocker)) -class TestGatewayTeardown: - def _provider(self, mocker, trunk_after_removal): - tp = mocker.Mock() - tp.get_trunk.return_value = trunk_after_removal - mocker.patch.object(palo_alto.utils, "fetch_trunk_plugin", return_value=tp) - ironic = mocker.Mock() - ironic.node_by_instance_uuid.return_value = mocker.Mock(id="node-1") - core = mocker.Mock() - provider = _make_provider( - mocker, - FakeFlavorPlugin(_palo_alto_driver()), - ironic=ironic, - core_plugin=core, + def test_abort_after_successful_rollback_does_nothing(self, mocker, provider): + # A rollback that succeeded must drop its snapshot, so the ABORT_CREATE + # Neutron publishes next does not run the undo a second time. + mocker.patch.object( + provider._wiring, "_parent_port_for_router", return_value=None ) - return provider, tp, ironic, core - - def test_remove_subport_when_present(self, mocker): - provider, tp, _, _ = self._provider(mocker, trunk_after_removal={}) - trunk = {"id": "trunk-1", "sub_ports": [{"port_id": "gw-1"}]} - - provider._remove_gateway_subport(trunk, "gw-1") - - tp.remove_subports.assert_called_once() - _ctx, tid, body = tp.remove_subports.call_args[0] - assert tid == "trunk-1" - assert body["sub_ports"] == [{"port_id": "gw-1"}] - - def test_remove_subport_idempotent(self, mocker): - provider, tp, _, _ = self._provider(mocker, trunk_after_removal={}) - trunk = {"id": "trunk-1", "sub_ports": []} - - provider._remove_gateway_subport(trunk, "gw-1") - - tp.remove_subports.assert_not_called() - - def test_remove_interface_subport_when_present(self, mocker): - provider, tp, _, _ = self._provider(mocker, trunk_after_removal={}) - trunk = {"id": "trunk-1", "sub_ports": [{"port_id": "intf-1"}]} - - provider._remove_interface_subport(trunk, "intf-1") - - tp.remove_subports.assert_called_once() - _ctx, tid, body = tp.remove_subports.call_args[0] - assert tid == "trunk-1" - assert body["sub_ports"] == [{"port_id": "intf-1"}] - - def test_deletes_stack_when_no_subports_left(self, mocker): - # after removal the trunk has no subports -> delete trunk + parent - provider, tp, ironic, core = self._provider( - mocker, - trunk_after_removal={ - "id": "trunk-1", - "port_id": "parent-1", - "sub_ports": [], - }, + trunk_lookup = mocker.patch.object( + provider._wiring, "_trunk_for_router", return_value=None + ) + mocker.patch.object( + provider._wiring, "_ensure_parent_port", side_effect=RuntimeError("boom") ) + payload = _payload(mocker, port=dict(_INTERFACE_PORT)) - provider._delete_parent_stack_if_unused("r1", {"id": "trunk-1"}) + with pytest.raises(RuntimeError, match="boom"): + provider._process_router_interface_create("r", "e", "t", payload) + lookups = trunk_lookup.call_count - tp.delete_trunk.assert_called_once() - ironic.detach_vif_from_node.assert_called_once() - core.delete_port.assert_called_once() - _ctx, parent_id = core.delete_port.call_args[0] - assert parent_id == "parent-1" + provider._process_router_interface_abort("r", "e", "t", payload) - def test_keeps_stack_when_subports_remain(self, mocker): - # a subnet subport still present -> leave trunk + parent alone - provider, tp, ironic, core = self._provider( - mocker, - trunk_after_removal={ - "id": "trunk-1", - "port_id": "parent-1", - "sub_ports": [{"port_id": "subnet-x"}], - }, - ) + assert trunk_lookup.call_count == lookups - provider._delete_parent_stack_if_unused("r1", {"id": "trunk-1"}) + def test_abort_without_payload_does_nothing(self, mocker, provider): + rollback = mocker.patch.object(provider, "_rollback_interface_attachment") - tp.delete_trunk.assert_not_called() - ironic.detach_vif_from_node.assert_not_called() - core.delete_port.assert_not_called() + provider._process_router_interface_abort("r", "e", "t", payload=None) + rollback.assert_not_called() -class TestGatewayDeleteHandler: - def _payload(self, mocker, router_id="r1"): - payload = mocker.Mock() - payload.context = "ctx" - payload.resource_id = router_id - return payload - def test_cleans_up_when_palo_alto(self, mocker): - provider = _make_provider(mocker, FakeFlavorPlugin(_palo_alto_driver())) - router = {"id": "r1", "flavor_id": "f1"} - provider.l3plugin.get_router.return_value = router - mocker.patch.object(provider, "_is_palo_alto_provider", return_value=True) +class TestGatewayDeleteHandler: + def test_cleans_up_when_palo_alto(self, mocker, provider): mocker.patch.object( - provider, "_gateway_port_for_router", return_value={"id": "gw-1"} + provider._wiring, "gateway_port_for_router", return_value={"id": "gw-1"} ) - m_cleanup = mocker.patch.object(provider, "_cleanup_gateway_attachment") + m_cleanup = mocker.patch.object(provider._wiring, "cleanup_attachment") - provider._process_gateway_delete("r", "e", "t", self._payload(mocker)) + provider._process_gateway_delete("r", "e", "t", _payload(mocker)) - m_cleanup.assert_called_once_with(router, {"id": "gw-1"}) + m_cleanup.assert_called_once_with("r1", "gw-1", "gateway") - def test_skips_non_palo_alto(self, mocker): - provider = _make_provider( - mocker, FakeFlavorPlugin("neutron_understack.l3_router.vrf.Vrf") - ) - provider.l3plugin.get_router.return_value = {"id": "r1", "flavor_id": "f1"} - m_cleanup = mocker.patch.object(provider, "_cleanup_gateway_attachment") + def test_skips_non_palo_alto(self, mocker, other_provider): + m_cleanup = mocker.patch.object(other_provider._wiring, "cleanup_attachment") - provider._process_gateway_delete("r", "e", "t", self._payload(mocker)) + other_provider._process_gateway_delete("r", "e", "t", _payload(mocker)) m_cleanup.assert_not_called() - def test_skips_when_no_gateway_port(self, mocker): - provider = _make_provider(mocker, FakeFlavorPlugin(_palo_alto_driver())) - provider.l3plugin.get_router.return_value = {"id": "r1", "flavor_id": "f1"} - mocker.patch.object(provider, "_is_palo_alto_provider", return_value=True) - mocker.patch.object(provider, "_gateway_port_for_router", return_value=None) - m_cleanup = mocker.patch.object(provider, "_cleanup_gateway_attachment") + def test_skips_when_no_gateway_port(self, mocker, provider): + mocker.patch.object( + provider._wiring, "gateway_port_for_router", return_value=None + ) + m_cleanup = mocker.patch.object(provider._wiring, "cleanup_attachment") - provider._process_gateway_delete("r", "e", "t", self._payload(mocker)) + provider._process_gateway_delete("r", "e", "t", _payload(mocker)) m_cleanup.assert_not_called() class TestRouterInterfacePortDeleteHandler: - def _payload(self, mocker, port=None): + @pytest.fixture(autouse=True) + def _router_port_exists(self, mocker): mocker.patch.object( palo_alto.l3_obj.RouterPort, "objects_exist", return_value=True ) - payload = mocker.Mock() - payload.context = "ctx" - payload.resource_id = "intf-1" - payload.metadata = { - "port": port if port is not None else dict(_INTERFACE_PORT), - "port_check": False, - } - return payload + + def _port_delete_payload(self, mocker, port=None): + port = port if port is not None else dict(_INTERFACE_PORT) + return _payload(mocker, "intf-1", port=port, port_check=False) @pytest.mark.parametrize("port_check", [True, None, "missing"]) - def test_skips_unless_l3_explicitly_authorized_removal(self, mocker, port_check): - provider = _make_provider(mocker, FakeFlavorPlugin(_palo_alto_driver())) - cleanup = mocker.patch.object(provider, "_cleanup_interface_attachment") - payload = self._payload(mocker) + def test_skips_unless_l3_explicitly_authorized_removal( + self, mocker, provider, port_check + ): + cleanup = mocker.patch.object(provider._wiring, "cleanup_attachment") + payload = self._port_delete_payload(mocker) if port_check == "missing": payload.metadata.pop("port_check") else: @@ -1022,128 +756,83 @@ def test_skips_unless_l3_explicitly_authorized_removal(self, mocker, port_check) cleanup.assert_not_called() provider.l3plugin.get_router.assert_not_called() - def test_cleans_up_before_port_delete_when_palo_alto(self, mocker): - provider = _make_provider(mocker, FakeFlavorPlugin(_palo_alto_driver())) - router = {"id": "r1", "flavor_id": "f1"} - provider.l3plugin.get_router.return_value = router - mocker.patch.object(provider, "_is_palo_alto_provider", return_value=True) - m_cleanup = mocker.patch.object(provider, "_cleanup_interface_attachment") + def test_cleans_up_before_port_delete_when_palo_alto(self, mocker, provider): + m_cleanup = mocker.patch.object(provider._wiring, "cleanup_attachment") provider._process_router_interface_port_delete( - "r", "e", "t", self._payload(mocker) + "r", "e", "t", self._port_delete_payload(mocker) ) - m_cleanup.assert_called_once_with(router, dict(_INTERFACE_PORT)) + m_cleanup.assert_called_once_with("r1", "intf-1", "interface") - def test_skips_non_router_interface_port(self, mocker): - provider = _make_provider(mocker, FakeFlavorPlugin(_palo_alto_driver())) - m_cleanup = mocker.patch.object(provider, "_cleanup_interface_attachment") + def test_skips_non_router_interface_port(self, mocker, provider): + m_cleanup = mocker.patch.object(provider._wiring, "cleanup_attachment") port = {**_INTERFACE_PORT, "device_owner": "network:dhcp"} provider._process_router_interface_port_delete( - "r", "e", "t", self._payload(mocker, port=port) + "r", "e", "t", self._port_delete_payload(mocker, port=port) ) m_cleanup.assert_not_called() provider.l3plugin.get_router.assert_not_called() - def test_skips_non_palo_alto_router(self, mocker): - provider = _make_provider( - mocker, FakeFlavorPlugin("neutron_understack.l3_router.vrf.Vrf") - ) - provider.l3plugin.get_router.return_value = {"id": "r1", "flavor_id": "f1"} - m_cleanup = mocker.patch.object(provider, "_cleanup_interface_attachment") + def test_skips_port_without_router(self, mocker, provider): + m_cleanup = mocker.patch.object(provider._wiring, "cleanup_attachment") + port = {**_INTERFACE_PORT, "device_id": ""} provider._process_router_interface_port_delete( - "r", "e", "t", self._payload(mocker) + "r", "e", "t", self._port_delete_payload(mocker, port=port) ) m_cleanup.assert_not_called() + provider.l3plugin.get_router.assert_not_called() - def test_skips_when_router_lookup_fails(self, mocker): - provider = _make_provider(mocker, FakeFlavorPlugin(_palo_alto_driver())) + def test_skips_non_palo_alto_router(self, mocker, other_provider): + m_cleanup = mocker.patch.object(other_provider._wiring, "cleanup_attachment") + + other_provider._process_router_interface_port_delete( + "r", "e", "t", self._port_delete_payload(mocker) + ) + + m_cleanup.assert_not_called() + + def test_skips_when_router_lookup_fails(self, mocker, provider): provider.l3plugin.get_router.side_effect = RuntimeError("db hiccup") - m_cleanup = mocker.patch.object(provider, "_cleanup_interface_attachment") + m_cleanup = mocker.patch.object(provider._wiring, "cleanup_attachment") provider._process_router_interface_port_delete( - "r", "e", "t", self._payload(mocker) + "r", "e", "t", self._port_delete_payload(mocker) ) m_cleanup.assert_not_called() - def test_skips_when_palo_alto_check_fails(self, mocker): - provider = _make_provider(mocker, FakeFlavorPlugin(_palo_alto_driver())) - provider.l3plugin.get_router.return_value = {"id": "r1", "flavor_id": "f1"} + def test_skips_when_palo_alto_check_fails(self, mocker, provider): mocker.patch.object( provider, "_is_palo_alto_provider", side_effect=RuntimeError("flavor lookup failed"), ) - m_cleanup = mocker.patch.object(provider, "_cleanup_interface_attachment") + m_cleanup = mocker.patch.object(provider._wiring, "cleanup_attachment") provider._process_router_interface_port_delete( - "r", "e", "t", self._payload(mocker) + "r", "e", "t", self._port_delete_payload(mocker) ) m_cleanup.assert_not_called() - def test_cleanup_error_still_raises_for_confirmed_palo_alto(self, mocker): - provider = _make_provider(mocker, FakeFlavorPlugin(_palo_alto_driver())) - router = {"id": "r1", "flavor_id": "f1"} - provider.l3plugin.get_router.return_value = router - mocker.patch.object(provider, "_is_palo_alto_provider", return_value=True) + def test_cleanup_error_still_raises_for_confirmed_palo_alto(self, mocker, provider): mocker.patch.object( - provider, - "_cleanup_interface_attachment", + provider._wiring, + "cleanup_attachment", side_effect=RuntimeError("cleanup failed"), ) with pytest.raises(RuntimeError, match="cleanup failed"): provider._process_router_interface_port_delete( - "r", "e", "t", self._payload(mocker) + "r", "e", "t", self._port_delete_payload(mocker) ) -class TestGatewayCleanupPartialAdd: - def _provider(self, mocker, trunks, ports): - tp = mocker.Mock() - tp.get_trunks.return_value = trunks - mocker.patch.object(palo_alto.utils, "fetch_trunk_plugin", return_value=tp) - ironic = mocker.Mock() - ironic.node_by_instance_uuid.return_value = mocker.Mock(id="node-1") - core = mocker.Mock() - core.get_networks.return_value = [{"id": "anchor-net"}] - core.get_ports.return_value = ports - provider = _make_provider( - mocker, - FakeFlavorPlugin(_palo_alto_driver()), - ironic=ironic, - core_plugin=core, - ) - return provider, ironic, core - - def test_deletes_orphan_parent_when_no_trunk(self, mocker): - # partial add left a parent port but no trunk - provider, ironic, core = self._provider( - mocker, trunks=[], ports=[{"id": "parent-1"}] - ) - - provider._cleanup_gateway_attachment({"id": "r1"}, {"id": "gw-1"}) - - ironic.detach_vif_from_node.assert_called_once() - core.delete_port.assert_called_once() - _ctx, parent_id = core.delete_port.call_args[0] - assert parent_id == "parent-1" - - def test_noop_when_no_trunk_and_no_parent(self, mocker): - provider, ironic, core = self._provider(mocker, trunks=[], ports=[]) - - provider._cleanup_gateway_attachment({"id": "r1"}, {"id": "gw-1"}) - - core.delete_port.assert_not_called() - ironic.detach_vif_from_node.assert_not_called() - - @pytest.fixture def interface_lifecycle(mocker): """Run Neutron's L3 flow/guards with stateful core, trunk and Ironic doubles.""" @@ -1248,22 +937,22 @@ def remove_subports(_context, trunk_id, body): ) trunk_plugin.delete_trunk.side_effect = lambda _ctx, tid: state.trunks.pop(tid) mocker.patch.object( - palo_alto.utils, "fetch_trunk_plugin", return_value=trunk_plugin + palo_alto_wiring.utils, "fetch_trunk_plugin", return_value=trunk_plugin ) mocker.patch.object( - palo_alto.utils, + palo_alto_wiring.utils, "allowed_tenant_vlan_id_ranges", side_effect=lambda: ( [(200, 200)] if state.fail == "vlan_allocation" else [(200, 210)] ), ) mocker.patch.object( - palo_alto.utils, + palo_alto_wiring.utils, "clear_device_id_for_port", side_effect=lambda pid: update_port(context, pid, {"port": {"device_id": ""}}), ) mocker.patch.object( - palo_alto.utils, + palo_alto_wiring.utils, "set_device_id_and_owner_for_port", side_effect=lambda pid, did, owner: update_port( context, pid, {"port": {"device_id": did, "device_owner": owner}} @@ -1293,7 +982,7 @@ def attach_vif(_node, port_id): provider = _make_provider( mocker, FakeFlavorPlugin(_palo_alto_driver()), ironic=ironic, core_plugin=core ) - palo_alto.n_context.get_admin_context.return_value = context + palo_alto_wiring.n_context.get_admin_context.return_value = context # Bind the installed Neutron methods, including its real rollback context # managers. Only persistence and pre-event subnet validation are doubled. @@ -1502,11 +1191,11 @@ def test_failure_preserves_preexisting_empty_stack( self, interface_lifecycle, existing_trunk, existing_vif ): env = interface_lifecycle - parent = env.provider._ensure_parent_port(env.router) + parent = env.provider._wiring._ensure_parent_port(env.router["id"]) if existing_vif: - env.provider._ensure_parent_vif_attached(env.router, parent) + env.provider._wiring._ensure_parent_vif_attached(env.router["id"], parent) if existing_trunk: - env.provider._ensure_trunk(env.router, parent) + env.provider._wiring._ensure_trunk(env.router["id"], parent) trunks = copy.deepcopy(env.state.trunks) vifs = set(env.state.vifs) env.state.fail = "subport_postcommit" @@ -1521,9 +1210,9 @@ def test_failure_preserves_preexisting_empty_stack( def test_failed_subnet_preserves_existing_gateway(self, interface_lifecycle): env = interface_lifecycle - parent = env.provider._ensure_parent_port(env.router) - env.provider._ensure_parent_vif_attached(env.router, parent) - env.provider._ensure_trunk(env.router, parent) + parent = env.provider._wiring._ensure_parent_port(env.router["id"]) + env.provider._wiring._ensure_parent_vif_attached(env.router["id"], parent) + env.provider._wiring._ensure_trunk(env.router["id"], parent) env.state.trunks["trunk-1"]["sub_ports"] = [ {"port_id": "gw-1", "segmentation_type": "vlan", "segmentation_id": 200} ] @@ -1543,7 +1232,7 @@ def test_abort_retries_failed_compensation( ): env = interface_lifecycle env.state.fail = "subport_postcommit" - cleanup = env.provider._remove_interface_subport + cleanup = env.provider._wiring._remove_router_port_subport attempts = [] def fail_once(*args): @@ -1553,7 +1242,7 @@ def fail_once(*args): return cleanup(*args) mocker.patch.object( - env.provider, "_remove_interface_subport", side_effect=fail_once + env.provider._wiring, "_remove_router_port_subport", side_effect=fail_once ) with pytest.raises( l3_exc.RouterInterfaceAttachmentConflict, match="subport_postcommit" diff --git a/python/neutron-understack/neutron_understack/tests/test_palo_alto_wiring.py b/python/neutron-understack/neutron_understack/tests/test_palo_alto_wiring.py new file mode 100644 index 000000000..e6e22ca72 --- /dev/null +++ b/python/neutron-understack/neutron_understack/tests/test_palo_alto_wiring.py @@ -0,0 +1,877 @@ +import logging +from types import SimpleNamespace +from unittest.mock import ANY + +import pytest +from neutron_lib import exceptions as n_exc + +from neutron_understack.l3_router import palo_alto_wiring + +_ANNOTATED_PARENT = { + "id": "parent-1", + "binding:host_id": "node-1", + "binding:profile": { + "physical_network": "n11-22-network", + "local_link_information": [{"switch_id": "aa", "port_id": "Eth1/1"}], + }, +} + +# e.g. the enrolled baremetal port had no physical_network +_UNANNOTATED_PARENT = {"id": "parent-1", "binding:host_id": "", "binding:profile": {}} + +_GATEWAY_PORT = { + "id": "gw-1", + "device_id": "r1", + "device_owner": "network:router_gateway", +} + +_INTERFACE_PORT = { + "id": "intf-1", + "device_id": "r1", + "device_owner": "network:router_interface", +} + +# A trunk whose gateway and first interface already use VLANs 200 and 201. +_TRUNK_USING_200_AND_201 = { + "id": "trunk-1", + "sub_ports": [ + {"port_id": "gw-1", "segmentation_type": "vlan", "segmentation_id": 200}, + {"port_id": "intf-1", "segmentation_type": "vlan", "segmentation_id": 201}, + ], +} + +_PARENT = {"id": "parent-1"} + + +def _trunk(*port_ids): + """Router r1's trunk on parent-1, carrying the given subport ports.""" + return { + "id": "trunk-1", + "port_id": "parent-1", + "sub_ports": [{"port_id": port_id} for port_id in port_ids], + } + + +def _snapshot(**changes): + """Snapshot for interface port intf-1 on r1; by default nothing existed.""" + fields = { + "router_id": "r1", + "port_id": "intf-1", + "parent_id": None, + "trunk_id": None, + "subport_present": False, + "parent_vif_attached": False, + } + return palo_alto_wiring.AttachmentSnapshot(**{**fields, **changes}) + + +@pytest.fixture +def core_plugin(mocker): + core = mocker.Mock() + core.get_networks.return_value = [{"id": "anchor-net"}] # anchor exists + mocker.patch.object(palo_alto_wiring.directory, "get_plugin", return_value=core) + return core + + +@pytest.fixture +def trunk_plugin(mocker): + trunk_plugin = mocker.Mock() + mocker.patch.object( + palo_alto_wiring.utils, "fetch_trunk_plugin", return_value=trunk_plugin + ) + return trunk_plugin + + +@pytest.fixture +def ironic(mocker): + return mocker.Mock() + + +@pytest.fixture +def node(mocker, ironic): + """The Ironic node adopted for router r1.""" + node = mocker.Mock(id="node-1") + ironic.node_by_instance_uuid.return_value = node + return node + + +@pytest.fixture +def device_id(mocker): + """The DB helpers that clear and restore a port's device_id around a subport add.""" + return SimpleNamespace( + clear=mocker.patch.object(palo_alto_wiring.utils, "clear_device_id_for_port"), + restore=mocker.patch.object( + palo_alto_wiring.utils, "set_device_id_and_owner_for_port" + ), + ) + + +@pytest.fixture +def wiring(mocker, ironic, core_plugin, trunk_plugin): + # get_admin_context() would init oslo policy; not needed for these tests. + mocker.patch.object(palo_alto_wiring.n_context, "get_admin_context") + return palo_alto_wiring.PaloAltoWiring(ironic=lambda: ironic) + + +class TestExceptionHttpCodes: + """Exceptions must map to real HTTP codes, not 500. + + neutron's FAULT_MAP maps Conflict->409 and BadRequest->400; the base + NeutronException falls through to 500. + """ + + def test_no_subport_vlan_available_is_conflict(self): + assert issubclass( + palo_alto_wiring.NoPaloAltoSubportVlanAvailable, n_exc.Conflict + ) + + @pytest.mark.parametrize( + "exc", + [ + palo_alto_wiring.PaloAltoNodeNotAdopted, + palo_alto_wiring.PaloAltoParentNotAnnotated, + ], + ) + def test_wiring_errors_are_bad_request(self, exc): + assert issubclass(exc, n_exc.BadRequest) + + +class TestNames: + def test_names_are_deterministic(self): + assert palo_alto_wiring._parent_port_name("r1") == "palo-alto-router-anchor-r1" + assert palo_alto_wiring._trunk_name("r1") == "palo-alto-router-trunk-r1" + + +class TestFirstFreeVlan: + def test_starts_at_requested_vlan(self): + assert palo_alto_wiring._first_free_vlan([(1, 300)], set(), 201) == 201 + + def test_skips_used_vlans(self): + assert palo_alto_wiring._first_free_vlan([(200, 205)], {201, 202}, 201) == 203 + + def test_continues_into_next_range(self): + assert ( + palo_alto_wiring._first_free_vlan([(200, 201), (300, 301)], {201}, 201) + == 300 + ) + + def test_accepts_unsorted_ranges(self): + assert ( + palo_alto_wiring._first_free_vlan([(300, 301), (200, 210)], set(), 201) + == 201 + ) + + def test_returns_none_when_exhausted(self): + assert palo_alto_wiring._first_free_vlan([(200, 201)], {200, 201}, 200) is None + + +class TestSubportHelpers: + def test_has_subport(self): + trunk = {"id": "trunk-1", "sub_ports": [{"port_id": "gw-1"}]} + + assert palo_alto_wiring._has_subport(trunk, "gw-1") is True + assert palo_alto_wiring._has_subport(trunk, "intf-1") is False + assert palo_alto_wiring._has_subport({"id": "trunk-1"}, "gw-1") is False + + def test_used_subport_vlans_counts_only_vlan_tags(self): + trunk = { + "id": "trunk-1", + "sub_ports": [ + {"port_id": "a", "segmentation_type": "vlan", "segmentation_id": 200}, + {"port_id": "b", "segmentation_type": "vlan", "segmentation_id": None}, + {"port_id": "c", "segmentation_type": "inherit"}, + ], + } + + assert palo_alto_wiring._used_subport_vlans(trunk) == {200} + + +class TestMissingBindingFields: + def test_none_missing_when_annotated(self): + assert palo_alto_wiring._missing_binding_fields(_ANNOTATED_PARENT) == [] + + def test_all_missing_when_unbound(self): + port = {"id": "parent-1", "binding:host_id": "", "binding:profile": None} + + assert palo_alto_wiring._missing_binding_fields(port) == [ + "binding:host_id", + "physical_network", + "local_link_information", + ] + + def test_reports_only_missing_profile_field(self): + port = { + **_ANNOTATED_PARENT, + "binding:profile": {"local_link_information": [{"port_id": "Eth1/1"}]}, + } + + assert palo_alto_wiring._missing_binding_fields(port) == ["physical_network"] + + +class TestLookups: + def test_trunk_plugin_delegates_to_utils(self, wiring, trunk_plugin): + assert wiring._trunk_plugin() is trunk_plugin + + def test_gateway_port_found_filters_by_owner_and_router(self, wiring, core_plugin): + core_plugin.get_ports.return_value = [{"id": "gw-1"}] + + assert wiring.gateway_port_for_router("r1") == {"id": "gw-1"} + _args, kwargs = core_plugin.get_ports.call_args + assert kwargs["filters"]["device_id"] == ["r1"] + assert kwargs["filters"]["device_owner"] == ["network:router_gateway"] + + def test_gateway_port_none_when_absent(self, wiring, core_plugin): + core_plugin.get_ports.return_value = [] + + assert wiring.gateway_port_for_router("r1") is None + + def test_gateway_port_uses_first_of_several_and_warns( + self, wiring, core_plugin, caplog + ): + core_plugin.get_ports.return_value = [{"id": "gw-1"}, {"id": "gw-2"}] + + with caplog.at_level(logging.WARNING, logger=palo_alto_wiring.__name__): + assert wiring.gateway_port_for_router("r1") == {"id": "gw-1"} + + assert "found 2; using gw-1" in caplog.text + + +class TestAnchorNetwork: + def test_reuses_existing_anchor_network(self, wiring, core_plugin): + assert wiring.ensure_anchor_network() == {"id": "anchor-net"} + + core_plugin.get_networks.assert_called_once_with( + ANY, filters={"name": [palo_alto_wiring.ANCHOR_NETWORK_NAME]} + ) + core_plugin.create_network.assert_not_called() + + def test_creates_anchor_network_with_required_defaults(self, wiring, core_plugin): + core_plugin.get_networks.return_value = [] + core_plugin.create_network.return_value = {"id": "anchor-new"} + + assert wiring.ensure_anchor_network() == {"id": "anchor-new"} + + _ctx, body = core_plugin.create_network.call_args[0] + assert body["network"]["name"] == palo_alto_wiring.ANCHOR_NETWORK_NAME + assert body["network"]["shared"] is False + # A direct plugin call skips API defaults; these two must be explicit. + assert body["network"]["router:external"] is False + assert "project_id" in body["network"] + + +class TestParentPort: + def test_creates_parent_when_absent(self, wiring, core_plugin): + core_plugin.get_ports.return_value = [] + core_plugin.create_port.return_value = {"id": "parent-new"} + + port = wiring._ensure_parent_port("r1") + + assert port == {"id": "parent-new"} + core_plugin.create_port.assert_called_once() + _ctx, body = core_plugin.create_port.call_args[0] + net = body["port"] + assert net["name"] == "palo-alto-router-anchor-r1" + assert net["network_id"] == "anchor-net" + assert net["binding:vnic_type"] == "baremetal" + assert net["device_id"] == "r1" + + def test_reuses_existing_parent(self, wiring, core_plugin): + core_plugin.get_ports.return_value = [{"id": "parent-existing"}] + + port = wiring._ensure_parent_port("r1") + + assert port == {"id": "parent-existing"} + core_plugin.create_port.assert_not_called() + + +class TestParentVifAttach: + @pytest.fixture(autouse=True) + def _not_attached_yet(self, ironic, core_plugin): + ironic.node_vif_ids.return_value = [] + core_plugin.get_port.return_value = _ANNOTATED_PARENT + + def test_attaches_when_not_already(self, wiring, ironic, node): + result = wiring._ensure_parent_vif_attached("r1", {"id": "parent-1"}) + + ironic.attach_vif_to_node.assert_called_once_with(node, "parent-1") + assert result == _ANNOTATED_PARENT # fresh, annotated copy returned + + def test_recovers_when_attach_raises_after_vif_lands(self, wiring, ironic, node): + ironic.node_vif_ids.side_effect = [[], ["parent-1"]] + ironic.attach_vif_to_node.side_effect = RuntimeError("rpc timeout") + + result = wiring._ensure_parent_vif_attached("r1", {"id": "parent-1"}) + + ironic.attach_vif_to_node.assert_called_once_with(node, "parent-1") + assert result == _ANNOTATED_PARENT + + def test_reraises_attach_error_when_vif_did_not_land(self, wiring, ironic, node): + ironic.node_vif_ids.side_effect = [[], []] + ironic.attach_vif_to_node.side_effect = RuntimeError("rpc timeout") + + with pytest.raises(RuntimeError, match="rpc timeout"): + wiring._ensure_parent_vif_attached("r1", {"id": "parent-1"}) + + def test_skips_attach_when_already_attached(self, wiring, ironic, node): + ironic.node_vif_ids.return_value = ["parent-1"] + + wiring._ensure_parent_vif_attached("r1", {"id": "parent-1"}) + + ironic.attach_vif_to_node.assert_not_called() + + def test_raises_when_no_adopted_node(self, wiring, ironic): + ironic.node_by_instance_uuid.return_value = None + + with pytest.raises(palo_alto_wiring.PaloAltoNodeNotAdopted, match="router r1 "): + wiring._ensure_parent_vif_attached("r1", {"id": "parent-1"}) + ironic.attach_vif_to_node.assert_not_called() + + def test_raises_when_parent_not_annotated(self, wiring, core_plugin, node): + core_plugin.get_port.return_value = _UNANNOTATED_PARENT + + with pytest.raises( + palo_alto_wiring.PaloAltoParentNotAnnotated, + match=r"parent port parent-1 .*\(missing binding:host_id, physical_network", + ): + wiring._ensure_parent_vif_attached("r1", {"id": "parent-1"}) + + def test_recovery_logs_original_attach_error(self, wiring, ironic, node, caplog): + ironic.node_vif_ids.side_effect = [[], ["parent-1"]] + error = RuntimeError("rpc timeout") + ironic.attach_vif_to_node.side_effect = error + + with caplog.at_level(logging.WARNING, logger=palo_alto_wiring.__name__): + wiring._ensure_parent_vif_attached("r1", {"id": "parent-1"}) + + [record] = [r for r in caplog.records if r.levelno == logging.WARNING] + assert "raised, but the VIF is attached" in record.getMessage() + assert record.exc_info[1] is error + + def test_recovery_raises_when_landed_vif_is_not_annotated( + self, wiring, ironic, core_plugin, node + ): + core_plugin.get_port.return_value = _UNANNOTATED_PARENT + ironic.node_vif_ids.side_effect = [[], ["parent-1"]] + ironic.attach_vif_to_node.side_effect = RuntimeError("rpc timeout") + + with pytest.raises(palo_alto_wiring.PaloAltoParentNotAnnotated): + wiring._ensure_parent_vif_attached("r1", {"id": "parent-1"}) + + def test_reraises_attach_error_when_recheck_fails(self, wiring, ironic, node): + ironic.node_vif_ids.side_effect = [[], RuntimeError("ironic unavailable")] + ironic.attach_vif_to_node.side_effect = RuntimeError("rpc timeout") + + with pytest.raises(RuntimeError, match="rpc timeout"): + wiring._ensure_parent_vif_attached("r1", {"id": "parent-1"}) + + def test_raises_when_already_attached_parent_is_not_annotated( + self, wiring, ironic, core_plugin, node + ): + ironic.node_vif_ids.return_value = ["parent-1"] + core_plugin.get_port.return_value = _UNANNOTATED_PARENT + + with pytest.raises(palo_alto_wiring.PaloAltoParentNotAnnotated): + wiring._ensure_parent_vif_attached("r1", {"id": "parent-1"}) + ironic.attach_vif_to_node.assert_not_called() + + +class TestTrunk: + def test_creates_trunk_when_absent(self, wiring, trunk_plugin): + trunk_plugin.get_trunks.return_value = [] + trunk_plugin.create_trunk.return_value = {"id": "trunk-new"} + + trunk = wiring._ensure_trunk("r1", {"id": "parent-1"}) + + assert trunk == {"id": "trunk-new"} + trunk_plugin.create_trunk.assert_called_once() + _ctx, body = trunk_plugin.create_trunk.call_args[0] + assert body["trunk"]["name"] == "palo-alto-router-trunk-r1" + assert body["trunk"]["port_id"] == "parent-1" + assert body["trunk"]["sub_ports"] == [] + + def test_reuses_existing_trunk(self, wiring, trunk_plugin): + trunk_plugin.get_trunks.return_value = [{"id": "trunk-existing"}] + + trunk = wiring._ensure_trunk("r1", {"id": "parent-1"}) + + assert trunk == {"id": "trunk-existing"} + trunk_plugin.create_trunk.assert_not_called() + + +class TestEnsureStack: + def test_binds_parent_before_building_trunk(self, mocker, wiring): + parent = {"id": "parent-1"} + bound = {"id": "parent-1", "bound": True} + trunk = {"id": "trunk-1"} + m_parent = mocker.patch.object( + wiring, "_ensure_parent_port", return_value=parent + ) + m_vif = mocker.patch.object( + wiring, "_ensure_parent_vif_attached", return_value=bound + ) + m_trunk = mocker.patch.object(wiring, "_ensure_trunk", return_value=trunk) + + stack = wiring.ensure_stack("r1") + + m_parent.assert_called_once_with("r1") + # VIF-attach runs on the parent BEFORE the trunk/subport + m_vif.assert_called_once_with("r1", parent) + # trunk + subport use the BOUND parent + m_trunk.assert_called_once_with("r1", bound) + assert stack == palo_alto_wiring.AttachmentStack(parent=bound, trunk=trunk) + + +class TestGatewaySubport: + def test_adds_subport_with_fixed_vlan(self, wiring, trunk_plugin, device_id): + trunk = {"id": "trunk-1", "sub_ports": []} + + wiring.add_gateway_subport("r1", trunk, dict(_GATEWAY_PORT)) + + trunk_plugin.add_subports.assert_called_once() + _ctx, trunk_id, body = trunk_plugin.add_subports.call_args[0] + assert trunk_id == "trunk-1" + sub = body["sub_ports"][0] + assert sub["port_id"] == "gw-1" + assert sub["segmentation_type"] == "vlan" + assert sub["segmentation_id"] == palo_alto_wiring.GATEWAY_SUBPORT_VLAN + # device_id cleared for the add (trunk validator rejects it) and restored + device_id.clear.assert_called_once_with("gw-1") + device_id.restore.assert_called_once_with( + "gw-1", "r1", "network:router_gateway" + ) + + def test_add_subport_is_idempotent(self, wiring, trunk_plugin, device_id): + trunk = { + "id": "trunk-1", + "sub_ports": [ + { + "port_id": "gw-1", + "segmentation_id": palo_alto_wiring.GATEWAY_SUBPORT_VLAN, + } + ], + } + + wiring.add_gateway_subport("r1", trunk, dict(_GATEWAY_PORT)) + + trunk_plugin.add_subports.assert_not_called() + device_id.clear.assert_not_called() + + def test_restores_device_id_when_add_fails(self, wiring, trunk_plugin, device_id): + trunk_plugin.add_subports.side_effect = RuntimeError("trunk rejected") + trunk = {"id": "trunk-1", "sub_ports": []} + + with pytest.raises(RuntimeError, match="trunk rejected"): + wiring.add_gateway_subport("r1", trunk, dict(_GATEWAY_PORT)) + + device_id.clear.assert_called_once_with("gw-1") + device_id.restore.assert_called_once_with( + "gw-1", "r1", "network:router_gateway" + ) + + +class TestInterfaceSubport: + def test_adds_subport_with_next_available_vlan( + self, mocker, wiring, trunk_plugin, device_id + ): + next_vlan = mocker.patch.object( + wiring, "_next_available_subport_vlan", return_value=201 + ) + trunk = { + "id": "trunk-1", + "sub_ports": [ + { + "port_id": "gw-1", + "segmentation_type": "vlan", + "segmentation_id": palo_alto_wiring.GATEWAY_SUBPORT_VLAN, + } + ], + } + + wiring.add_interface_subport("r1", trunk, dict(_INTERFACE_PORT)) + + next_vlan.assert_called_once_with( + "r1", trunk, palo_alto_wiring.INTERFACE_SUBPORT_VLAN_START + ) + trunk_plugin.add_subports.assert_called_once() + _ctx, trunk_id, body = trunk_plugin.add_subports.call_args[0] + assert trunk_id == "trunk-1" + sub = body["sub_ports"][0] + assert sub["port_id"] == "intf-1" + assert sub["segmentation_type"] == "vlan" + assert sub["segmentation_id"] == 201 + device_id.clear.assert_called_once_with("intf-1") + device_id.restore.assert_called_once_with( + "intf-1", "r1", "network:router_interface" + ) + + def test_add_subport_is_idempotent(self, mocker, wiring, trunk_plugin, device_id): + next_vlan = mocker.patch.object(wiring, "_next_available_subport_vlan") + trunk = { + "id": "trunk-1", + "sub_ports": [ + { + "port_id": "intf-1", + "segmentation_type": "vlan", + "segmentation_id": 201, + } + ], + } + + wiring.add_interface_subport("r1", trunk, dict(_INTERFACE_PORT)) + + next_vlan.assert_not_called() + trunk_plugin.add_subports.assert_not_called() + device_id.clear.assert_not_called() + + def test_vlan_exhaustion_raises_before_device_id_is_cleared( + self, mocker, wiring, trunk_plugin, device_id + ): + mocker.patch.object( + wiring, + "_next_available_subport_vlan", + side_effect=palo_alto_wiring.NoPaloAltoSubportVlanAvailable( + router_id="r1", trunk_id="trunk-1", network_segment_ranges="200" + ), + ) + trunk = {"id": "trunk-1", "sub_ports": []} + + with pytest.raises(palo_alto_wiring.NoPaloAltoSubportVlanAvailable): + wiring.add_interface_subport("r1", trunk, dict(_INTERFACE_PORT)) + + trunk_plugin.add_subports.assert_not_called() + device_id.clear.assert_not_called() + device_id.restore.assert_not_called() + + +class TestSubportVlanAllocation: + def _allowed_ranges(self, mocker, ranges): + mocker.patch.object( + palo_alto_wiring.utils, + "allowed_tenant_vlan_id_ranges", + return_value=ranges, + ) + + def test_starts_at_requested_vlan(self, mocker, wiring): + self._allowed_ranges(mocker, [(1, 199), (200, 202)]) + trunk = {"id": "trunk-1", "sub_ports": []} + + assert ( + wiring._next_available_subport_vlan( + "r1", trunk, palo_alto_wiring.INTERFACE_SUBPORT_VLAN_START + ) + == 201 + ) + + def test_skips_used_vlans(self, mocker, wiring): + self._allowed_ranges(mocker, [(200, 202)]) + + assert ( + wiring._next_available_subport_vlan( + "r1", + _TRUNK_USING_200_AND_201, + palo_alto_wiring.INTERFACE_SUBPORT_VLAN_START, + ) + == 202 + ) + + def test_raises_when_no_vlan_available(self, mocker, wiring): + self._allowed_ranges(mocker, [(200, 201)]) + + with pytest.raises(palo_alto_wiring.NoPaloAltoSubportVlanAvailable): + wiring._next_available_subport_vlan( + "r1", + _TRUNK_USING_200_AND_201, + palo_alto_wiring.INTERFACE_SUBPORT_VLAN_START, + ) + + +class TestSubportRemoval: + def test_remove_subport_when_present(self, wiring, trunk_plugin): + trunk = {"id": "trunk-1", "sub_ports": [{"port_id": "gw-1"}]} + + wiring._remove_router_port_subport(trunk, "gw-1", "gateway") + + trunk_plugin.remove_subports.assert_called_once() + _ctx, tid, body = trunk_plugin.remove_subports.call_args[0] + assert tid == "trunk-1" + assert body["sub_ports"] == [{"port_id": "gw-1"}] + + def test_remove_subport_idempotent(self, wiring, trunk_plugin): + trunk = {"id": "trunk-1", "sub_ports": []} + + wiring._remove_router_port_subport(trunk, "gw-1", "gateway") + + trunk_plugin.remove_subports.assert_not_called() + + def test_remove_interface_subport_when_present(self, wiring, trunk_plugin): + trunk = {"id": "trunk-1", "sub_ports": [{"port_id": "intf-1"}]} + + wiring._remove_router_port_subport(trunk, "intf-1", "interface") + + trunk_plugin.remove_subports.assert_called_once() + _ctx, tid, body = trunk_plugin.remove_subports.call_args[0] + assert tid == "trunk-1" + assert body["sub_ports"] == [{"port_id": "intf-1"}] + + +class TestDeleteParentStack: + def test_deletes_stack_when_no_subports_left( + self, wiring, trunk_plugin, ironic, core_plugin, node + ): + # after removal the trunk has no subports -> delete trunk + parent + trunk_plugin.get_trunk.return_value = { + "id": "trunk-1", + "port_id": "parent-1", + "sub_ports": [], + } + + wiring._delete_parent_stack_if_unused("r1", {"id": "trunk-1"}) + + trunk_plugin.delete_trunk.assert_called_once() + ironic.detach_vif_from_node.assert_called_once() + core_plugin.delete_port.assert_called_once() + _ctx, parent_id = core_plugin.delete_port.call_args[0] + assert parent_id == "parent-1" + + def test_keeps_stack_when_subports_remain( + self, wiring, trunk_plugin, ironic, core_plugin, node + ): + # a subnet subport still present -> leave trunk + parent alone + trunk_plugin.get_trunk.return_value = { + "id": "trunk-1", + "port_id": "parent-1", + "sub_ports": [{"port_id": "subnet-x"}], + } + + wiring._delete_parent_stack_if_unused("r1", {"id": "trunk-1"}) + + trunk_plugin.delete_trunk.assert_not_called() + ironic.detach_vif_from_node.assert_not_called() + core_plugin.delete_port.assert_not_called() + + +class TestDetachAndDeleteParent: + def test_detaches_vif_then_deletes_parent(self, wiring, ironic, core_plugin, node): + wiring._detach_and_delete_parent("r1", "parent-1") + + ironic.detach_vif_from_node.assert_called_once_with(node, "parent-1") + core_plugin.delete_port.assert_called_once_with(ANY, "parent-1") + + def test_deletes_parent_even_when_node_is_gone( + self, wiring, ironic, core_plugin, caplog + ): + # The port is still removed if the node can no longer be found by router. + ironic.node_by_instance_uuid.return_value = None + + with caplog.at_level(logging.WARNING, logger=palo_alto_wiring.__name__): + wiring._detach_and_delete_parent("r1", "parent-1") + + ironic.detach_vif_from_node.assert_not_called() + core_plugin.delete_port.assert_called_once_with(ANY, "parent-1") + assert "No node found for router r1" in caplog.text + + +class TestCleanupAttachment: + def test_removes_subport_then_unused_stack( + self, wiring, trunk_plugin, ironic, core_plugin, node + ): + trunk_plugin.get_trunks.return_value = [_trunk("gw-1")] + trunk_plugin.get_trunk.return_value = _trunk() # after the removal + + wiring.cleanup_attachment("r1", "gw-1", "gateway") + + trunk_plugin.remove_subports.assert_called_once_with( + ANY, "trunk-1", {"sub_ports": [{"port_id": "gw-1"}]} + ) + trunk_plugin.delete_trunk.assert_called_once_with(ANY, "trunk-1") + ironic.detach_vif_from_node.assert_called_once_with(node, "parent-1") + core_plugin.delete_port.assert_called_once_with(ANY, "parent-1") + + def test_deletes_orphan_parent_when_no_trunk( + self, wiring, trunk_plugin, ironic, core_plugin, node + ): + # partial add left a parent port but no trunk + trunk_plugin.get_trunks.return_value = [] + core_plugin.get_ports.return_value = [{"id": "parent-1"}] + + wiring.cleanup_attachment("r1", "gw-1", "gateway") + + ironic.detach_vif_from_node.assert_called_once() + core_plugin.delete_port.assert_called_once() + _ctx, parent_id = core_plugin.delete_port.call_args[0] + assert parent_id == "parent-1" + + def test_noop_when_no_trunk_and_no_parent( + self, wiring, trunk_plugin, ironic, core_plugin, node + ): + trunk_plugin.get_trunks.return_value = [] + core_plugin.get_ports.return_value = [] + + wiring.cleanup_attachment("r1", "gw-1", "gateway") + + core_plugin.delete_port.assert_not_called() + ironic.detach_vif_from_node.assert_not_called() + + +class TestSnapshotInterfaceAttachment: + def test_records_nothing_when_no_wiring_exists( + self, wiring, core_plugin, trunk_plugin + ): + core_plugin.get_ports.return_value = [] + trunk_plugin.get_trunks.return_value = [] + + assert wiring.snapshot_interface_attachment("r1", "intf-1") == _snapshot() + + def test_records_existing_stack( + self, wiring, core_plugin, trunk_plugin, ironic, node + ): + core_plugin.get_ports.return_value = [_PARENT] + trunk_plugin.get_trunks.return_value = [_trunk("intf-1")] + ironic.node_vif_ids.return_value = ["parent-1"] + + assert wiring.snapshot_interface_attachment("r1", "intf-1") == _snapshot( + parent_id="parent-1", + trunk_id="trunk-1", + subport_present=True, + parent_vif_attached=True, + ) + ironic.node_vif_ids.assert_called_once_with(node) + + def test_treats_vif_as_detached_when_node_is_gone( + self, wiring, core_plugin, trunk_plugin, ironic + ): + core_plugin.get_ports.return_value = [_PARENT] + trunk_plugin.get_trunks.return_value = [] + ironic.node_by_instance_uuid.return_value = None + + assert wiring.snapshot_interface_attachment("r1", "intf-1") == _snapshot( + parent_id="parent-1" + ) + ironic.node_vif_ids.assert_not_called() + + +class TestUndoInterfaceAttachment: + """Undo compares current wiring with the snapshot taken before the attach. + + It removes only what the attach request added, and keeps anything that + existed before or that other router ports still use. + """ + + def test_removes_everything_the_request_created( + self, wiring, core_plugin, trunk_plugin, ironic, node + ): + core_plugin.get_ports.return_value = [_PARENT] + trunk_plugin.get_trunks.return_value = [_trunk("intf-1")] + trunk_plugin.get_trunk.return_value = _trunk() # after the removal + + wiring.undo_interface_attachment(_snapshot()) + + trunk_plugin.remove_subports.assert_called_once_with( + ANY, "trunk-1", {"sub_ports": [{"port_id": "intf-1"}]} + ) + trunk_plugin.delete_trunk.assert_called_once_with(ANY, "trunk-1") + ironic.detach_vif_from_node.assert_called_once_with(node, "parent-1") + core_plugin.delete_port.assert_called_once_with(ANY, "parent-1") + + def test_deletes_parent_created_before_the_trunk( + self, wiring, core_plugin, trunk_plugin, ironic, node + ): + # The attach failed after creating the parent but before the trunk. + core_plugin.get_ports.return_value = [_PARENT] + trunk_plugin.get_trunks.return_value = [] + + wiring.undo_interface_attachment(_snapshot()) + + trunk_plugin.delete_trunk.assert_not_called() + ironic.detach_vif_from_node.assert_called_once_with(node, "parent-1") + core_plugin.delete_port.assert_called_once_with(ANY, "parent-1") + + def test_does_nothing_when_no_wiring_exists( + self, wiring, core_plugin, trunk_plugin, ironic + ): + core_plugin.get_ports.return_value = [] + trunk_plugin.get_trunks.return_value = [] + + wiring.undo_interface_attachment(_snapshot()) + + trunk_plugin.remove_subports.assert_not_called() + ironic.detach_vif_from_node.assert_not_called() + core_plugin.delete_port.assert_not_called() + + def test_keeps_subport_that_existed_before( + self, wiring, core_plugin, trunk_plugin, ironic + ): + # Retrying an attach that already succeeded must not unwire the port. + trunk_plugin.get_trunks.return_value = [_trunk("intf-1")] + trunk_plugin.get_trunk.return_value = _trunk("intf-1") + + wiring.undo_interface_attachment( + _snapshot( + parent_id="parent-1", + trunk_id="trunk-1", + subport_present=True, + parent_vif_attached=True, + ) + ) + + trunk_plugin.remove_subports.assert_not_called() + trunk_plugin.delete_trunk.assert_not_called() + ironic.detach_vif_from_node.assert_not_called() + core_plugin.delete_port.assert_not_called() + + def test_keeps_stack_that_other_ports_still_use( + self, wiring, core_plugin, trunk_plugin, ironic + ): + trunk_plugin.get_trunks.return_value = [_trunk("gw-1", "intf-1")] + trunk_plugin.get_trunk.return_value = _trunk("gw-1") # after the removal + + wiring.undo_interface_attachment(_snapshot()) + + trunk_plugin.remove_subports.assert_called_once_with( + ANY, "trunk-1", {"sub_ports": [{"port_id": "intf-1"}]} + ) + trunk_plugin.delete_trunk.assert_not_called() + ironic.detach_vif_from_node.assert_not_called() + core_plugin.delete_port.assert_not_called() + + def test_keeps_preexisting_empty_stack( + self, wiring, core_plugin, trunk_plugin, ironic, node + ): + core_plugin.get_ports.return_value = [_PARENT] + trunk_plugin.get_trunks.return_value = [_trunk("intf-1")] + trunk_plugin.get_trunk.return_value = _trunk() # after the removal + + wiring.undo_interface_attachment( + _snapshot( + parent_id="parent-1", trunk_id="trunk-1", parent_vif_attached=True + ) + ) + + trunk_plugin.remove_subports.assert_called_once() + trunk_plugin.delete_trunk.assert_not_called() + ironic.detach_vif_from_node.assert_not_called() + core_plugin.delete_port.assert_not_called() + + def test_detaches_vif_this_request_attached( + self, wiring, core_plugin, trunk_plugin, ironic, node + ): + # The parent existed before, but this request attached its VIF. + core_plugin.get_ports.return_value = [_PARENT] + trunk_plugin.get_trunks.return_value = [] + + wiring.undo_interface_attachment(_snapshot(parent_id="parent-1")) + + ironic.detach_vif_from_node.assert_called_once_with(node, "parent-1") + core_plugin.delete_port.assert_not_called() + + def test_skips_vif_detach_when_node_is_gone( + self, wiring, core_plugin, trunk_plugin, ironic + ): + core_plugin.get_ports.return_value = [_PARENT] + trunk_plugin.get_trunks.return_value = [] + ironic.node_by_instance_uuid.return_value = None + + wiring.undo_interface_attachment(_snapshot(parent_id="parent-1")) + + ironic.detach_vif_from_node.assert_not_called() + core_plugin.delete_port.assert_not_called() From 09d8abe9be883bd70c150189c0db4541f0d43c6f Mon Sep 17 00:00:00 2001 From: Nidhi Rai Date: Tue, 13 Oct 2026 18:24:57 +0530 Subject: [PATCH 2/2] fix(neutron-understack): roll back failed Palo Alto gateway attach Undo partial wiring on failure, and reject gateways that can't be wired up front. --- .../neutron_understack/ironic.py | 6 + .../neutron_understack/l3_router/palo_alto.py | 143 ++++++--- .../l3_router/palo_alto_wiring.py | 124 ++++--- .../neutron_understack/tests/test_ironic.py | 9 + .../tests/test_palo_alto_provider.py | 294 ++++++++++++----- .../tests/test_palo_alto_wiring.py | 302 ++++-------------- 6 files changed, 469 insertions(+), 409 deletions(-) diff --git a/python/neutron-understack/neutron_understack/ironic.py b/python/neutron-understack/neutron_understack/ironic.py index 81a2c8c47..6ed8238cd 100644 --- a/python/neutron-understack/neutron_understack/ironic.py +++ b/python/neutron-understack/neutron_understack/ironic.py @@ -6,6 +6,7 @@ from openstack import exceptions as sdk_exc 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 @@ -178,6 +179,11 @@ def node_vif_ids(self, node: str | BaremetalNode) -> list[str]: """Return the Neutron port (VIF) ids currently attached to the node.""" return self.irclient.list_node_vifs(node) + def node_ports(self, node: str | BaremetalNode) -> list[BaremetalPort]: + """Return the node's baremetal ports, including their network fields.""" + node_id = node.id if isinstance(node, BaremetalNode) else node + return list(self.irclient.ports(details=True, node_id=node_id)) + def adopt_node_for_router( self, node: str | BaremetalNode, diff --git a/python/neutron-understack/neutron_understack/l3_router/palo_alto.py b/python/neutron-understack/neutron_understack/l3_router/palo_alto.py index e50030dcf..c4cff3a98 100644 --- a/python/neutron-understack/neutron_understack/l3_router/palo_alto.py +++ b/python/neutron-understack/neutron_understack/l3_router/palo_alto.py @@ -1,3 +1,4 @@ +import functools import json import logging import weakref @@ -7,13 +8,16 @@ from neutron_lib import constants as const from neutron_lib import exceptions as n_exc from neutron_lib.callbacks import events +from neutron_lib.callbacks import exceptions as callback_exc from neutron_lib.callbacks import priority_group from neutron_lib.callbacks import registry from neutron_lib.callbacks import resources +from neutron_lib.db import api as db_api from neutron_lib.plugins import constants as plugin_constants from neutron_lib.plugins import directory from neutron_understack.ironic import IronicClient +from neutron_understack.l3_router.palo_alto_wiring import AttachmentSnapshot from neutron_understack.l3_router.palo_alto_wiring import PaloAltoWiring LOG = logging.getLogger(__name__) @@ -52,6 +56,28 @@ class PaloAltoInterfacePortMissing(n_exc.BadRequest): ) +# Conflict -> HTTP 409: a transient error interrupted gateway wiring. The wiring +# was rolled back, but Neutron keeps the gateway, so it must be set again. +class PaloAltoGatewayWiringFailed(n_exc.Conflict): + message = ( + "Router %(router_id)s gateway was created but could not be wired " + "(%(reason)s); any Palo Alto wiring was rolled back. Unset and set the " + "gateway again to retry." + ) + + +def _registry_would_retry(error: Exception) -> bool: + """Return True if the callback registry would turn this error into a retry. + + Checked on the CallbackFailure wrapper, as the registry does: that check + ignores the "retries exhausted" flag that is_retriable(error) honours. + """ + wrapped = callback_exc.CallbackFailure( + errors=[callback_exc.NotificationError(None, error)] + ) + return db_api.is_retriable(wrapped) + + def _parse_metainfo(raw) -> dict: """Service-profile metainfo is stored as a JSON string.""" if not raw: @@ -85,6 +111,14 @@ def __init__(self, l3_plugin): self._palo_alto_provider = f"{__name__}.{self.__class__.__name__}" self._interface_snapshots = weakref.WeakKeyDictionary() self._wiring = PaloAltoWiring(ironic=lambda: self._ironic) + # Check the appliance can be wired on BEFORE_CREATE, before Neutron + # creates the gateway port: rejecting there leaves nothing behind and + # the request can be retried. BEFORE events always re-raise errors. + registry.subscribe( + self._validate_gateway_create, + resources.ROUTER_GATEWAY, + events.BEFORE_CREATE, + ) # Gateway attach must run on AFTER_CREATE (the gateway port does not # exist earlier) and must be cancellable so a wiring failure returns a # real API error instead of a swallowed 200. @registry.receives cannot @@ -131,23 +165,15 @@ def __init__(self, l3_plugin): self._palo_alto_provider, ) - @property + @functools.cached_property def _flavor_plugin(self): - try: - return self._flavor_plugin_ref - except AttributeError: - self._flavor_plugin_ref = directory.get_plugin(plugin_constants.FLAVORS) - return self._flavor_plugin_ref + return directory.get_plugin(plugin_constants.FLAVORS) - @property + @functools.cached_property def _ironic(self) -> IronicClient: # Instantiated lazily so importing/loading this provider does not require # Ironic credentials in environments that never create such a router. - try: - return self._ironic_ref - except AttributeError: - self._ironic_ref = IronicClient() - return self._ironic_ref + return IronicClient() def _is_palo_alto_provider(self, context, router) -> bool: flavor_id = router.get("flavor_id") @@ -173,12 +199,10 @@ def _is_palo_alto_provider(self, context, router) -> bool: ) return matched - def _palo_alto_router(self, context, router_id: str) -> dict | None: - """Return the router if it uses the Palo Alto flavor, else None.""" + def _is_palo_alto_router(self, context, router_id: str) -> bool: + """Look the router up by id; return True if it uses the Palo Alto flavor.""" router = self.l3plugin.get_router(context, router_id) - if not self._is_palo_alto_provider(context, router): - return None - return router + return self._is_palo_alto_provider(context, router) def _resource_class_for_router(self, context, router) -> str: """Read the target resource_class from the flavor's profile metainfo. @@ -274,31 +298,55 @@ def _process_router_delete(self, resource, event, trigger, payload=None): router["id"], ) + def _validate_gateway_create(self, resource, event, trigger, payload=None): + """ROUTER_GATEWAY / BEFORE_CREATE: reject gateways that cannot be wired.""" + router_id = payload.resource_id + if not self._is_palo_alto_router(payload.context, router_id): + return + + self._wiring.check_can_attach(router_id) + def _process_gateway_create(self, resource, event, trigger, payload=None): """ROUTER_GATEWAY / AFTER_CREATE (cancellable): wire the gateway. Orders the building blocks so the parent port is VIF-bound before the subport is added -- the trunk driver only programs the switchport once the parent is bound. + + Neutron has already committed the gateway port here and does not undo + it when this raises, so a failure rolls back the wiring this request + added before re-raising. """ router_id = payload.resource_id - if self._palo_alto_router(payload.context, router_id) is None: + try: + self._wire_gateway(payload.context, router_id) + except Exception as error: + if _registry_would_retry(error): + # Neutron would retry update_router; the retry finds the + # gateway already created, skips this event and reports + # success for a gateway that is not wired. + raise PaloAltoGatewayWiringFailed( + router_id=router_id, reason=type(error).__name__ + ) from error + raise + + def _wire_gateway(self, context, router_id: str) -> None: + """Wire a Palo Alto router's gateway; undo this request's wiring on error.""" + if not self._is_palo_alto_router(context, router_id): return gateway_port = self._wiring.gateway_port_for_router(router_id) if gateway_port is None: raise PaloAltoGatewayPortNotFound(router_id=router_id) - stack = self._wiring.ensure_stack(router_id) - self._wiring.add_gateway_subport(router_id, stack.trunk, gateway_port) - - LOG.info( - "Attached Palo Alto router %s gateway port %s via parent %s trunk %s", - router_id, - gateway_port["id"], - stack.parent["id"], - stack.trunk["id"], + snapshot = self._wiring.snapshot_attachment( + router_id, gateway_port["id"], "gateway" ) + try: + self._wiring.attach_port(router_id, gateway_port, "gateway") + except Exception: + self._undo_attachment(snapshot) + raise def _process_gateway_delete(self, resource, event, trigger, payload=None): """ROUTER_GATEWAY / BEFORE_DELETE (cancellable): tear down the wiring. @@ -307,7 +355,7 @@ def _process_gateway_delete(self, resource, event, trigger, payload=None): remove its subport before Neutron deletes it. """ router_id = payload.resource_id - if self._palo_alto_router(payload.context, router_id) is None: + if not self._is_palo_alto_router(payload.context, router_id): return gateway_port = self._wiring.gateway_port_for_router(router_id) @@ -328,7 +376,7 @@ def _process_gateway_delete(self, resource, event, trigger, payload=None): def _process_router_interface_create(self, resource, event, trigger, payload=None): """Wire the interface before Neutron commits its RouterPort association.""" router_id = payload.resource_id - if self._palo_alto_router(payload.context, router_id) is None: + if not self._is_palo_alto_router(payload.context, router_id): return interface_port = payload.metadata.get("port") @@ -337,44 +385,41 @@ def _process_router_interface_create(self, resource, event, trigger, payload=Non if interface_port.get("device_owner") not in const.ROUTER_INTERFACE_OWNERS: return - self._interface_snapshots[payload] = self._wiring.snapshot_interface_attachment( - router_id, interface_port["id"] + self._interface_snapshots[payload] = self._wiring.snapshot_attachment( + router_id, interface_port["id"], "interface" ) try: - stack = self._wiring.ensure_stack(router_id) - self._wiring.add_interface_subport(router_id, stack.trunk, interface_port) + self._wiring.attach_port(router_id, interface_port, "interface") except Exception: # Attach-by-port is reverted with a port update by Neutron, so it # cannot rely on PORT/BEFORE_DELETE to undo partial realization. self._rollback_interface_attachment(payload) raise - LOG.info( - "Attached Palo Alto router %s interface port %s via parent %s trunk %s", - router_id, - interface_port["id"], - stack.parent["id"], - stack.trunk["id"], - ) - def _rollback_interface_attachment(self, payload) -> None: """Undo this request's changes, including calls that failed postcommit.""" snapshot = self._interface_snapshots.get(payload) if snapshot is None: return + # Preserve the original attach error. Keep the snapshot so the + # subsequent ABORT_CREATE can retry compensation if it failed here. + if self._undo_attachment(snapshot): + self._interface_snapshots.pop(payload, None) + + def _undo_attachment(self, snapshot: AttachmentSnapshot) -> bool: + """Undo an attach's wiring. A failed undo is logged and returns False.""" try: - self._wiring.undo_interface_attachment(snapshot) + self._wiring.undo_attachment(snapshot) except Exception: - # Preserve the original attach error. Keep the snapshot so the - # subsequent ABORT_CREATE can retry compensation if it failed here. LOG.exception( - "Failed to roll back Palo Alto router %s interface port %s; " + "Failed to roll back Palo Alto router %s %s port %s; " "attachment cleanup is incomplete", snapshot.router_id, + snapshot.label, snapshot.port_id, ) - return - self._interface_snapshots.pop(payload, None) + return False + return True def _process_router_interface_abort(self, resource, event, trigger, payload=None): # The registry continues invoking callbacks after validation errors. An @@ -403,7 +448,7 @@ def _process_router_interface_port_delete( return try: - router = self._palo_alto_router(context, router_id) + is_palo_alto = self._is_palo_alto_router(context, router_id) except Exception: LOG.debug( "Skipping Palo Alto interface cleanup for port %s; router %s " @@ -414,7 +459,7 @@ def _process_router_interface_port_delete( ) return - if router is None: + if not is_palo_alto: return # Neutron also deletes newly-created ports when attachment aborts. That diff --git a/python/neutron-understack/neutron_understack/l3_router/palo_alto_wiring.py b/python/neutron-understack/neutron_understack/l3_router/palo_alto_wiring.py index 30120498b..41bb35daf 100644 --- a/python/neutron-understack/neutron_understack/l3_router/palo_alto_wiring.py +++ b/python/neutron-understack/neutron_understack/l3_router/palo_alto_wiring.py @@ -19,6 +19,7 @@ from neutron_lib.api.definitions import portbindings from neutron_lib.plugins import directory from neutron_lib.services.trunk import constants as trunk_consts +from openstack.baremetal.v1.node import Node as BaremetalNode from neutron_understack import utils from neutron_understack.ironic import IronicClient @@ -46,7 +47,7 @@ @dataclass(frozen=True) class AttachmentSnapshot: - """Router wiring that existed before an interface attach request. + """Router wiring that existed before a gateway or interface attach request. Rollback compares current state against this, so it removes only what the request added and leaves pre-existing parent, trunk and VIF in place. @@ -54,6 +55,7 @@ class AttachmentSnapshot: router_id: str port_id: str + label: PortLabel parent_id: str | None trunk_id: str | None subport_present: bool @@ -93,6 +95,15 @@ class PaloAltoParentNotAnnotated(n_exc.BadRequest): ) +# BadRequest -> HTTP 400: no NIC on the node can carry the parent port's VIF. +class PaloAltoNodeHasNoUsablePort(n_exc.BadRequest): + message = ( + "Ironic node %(node_id)s for Palo Alto router %(router_id)s has no " + "baremetal port with both physical_network and local_link_connection; " + "check the node's port enrollment." + ) + + def _parent_port_name(router_id: str) -> str: """Deterministic name for the router's anchor-network parent port.""" return f"{ANCHOR_PARENT_PORT_NAME_PREFIX}-{router_id}" @@ -147,6 +158,11 @@ def _device_id_cleared(port: dict) -> Generator[None, None, None]: ) +def _is_bindable_port(port) -> bool: + """Return True if Ironic can annotate a parent VIF attached to this port.""" + return bool(port.physical_network) and bool(port.local_link_connection) + + def _missing_binding_fields(port: dict) -> list[str]: """Return the binding fields Ironic should have set on the port but did not.""" profile = port.get(portbindings.PROFILE) or {} @@ -162,12 +178,10 @@ def _missing_binding_fields(port: dict) -> list[str]: class PaloAltoWiring: - """Build and tear down a Palo Alto router's parent, VIF and trunk stack. + """Build and tear down a Palo Alto router's parent port, VIF and trunk. - Holds no callback subscriptions, so code outside the L3 service provider - can use it too. Plugin and Ironic lookups are plain methods, not - properties, so inspecting this object (inspect.getmembers() evaluates - properties) never contacts Neutron or Ironic. + Lookups are methods, not properties, so inspecting this object never + contacts Neutron or Ironic. """ def __init__(self, ironic: Callable[[], IronicClient]): @@ -181,10 +195,20 @@ def _ironic(self) -> IronicClient: def _trunk_plugin(self): return utils.fetch_trunk_plugin() + def _core_plugin(self): + return directory.get_plugin() + + def _admin_context(self): + return n_context.get_admin_context() + + def _node_for_router(self, router_id: str) -> BaremetalNode | None: + """Return the Ironic node adopted for the router, or None.""" + return self._ironic().node_by_instance_uuid(router_id) + def ensure_anchor_network(self) -> dict: """Create the shared sentinel anchor network if it does not exist.""" - core_plugin = directory.get_plugin() - admin_context = n_context.get_admin_context() + core_plugin = self._core_plugin() + admin_context = self._admin_context() existing = core_plugin.get_networks( admin_context, filters={"name": [ANCHOR_NETWORK_NAME]} ) @@ -220,8 +244,8 @@ def gateway_port_for_router(self, router_id: str) -> dict | None: The gateway port is owned by the router (``device_id == router_id``) with ``device_owner == network:router_gateway``. """ - core_plugin = directory.get_plugin() - admin_context = n_context.get_admin_context() + core_plugin = self._core_plugin() + admin_context = self._admin_context() ports = core_plugin.get_ports( admin_context, filters={ @@ -243,8 +267,8 @@ def gateway_port_for_router(self, router_id: str) -> dict | None: def _parent_port_for_router(self, router_id: str) -> dict | None: """Return the router's existing anchor-network parent port, or None.""" - core_plugin = directory.get_plugin() - admin_context = n_context.get_admin_context() + core_plugin = self._core_plugin() + admin_context = self._admin_context() anchor_network = self.ensure_anchor_network() ports = core_plugin.get_ports( admin_context, @@ -259,11 +283,9 @@ def _create_parent_port(self, router_id: str) -> dict: """Create the router's parent port on the anchor network. vnic_type=baremetal so Ironic can VIF-attach it to the adopted node. - Direct core-plugin call (server-side), so extension-default fields are - supplied explicitly, matching the codebase's other direct port creates. """ - core_plugin = directory.get_plugin() - admin_context = n_context.get_admin_context() + core_plugin = self._core_plugin() + admin_context = self._admin_context() anchor_network = self.ensure_anchor_network() port_name = _parent_port_name(router_id) LOG.info( @@ -302,8 +324,8 @@ def _ensure_parent_port(self, router_id: str) -> dict: def _fresh_port(self, port_id: str) -> dict: """Re-read a port so callers see its current binding profile.""" - core_plugin = directory.get_plugin() - admin_context = n_context.get_admin_context() + core_plugin = self._core_plugin() + admin_context = self._admin_context() return core_plugin.get_port(admin_context, port_id) def _vif_landed_after_error( @@ -335,7 +357,7 @@ def _ensure_parent_vif_attached(self, router_id: str, parent_port: dict) -> dict Returns a fresh copy of the parent port reflecting the new binding. """ - node = self._ironic().node_by_instance_uuid(router_id) + node = self._node_for_router(router_id) if node is None: raise PaloAltoNodeNotAdopted(router_id=router_id) @@ -371,7 +393,7 @@ def _verify_parent_annotated(self, router_id: str, parent_port: dict) -> None: The trunk + undersync need host_id, physical_network and local_link_information on the binding profile. If they are missing the node's baremetal port most likely has no physical_network (enroll side); - surface a clear error here instead of a silent no-op at undersync. + surface a clear error here. """ missing = _missing_binding_fields(parent_port) if missing: @@ -383,7 +405,7 @@ def _verify_parent_annotated(self, router_id: str, parent_port: dict) -> None: def _trunk_for_router(self, router_id: str) -> dict | None: """Return the router's existing trunk (by deterministic name), or None.""" - admin_context = n_context.get_admin_context() + admin_context = self._admin_context() trunks = self._trunk_plugin().get_trunks( admin_context, filters={"name": [_trunk_name(router_id)]} ) @@ -391,7 +413,7 @@ def _trunk_for_router(self, router_id: str) -> dict | None: def _create_trunk(self, router_id: str, parent_port: dict) -> dict: """Create the router's trunk with the parent port as its trunk parent.""" - admin_context = n_context.get_admin_context() + admin_context = self._admin_context() trunk_name = _trunk_name(router_id) LOG.info( "Creating Palo Alto trunk %s on parent port %s for router %s", @@ -424,6 +446,14 @@ def _ensure_trunk(self, router_id: str, parent_port: dict) -> dict: return existing return self._create_trunk(router_id, parent_port) + def check_can_attach(self, router_id: str) -> None: + """Raise if the router's node cannot be wired. Read-only.""" + node = self._node_for_router(router_id) + if node is None: + raise PaloAltoNodeNotAdopted(router_id=router_id) + if not any(_is_bindable_port(port) for port in self._ironic().node_ports(node)): + raise PaloAltoNodeHasNoUsablePort(router_id=router_id, node_id=node.id) + def ensure_stack(self, router_id: str) -> AttachmentStack: """Find or create the router's parent port, VIF attachment and trunk. @@ -478,7 +508,7 @@ def _add_router_port_subport( return trunk segmentation_id = pick_vlan() - admin_context = n_context.get_admin_context() + admin_context = self._admin_context() LOG.info( "Adding Palo Alto %s port %s to trunk %s as VLAN %s subport for router %s", label, @@ -524,6 +554,25 @@ def add_interface_subport( ), ) + def attach_port( + self, router_id: str, port: dict, label: PortLabel + ) -> AttachmentStack: + """Build the router's stack, then add the port as a subport.""" + stack = self.ensure_stack(router_id) + if label == "gateway": + self.add_gateway_subport(router_id, stack.trunk, port) + else: + self.add_interface_subport(router_id, stack.trunk, port) + LOG.info( + "Attached Palo Alto router %s %s port %s via parent %s trunk %s", + router_id, + label, + port["id"], + stack.parent["id"], + stack.trunk["id"], + ) + return stack + def _remove_router_port_subport( self, trunk: dict, port_id: str, label: PortLabel ) -> dict: @@ -540,7 +589,7 @@ def _remove_router_port_subport( trunk["id"], ) return trunk - admin_context = n_context.get_admin_context() + admin_context = self._admin_context() LOG.info( "Removing Palo Alto %s subport %s from trunk %s", label, @@ -555,7 +604,7 @@ def _remove_router_port_subport( def _detach_and_delete_parent(self, router_id: str, parent_id: str) -> None: """Detach the parent VIF from the node and delete the parent port.""" - node = self._ironic().node_by_instance_uuid(router_id) + node = self._node_for_router(router_id) if node is not None: self._ironic().detach_vif_from_node(node, parent_id) else: @@ -564,8 +613,8 @@ def _detach_and_delete_parent(self, router_id: str, parent_id: str) -> None: router_id, parent_id, ) - core_plugin = directory.get_plugin() - admin_context = n_context.get_admin_context() + core_plugin = self._core_plugin() + admin_context = self._admin_context() LOG.info("Deleting Palo Alto anchor parent port %s", parent_id) core_plugin.delete_port(admin_context, parent_id) @@ -576,7 +625,7 @@ def _delete_parent_stack_if_unused(self, router_id: str, trunk: dict) -> None: other subports are still present (e.g. tenant subnets), leave the trunk and parent for them to share. """ - admin_context = n_context.get_admin_context() + admin_context = self._admin_context() fresh = self._trunk_plugin().get_trunk(admin_context, trunk["id"]) if fresh.get("sub_ports"): LOG.debug( @@ -625,27 +674,28 @@ def cleanup_attachment( self._remove_router_port_subport(trunk, port_id, label) self._delete_parent_stack_if_unused(router_id, trunk) - def snapshot_interface_attachment( - self, router_id: str, port_id: str + def snapshot_attachment( + self, router_id: str, port_id: str, label: PortLabel ) -> AttachmentSnapshot: - """Record which wiring already exists before an interface attach.""" + """Record which wiring already exists before attaching a router port.""" parent = self._parent_port_for_router(router_id) trunk = self._trunk_for_router(router_id) parent_vif_attached = False if parent is not None: - node = self._ironic().node_by_instance_uuid(router_id) + node = self._node_for_router(router_id) if node is not None: parent_vif_attached = parent["id"] in self._ironic().node_vif_ids(node) return AttachmentSnapshot( router_id=router_id, port_id=port_id, + label=label, parent_id=parent["id"] if parent else None, trunk_id=trunk["id"] if trunk else None, subport_present=trunk is not None and _has_subport(trunk, port_id), parent_vif_attached=parent_vif_attached, ) - def undo_interface_attachment(self, snapshot: AttachmentSnapshot) -> None: + def undo_attachment(self, snapshot: AttachmentSnapshot) -> None: """Return the router's wiring to the state recorded in the snapshot. Re-reads current state rather than replaying the calls that succeeded, @@ -654,8 +704,10 @@ def undo_interface_attachment(self, snapshot: AttachmentSnapshot) -> None: trunk = self._trunk_for_router(snapshot.router_id) if trunk is not None: if not snapshot.subport_present: - self._remove_router_port_subport(trunk, snapshot.port_id, "interface") - admin_context = n_context.get_admin_context() + self._remove_router_port_subport( + trunk, snapshot.port_id, snapshot.label + ) + admin_context = self._admin_context() trunk = self._trunk_plugin().get_trunk(admin_context, trunk["id"]) if trunk.get("sub_ports"): # Other interfaces still need the shared parent and VIF. @@ -668,6 +720,6 @@ def undo_interface_attachment(self, snapshot: AttachmentSnapshot) -> None: if parent["id"] != snapshot.parent_id: self._detach_and_delete_parent(snapshot.router_id, parent["id"]) elif not snapshot.parent_vif_attached: - node = self._ironic().node_by_instance_uuid(snapshot.router_id) + node = self._node_for_router(snapshot.router_id) if node is not None: self._ironic().detach_vif_from_node(node, parent["id"]) diff --git a/python/neutron-understack/neutron_understack/tests/test_ironic.py b/python/neutron-understack/neutron_understack/tests/test_ironic.py index f9d7e50fd..abbe6a226 100644 --- a/python/neutron-understack/neutron_understack/tests/test_ironic.py +++ b/python/neutron-understack/neutron_understack/tests/test_ironic.py @@ -245,6 +245,15 @@ def test_node_vif_ids(self, mocker): assert client.node_vif_ids(node) == ["p1", "p2"] +class TestNodePorts: + def test_lists_the_nodes_ports_with_details(self, mocker): + client = _client(mocker) + client.irclient.ports.return_value = iter(["port-a", "port-b"]) + + assert client.node_ports(_node()) == ["port-a", "port-b"] + client.irclient.ports.assert_called_once_with(details=True, node_id="n1") + + class TestReleaseClearsOwnership: def test_active_node_is_undeployed_then_ownership_cleared(self, mocker): client = _client(mocker) diff --git a/python/neutron-understack/neutron_understack/tests/test_palo_alto_provider.py b/python/neutron-understack/neutron_understack/tests/test_palo_alto_provider.py index a5b382e19..8ae8753b6 100644 --- a/python/neutron-understack/neutron_understack/tests/test_palo_alto_provider.py +++ b/python/neutron-understack/neutron_understack/tests/test_palo_alto_provider.py @@ -14,7 +14,9 @@ from neutron_lib.callbacks import priority_group from neutron_lib.callbacks import registry from neutron_lib.callbacks import resources +from neutron_lib.db import api as db_api from neutron_lib.exceptions import l3 as l3_exc +from oslo_db import exception as db_exc from neutron_understack.ironic import NodeReleaseResult from neutron_understack.l3_router import palo_alto @@ -64,9 +66,10 @@ def _make_provider(mocker, flavor_plugin, ironic=None, core_plugin=None): # get_admin_context() would init oslo policy; not needed for these tests. mocker.patch.object(palo_alto_wiring.n_context, "get_admin_context") provider = palo_alto.PaloAlto(mocker.Mock()) - provider._flavor_plugin_ref = flavor_plugin + # Assigning fills the cached_property cache, so neither lookup runs. + provider._flavor_plugin = flavor_plugin if ironic is not None: - provider._ironic_ref = ironic + provider._ironic = ironic return provider @@ -167,36 +170,6 @@ def test_is_palo_alto_provider_returns_false_for_different_driver(self, mocker): is False ) - def test_palo_alto_router_returns_router_for_palo_alto_flavor(self, mocker): - provider = _make_provider(mocker, FakeFlavorPlugin(_palo_alto_driver())) - router = {"id": "r1", "flavor_id": "f1"} - provider.l3plugin.get_router.return_value = router - - assert provider._palo_alto_router("ctx", "r1") is router - provider.l3plugin.get_router.assert_called_once_with("ctx", "r1") - - @pytest.mark.parametrize( - ("driver", "router"), - [ - (_OTHER_DRIVER, {"id": "r1", "flavor_id": "f1"}), - (_palo_alto_driver(), {"id": "r1"}), - ], - ids=["other-flavor", "no-flavor"], - ) - def test_palo_alto_router_returns_none_otherwise(self, mocker, driver, router): - provider = _make_provider(mocker, FakeFlavorPlugin(driver)) - provider.l3plugin.get_router.return_value = router - - assert provider._palo_alto_router("ctx", "r1") is None - - def test_palo_alto_router_propagates_lookup_errors(self, mocker): - # Callers decide: most handlers let it raise, port delete skips cleanup. - provider = _make_provider(mocker, FakeFlavorPlugin(_palo_alto_driver())) - provider.l3plugin.get_router.side_effect = RuntimeError("db hiccup") - - with pytest.raises(RuntimeError, match="db hiccup"): - provider._palo_alto_router("ctx", "r1") - class TestIronicClient: def test_created_lazily_once_and_shared_with_wiring(self, mocker): @@ -447,12 +420,7 @@ def test_skips_non_palo_alto_router(self, mocker): class TestEventSubscriptions: - """Publish through the real registry and check what Neutron's caller sees. - - These pin the event phase and cancellable flag of each subscription, which - calling a handler directly cannot. They mock only the plugins and Ironic, - never provider internals, so they hold across internal refactors. - """ + """Publish through the real registry to pin each subscription's event.""" _ROUTER = {"id": "r1", "name": "pa-router", "project_id": "p1", "flavor_id": "f1"} @@ -532,6 +500,41 @@ def test_gateway_wiring_error_reaches_the_caller(self, mocker): for exc in failure.value.inner_exceptions ) + def _gateway_create_check(self): + registry.publish( + resources.ROUTER_GATEWAY, + events.BEFORE_CREATE, + self, + payload=self._payload(metadata={"network_id": "ext-net"}), + ) + + def test_gateway_check_rejects_before_neutron_creates_the_gateway(self, mocker): + # BEFORE_CREATE runs before the gateway port exists, so rejecting here + # leaves nothing to clean up and the same request can be retried. + ironic = mocker.Mock() + ironic.node_by_instance_uuid.return_value = None + provider = _make_provider( + mocker, FakeFlavorPlugin(_palo_alto_driver()), ironic=ironic + ) + provider.l3plugin.get_router.return_value = dict(self._ROUTER) + + with pytest.raises(callback_exc.CallbackFailure) as failure: + self._gateway_create_check() + + [error] = failure.value.inner_exceptions + assert isinstance(error, palo_alto_wiring.PaloAltoNodeNotAdopted) + + def test_gateway_check_ignores_other_flavors(self, mocker): + ironic = mocker.Mock() + provider = _make_provider( + mocker, FakeFlavorPlugin(_OTHER_DRIVER), ironic=ironic + ) + provider.l3plugin.get_router.return_value = dict(self._ROUTER) + + self._gateway_create_check() + + ironic.node_by_instance_uuid.assert_not_called() + def test_gateway_teardown_runs_before_gateway_port_delete(self, mocker): # BEFORE_DELETE is the last point where the gateway port still exists. core = mocker.Mock() @@ -587,6 +590,10 @@ def test_orchestrates_in_order(self, mocker, provider): mocker.patch.object( provider._wiring, "gateway_port_for_router", return_value={"id": "gw-1"} ) + mocker.patch.object( + provider._wiring, "_parent_port_for_router", return_value=None + ) + mocker.patch.object(provider._wiring, "_trunk_for_router", return_value=None) stack = palo_alto_wiring.AttachmentStack( parent={"id": "parent-1"}, trunk={"id": "trunk-1"} ) @@ -692,13 +699,6 @@ def test_abort_after_successful_rollback_does_nothing(self, mocker, provider): assert trunk_lookup.call_count == lookups - def test_abort_without_payload_does_nothing(self, mocker, provider): - rollback = mocker.patch.object(provider, "_rollback_interface_attachment") - - provider._process_router_interface_abort("r", "e", "t", payload=None) - - rollback.assert_not_called() - class TestGatewayDeleteHandler: def test_cleans_up_when_palo_alto(self, mocker, provider): @@ -776,17 +776,6 @@ def test_skips_non_router_interface_port(self, mocker, provider): m_cleanup.assert_not_called() provider.l3plugin.get_router.assert_not_called() - def test_skips_port_without_router(self, mocker, provider): - m_cleanup = mocker.patch.object(provider._wiring, "cleanup_attachment") - port = {**_INTERFACE_PORT, "device_id": ""} - - provider._process_router_interface_port_delete( - "r", "e", "t", self._port_delete_payload(mocker, port=port) - ) - - m_cleanup.assert_not_called() - provider.l3plugin.get_router.assert_not_called() - def test_skips_non_palo_alto_router(self, mocker, other_provider): m_cleanup = mocker.patch.object(other_provider._wiring, "cleanup_attachment") @@ -834,7 +823,7 @@ def test_cleanup_error_still_raises_for_confirmed_palo_alto(self, mocker, provid @pytest.fixture -def interface_lifecycle(mocker): +def l3_lifecycle(mocker): """Run Neutron's L3 flow/guards with stateful core, trunk and Ironic doubles.""" state = SimpleNamespace( ports={}, trunks={}, vifs=set(), associations=set(), fail=None @@ -896,6 +885,7 @@ def delete_port(ctx, port_id, l3_port_check=True): core.get_port.side_effect = lambda _ctx, port_id: copy.deepcopy( state.ports[port_id] ) + core.get_ports.side_effect = lambda _ctx, filters: [ copy.deepcopy(port) for port in state.ports.values() @@ -911,6 +901,10 @@ def create_trunk(_context, body): def add_subports(_context, trunk_id, body): fail_at("subport_precommit") + if state.fail == "subport_deadlock": + raise db_exc.DBDeadlock() + if state.fail == "subport_deadlock_exhausted": + raise _exhausted_deadlock() state.trunks[trunk_id]["sub_ports"].extend(copy.deepcopy(body["sub_ports"])) fail_at("subport_postcommit") return copy.deepcopy(state.trunks[trunk_id]) @@ -927,11 +921,17 @@ def remove_subports(_context, trunk_id, body): trunk_plugin.create_trunk.side_effect = create_trunk trunk_plugin.add_subports.side_effect = add_subports trunk_plugin.remove_subports.side_effect = remove_subports - trunk_plugin.get_trunks.side_effect = lambda _ctx, filters: [ - copy.deepcopy(trunk) - for trunk in state.trunks.values() - if trunk["name"] in filters["name"] - ] + + def get_trunks(_context, filters): + if state.fail == "trunk_lookup_deadlock": + raise db_exc.DBDeadlock() + return [ + copy.deepcopy(trunk) + for trunk in state.trunks.values() + if trunk["name"] in filters["name"] + ] + + trunk_plugin.get_trunks.side_effect = get_trunks trunk_plugin.get_trunk.side_effect = lambda _ctx, tid: copy.deepcopy( state.trunks[tid] ) @@ -1083,8 +1083,8 @@ def _add(self, env, by_port=False): info = {"port_id": "intf-1"} if by_port else {"subnet_id": "subnet-1"} return env.l3.add_router_interface(env.context, "r1", info) - def test_rejected_direct_delete_preserves_live_subport(self, interface_lifecycle): - env = interface_lifecycle + def test_rejected_direct_delete_preserves_live_subport(self, l3_lifecycle): + env = l3_lifecycle self._add(env) before = copy.deepcopy(env.state) @@ -1099,8 +1099,8 @@ def test_rejected_direct_delete_preserves_live_subport(self, interface_lifecycle env.trunk_plugin.remove_subports.assert_not_called() env.ironic.detach_vif_from_node.assert_not_called() - def test_authorized_delete_unwires_before_trunk_guard(self, interface_lifecycle): - env = interface_lifecycle + def test_authorized_delete_unwires_before_trunk_guard(self, l3_lifecycle): + env = l3_lifecycle self._add(env) env.core.delete_port(env.context, "intf-1", l3_port_check=False) @@ -1124,9 +1124,9 @@ def test_authorized_delete_unwires_before_trunk_guard(self, interface_lifecycle) ], ) def test_attach_failure_unwinds_and_allows_retry( - self, interface_lifecycle, by_port, stage + self, l3_lifecycle, by_port, stage ): - env = interface_lifecycle + env = l3_lifecycle original = {**env.interface_port, "device_id": "", "device_owner": ""} if by_port: env.state.ports["intf-1"] = copy.deepcopy(original) @@ -1161,9 +1161,9 @@ def test_attach_failure_unwinds_and_allows_retry( ids=["earlier-validator", "later-validator"], ) def test_other_callback_rejection_unwinds_successful_attach( - self, interface_lifecycle, mocker, priority + self, l3_lifecycle, mocker, priority ): - env = interface_lifecycle + env = l3_lifecycle env.state.ports["intf-1"] = { **env.interface_port, "device_id": "", @@ -1188,9 +1188,9 @@ def test_other_callback_rejection_unwinds_successful_attach( @pytest.mark.parametrize("existing_trunk", [False, True]) @pytest.mark.parametrize("existing_vif", [False, True]) def test_failure_preserves_preexisting_empty_stack( - self, interface_lifecycle, existing_trunk, existing_vif + self, l3_lifecycle, existing_trunk, existing_vif ): - env = interface_lifecycle + env = l3_lifecycle parent = env.provider._wiring._ensure_parent_port(env.router["id"]) if existing_vif: env.provider._wiring._ensure_parent_vif_attached(env.router["id"], parent) @@ -1208,8 +1208,8 @@ def test_failure_preserves_preexisting_empty_stack( assert set(env.state.ports) == {"parent-1"} assert not env.state.associations - def test_failed_subnet_preserves_existing_gateway(self, interface_lifecycle): - env = interface_lifecycle + def test_failed_subnet_preserves_existing_gateway(self, l3_lifecycle): + env = l3_lifecycle parent = env.provider._wiring._ensure_parent_port(env.router["id"]) env.provider._wiring._ensure_parent_vif_attached(env.router["id"], parent) env.provider._wiring._ensure_trunk(env.router["id"], parent) @@ -1227,10 +1227,8 @@ def test_failed_subnet_preserves_existing_gateway(self, interface_lifecycle): assert env.state.vifs == before.vifs env.ironic.detach_vif_from_node.assert_not_called() - def test_abort_retries_failed_compensation( - self, interface_lifecycle, mocker, caplog - ): - env = interface_lifecycle + def test_abort_retries_failed_compensation(self, l3_lifecycle, mocker, caplog): + env = l3_lifecycle env.state.fail = "subport_postcommit" cleanup = env.provider._wiring._remove_router_port_subport attempts = [] @@ -1255,3 +1253,143 @@ def fail_once(*args): assert not env.state.vifs assert not env.state.ports assert not env.state.associations + + +def _exhausted_deadlock(): + # As neutron_lib's retry decorators tag an error once they give up. + error = db_exc.DBDeadlock() + error._RETRY_EXCEEDED = True + return error + + +class TestRetryClassification: + @pytest.mark.parametrize( + "error", + [db_exc.DBDeadlock(), _exhausted_deadlock(), RuntimeError("boom")], + ids=["deadlock", "exhausted-deadlock", "runtime-error"], + ) + def test_agrees_with_the_callback_registry(self, error): + def failing_callback(resource, event, trigger, payload=None): + raise error + + registry.subscribe( + failing_callback, + resources.ROUTER_GATEWAY, + events.AFTER_CREATE, + cancellable=True, + ) + with pytest.raises( + (db_exc.RetryRequest, callback_exc.CallbackFailure) + ) as raised: + registry.publish( + resources.ROUTER_GATEWAY, + events.AFTER_CREATE, + self, + payload=events.DBEventPayload("ctx", resource_id="r1"), + ) + + registry_retries = isinstance(raised.value, db_exc.RetryRequest) + assert palo_alto._registry_would_retry(error) is registry_retries + + +class TestRouterGatewayLifecycle: + # Neutron has committed the gateway port by AFTER_CREATE, so on failure we + # must undo our own wiring. + + @pytest.fixture + def env(self, l3_lifecycle): + env = l3_lifecycle + env.state.ports["gw-1"] = {**_GATEWAY_PORT, "network_id": "ext-net"} + return env + + def _attach(self, env): + env.provider._process_gateway_create( + resources.ROUTER_GATEWAY, + events.AFTER_CREATE, + None, + payload=events.DBEventPayload(env.context, resource_id="r1"), + ) + + @pytest.mark.parametrize( + "stage", + [ + "parent_create", + "vif_attach", + "vif_annotation", + "trunk_create", + "subport_precommit", + "subport_postcommit", + ], + ) + def test_failure_leaves_no_wiring_and_allows_retry(self, env, stage): + env.state.fail = stage + + with pytest.raises((RuntimeError, palo_alto_wiring.PaloAltoParentNotAnnotated)): + self._attach(env) + + assert not env.state.trunks + assert not env.state.vifs + assert set(env.state.ports) == {"gw-1"} + assert env.state.ports["gw-1"]["device_id"] == "r1" + + env.state.fail = None + self._attach(env) + assert env.state.trunks["trunk-1"]["sub_ports"] == [ + {"port_id": "gw-1", "segmentation_type": "vlan", "segmentation_id": 200} + ] + + def test_failure_preserves_stack_used_by_interfaces(self, env): + env.l3.add_router_interface(env.context, "r1", {"subnet_id": "subnet-1"}) + before = copy.deepcopy(env.state) + env.state.fail = "subport_postcommit" + + with pytest.raises(RuntimeError, match="subport_postcommit"): + self._attach(env) + + assert env.state.ports == before.ports + assert env.state.trunks == before.trunks + assert env.state.vifs == before.vifs + + def _set_gateway_with_neutron_retry(self, env): + # Like update_router: a retry finds the gateway created and skips the event. + attempts = [] + + @db_api.retry_db_errors + def update_router_gateway(): + attempts.append(1) + if len(attempts) == 1: + registry.publish( + resources.ROUTER_GATEWAY, + events.AFTER_CREATE, + self, + payload=events.DBEventPayload(env.context, resource_id="r1"), + ) + + update_router_gateway() + + @pytest.mark.parametrize( + "stage", + ["trunk_lookup_deadlock", "subport_deadlock", "subport_deadlock_exhausted"], + ) + def test_transient_db_error_does_not_become_a_false_success(self, env, stage): + env.state.fail = stage + + with pytest.raises(callback_exc.CallbackFailure) as failure: + self._set_gateway_with_neutron_retry(env) + + assert not db_api.is_retriable(failure.value) + [error] = failure.value.inner_exceptions + assert isinstance(error, palo_alto.PaloAltoGatewayWiringFailed) + assert "(DBDeadlock)" in str(error) + assert not env.state.trunks + assert not env.state.vifs + + def test_running_twice_is_harmless(self, env): + # update_router's retry decorator can run the handler more than once. + self._attach(env) + wired = copy.deepcopy(env.state) + + self._attach(env) + + assert env.state == wired + env.trunk_plugin.add_subports.assert_called_once() diff --git a/python/neutron-understack/neutron_understack/tests/test_palo_alto_wiring.py b/python/neutron-understack/neutron_understack/tests/test_palo_alto_wiring.py index e6e22ca72..493f24f67 100644 --- a/python/neutron-understack/neutron_understack/tests/test_palo_alto_wiring.py +++ b/python/neutron-understack/neutron_understack/tests/test_palo_alto_wiring.py @@ -57,6 +57,7 @@ def _snapshot(**changes): fields = { "router_id": "r1", "port_id": "intf-1", + "label": "interface", "parent_id": None, "trunk_id": None, "subport_present": False, @@ -65,6 +66,16 @@ def _snapshot(**changes): return palo_alto_wiring.AttachmentSnapshot(**{**fields, **changes}) +def _ironic_port(physical_network="physnet-1", local_link_connection=None): + """An Ironic baremetal port; bindable unless a field is overridden empty.""" + if local_link_connection is None: + local_link_connection = {"switch_id": "aa", "port_id": "Eth1/1"} + return SimpleNamespace( + physical_network=physical_network, + local_link_connection=local_link_connection, + ) + + @pytest.fixture def core_plugin(mocker): core = mocker.Mock() @@ -130,18 +141,13 @@ def test_no_subport_vlan_available_is_conflict(self): [ palo_alto_wiring.PaloAltoNodeNotAdopted, palo_alto_wiring.PaloAltoParentNotAnnotated, + palo_alto_wiring.PaloAltoNodeHasNoUsablePort, ], ) def test_wiring_errors_are_bad_request(self, exc): assert issubclass(exc, n_exc.BadRequest) -class TestNames: - def test_names_are_deterministic(self): - assert palo_alto_wiring._parent_port_name("r1") == "palo-alto-router-anchor-r1" - assert palo_alto_wiring._trunk_name("r1") == "palo-alto-router-trunk-r1" - - class TestFirstFreeVlan: def test_starts_at_requested_vlan(self): assert palo_alto_wiring._first_free_vlan([(1, 300)], set(), 201) == 201 @@ -165,49 +171,6 @@ def test_returns_none_when_exhausted(self): assert palo_alto_wiring._first_free_vlan([(200, 201)], {200, 201}, 200) is None -class TestSubportHelpers: - def test_has_subport(self): - trunk = {"id": "trunk-1", "sub_ports": [{"port_id": "gw-1"}]} - - assert palo_alto_wiring._has_subport(trunk, "gw-1") is True - assert palo_alto_wiring._has_subport(trunk, "intf-1") is False - assert palo_alto_wiring._has_subport({"id": "trunk-1"}, "gw-1") is False - - def test_used_subport_vlans_counts_only_vlan_tags(self): - trunk = { - "id": "trunk-1", - "sub_ports": [ - {"port_id": "a", "segmentation_type": "vlan", "segmentation_id": 200}, - {"port_id": "b", "segmentation_type": "vlan", "segmentation_id": None}, - {"port_id": "c", "segmentation_type": "inherit"}, - ], - } - - assert palo_alto_wiring._used_subport_vlans(trunk) == {200} - - -class TestMissingBindingFields: - def test_none_missing_when_annotated(self): - assert palo_alto_wiring._missing_binding_fields(_ANNOTATED_PARENT) == [] - - def test_all_missing_when_unbound(self): - port = {"id": "parent-1", "binding:host_id": "", "binding:profile": None} - - assert palo_alto_wiring._missing_binding_fields(port) == [ - "binding:host_id", - "physical_network", - "local_link_information", - ] - - def test_reports_only_missing_profile_field(self): - port = { - **_ANNOTATED_PARENT, - "binding:profile": {"local_link_information": [{"port_id": "Eth1/1"}]}, - } - - assert palo_alto_wiring._missing_binding_fields(port) == ["physical_network"] - - class TestLookups: def test_trunk_plugin_delegates_to_utils(self, wiring, trunk_plugin): assert wiring._trunk_plugin() is trunk_plugin @@ -225,39 +188,6 @@ def test_gateway_port_none_when_absent(self, wiring, core_plugin): assert wiring.gateway_port_for_router("r1") is None - def test_gateway_port_uses_first_of_several_and_warns( - self, wiring, core_plugin, caplog - ): - core_plugin.get_ports.return_value = [{"id": "gw-1"}, {"id": "gw-2"}] - - with caplog.at_level(logging.WARNING, logger=palo_alto_wiring.__name__): - assert wiring.gateway_port_for_router("r1") == {"id": "gw-1"} - - assert "found 2; using gw-1" in caplog.text - - -class TestAnchorNetwork: - def test_reuses_existing_anchor_network(self, wiring, core_plugin): - assert wiring.ensure_anchor_network() == {"id": "anchor-net"} - - core_plugin.get_networks.assert_called_once_with( - ANY, filters={"name": [palo_alto_wiring.ANCHOR_NETWORK_NAME]} - ) - core_plugin.create_network.assert_not_called() - - def test_creates_anchor_network_with_required_defaults(self, wiring, core_plugin): - core_plugin.get_networks.return_value = [] - core_plugin.create_network.return_value = {"id": "anchor-new"} - - assert wiring.ensure_anchor_network() == {"id": "anchor-new"} - - _ctx, body = core_plugin.create_network.call_args[0] - assert body["network"]["name"] == palo_alto_wiring.ANCHOR_NETWORK_NAME - assert body["network"]["shared"] is False - # A direct plugin call skips API defaults; these two must be explicit. - assert body["network"]["router:external"] is False - assert "project_id" in body["network"] - class TestParentPort: def test_creates_parent_when_absent(self, wiring, core_plugin): @@ -398,6 +328,45 @@ def test_reuses_existing_trunk(self, wiring, trunk_plugin): trunk_plugin.create_trunk.assert_not_called() +class TestCheckCanAttach: + """The read-only check run before Neutron creates a gateway.""" + + def test_passes_when_node_has_a_bindable_port( + self, wiring, ironic, node, core_plugin, trunk_plugin + ): + ironic.node_ports.return_value = [ + _ironic_port(physical_network=None), + _ironic_port(), + ] + + wiring.check_can_attach("r1") + + ironic.node_ports.assert_called_once_with(node) + # Nothing is created or attached: the check must have no side effects. + ironic.attach_vif_to_node.assert_not_called() + core_plugin.create_network.assert_not_called() + core_plugin.create_port.assert_not_called() + trunk_plugin.create_trunk.assert_not_called() + + @pytest.mark.parametrize( + "ports", + [ + [], + [_ironic_port(physical_network=None)], + [_ironic_port(local_link_connection={})], + ], + ids=["no-ports", "no-physical-network", "no-local-link-connection"], + ) + def test_rejects_node_without_bindable_port(self, wiring, ironic, node, ports): + ironic.node_ports.return_value = ports + + with pytest.raises( + palo_alto_wiring.PaloAltoNodeHasNoUsablePort, + match="node node-1 for Palo Alto router r1", + ): + wiring.check_can_attach("r1") + + class TestEnsureStack: def test_binds_parent_before_building_trunk(self, mocker, wiring): parent = {"id": "parent-1"} @@ -551,29 +520,6 @@ def _allowed_ranges(self, mocker, ranges): return_value=ranges, ) - def test_starts_at_requested_vlan(self, mocker, wiring): - self._allowed_ranges(mocker, [(1, 199), (200, 202)]) - trunk = {"id": "trunk-1", "sub_ports": []} - - assert ( - wiring._next_available_subport_vlan( - "r1", trunk, palo_alto_wiring.INTERFACE_SUBPORT_VLAN_START - ) - == 201 - ) - - def test_skips_used_vlans(self, mocker, wiring): - self._allowed_ranges(mocker, [(200, 202)]) - - assert ( - wiring._next_available_subport_vlan( - "r1", - _TRUNK_USING_200_AND_201, - palo_alto_wiring.INTERFACE_SUBPORT_VLAN_START, - ) - == 202 - ) - def test_raises_when_no_vlan_available(self, mocker, wiring): self._allowed_ranges(mocker, [(200, 201)]) @@ -651,12 +597,6 @@ def test_keeps_stack_when_subports_remain( class TestDetachAndDeleteParent: - def test_detaches_vif_then_deletes_parent(self, wiring, ironic, core_plugin, node): - wiring._detach_and_delete_parent("r1", "parent-1") - - ironic.detach_vif_from_node.assert_called_once_with(node, "parent-1") - core_plugin.delete_port.assert_called_once_with(ANY, "parent-1") - def test_deletes_parent_even_when_node_is_gone( self, wiring, ironic, core_plugin, caplog ): @@ -672,21 +612,6 @@ def test_deletes_parent_even_when_node_is_gone( class TestCleanupAttachment: - def test_removes_subport_then_unused_stack( - self, wiring, trunk_plugin, ironic, core_plugin, node - ): - trunk_plugin.get_trunks.return_value = [_trunk("gw-1")] - trunk_plugin.get_trunk.return_value = _trunk() # after the removal - - wiring.cleanup_attachment("r1", "gw-1", "gateway") - - trunk_plugin.remove_subports.assert_called_once_with( - ANY, "trunk-1", {"sub_ports": [{"port_id": "gw-1"}]} - ) - trunk_plugin.delete_trunk.assert_called_once_with(ANY, "trunk-1") - ironic.detach_vif_from_node.assert_called_once_with(node, "parent-1") - core_plugin.delete_port.assert_called_once_with(ANY, "parent-1") - def test_deletes_orphan_parent_when_no_trunk( self, wiring, trunk_plugin, ironic, core_plugin, node ): @@ -713,30 +638,7 @@ def test_noop_when_no_trunk_and_no_parent( ironic.detach_vif_from_node.assert_not_called() -class TestSnapshotInterfaceAttachment: - def test_records_nothing_when_no_wiring_exists( - self, wiring, core_plugin, trunk_plugin - ): - core_plugin.get_ports.return_value = [] - trunk_plugin.get_trunks.return_value = [] - - assert wiring.snapshot_interface_attachment("r1", "intf-1") == _snapshot() - - def test_records_existing_stack( - self, wiring, core_plugin, trunk_plugin, ironic, node - ): - core_plugin.get_ports.return_value = [_PARENT] - trunk_plugin.get_trunks.return_value = [_trunk("intf-1")] - ironic.node_vif_ids.return_value = ["parent-1"] - - assert wiring.snapshot_interface_attachment("r1", "intf-1") == _snapshot( - parent_id="parent-1", - trunk_id="trunk-1", - subport_present=True, - parent_vif_attached=True, - ) - ironic.node_vif_ids.assert_called_once_with(node) - +class TestSnapshotAttachment: def test_treats_vif_as_detached_when_node_is_gone( self, wiring, core_plugin, trunk_plugin, ironic ): @@ -744,60 +646,13 @@ def test_treats_vif_as_detached_when_node_is_gone( trunk_plugin.get_trunks.return_value = [] ironic.node_by_instance_uuid.return_value = None - assert wiring.snapshot_interface_attachment("r1", "intf-1") == _snapshot( + assert wiring.snapshot_attachment("r1", "intf-1", "interface") == _snapshot( parent_id="parent-1" ) ironic.node_vif_ids.assert_not_called() -class TestUndoInterfaceAttachment: - """Undo compares current wiring with the snapshot taken before the attach. - - It removes only what the attach request added, and keeps anything that - existed before or that other router ports still use. - """ - - def test_removes_everything_the_request_created( - self, wiring, core_plugin, trunk_plugin, ironic, node - ): - core_plugin.get_ports.return_value = [_PARENT] - trunk_plugin.get_trunks.return_value = [_trunk("intf-1")] - trunk_plugin.get_trunk.return_value = _trunk() # after the removal - - wiring.undo_interface_attachment(_snapshot()) - - trunk_plugin.remove_subports.assert_called_once_with( - ANY, "trunk-1", {"sub_ports": [{"port_id": "intf-1"}]} - ) - trunk_plugin.delete_trunk.assert_called_once_with(ANY, "trunk-1") - ironic.detach_vif_from_node.assert_called_once_with(node, "parent-1") - core_plugin.delete_port.assert_called_once_with(ANY, "parent-1") - - def test_deletes_parent_created_before_the_trunk( - self, wiring, core_plugin, trunk_plugin, ironic, node - ): - # The attach failed after creating the parent but before the trunk. - core_plugin.get_ports.return_value = [_PARENT] - trunk_plugin.get_trunks.return_value = [] - - wiring.undo_interface_attachment(_snapshot()) - - trunk_plugin.delete_trunk.assert_not_called() - ironic.detach_vif_from_node.assert_called_once_with(node, "parent-1") - core_plugin.delete_port.assert_called_once_with(ANY, "parent-1") - - def test_does_nothing_when_no_wiring_exists( - self, wiring, core_plugin, trunk_plugin, ironic - ): - core_plugin.get_ports.return_value = [] - trunk_plugin.get_trunks.return_value = [] - - wiring.undo_interface_attachment(_snapshot()) - - trunk_plugin.remove_subports.assert_not_called() - ironic.detach_vif_from_node.assert_not_called() - core_plugin.delete_port.assert_not_called() - +class TestUndoAttachment: def test_keeps_subport_that_existed_before( self, wiring, core_plugin, trunk_plugin, ironic ): @@ -805,7 +660,7 @@ def test_keeps_subport_that_existed_before( trunk_plugin.get_trunks.return_value = [_trunk("intf-1")] trunk_plugin.get_trunk.return_value = _trunk("intf-1") - wiring.undo_interface_attachment( + wiring.undo_attachment( _snapshot( parent_id="parent-1", trunk_id="trunk-1", @@ -819,51 +674,6 @@ def test_keeps_subport_that_existed_before( ironic.detach_vif_from_node.assert_not_called() core_plugin.delete_port.assert_not_called() - def test_keeps_stack_that_other_ports_still_use( - self, wiring, core_plugin, trunk_plugin, ironic - ): - trunk_plugin.get_trunks.return_value = [_trunk("gw-1", "intf-1")] - trunk_plugin.get_trunk.return_value = _trunk("gw-1") # after the removal - - wiring.undo_interface_attachment(_snapshot()) - - trunk_plugin.remove_subports.assert_called_once_with( - ANY, "trunk-1", {"sub_ports": [{"port_id": "intf-1"}]} - ) - trunk_plugin.delete_trunk.assert_not_called() - ironic.detach_vif_from_node.assert_not_called() - core_plugin.delete_port.assert_not_called() - - def test_keeps_preexisting_empty_stack( - self, wiring, core_plugin, trunk_plugin, ironic, node - ): - core_plugin.get_ports.return_value = [_PARENT] - trunk_plugin.get_trunks.return_value = [_trunk("intf-1")] - trunk_plugin.get_trunk.return_value = _trunk() # after the removal - - wiring.undo_interface_attachment( - _snapshot( - parent_id="parent-1", trunk_id="trunk-1", parent_vif_attached=True - ) - ) - - trunk_plugin.remove_subports.assert_called_once() - trunk_plugin.delete_trunk.assert_not_called() - ironic.detach_vif_from_node.assert_not_called() - core_plugin.delete_port.assert_not_called() - - def test_detaches_vif_this_request_attached( - self, wiring, core_plugin, trunk_plugin, ironic, node - ): - # The parent existed before, but this request attached its VIF. - core_plugin.get_ports.return_value = [_PARENT] - trunk_plugin.get_trunks.return_value = [] - - wiring.undo_interface_attachment(_snapshot(parent_id="parent-1")) - - ironic.detach_vif_from_node.assert_called_once_with(node, "parent-1") - core_plugin.delete_port.assert_not_called() - def test_skips_vif_detach_when_node_is_gone( self, wiring, core_plugin, trunk_plugin, ironic ): @@ -871,7 +681,7 @@ def test_skips_vif_detach_when_node_is_gone( trunk_plugin.get_trunks.return_value = [] ironic.node_by_instance_uuid.return_value = None - wiring.undo_interface_attachment(_snapshot(parent_id="parent-1")) + wiring.undo_attachment(_snapshot(parent_id="parent-1")) ironic.detach_vif_from_node.assert_not_called() core_plugin.delete_port.assert_not_called()