From ddf762b901173d7347d87c693d6b0edb6907ac57 Mon Sep 17 00:00:00 2001 From: Nidhi Rai Date: Mon, 3 Aug 2026 18:12:44 +0530 Subject: [PATCH] Adopt netdev node for Palo Alto router flavor --- .../neutron_understack/ironic.py | 266 ++++++++++++++++++ .../neutron_understack/l3_router/palo_alto.py | 177 +++++++++++- .../neutron_understack/tests/test_ironic.py | 138 +++++++++ .../tests/test_palo_alto_provider.py | 259 +++++++++++++++-- 4 files changed, 808 insertions(+), 32 deletions(-) create mode 100644 python/neutron-understack/neutron_understack/tests/test_ironic.py diff --git a/python/neutron-understack/neutron_understack/ironic.py b/python/neutron-understack/neutron_understack/ironic.py index c182a2739..3dbf8e131 100644 --- a/python/neutron-understack/neutron_understack/ironic.py +++ b/python/neutron-understack/neutron_understack/ironic.py @@ -1,12 +1,47 @@ import importlib.metadata +import logging from openstack import connection from openstack.baremetal.baremetal_service import BaremetalService +from openstack.baremetal.v1.node import Node as BaremetalNode from openstack.baremetal.v1.port import Port as BaremetalPort from oslo_config import cfg from neutron_understack import config +LOG = logging.getLogger(__name__) + +# Ironic provision-state targets (verbs) used by the netdev router flavor +# lifecycle. available -> (manage) -> manageable -> (adopt) -> active on adopt; +# manageable -> (provide) -> available to roll back a partial adoption; and +# active -> (deleted/undeploy) -> available (triggering cleaning) on release. +_PROVISION_MANAGE = "manage" +_PROVISION_ADOPT = "adopt" +_PROVISION_PROVIDE = "provide" +_PROVISION_UNDEPLOY = "deleted" + +# Stable provision states (not verbs). We wait for adopt to reach "active" +# explicitly rather than via set_node_provision_state(wait=True): the SDK's +# EXPECTED_STATES maps the "adopt" verb to "available" (in +# openstack/baremetal/v1/_common.py), which is wrong -- Ironic drives adopt to +# "active" -- so the built-in wait would poll for the wrong state and time out. +# https://review.opendev.org/c/openstack/openstacksdk/+/999686 +# Will remove this ones the changes gets raised +_STATE_ACTIVE = "active" +_STATE_MANAGEABLE = "manageable" +_STATE_AVAILABLE = "available" + +# Seconds to wait for each provision-state transition to settle. netdev nodes +# have noop deploy/clean interfaces so these transitions are effectively +# instantaneous, but we still bound the wait so an API worker cannot hang +# forever on an unresponsive Ironic. +_PROVISION_TIMEOUT = 300 + +# Ironic hardware type for network appliance devices. Router flavors only adopt +# nodes of this driver, so a resource_class shared with other hardware types +# (e.g. servers) cannot cause us to adopt the wrong node. +_NETDEV_DRIVER = "netdev" + class IronicClient: def __init__(self): @@ -51,3 +86,234 @@ def baremetal_node_uuid(self, node_name: str) -> str | None: return node.id if node else None except Exception: return None + + def available_node_for_resource_class( + self, resource_class: str + ) -> BaremetalNode | None: + """Return the first available Ironic node with the given resource class. + + Ironic filters server-side by ``driver=netdev``, ``resource_class``, + ``provision_state=available`` and not-in-maintenance, so any returned + node is a netdev appliance that is actually usable, in the interchangeable + pool for this flavor. Selection is first-match; there is no + scheduling/ranking. + (WIP circle back here , if there is any rule select netdev). + """ + try: + node = next( + self.irclient.nodes( + driver=_NETDEV_DRIVER, + resource_class=resource_class, + provision_state="available", + # Skip nodes an operator has parked in maintenance. + # Ironic will still let us adopt such a node, so + # without this filter we would silently put a router on + # hardware that was deliberately taken out of service. + is_maintenance=False, + details=True, + ) + ) + except StopIteration: + LOG.info( + "No available netdev node found for resource_class=%s", + resource_class, + ) + return None + LOG.info( + "Selected available netdev node %s (name=%s) for resource_class=%s", + node.id, + node.name, + resource_class, + ) + return node + + def node_by_instance_uuid(self, instance_uuid: str) -> BaremetalNode | None: + """Return the node currently adopted for the given instance UUID.""" + try: + return next(self.irclient.nodes(instance_id=instance_uuid, details=True)) + except StopIteration: + return None + + def adopt_node_for_router( + self, + node: str | BaremetalNode, + *, + project_id: str, + router_id: str, + router_name: str, + ) -> None: + """Adopt a node and bind it to the owning project and router. + + Drives available -> manageable -> active via the Ironic ``adopt`` verb, + stamping ``lessee`` (owning project), ``instance_uuid`` (router UUID) and + ``instance_name`` (router name). ``instance_name`` is a distinct field + from the node's own ``name``, so the node's enrollment name is preserved. + """ + node_id = node.id if isinstance(node, BaremetalNode) else node + LOG.info( + "Adopting node %s for router %s (name=%s project=%s): manage " + "(available -> manageable)", + node_id, + router_id, + router_name, + project_id, + ) + try: + # available -> manageable, required before the adopt verb is valid. + # Kept inside the try so a manage failure like the wait timing out + # after the node already reached manageable, or a concurrent create + # having claimed the node and is rolled back too, instead of stranding + # the node in manageable. + managed = self.irclient.set_node_provision_state( + node, _PROVISION_MANAGE, wait=True, timeout=_PROVISION_TIMEOUT + ) + LOG.debug( + "Node %s provision_state=%s after manage", + node_id, + getattr(managed, "provision_state", "?"), + ) + # Stamp ownership while manageable. A CONFLICT on instance_uuid means + # the node was claimed by another router concurrently; it is terminal, + # not a transient lock, so do not retry it. + LOG.info( + "Stamping node %s: lessee=%s instance_uuid=%s instance_name=%s", + node_id, + project_id, + router_id, + router_name, + ) + self.irclient.update_node( + node, + retry_on_conflict=False, + lessee=project_id, + instance_id=router_id, + instance_name=router_name, + ) + # manageable -> active via adopt (no real deploy for netdev nodes). + # Issue with wait=False and wait explicitly for "active": the SDK's + # built-in wait for the "adopt" verb targets "available" (wrong). + LOG.info("Node %s: adopt (manageable -> active)", node_id) + self.irclient.set_node_provision_state(node, _PROVISION_ADOPT, wait=False) + adopted = self.irclient.wait_for_nodes_provision_state( + [node], _STATE_ACTIVE, timeout=_PROVISION_TIMEOUT + )[0] + except Exception: + # Adopt was not confirmed. The node may be manageable (maybe stamped), + # still adopting, adopt-failed, or even active if the wait aborted + # after the transition completed. _return_node_to_available re-reads + # the state and picks the right recovery, then we re-raise so the + # caller aborts the router create. + LOG.warning( + "Adoption of node %s for router %s failed; rolling back to available", + node_id, + router_id, + ) + self._return_node_to_available(node) + raise + LOG.info( + "Node %s adopted for router %s: provision_state=%s lessee=%s " + "instance_uuid=%s instance_name=%s", + node_id, + router_id, + adopted.provision_state, + adopted.lessee, + adopted.instance_id, + adopted.instance_name, + ) + + def _return_node_to_available(self, node: str | BaremetalNode) -> None: + """Return a node to the available pool from whatever state it is in. + + Re-reads the node's current provision state and picks the correct verb, + because this runs both as adopt rollback (where a timed-out or aborted + adopt may have left the node ``manageable``, ``active`` or in a failure + state) and as normal release. Best-effort and guarded so it never masks + a caller's original error: + + * ``available`` -> just clear any stale ownership stamps; + * ``manageable`` -> clear our ownership stamps, then ``provide``; + * ``active`` -> ``undeploy`` (triggers cleaning), then clear ownership + -- undeploy tears down instance_uuid/instance_name but NOT lessee; + * anything else (e.g. ``adopt failed``, ``adopting``) -> leave for + reconciliation rather than issue an invalid transition. + """ + try: + node = self.irclient.get_node(node) + except Exception: + LOG.exception("Could not fetch node to return it to available") + return + node_id = node.id + state = node.provision_state + + if state == _STATE_AVAILABLE: + # Already available, but may still carry a lessee from a prior + # adoption (undeploy does not clear it); make sure it is truly free. + self._clear_ownership(node, node_id) + elif state == _STATE_MANAGEABLE: + LOG.info("Returning node %s to available (clear stamps + provide)", node_id) + self._clear_ownership(node, node_id) + self._guarded_provision(node, _PROVISION_PROVIDE, node_id) + elif state == _STATE_ACTIVE: + LOG.info("Returning node %s to available (undeploy)", node_id) + self._guarded_provision(node, _PROVISION_UNDEPLOY, node_id) + # undeploy clears instance_uuid/instance_name but leaves lessee, so + # the node would rejoin the pool still leased to the deleted router's + # project. Clear ownership explicitly. + self._clear_ownership(node, node_id) + else: + LOG.warning( + "Node %s is in state %s; cannot auto-return it to available, " + "leaving for reconciliation", + node_id, + state, + ) + + def _clear_ownership(self, node: BaremetalNode, node_id: str) -> None: + """Clear lessee + instance association so the node rejoins the pool free.""" + try: + self.irclient.update_node( + node, + retry_on_conflict=False, + lessee=None, + instance_id=None, + instance_name=None, + ) + LOG.info("Cleared ownership stamps on node %s", node_id) + except Exception: + LOG.exception("Failed to clear ownership on node %s", node_id) + + def _guarded_provision( + self, node: BaremetalNode, target: str, node_id: str + ) -> None: + """Drive a provision-state transition, logging (not raising) on failure.""" + try: + self.irclient.set_node_provision_state( + node, target, wait=True, timeout=_PROVISION_TIMEOUT + ) + LOG.info("Node %s reached available via %s", node_id, target) + except Exception: + LOG.exception( + "Failed to return node %s to available via %s; manual cleanup " + "may be required", + node_id, + target, + ) + + def release_node_for_router(self, router_id: str) -> BaremetalNode | None: + """Return the router's node to the available pool, whatever its state. + + A fully adopted node is ``active`` and is undeployed (triggering + cleaning); other states are handled by ``_return_node_to_available``. + Returns the node, or None if none is bound to this router. + """ + node = self.node_by_instance_uuid(router_id) + if node is None: + return None + LOG.info( + "Releasing node %s bound to router %s (current provision_state=%s)", + node.id, + router_id, + node.provision_state, + ) + self._return_node_to_available(node) + return node 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 5aa473f1f..7f83650e8 100644 --- a/python/neutron-understack/neutron_understack/l3_router/palo_alto.py +++ b/python/neutron-understack/neutron_understack/l3_router/palo_alto.py @@ -1,23 +1,66 @@ +import json import logging 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.callbacks import events from neutron_lib.callbacks import registry from neutron_lib.callbacks import resources from neutron_lib.plugins import constants as plugin_constants from neutron_lib.plugins import directory +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" + + +# Conflict -> HTTP 409: the request cannot be satisfied because the hardware +# pool is exhausted. +class NoNetdevNodeAvailable(n_exc.Conflict): + message = ( + "No Ironic node with resource_class %(resource_class)s is available to " + "realize router %(router_id)s." + ) + + +# BadRequest -> HTTP 400: the flavor/profile is misconfigured. +class PaloAltoFlavorMisconfigured(n_exc.BadRequest): + message = ( + "Router %(router_id)s flavor %(flavor_id)s does not define a " + "resource_class in its service profile metainfo." + ) + + +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): + return {} + return parsed if isinstance(parsed, dict) else {} + @registry.has_registry_receivers class PaloAlto(base.L3ServiceProvider): - """Stub L3 service provider for the Palo Alto router flavor. + """L3 service provider for the Palo Alto router flavor. - Inherits from the base L3 service provider instead - of UserDefined. Routers of this flavor are detected - via their flavor's service profile driver. + A router of this flavor is realized on a netdev bare metal appliance. On + create it adopts an available Ironic node (selected by the resource_class + 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. """ ha_support = base.OPTIONAL @@ -38,6 +81,16 @@ def _flavor_plugin(self): self._flavor_plugin_ref = directory.get_plugin(plugin_constants.FLAVORS) return self._flavor_plugin_ref + @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 + def _is_palo_alto_provider(self, context, router): flavor_id = router.get("flavor_id") if flavor_id is None or flavor_id is const.ATTR_NOT_SPECIFIED: @@ -71,18 +124,122 @@ def _is_palo_alto_provider(self, context, router): ) return matched - @registry.receives(resources.ROUTER, [events.AFTER_CREATE]) + def _resource_class_for_router(self, context, router) -> str: + """Read the target resource_class from the flavor's profile metainfo. + + This is a separate lookup from the driver match: the driver string + selects *this code*, the metainfo resource_class selects *which + hardware pool* to adopt from. + """ + flavor = self._flavor_plugin.get_flavor(context, router["flavor_id"]) + for sp_id in flavor.get("service_profiles") or []: + service_profile = self._flavor_plugin.get_service_profile(context, sp_id) + resource_class = _parse_metainfo(service_profile.get("metainfo")).get( + "resource_class" + ) + if resource_class: + return resource_class + raise PaloAltoFlavorMisconfigured( + 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) + # Not using API,coz _process_router_create runs as a callback inside the + # 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 "", + } + }, + ) + + @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. + + BEFORE_CREATE is a cancellable event published outside the DB + transaction, and the router UUID is already pre-generated at this point. + Doing the whole adoption here means every failure (no node, misconfigured + flavor, or a failed Ironic transition) raises and is returned to the API + as a clean error with no router created -- and adopt_node_for_router + rolls a partially-adopted node back to available, so nothing is stranded. + """ router = payload.states[0] context = payload.context if not self._is_palo_alto_provider(context, router): return + + resource_class = self._resource_class_for_router(context, router) + node = self._ironic.available_node_for_resource_class(resource_class) + if node is None: + raise NoNetdevNodeAvailable( + resource_class=resource_class, router_id=router["id"] + ) + + # 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._ironic.adopt_node_for_router( + node, + project_id=router.get("project_id"), + router_id=router["id"], + router_name=router.get("name") or router["id"], + ) + LOG.info( - "Palo Alto stub router create: no action taken for router=%s " - "name=%s project=%s flavor=%s request_id=%s", - router.get("id"), + "Adopted Ironic node %s for Palo Alto router=%s name=%s project=%s " + "resource_class=%s", + node.id, + router["id"], router.get("name"), router.get("project_id"), - router.get("flavor_id"), - getattr(context, "request_id", None), + resource_class, + ) + + # avoiding Before_delete coz It fires before the "router in use" check + # releases node for a router that did not get deleted + @registry.receives(resources.ROUTER, [events.AFTER_DELETE]) + def _process_router_delete(self, resource, event, trigger, payload=None): + router = payload.states[0] + context = payload.context + if not self._is_palo_alto_provider(context, router): + return + + node = self._ironic.release_node_for_router(router["id"]) + if node is None: + LOG.warning( + "Palo Alto router %s deleted but no adopted Ironic node was " + "found to release", + router["id"], + ) + return + LOG.info( + "Released Ironic node %s from deleted Palo Alto router %s " + "(active -> available, cleaning triggered)", + node.id, + router["id"], ) diff --git a/python/neutron-understack/neutron_understack/tests/test_ironic.py b/python/neutron-understack/neutron_understack/tests/test_ironic.py new file mode 100644 index 000000000..7adcff568 --- /dev/null +++ b/python/neutron-understack/neutron_understack/tests/test_ironic.py @@ -0,0 +1,138 @@ +"""Unit tests for the IronicClient adopt/release state handling. + +IronicClient.__init__ needs Ironic config, so these bypass it with __new__ and +inject a mock ``irclient`` (the openstacksdk baremetal proxy). +""" + +import pytest + +from neutron_understack.ironic import IronicClient + + +def _client(mocker): + client = IronicClient.__new__(IronicClient) + client.irclient = mocker.Mock() + return client + + +class TestAdoptRollback: + def test_manage_failure_is_rolled_back_and_reraised(self, mocker): + # A manage failure must route through _return_node_to_available (not + # strand the node in manageable) and re-raise so the create aborts. + client = _client(mocker) + node = mocker.Mock(id="n1", provision_state="manageable") + # 1st set_node_provision_state (manage) raises; the rollback's provide + # (2nd call) succeeds. + client.irclient.set_node_provision_state.side_effect = [ + RuntimeError("manage boom"), + None, + ] + client.irclient.get_node.return_value = node + + with pytest.raises(RuntimeError): + client.adopt_node_for_router( + node, project_id="p", router_id="r", router_name="n" + ) + + # rollback re-fetched state and tried to return it to available + client.irclient.get_node.assert_called_once() + assert ( + client.irclient.set_node_provision_state.call_count == 2 + ) # manage + provide + + +class TestReleaseClearsOwnership: + def test_active_node_is_undeployed_then_ownership_cleared(self, mocker): + client = _client(mocker) + node = mocker.Mock(id="n1", provision_state="active") + client.irclient.get_node.return_value = node + + client._return_node_to_available(node) + + # active -> undeploy ("deleted") + (_, target), _ = client.irclient.set_node_provision_state.call_args + assert target == "deleted" + # undeploy leaves lessee, so we must clear ownership afterwards + _, kwargs = client.irclient.update_node.call_args + assert kwargs["lessee"] is None + assert kwargs["instance_id"] is None + assert kwargs["instance_name"] is None + + def test_manageable_node_is_cleared_then_provided(self, mocker): + client = _client(mocker) + node = mocker.Mock(id="n1", provision_state="manageable") + client.irclient.get_node.return_value = node + + client._return_node_to_available(node) + + client.irclient.update_node.assert_called_once() + (_, target), _ = client.irclient.set_node_provision_state.call_args + assert target == "provide" + + def test_available_node_still_gets_ownership_cleared(self, mocker): + # e.g. a node left available with a stale lessee from a prior adoption + client = _client(mocker) + node = mocker.Mock(id="n1", provision_state="available") + client.irclient.get_node.return_value = node + + client._return_node_to_available(node) + + client.irclient.update_node.assert_called_once() + client.irclient.set_node_provision_state.assert_not_called() + + def test_unexpected_state_is_left_for_reconciliation(self, mocker): + client = _client(mocker) + node = mocker.Mock(id="n1", provision_state="adopt failed") + client.irclient.get_node.return_value = node + + client._return_node_to_available(node) + + client.irclient.update_node.assert_not_called() + client.irclient.set_node_provision_state.assert_not_called() + + +class TestNodeSelection: + def test_filters_available_non_maintenance_netdev(self, mocker): + client = _client(mocker) + node = mocker.Mock(id="n1") + client.irclient.nodes.return_value = iter([node]) + + result = client.available_node_for_resource_class("pa1410") + + assert result is node + _, kwargs = client.irclient.nodes.call_args + assert kwargs["driver"] == "netdev" + assert kwargs["resource_class"] == "pa1410" + assert kwargs["provision_state"] == "available" + # a node parked in maintenance must never be selected + assert kwargs["is_maintenance"] is False + + def test_returns_none_when_pool_empty(self, mocker): + client = _client(mocker) + client.irclient.nodes.return_value = iter([]) + + assert client.available_node_for_resource_class("pa1410") is None + + +class TestReleaseNodeForRouter: + def test_returns_none_when_no_node_bound(self, mocker): + client = _client(mocker) + client.irclient.nodes.return_value = iter([]) + + assert client.release_node_for_router("router-1") is None + + def test_releases_bound_node(self, mocker): + client = _client(mocker) + node = mocker.Mock(id="n1", provision_state="active") + # node_by_instance_uuid uses irclient.nodes(); _return_node_to_available + # re-fetches via get_node. + client.irclient.nodes.return_value = iter([node]) + client.irclient.get_node.return_value = node + + result = client.release_node_for_router("router-1") + + assert result is node + (_, target), _ = client.irclient.set_node_provision_state.call_args + assert target == "deleted" + _, kwargs = client.irclient.update_node.call_args + assert kwargs["lessee"] is None 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 af23e2999..9d2466d71 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 @@ -1,18 +1,25 @@ +import pytest from neutron_lib import constants as const +from neutron_lib import exceptions as n_exc from neutron_understack.l3_router import palo_alto class FakeFlavorPlugin: - def __init__(self, driver): + def __init__(self, driver, service_profiles=None, profiles=None): self.driver = driver + self._service_profiles = service_profiles or [] + self._profiles = profiles or {} def get_flavor(self, _context, flavor_id): - return {"id": flavor_id} + return {"id": flavor_id, "service_profiles": self._service_profiles} def get_flavor_next_provider(self, _context, _flavor_id): return [{"driver": self.driver}] + def get_service_profile(self, _context, sp_id): + return self._profiles[sp_id] + class FakePayload: def __init__(self, router, context="context"): @@ -25,6 +32,39 @@ def _palo_alto_driver(): return f"{palo_alto.PaloAlto.__module__}.{palo_alto.PaloAlto.__name__}" +def _make_provider(mocker, flavor_plugin, ironic=None, core_plugin=None): + # directory.get_plugin(FLAVORS) -> flavor plugin (also pre-cached below); + # directory.get_plugin() with no args -> core plugin (anchor network). + mocker.patch.object( + palo_alto.directory, + "get_plugin", + 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") + provider = palo_alto.PaloAlto(mocker.Mock()) + provider._flavor_plugin_ref = flavor_plugin + if ironic is not None: + provider._ironic_ref = ironic + return provider + + +class TestMetainfoParsing: + def test_parses_json_string(self): + assert _parse('{"resource_class": "BLAH"}') == {"resource_class": "BLAH"} + + def test_accepts_dict(self): + assert _parse({"resource_class": "BLAH"}) == {"resource_class": "BLAH"} + + @pytest.mark.parametrize("raw", [None, "", "not-json", "[1, 2]"]) + def test_returns_empty_for_bad_input(self, raw): + assert _parse(raw) == {} + + +def _parse(raw): + return palo_alto._parse_metainfo(raw) + + class TestPaloAltoProvider: def test_flavor_plugin_is_cached(self, mocker): plugin = FakeFlavorPlugin(_palo_alto_driver()) @@ -77,31 +117,206 @@ def test_is_palo_alto_provider_returns_false_for_different_driver(self, mocker): is False ) - def test_router_create_is_noop_for_palo_alto_router(self, mocker): - plugin = FakeFlavorPlugin(_palo_alto_driver()) - mocker.patch.object(palo_alto.directory, "get_plugin", return_value=plugin) - l3_plugin = mocker.Mock() - provider = palo_alto.PaloAlto(l3_plugin) - payload = FakePayload({"id": "router-a", "flavor_id": "palo-alto-flavor-id"}) - result = provider._process_router_create( - "router", "after_create", "trigger", payload +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_node_available_is_conflict(self): + assert issubclass(palo_alto.NoNetdevNodeAvailable, n_exc.Conflict) + + def test_flavor_misconfigured_is_bad_request(self): + assert issubclass(palo_alto.PaloAltoFlavorMisconfigured, n_exc.BadRequest) + + +class TestResourceClassLookup: + def test_reads_resource_class_from_profile_metainfo(self, mocker): + plugin = FakeFlavorPlugin( + _palo_alto_driver(), + service_profiles=["sp1"], + profiles={"sp1": {"metainfo": '{"resource_class": "PA-FW"}'}}, + ) + provider = _make_provider(mocker, plugin) + rc = provider._resource_class_for_router("ctx", {"id": "r1", "flavor_id": "f1"}) + assert rc == "PA-FW" + + def test_raises_when_no_resource_class(self, mocker): + plugin = FakeFlavorPlugin( + _palo_alto_driver(), + service_profiles=["sp1"], + profiles={"sp1": {"metainfo": "{}"}}, + ) + provider = _make_provider(mocker, plugin) + with pytest.raises(palo_alto.PaloAltoFlavorMisconfigured): + provider._resource_class_for_router("ctx", {"id": "r1", "flavor_id": "f1"}) + + +def _router(): + return { + "id": "router-uuid", + "name": "my-router", + "project_id": "proj-1", + "flavor_id": "f1", + } + + +def _adopting_plugin(): + return FakeFlavorPlugin( + _palo_alto_driver(), + service_profiles=["sp1"], + profiles={"sp1": {"metainfo": '{"resource_class": "PA-FW"}'}}, + ) + + +class TestRouterCreate: + """Adoption happens on the cancellable BEFORE_CREATE event. + + The router UUID is pre-generated there, so the handler validates *and* + adopts. Any failure raises out of a cancellable event, so the router is + never created without hardware. + """ + + def test_adopts_node_and_creates_anchor_network(self, mocker): + ironic = mocker.Mock() + node = mocker.Mock(id="node-uuid") + ironic.available_node_for_resource_class.return_value = node + core_plugin = mocker.Mock() + core_plugin.get_networks.return_value = [] + + provider = _make_provider( + mocker, _adopting_plugin(), ironic=ironic, core_plugin=core_plugin + ) + provider._process_router_create( + "router", "before_create", "trigger", FakePayload(_router()) + ) + + ironic.available_node_for_resource_class.assert_called_once_with("PA-FW") + core_plugin.create_network.assert_called_once() + # project_id must be supplied explicitly when calling the core plugin + # directly (the API layer would otherwise fill it in). + _ctx, body = core_plugin.create_network.call_args[0] + # Direct plugin call skips API-layer extension defaults, so we must + # supply project_id and router:external (the latter is read by the + # auto_allocate NETWORK-create callback). + assert "project_id" in body["network"] + assert body["network"]["router:external"] is False + ironic.adopt_node_for_router.assert_called_once_with( + node, + project_id="proj-1", + router_id="router-uuid", + router_name="my-router", ) - # Stub flavor: no action is taken on a matching router. - assert result is None - assert l3_plugin.mock_calls == [] + def test_reuses_existing_anchor_network(self, mocker): + ironic = mocker.Mock() + ironic.available_node_for_resource_class.return_value = mocker.Mock(id="n1") + core_plugin = mocker.Mock() + core_plugin.get_networks.return_value = [{"id": "existing-net"}] - def test_router_create_skips_non_palo_alto_router(self, mocker): + provider = _make_provider( + mocker, _adopting_plugin(), ironic=ironic, core_plugin=core_plugin + ) + provider._process_router_create( + "router", "before_create", "trigger", FakePayload(_router()) + ) + + core_plugin.create_network.assert_not_called() + + def test_raises_when_no_node_available(self, mocker): + ironic = mocker.Mock() + ironic.available_node_for_resource_class.return_value = None + core_plugin = mocker.Mock() + + provider = _make_provider( + mocker, _adopting_plugin(), ironic=ironic, core_plugin=core_plugin + ) + with pytest.raises(palo_alto.NoNetdevNodeAvailable): + provider._process_router_create( + "router", "before_create", "trigger", FakePayload(_router()) + ) + ironic.adopt_node_for_router.assert_not_called() + core_plugin.create_network.assert_not_called() + + def test_raises_when_flavor_misconfigured(self, mocker): + plugin = FakeFlavorPlugin( + _palo_alto_driver(), + service_profiles=["sp1"], + profiles={"sp1": {"metainfo": "{}"}}, + ) + ironic = mocker.Mock() + provider = _make_provider(mocker, plugin, ironic=ironic) + + with pytest.raises(palo_alto.PaloAltoFlavorMisconfigured): + provider._process_router_create( + "router", "before_create", "trigger", FakePayload(_router()) + ) + ironic.available_node_for_resource_class.assert_not_called() + + def test_propagates_adopt_failure(self, mocker): + # adopt_node_for_router rolls the node back internally, then re-raises; + # because this is BEFORE_CREATE the raise aborts the router create. + ironic = mocker.Mock() + ironic.available_node_for_resource_class.return_value = mocker.Mock(id="n1") + ironic.adopt_node_for_router.side_effect = RuntimeError("boom") + core_plugin = mocker.Mock() + core_plugin.get_networks.return_value = [] + + provider = _make_provider( + mocker, _adopting_plugin(), ironic=ironic, core_plugin=core_plugin + ) + with pytest.raises(RuntimeError): + provider._process_router_create( + "router", "before_create", "trigger", FakePayload(_router()) + ) + + def test_skips_non_palo_alto_router(self, mocker): + ironic = mocker.Mock() plugin = FakeFlavorPlugin("neutron_understack.l3_router.vrf.Vrf") - mocker.patch.object(palo_alto.directory, "get_plugin", return_value=plugin) - l3_plugin = mocker.Mock() - provider = palo_alto.PaloAlto(l3_plugin) - payload = FakePayload({"id": "router-b", "flavor_id": "vrf-flavor-id"}) + provider = _make_provider(mocker, plugin, ironic=ironic) + + provider._process_router_create( + "router", "before_create", "trigger", FakePayload(_router()) + ) + ironic.available_node_for_resource_class.assert_not_called() + + +class TestRouterDelete: + def _router(self): + return {"id": "router-uuid", "name": "my-router", "flavor_id": "f1"} + + def test_releases_adopted_node(self, mocker): + ironic = mocker.Mock() + ironic.release_node_for_router.return_value = mocker.Mock(id="node-uuid") + provider = _make_provider( + mocker, FakeFlavorPlugin(_palo_alto_driver()), ironic=ironic + ) - result = provider._process_router_create( - "router", "after_create", "trigger", payload + provider._process_router_delete( + "router", "after_delete", "trigger", FakePayload(self._router()) ) + ironic.release_node_for_router.assert_called_once_with("router-uuid") - assert result is None - assert l3_plugin.mock_calls == [] + def test_warns_when_no_node_bound(self, mocker): + ironic = mocker.Mock() + ironic.release_node_for_router.return_value = None + provider = _make_provider( + mocker, FakeFlavorPlugin(_palo_alto_driver()), ironic=ironic + ) + + # Should not raise even though nothing was released. + provider._process_router_delete( + "router", "after_delete", "trigger", FakePayload(self._router()) + ) + + def test_skips_non_palo_alto_router(self, mocker): + ironic = mocker.Mock() + plugin = FakeFlavorPlugin("neutron_understack.l3_router.vrf.Vrf") + provider = _make_provider(mocker, plugin, ironic=ironic) + + provider._process_router_delete( + "router", "after_delete", "trigger", FakePayload(self._router()) + ) + ironic.release_node_for_router.assert_not_called()