diff --git a/components/images-openstack.yaml b/components/images-openstack.yaml index d86ccb51a..bcef5f670 100644 --- a/components/images-openstack.yaml +++ b/components/images-openstack.yaml @@ -35,22 +35,20 @@ images: ironic_retrive_swift_config: "ghcr.io/rackerlabs/understack/openstack-client:2025.2" # neutron - neutron_db_sync: "ghcr.io/rackerlabs/understack/neutron:2026.1" - neutron_dhcp: "ghcr.io/rackerlabs/understack/neutron:2026.1" - neutron_l3: "ghcr.io/rackerlabs/understack/neutron:2026.1" - neutron_l2gw: "ghcr.io/rackerlabs/understack/neutron:2026.1" - neutron_linuxbridge_agent: "ghcr.io/rackerlabs/understack/neutron:2026.1" - neutron_metadata: "ghcr.io/rackerlabs/understack/neutron:2026.1" - neutron_ovn_metadata_agent: "ghcr.io/rackerlabs/understack/neutron:2026.1" - neutron_openvswitch_agent: "ghcr.io/rackerlabs/understack/neutron:2026.1" - neutron_server: "ghcr.io/rackerlabs/understack/neutron:2026.1" - neutron_rpc_server: "ghcr.io/rackerlabs/understack/neutron:2026.1" - neutron_ovn_maintenance_worker: "ghcr.io/rackerlabs/understack/neutron:2026.1" - neutron_ironic_agent: "ghcr.io/rackerlabs/understack/neutron:2026.1" - neutron_ironic_agent_init: "ghcr.io/rackerlabs/understack/neutron:2026.1" - neutron_periodic_worker: "ghcr.io/rackerlabs/understack/neutron:2026.1" - neutron_bagpipe_bgp: "ghcr.io/rackerlabs/understack/neutron:2026.1" - neutron_netns_cleanup_cron: "ghcr.io/rackerlabs/understack/neutron:2026.1" + # DEV-TEST ONLY: pinned to pr-2370 for the netdev-router reconciler. + neutron_db_sync: "ghcr.io/rackerlabs/understack/neutron:pr-2370" + neutron_dhcp: "ghcr.io/rackerlabs/understack/neutron:pr-2370" + neutron_l3: "ghcr.io/rackerlabs/understack/neutron:pr-2370" + neutron_l2gw: "ghcr.io/rackerlabs/understack/neutron:pr-2370" + neutron_linuxbridge_agent: "ghcr.io/rackerlabs/understack/neutron:pr-2370" + neutron_metadata: "ghcr.io/rackerlabs/understack/neutron:pr-2370" + neutron_ovn_metadata: "ghcr.io/rackerlabs/understack/neutron:pr-2370" + neutron_openvswitch_agent: "ghcr.io/rackerlabs/understack/neutron:pr-2370" + neutron_server: "ghcr.io/rackerlabs/understack/neutron:pr-2370" + neutron_rpc_server: "ghcr.io/rackerlabs/understack/neutron:pr-2370" + neutron_ovn_maintenance_worker: "ghcr.io/rackerlabs/understack/neutron:pr-2370" + neutron_bagpipe_bgp: "ghcr.io/rackerlabs/understack/neutron:pr-2370" + neutron_netns_cleanup_cron: "ghcr.io/rackerlabs/understack/neutron:pr-2370" # nova nova_api: "ghcr.io/rackerlabs/understack/nova:2026.1" diff --git a/python/neutron-understack/neutron_understack/config.py b/python/neutron-understack/neutron_understack/config.py index 10d75d036..3238b5980 100644 --- a/python/neutron-understack/neutron_understack/config.py +++ b/python/neutron-understack/neutron_understack/config.py @@ -6,6 +6,7 @@ _OPT_GRP_IRONIC = "ironic" _OPT_GRP_L3_SVC_CISCO_ASA = "l3_service_cisco_asa" _OPT_GRP_UNDERSTACK_VNI = "understack_vni" +_OPT_GRP_NETDEV_RECONCILE = "netdev_router_reconcile" _mech_understack_opts = [ cfg.StrOpt( @@ -69,6 +70,29 @@ ] +_netdev_reconcile_opts = [ + cfg.BoolOpt( + "enabled", + default=True, + help=( + "Run the periodic netdev-router reconciler in the OVN maintenance " + "worker. It releases netdev Ironic nodes whose instance_uuid names " + "a router that no longer exists. Set to false to stop it from " + "changing any node state." + ), + ), + cfg.BoolOpt( + "dry_run", + default=False, + help=( + "Log the netdev nodes the reconciler would release without " + "releasing them. Use this to confirm the candidate set in a new " + "region before letting it act." + ), + ), +] + + def list_understack_opts(): return [ (_OPT_GRP_ML2_UNDERSTACK, _mech_understack_opts), @@ -100,6 +124,12 @@ def list_understack_vni_opts(): ] +def list_netdev_reconcile_opts(): + return [ + (_OPT_GRP_NETDEV_RECONCILE, _netdev_reconcile_opts), + ] + + def register_ml2_understack_opts(config): config.register_opts(_mech_understack_opts, _OPT_GRP_ML2_UNDERSTACK) @@ -118,6 +148,10 @@ def register_understack_vni_opts(config): config.register_opts(_understack_vni_opts, _OPT_GRP_UNDERSTACK_VNI) +def register_netdev_reconcile_opts(config): + config.register_opts(_netdev_reconcile_opts, _OPT_GRP_NETDEV_RECONCILE) + + def get_session(group: str) -> ks_session.Session: auth = ks_loading.load_auth_from_conf_options(cfg.CONF, group) session = ks_loading.load_session_from_conf_options(cfg.CONF, group, auth=auth) diff --git a/python/neutron-understack/neutron_understack/ironic.py b/python/neutron-understack/neutron_understack/ironic.py index 485e31e57..0b285de7c 100644 --- a/python/neutron-understack/neutron_understack/ironic.py +++ b/python/neutron-understack/neutron_understack/ironic.py @@ -1,7 +1,9 @@ import importlib.metadata import logging +from dataclasses import dataclass from openstack import connection +from openstack import exceptions as sdk_exc from openstack.baremetal.baremetal_service import BaremetalService from openstack.baremetal.v1.node import Node as BaremetalNode from oslo_config import cfg @@ -12,21 +14,15 @@ # 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. +# and active -> (deleted/undeploy) -> available (triggering cleaning) on +# release. Failed adoption/cleaning is recovered to manageable and parked. _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_ADOPT_FAILED = "adopt failed" +_STATE_CLEAN_FAILED = "clean failed" _STATE_MANAGEABLE = "manageable" _STATE_AVAILABLE = "available" @@ -36,10 +32,22 @@ # 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" +# Ironic hardware type for Palo Alto appliances. The driver is the ownership +# boundary: ``netdev`` stays the generic type for any network device, so only +# nodes of this driver are ever selected or reconciled by the router flavor. +_PALOALTO_DRIVER = "paloalto" + +# Ironic clears instance_uuid during undeploy, including a failed one, so this +# is recorded outside the instance fields to survive it. +_ROUTER_RELEASE_MARKER = "understack_router_release" # release is in progress + + +@dataclass(frozen=True) +class NodeReleaseResult: + """Outcome of looking up and releasing a router's Ironic node.""" + + node: BaremetalNode | None + released: bool class IronicClient: @@ -79,40 +87,39 @@ def available_node_for_resource_class( ) -> BaremetalNode | None: """Return the first available Ironic node with the given resource class. - Ironic filters server-side by ``driver=netdev``, ``resource_class``, + Ironic filters server-side by ``driver=paloalto``, ``resource_class``, ``provision_state=available`` and not-in-maintenance, so any returned - node is a netdev appliance that is actually usable, in the interchangeable + node is an 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: + for node in self.irclient.nodes( + driver=_PALOALTO_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, + ): + # Already claimed, or reserved by a release still cleaning up. + if node.instance_id or _ROUTER_RELEASE_MARKER in (node.extra or {}): + continue LOG.info( - "No available netdev node found for resource_class=%s", + "Selected available Palo Alto node %s (name=%s) for resource_class=%s", + node.id, + node.name, resource_class, ) - return None + return node LOG.info( - "Selected available netdev node %s (name=%s) for resource_class=%s", - node.id, - node.name, + "No available Palo Alto node found for resource_class=%s", resource_class, ) - return node + return None def node_by_instance_uuid(self, instance_uuid: str) -> BaremetalNode | None: """Return the node currently adopted for the given instance UUID.""" @@ -121,6 +128,51 @@ def node_by_instance_uuid(self, instance_uuid: str) -> BaremetalNode | None: except StopIteration: return None + def paloalto_reconcile_nodes(self) -> list[BaremetalNode]: + """Find bound nodes, and releases whose association was cleared. + + Scoped by driver, so another ``netdev`` consumer's node is never a + candidate. A node whose stamp never landed is not discoverable here; + see the note in ``adopt_node_for_router``. + """ + return [ + node + for node in self.irclient.nodes( + driver=_PALOALTO_DRIVER, + is_maintenance=False, + details=True, + ) + if self._is_router_reconcile_candidate(node) + ] + + @staticmethod + def _is_router_reconcile_candidate(node: BaremetalNode) -> bool: + extra = node.extra or {} + if not isinstance(extra, dict): + return False + return bool(node.instance_id) or _ROUTER_RELEASE_MARKER in extra + + @staticmethod + def router_id_for_release(node: BaremetalNode) -> str | None: + """Resolve ownership without authorizing a conflicting release marker.""" + extra = node.extra or {} + if not isinstance(extra, dict): + return None + identities = [] + marker = extra.get(_ROUTER_RELEASE_MARKER) + if _ROUTER_RELEASE_MARKER in extra: + if not isinstance(marker, str) or not marker: + return None + identities.append(marker) + instance_id = node.instance_id + if instance_id: + if not isinstance(instance_id, str): + return None + identities.append(instance_id) + if not identities or len(set(identities)) != 1: + return None + return identities[0] + def attach_vif_to_node(self, node: str | BaremetalNode, vif_id: str) -> None: """Attach a Neutron port (VIF) to the node.""" node_id = node.id if isinstance(node, BaremetalNode) else node @@ -155,6 +207,10 @@ def adopt_node_for_router( 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. + + Writes the ownership marker the reconciler keys off before any state + change. Two creates racing for the same node is a known open window, + tracked separately. """ node_id = node.id if isinstance(node, BaremetalNode) else node LOG.info( @@ -166,6 +222,11 @@ def adopt_node_for_router( project_id, ) try: + # NOTE: if the stamp below fails and its rollback also fails, the + # node is left manageable with no identity -- not discoverable by + # reconciliation, and needing a manual ``provide``. Narrow (one + # call), and no router believes it owns the node. Accepted rather + # than reintroducing an ownership marker now the driver scopes us. # 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 @@ -196,26 +257,18 @@ def adopt_node_for_router( 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) + self._return_node_to_available(node_id, router_id) raise LOG.info( "Node %s adopted for router %s: provision_state=%s lessee=%s " @@ -228,99 +281,186 @@ def adopt_node_for_router( 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. + def _return_node_to_available( + self, + node: str | BaremetalNode, + router_id: str, + ) -> bool: + """Best-effort release, keeping the node discoverable until it is clean. + + Marks before transitioning, since undeploy clears the instance fields + even when it fails. Acts only on a node whose router identity still + matches, so a failed create cannot undo another one's work. A healthy + active release ends in ``available``; a failed transition parks in + ``manageable``. """ 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, - ) + current = self.irclient.get_node(node) + if current is None: + return True + if current.driver != _PALOALTO_DRIVER or current.is_maintenance: + return False + extra = current.extra or {} + if self.router_id_for_release(current) != router_id: + LOG.warning( + "Node %s no longer belongs to router %s", current.id, router_id + ) + return False + state = current.provision_state + targets = { + _STATE_ADOPT_FAILED: _PROVISION_MANAGE, + _STATE_CLEAN_FAILED: _PROVISION_MANAGE, + _STATE_ACTIVE: _PROVISION_UNDEPLOY, + # Ironic's failed undeploy enters error; deleted retries it. + "error": _PROVISION_UNDEPLOY, + } + # Parked in manageable: identity cleared, but kept out of the + # pool so a node that just failed a transition is not reused. + park_manageable = state in { + _STATE_MANAGEABLE, + _STATE_ADOPT_FAILED, + _STATE_CLEAN_FAILED, + } + if ( + state != _STATE_AVAILABLE + and state != _STATE_MANAGEABLE + and state not in targets + ): + LOG.warning( + "Node %s is in state %s; deferring release for router %s", + current.id, + state, + router_id, + ) + return False - 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, + if _ROUTER_RELEASE_MARKER not in extra: + marker_patch = ( + { + "op": "add", + "path": f"/extra/{_ROUTER_RELEASE_MARKER}", + "value": router_id, + } + if isinstance(current.extra, dict) + else { + "op": "add", + "path": "/extra", + "value": {_ROUTER_RELEASE_MARKER: router_id}, + } + ) + self.irclient.patch_node( + current, + [marker_patch], + retry_on_conflict=False, + ) + # Re-read: patches and provision transitions lock independently, + # and Ironic offers no compare-and-set to do this atomically. + current = self.irclient.get_node(current.id) + if not self._release_matches(current, router_id): + return False + if current.provision_state != state: + return False + + # One transition for most states; ``adopt failed`` and + # ``clean failed`` need manage first. Bounded so an unexpected + # state cannot spin an API worker. + for _step in range(2): + state = current.provision_state + if state == _STATE_AVAILABLE: + break + if state in {_STATE_ADOPT_FAILED, _STATE_CLEAN_FAILED}: + park_manageable = True + if state == _STATE_MANAGEABLE: + park_manageable = True + break + target = targets.get(state) + if target is None: + LOG.warning( + "Node %s moved to state %s while releasing router %s; " + "deferring", + current.id, + state, + router_id, + ) + return False + self.irclient.set_node_provision_state( + current, target, wait=True, timeout=_PROVISION_TIMEOUT + ) + current = self.irclient.get_node(current.id) + if not self._release_matches(current, router_id): + return False + final_state = _STATE_MANAGEABLE if park_manageable else _STATE_AVAILABLE + if current.provision_state != final_state or not self._release_matches( + current, router_id + ): + return False + # One patch, touching only our own keys. + cleanup_patch = [ + {"op": "add", "path": "/lessee", "value": None}, + {"op": "add", "path": "/instance_uuid", "value": None}, + {"op": "add", "path": "/instance_name", "value": None}, + ] + cleanup_patch.append( + {"op": "remove", "path": f"/extra/{_ROUTER_RELEASE_MARKER}"} + ) + self.irclient.patch_node( + current, + cleanup_patch, 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 + current = self.irclient.get_node(current.id) + return ( + current.provision_state == final_state + and not current.instance_id + and not current.instance_name + and not current.lessee + and _ROUTER_RELEASE_MARKER not in (current.extra or {}) ) - LOG.info("Node %s reached available via %s", node_id, target) + except sdk_exc.NotFoundException: + # A removed Ironic node has nothing left to release. + return True except Exception: LOG.exception( - "Failed to return node %s to available via %s; manual cleanup " - "may be required", - node_id, - target, + "Failed to release node %s for router %s; will retry", node, router_id ) + return False - def release_node_for_router(self, router_id: str) -> BaremetalNode | None: - """Return the router's node to the available pool, whatever its state. + def _release_matches(self, node: BaremetalNode | None, router_id: str) -> bool: + if node is None: + return False + extra = node.extra or {} + if not isinstance(extra, dict): + return False + return ( + node.driver == _PALOALTO_DRIVER + and not node.is_maintenance + and self.router_id_for_release(node) == router_id + and extra.get(_ROUTER_RELEASE_MARKER) == router_id + ) + + def release_orphan_node(self, node_id: str, router_id: str) -> bool: + """Release the exact confirmed orphan, returning cleanup completion.""" + return self._return_node_to_available(node_id, router_id) + + def release_node_for_router(self, router_id: str) -> NodeReleaseResult: + """Release the router's node and clear its ownership, 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. + cleaning); failed or already manageable nodes are parked in + ``manageable`` after ownership cleanup. The result distinguishes no + matching node from a release that was found but remains incomplete and + will be retried by reconciliation. """ node = self.node_by_instance_uuid(router_id) if node is None: - return None + return NodeReleaseResult(node=None, released=False) 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 + return NodeReleaseResult( + node=node, + released=self._return_node_to_available(node, router_id), + ) 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 d3f552f13..6e8046898 100644 --- a/python/neutron-understack/neutron_understack/l3_router/palo_alto.py +++ b/python/neutron-understack/neutron_understack/l3_router/palo_alto.py @@ -799,18 +799,26 @@ def _process_router_delete(self, resource, event, trigger, payload=None): if not self._is_palo_alto_provider(context, router): return - node = self._ironic.release_node_for_router(router["id"]) - if node is None: + result = self._ironic.release_node_for_router(router["id"]) + if result.node is None: LOG.warning( "Palo Alto router %s deleted but no adopted Ironic node was " "found to release", router["id"], ) return + if not result.released: + LOG.warning( + "Release of Ironic node %s from deleted Palo Alto router %s " + "is incomplete; reconciliation will retry it", + result.node.id, + router["id"], + ) + return LOG.info( - "Released Ironic node %s from deleted Palo Alto router %s " - "(active -> available, cleaning triggered)", - node.id, + "Released Ironic node %s from deleted Palo Alto router %s; " + "ownership cleanup completed", + result.node.id, router["id"], ) diff --git a/python/neutron-understack/neutron_understack/l3_router/vrf.py b/python/neutron-understack/neutron_understack/l3_router/vrf.py index 9cbd4c5d6..9821a7070 100644 --- a/python/neutron-understack/neutron_understack/l3_router/vrf.py +++ b/python/neutron-understack/neutron_understack/l3_router/vrf.py @@ -131,7 +131,7 @@ def get_plugin_description(self): return "Understack router VNI allocation plugin" def ovn_maintenance_periodics(self, ovn_client): - LOG.warning("NETDEV ovn_maintenance_periodics called") + LOG.info("Registering netdev-router maintenance periodics") return [ understack_maintenance.NetdevRouterMaintenancePeriodics(self, ovn_client) ] diff --git a/python/neutron-understack/neutron_understack/maintenance.py b/python/neutron-understack/neutron_understack/maintenance.py index 5642bf397..facabda3d 100644 --- a/python/neutron-understack/neutron_understack/maintenance.py +++ b/python/neutron-understack/neutron_understack/maintenance.py @@ -1,30 +1,48 @@ """Periodics run in the OVN maintenance worker. -Proves the mechanism fires end to end (hook discovered, worker runs, once -cluster-wide) before the real reconciliation logic is added. +Neutron and Ironic share no transaction, so an adopt can outlive a failed +router-row insert and a release can fail after the row is gone. Either way a +node is left stamped with a router that does not exist. No synchronous callback +closes that window, so this periodic sweeps it up. + +Node-only by design; stale gateway/subnet trunk wiring is a separate follow-up. """ # NOTE: Re-verify these symbols still exist on every neutron upgrade has_lock_periodic # and MAINTENANCE_NB_IDL_LOCK_NAME coz they are neutron-internal module. +from neutron.objects import router as l3_obj from neutron.plugins.ml2.drivers.ovn.mech_driver.ovsdb import maintenance +from neutron_lib import context as n_context +from oslo_config import cfg from oslo_log import log as logging +from neutron_understack import config +from neutron_understack.ironic import IronicClient + LOG = logging.getLogger(__name__) -RECONCILE_SPACING = 600 # Temp +RECONCILE_SPACING = 600 + +# Not configurable: has_lock_periodic reads ``spacing`` at import time, before +# our opts are registered. The runtime flags are read in the method body. class NetdevRouterMaintenancePeriodics: """Reconcile netdev-router Ironic state from the OVN maintenance worker. - Body is a no-op log line. + Releases netdev nodes whose ``instance_uuid`` names a router that no longer + exists in Neutron, returning them to the available pool. """ def __init__(self, plugin, ovn_client): self._plugin = plugin + config.register_netdev_reconcile_opts(cfg.CONF) + # (node_id, router_id) pairs seen orphaned last pass. See + # _reconcile_once for why both halves of the key matter. + self._orphans_seen: set[tuple[str, str]] = set() # Take the maintenance lock so exactly one neutron-server runs these # periodics. - LOG.warning( - "NETDEV periodic __init__; _nb_idl=%r", + LOG.debug( + "netdev-router periodics starting; _nb_idl=%r", getattr(ovn_client, "_nb_idl", "MISSING"), ) self._idl = ovn_client._nb_idl.idl @@ -34,8 +52,117 @@ def __init__(self, plugin, ovn_client): def has_lock(self): return self._idl.has_lock + @property + def _ironic(self) -> IronicClient: + # Lazy: missing Ironic credentials fail a pass, not worker startup. + try: + return self._ironic_ref + except AttributeError: + self._ironic_ref = IronicClient() + return self._ironic_ref + @maintenance.has_lock_periodic(spacing=RECONCILE_SPACING, run_immediately=False) def reconcile_netdev_routers(self): - # No-op: proves the periodic is scheduled and fires on exactly one - # neutron-server. host= lets us confirm it is not firing per-worker. - LOG.debug("netdev-router reconcile: placeholder, no action yet") + if not cfg.CONF.netdev_router_reconcile.enabled: + self._orphans_seen.clear() + LOG.debug("netdev-router reconcile is disabled; skipping") + return + try: + self._reconcile_once() + except Exception: + # A failed scan cannot confirm consecutive orphan observations. + self._orphans_seen.clear() + LOG.exception("netdev-router reconcile pass failed") + + def _reconcile_once(self): + """Release netdev nodes bound to routers that no longer exist. + + Adoption stamps the node before the router row is inserted, so a + router being created right now is indistinguishable from an orphan. + Two consecutive sightings put a full interval between the two, with no + dependence on Ironic and neutron-server agreeing on the clock. + + Keyed on (node, router): a node re-adopted by a different router must + not inherit the old sighting. + """ + dry_run = cfg.CONF.netdev_router_reconcile.dry_run + context = n_context.get_admin_context() + + orphans_now = set() + for node in self._ironic.paloalto_reconcile_nodes(): + router_id = IronicClient.router_id_for_release(node) + if not router_id: + # A pending release marker survives Ironic clearing instance_uuid. + # Unmarked nodes and conflicting ownership are not ours to release. + continue + if l3_obj.Router.objects_exist(context, id=router_id): + continue + orphans_now.add((node.id, router_id)) + + confirmed = orphans_now & self._orphans_seen + # Only carry forward what is still orphaned, so a node that got a live + # router back does not stay armed for release. + self._orphans_seen = orphans_now + + if not orphans_now: + LOG.debug("netdev-router reconcile: no orphaned nodes") + return + + LOG.info( + "netdev-router reconcile: %d orphaned node(s), %d confirmed by a " + "previous pass%s", + len(orphans_now), + len(confirmed), + " (dry run)" if dry_run else "", + ) + + for node_id, router_id in sorted(orphans_now - confirmed): + LOG.info( + "Node %s is stamped with router %s which does not exist; " + "deferring release until the next pass confirms it", + node_id, + router_id, + ) + + for node_id, router_id in sorted(confirmed): + if not self.has_lock: + self._orphans_seen.clear() + return + if dry_run: + LOG.info( + "netdev-router reconcile (dry run): would release node %s " + "for nonexistent router %s", + node_id, + router_id, + ) + continue + self._release_orphan(node_id, router_id) + + def _release_orphan(self, node_id: str, router_id: str) -> None: + """Return one orphaned node to the available pool. + + Recheck desired state after the scan: earlier nodes may have taken time + to release. Ironic then rereads this exact node and verifies ownership + and maintenance status before acting. Its durable release marker keeps + partial cleanup discoverable across passes and worker restarts. + """ + try: + if l3_obj.Router.objects_exist(n_context.get_admin_context(), id=router_id): + self._orphans_seen.discard((node_id, router_id)) + return + LOG.info( + "Releasing orphaned netdev node %s for nonexistent router %s", + node_id, + router_id, + ) + complete = self._ironic.release_orphan_node(node_id, router_id) + except Exception: + LOG.exception( + "Failed to release orphaned netdev node %s (router %s); will " + "retry on the next pass", + node_id, + router_id, + ) + return + if complete: + self._orphans_seen.discard((node_id, router_id)) diff --git a/python/neutron-understack/neutron_understack/tests/scenarios/fakes.py b/python/neutron-understack/neutron_understack/tests/scenarios/fakes.py index 7db5a568e..608401e4f 100644 --- a/python/neutron-understack/neutron_understack/tests/scenarios/fakes.py +++ b/python/neutron-understack/neutron_understack/tests/scenarios/fakes.py @@ -9,6 +9,8 @@ import contextlib from unittest import mock +from neutron_understack.ironic import NodeReleaseResult + class FakeNbIdl: """Minimal OVN Northbound IDL: records localnet LSP create/delete.""" @@ -84,4 +86,4 @@ def release_node_for_router(self, router_id): if node is not None: self._available = True self.released.append(router_id) - return node + return NodeReleaseResult(node=node, released=node is not None) diff --git a/python/neutron-understack/neutron_understack/tests/test_ironic.py b/python/neutron-understack/neutron_understack/tests/test_ironic.py index 81756099c..a5d2ebf46 100644 --- a/python/neutron-understack/neutron_understack/tests/test_ironic.py +++ b/python/neutron-understack/neutron_understack/tests/test_ironic.py @@ -5,8 +5,34 @@ """ import pytest +from openstack.baremetal.v1.node import Node from neutron_understack.ironic import IronicClient +from neutron_understack.ironic import NodeReleaseResult + +_DEFAULT_EXTRA = object() + + +def _node( + node_id="n1", + *, + state="active", + router_id="router-1", + extra=_DEFAULT_EXTRA, + lessee="project-1", + instance_name="router-1", + maintenance=False, +): + return Node( + id=node_id, + driver="paloalto", + is_maintenance=maintenance, + provision_state=state, + instance_id=router_id, + instance_name=instance_name, + lessee=lessee, + extra={} if extra is _DEFAULT_EXTRA else extra, + ) def _client(mocker): @@ -15,41 +41,190 @@ def _client(mocker): return client +class _FakeBaremetal: + def __init__( + self, + node, + *, + fail_undeploy_once=False, + fail_cleanup_once=False, + fail_manage_once=False, + ): + self.node = node + self.fail_undeploy_once = fail_undeploy_once + self.fail_cleanup_once = fail_cleanup_once + self.fail_manage_once = fail_manage_once + + def _copy(self): + return Node(**self.node.to_dict()) + + def get_node(self, node): + node_id = node.id if isinstance(node, Node) else node + if node_id != self.node.id: + return None + return self._copy() + + def nodes(self, **filters): + node = self._copy() + for key, value in filters.items(): + if key == "details": + continue + if key == "associated": + if bool(node.instance_id) is not value: + return iter([]) + continue + if getattr(node, key) != value: + return iter([]) + return iter([node]) + + def patch_node(self, node, patch, retry_on_conflict=False): + if self.fail_cleanup_once and any( + op["path"] == "/extra/understack_router_release" and op["op"] == "remove" + for op in patch + ): + self.fail_cleanup_once = False + raise RuntimeError("cleanup failed") + for op in patch: + path = op["path"] + if path == "/instance_uuid": + self.node.instance_id = op["value"] + elif path == "/instance_name": + self.node.instance_name = op["value"] + elif path == "/lessee": + self.node.lessee = op["value"] + elif path.startswith("/extra/"): + key = path.removeprefix("/extra/") + extra = dict(self.node.extra or {}) + if op["op"] == "remove": + extra.pop(key, None) + else: + extra[key] = op["value"] + self.node.extra = extra + + def set_node_provision_state(self, node, target, wait=True, timeout=None): + if target == "manage": + self.node.provision_state = "manageable" + if self.fail_manage_once: + self.fail_manage_once = False + raise RuntimeError("manage response lost") + elif target == "deleted": + self.node.instance_id = None + self.node.instance_name = None + if self.fail_undeploy_once: + self.fail_undeploy_once = False + self.node.provision_state = "error" + raise RuntimeError("undeploy failed") + self.node.provision_state = "available" + elif target == "provide": + self.node.provision_state = "available" + elif target == "adopt": + self.node.provision_state = "active" + return self._copy() + + def update_node(self, node, retry_on_conflict=False, **fields): + for name, value in fields.items(): + setattr(self.node, name, value) + return self._copy() + + def wait_for_nodes_provision_state(self, nodes, state, timeout=None): + self.node.provision_state = state + return [self._copy()] + + +def _release_sequence( + client, + *, + initial_state="active", + transition_target="available", + router_id="router-1", +): + marker = {"understack_router_release": router_id} + states = [ + _node(state=initial_state, router_id=router_id), + _node(state=initial_state, router_id=router_id, extra=marker.copy()), + ] + if initial_state != "available": + states.append( + _node(state=transition_target, router_id=None, extra=marker.copy()) + ) + states.append( + _node( + state="available", + router_id=None, + instance_name=None, + lessee=None, + extra={}, + ) + ) + client.irclient.get_node.side_effect = states + + 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, + # A manage failure must route through _return_node_to_available and + # re-raise so the create aborts. The recovery leaves a manageable node + # parked after clearing ownership. + client = _client(mocker) + node = _node(state="available", router_id=None) + owner = {"understack_router_id": "r"} + marker = { + "understack_router_id": "r", + "understack_router_release": "r", + } + client.irclient.get_node.side_effect = [ + # rollback: read state, write release marker, re-read, confirm + _node( + state="manageable", + router_id="r", + lessee="p", + instance_name="n", + extra=owner.copy(), + ), + _node( + state="manageable", + router_id="r", + lessee="p", + instance_name="n", + extra=marker.copy(), + ), + _node( + state="manageable", + router_id=None, + instance_name=None, + lessee=None, + extra={}, + ), ] - client.irclient.get_node.return_value = node + client.irclient.set_node_provision_state.side_effect = RuntimeError( + "manage boom" + ) 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 + # the failed manage is the only provision call; recovery parks the node + assert client.irclient.set_node_provision_state.call_count == 1 class TestVifAttach: def test_attach_calls_proxy(self, mocker): client = _client(mocker) - node = mocker.Mock(id="n1") + node = _node(state="available", router_id=None) client.attach_vif_to_node(node, "port-1") client.irclient.attach_vif_to_node.assert_called_once_with(node, "port-1") + def test_attach_works_on_an_adopted_active_node(self, mocker): + client = _client(mocker) + adopted = _node(state="active", router_id="router-1") + + client.attach_vif_to_node(adopted, "parent-1") + + client.irclient.attach_vif_to_node.assert_called_once_with(adopted, "parent-1") + def test_detach_uses_ignore_missing_and_returns_result(self, mocker): client = _client(mocker) node = mocker.Mock(id="n1") @@ -73,64 +248,200 @@ def test_node_vif_ids(self, mocker): 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 + node = _node(state="active") + _release_sequence(client, initial_state="active") - client._return_node_to_available(node) + assert client._return_node_to_available(node, "router-1") is True # 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 + marker_patch, cleanup_patch = ( + call.args[1] for call in client.irclient.patch_node.call_args_list + ) + assert marker_patch == [ + { + "op": "add", + "path": "/extra/understack_router_release", + "value": "router-1", + } + ] + assert cleanup_patch == [ + {"op": "add", "path": "/lessee", "value": None}, + {"op": "add", "path": "/instance_uuid", "value": None}, + {"op": "add", "path": "/instance_name", "value": None}, + {"op": "remove", "path": "/extra/understack_router_release"}, + ] - def test_manageable_node_is_cleared_then_provided(self, mocker): + def test_manageable_node_is_marked_then_parked(self, mocker): client = _client(mocker) - node = mocker.Mock(id="n1", provision_state="manageable") - client.irclient.get_node.return_value = node + node = _node(state="manageable") + marker = {"understack_router_release": "router-1"} + client.irclient.get_node.side_effect = [ + node, + _node(state="manageable", extra=marker.copy()), + _node( + state="manageable", + router_id=None, + instance_name=None, + lessee=None, + extra={}, + ), + ] - client._return_node_to_available(node) + assert client._return_node_to_available(node, "router-1") is True - client.irclient.update_node.assert_called_once() - (_, target), _ = client.irclient.set_node_provision_state.call_args - assert target == "provide" + client.irclient.set_node_provision_state.assert_not_called() 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 + node = _node(state="available") + _release_sequence(client, initial_state="available") - client._return_node_to_available(node) + assert client._return_node_to_available(node, "router-1") is True - 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): + def test_adopt_failed_is_managed_then_parked(self, mocker): client = _client(mocker) - node = mocker.Mock(id="n1", provision_state="adopt failed") + node = _node(state="adopt failed") + marker = {"understack_router_release": "router-1"} + client.irclient.get_node.side_effect = [ + node, + _node(state="adopt failed", extra=marker.copy()), + _node(state="manageable", extra=marker.copy()), + _node( + state="manageable", + router_id=None, + instance_name=None, + lessee=None, + extra={}, + ), + ] + + assert client._return_node_to_available(node, "router-1") is True + + targets = [ + call.args[1] + for call in client.irclient.set_node_provision_state.call_args_list + ] + assert targets == ["manage"] + + def test_clean_failed_is_managed_then_parked(self, mocker): + client = _client(mocker) + node = _node(state="clean failed") + marker = {"understack_router_release": "router-1"} + client.irclient.get_node.side_effect = [ + node, + _node(state="clean failed", extra=marker.copy()), + _node(state="manageable", extra=marker.copy()), + _node( + state="manageable", + router_id=None, + instance_name=None, + lessee=None, + extra={}, + ), + ] + + assert client._return_node_to_available(node, "router-1") is True + + targets = [ + call.args[1] + for call in client.irclient.set_node_provision_state.call_args_list + ] + assert targets == ["manage"] + + def test_transient_state_is_left_for_reconciliation(self, mocker): + client = _client(mocker) + node = _node(state="adopting") client.irclient.get_node.return_value = node - client._return_node_to_available(node) + assert client._return_node_to_available(node, "router-1") is False - client.irclient.update_node.assert_not_called() + client.irclient.patch_node.assert_not_called() client.irclient.set_node_provision_state.assert_not_called() + def test_failed_undeploy_keeps_marker_for_retry(self, mocker): + client = _client(mocker) + node = _node(state="active") + marker = {"understack_router_release": "router-1"} + client.irclient.get_node.side_effect = [ + _node(state="active"), + _node(state="active", extra=marker.copy()), + ] + client.irclient.set_node_provision_state.side_effect = RuntimeError("boom") + + assert client._return_node_to_available(node, "router-1") is False + + client.irclient.patch_node.assert_called_once() + + def test_available_node_with_marker_resumes_cleanup(self, mocker): + client = _client(mocker) + marker = {"understack_router_release": "router-1"} + node = _node(state="available", router_id=None, extra=marker.copy()) + client.irclient.get_node.side_effect = [ + node, + node, + _node( + state="available", + router_id=None, + instance_name=None, + lessee=None, + extra={}, + ), + ] + + assert client._return_node_to_available(node, "router-1") is True + + client.irclient.set_node_provision_state.assert_not_called() + cleanup_patch = client.irclient.patch_node.call_args.args[1] + assert cleanup_patch[-1] == { + "op": "remove", + "path": "/extra/understack_router_release", + } + + def test_null_extra_is_replaced_when_release_marker_is_added(self, mocker): + client = _client(mocker) + node = _node(state="active", extra=None) + marker = {"understack_router_release": "router-1"} + client.irclient.get_node.side_effect = [ + node, + _node(state="active", extra=marker.copy()), + _node(state="available", router_id=None, extra=marker.copy()), + _node( + state="available", + router_id=None, + instance_name=None, + lessee=None, + extra={}, + ), + ] + + assert client._return_node_to_available(node, "router-1") is True + + marker_patch = client.irclient.patch_node.call_args_list[0].args[1] + assert marker_patch == [ + { + "op": "add", + "path": "/extra", + "value": {"understack_router_release": "router-1"}, + } + ] + class TestNodeSelection: - def test_filters_available_non_maintenance_netdev(self, mocker): + def test_filters_available_non_maintenance_paloalto(self, mocker): client = _client(mocker) - node = mocker.Mock(id="n1") + node = _node(state="available", router_id=None) 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["driver"] == "paloalto" assert kwargs["resource_class"] == "pa1410" assert kwargs["provision_state"] == "available" # a node parked in maintenance must never be selected @@ -142,26 +453,226 @@ def test_returns_none_when_pool_empty(self, mocker): assert client.available_node_for_resource_class("pa1410") is None + def test_skips_nodes_reserved_by_pending_release_marker(self, mocker): + client = _client(mocker) + client.irclient.nodes.return_value = iter( + [ + _node( + state="available", + router_id=None, + extra={"understack_router_release": "router-1"}, + ), + _node(node_id="n2", state="available", router_id=None), + ] + ) + + assert client.available_node_for_resource_class("pa1410").id == "n2" + + +class TestReconcileDiscovery: + def test_scan_is_scoped_to_the_paloalto_driver(self, mocker): + # netdev stays the generic type; only paloalto nodes are ours, and + # Ironic does that filtering server-side. + client = _client(mocker) + bound = _node() + client.irclient.nodes.return_value = iter([bound]) + + assert client.paloalto_reconcile_nodes() == [bound] + _, kwargs = client.irclient.nodes.call_args + assert kwargs["driver"] == "paloalto" + assert kwargs["is_maintenance"] is False + + def test_release_marker_alone_is_enough_to_be_a_candidate(self, mocker): + # Ironic clears instance_uuid on a failed undeploy; the release marker + # is what keeps a half-released node discoverable. + client = _client(mocker) + half_released = _node( + router_id=None, extra={"understack_router_release": "router-1"} + ) + client.irclient.nodes.return_value = iter([half_released]) + + assert client.paloalto_reconcile_nodes() == [half_released] + + def test_unbound_unmarked_node_is_not_a_candidate(self, mocker): + client = _client(mocker) + free = _node(state="available", router_id=None, lessee=None, instance_name=None) + client.irclient.nodes.return_value = iter([free]) + + assert client.paloalto_reconcile_nodes() == [] + + +class TestReconcileReleaseRetry: + def test_failed_undeploy_remains_discoverable_after_ironic_clears_instance(self): + backend = _FakeBaremetal(_node(state="active"), fail_undeploy_once=True) + client = IronicClient.__new__(IronicClient) + client.irclient = backend + + assert client.release_orphan_node("n1", "router-1") is False + assert backend.node.provision_state == "error" + assert backend.node.instance_id is None + assert backend.node.extra == {"understack_router_release": "router-1"} + + restarted_client = IronicClient.__new__(IronicClient) + restarted_client.irclient = backend + candidates = restarted_client.paloalto_reconcile_nodes() + + assert [node.id for node in candidates] == ["n1"] + assert IronicClient.router_id_for_release(candidates[0]) == "router-1" + assert restarted_client.release_orphan_node("n1", "router-1") is True + assert backend.node.provision_state == "available" + assert backend.node.instance_id is None + assert backend.node.lessee is None + assert backend.node.extra == {} + + def test_failed_final_cleanup_keeps_marker_for_next_pass(self): + backend = _FakeBaremetal( + _node( + state="available", + router_id=None, + extra={"understack_router_release": "router-1"}, + ), + fail_cleanup_once=True, + ) + client = IronicClient.__new__(IronicClient) + client.irclient = backend + + assert client.release_orphan_node("n1", "router-1") is False + assert backend.node.extra == {"understack_router_release": "router-1"} + + restarted_client = IronicClient.__new__(IronicClient) + restarted_client.irclient = backend + assert restarted_client.release_orphan_node("n1", "router-1") is True + assert backend.node.extra == {} + + def test_manage_timeout_from_adopt_failed_resumes_with_cleanup(self): + backend = _FakeBaremetal( + _node(state="adopt failed"), + fail_manage_once=True, + ) + client = IronicClient.__new__(IronicClient) + client.irclient = backend + + assert client.release_orphan_node("n1", "router-1") is False + assert backend.node.provision_state == "manageable" + assert backend.node.extra == {"understack_router_release": "router-1"} + + restarted_client = IronicClient.__new__(IronicClient) + restarted_client.irclient = backend + assert restarted_client.release_orphan_node("n1", "router-1") is True + assert backend.node.provision_state == "manageable" + assert backend.node.instance_id is None + assert backend.node.instance_name is None + assert backend.node.lessee is None + assert backend.node.extra == {} + assert restarted_client.paloalto_reconcile_nodes() == [] + 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 + assert client.release_node_for_router("router-1") == NodeReleaseResult( + node=None, released=False + ) def test_releases_bound_node(self, mocker): client = _client(mocker) - node = mocker.Mock(id="n1", provision_state="active") + node = _node(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 + _release_sequence(client, initial_state="active") result = client.release_node_for_router("router-1") - assert result is node + assert result.node is node + assert result.released is True (_, target), _ = client.irclient.set_node_provision_state.call_args assert target == "deleted" - _, kwargs = client.irclient.update_node.call_args - assert kwargs["lessee"] is None + + def test_reports_when_bound_node_release_is_incomplete(self, mocker): + client = _client(mocker) + node = _node(state="adopting") + client.irclient.nodes.return_value = iter([node]) + client.irclient.get_node.return_value = node + + result = client.release_node_for_router("router-1") + + assert result.node is node + assert result.released is False + + +class TestOwnershipIsDiscoverableBeforeAnyChange: + """The marker must land before the node is touched. + + If any step of adoption fails AND the rollback also fails, the marker is + the only thing that lets reconciliation find the node again. Stamping + first and marking second leaves a node carrying a router's instance_uuid + that nothing can discover -- stranded exactly the way this mechanism + exists to prevent. + """ + + +class TestLifecycleAgainstStatefulIronic: + """Whole flows against the stateful fake, not call-order assertions.""" + + def _client_for(self, node, **kw): + client = IronicClient.__new__(IronicClient) + client.irclient = _FakeBaremetal(node, **kw) + return client + + def test_create_then_delete_returns_a_clean_node_to_the_pool(self): + client = self._client_for( + _node(state="available", router_id=None, lessee=None, instance_name=None) + ) + client.adopt_node_for_router( + client.irclient.node, + project_id="p", + router_id="r-77", + router_name="rtr", + ) + assert client.irclient.node.instance_id == "r-77" + + result = client.release_node_for_router("r-77") + + assert result.released is True + node = client.irclient.node + assert node.provision_state == "available" + assert not node.instance_id + assert not node.lessee + assert not node.instance_name + assert node.extra == {} + + def test_orphan_is_found_and_released_by_the_reconciler_path(self): + client = self._client_for( + _node(state="available", router_id=None, lessee=None, instance_name=None) + ) + client.adopt_node_for_router( + client.irclient.node, + project_id="p", + router_id="r-gone", + router_name="rtr", + ) + + candidates = client.paloalto_reconcile_nodes() + assert [n.id for n in candidates] == ["n1"] + assert IronicClient.router_id_for_release(candidates[0]) == "r-gone" + + assert client.release_orphan_node("n1", "r-gone") is True + assert client.irclient.node.provision_state == "available" + assert client.irclient.node.extra == {} + + def test_a_maintenance_node_is_never_touched(self): + client = self._client_for( + _node( + state="active", + router_id="r-gone", + maintenance=True, + extra={"understack_router_id": "r-gone"}, + ) + ) + + assert client.paloalto_reconcile_nodes() == [] + assert client.release_orphan_node("n1", "r-gone") is False + assert client.irclient.node.instance_id == "r-gone" diff --git a/python/neutron-understack/neutron_understack/tests/test_maintenance.py b/python/neutron-understack/neutron_understack/tests/test_maintenance.py new file mode 100644 index 000000000..99760afa4 --- /dev/null +++ b/python/neutron-understack/neutron_understack/tests/test_maintenance.py @@ -0,0 +1,281 @@ +"""Unit tests for the netdev-router reconciler periodic. + +NetdevRouterMaintenancePeriodics.__init__ wires itself into the OVN NB idl to +take the maintenance lock, so these bypass it with __new__ and inject the small +amount of state the reconcile path actually reads. +""" + +import pytest +from openstack.baremetal.v1.node import Node +from oslo_config import cfg +from oslo_config import fixture as config_fixture + +from neutron_understack import config as understack_config +from neutron_understack.maintenance import NetdevRouterMaintenancePeriodics + + +@pytest.fixture +def reconcile_conf(): + conf = config_fixture.Config(cfg.CONF) + conf.setUp() + understack_config.register_netdev_reconcile_opts(cfg.CONF) + yield conf + conf.cleanUp() + + +def _node(mocker, node_id, router_id, extra=None): + return Node(id=node_id, instance_id=router_id, extra={} if extra is None else extra) + + +def _periodics(mocker, nodes=(), live_router_ids=()): + """Build a reconciler with Ironic and the Neutron router table mocked.""" + obj = NetdevRouterMaintenancePeriodics.__new__(NetdevRouterMaintenancePeriodics) + obj._orphans_seen = set() + obj._idl = mocker.Mock(has_lock=True) + obj._ironic_ref = mocker.Mock() + obj._ironic_ref.paloalto_reconcile_nodes.return_value = list(nodes) + obj._ironic_ref.release_orphan_node.return_value = True + + mocker.patch("neutron_understack.maintenance.n_context.get_admin_context") + live = set(live_router_ids) + mocker.patch( + "neutron_understack.maintenance.l3_obj.Router.objects_exist", + side_effect=lambda context, id: id in live, + ) + return obj + + +def _released_router_ids(obj): + return [call.args[1] for call in obj._ironic_ref.release_orphan_node.call_args_list] + + +class TestOrphanConfirmation: + def test_first_sighting_only_defers(self, mocker, reconcile_conf): + # Adoption stamps instance_uuid before the router row exists, so a + # router being created right now looks exactly like an orphan. One + # sighting must never be enough to release. + obj = _periodics(mocker, nodes=[_node(mocker, "n1", "r1")]) + + obj._reconcile_once() + + obj._ironic_ref.release_orphan_node.assert_not_called() + assert obj._orphans_seen == {("n1", "r1")} + + def test_second_sighting_releases(self, mocker, reconcile_conf): + obj = _periodics(mocker, nodes=[_node(mocker, "n1", "r1")]) + + obj._reconcile_once() + obj._reconcile_once() + + assert _released_router_ids(obj) == ["r1"] + + def test_live_router_node_is_never_touched(self, mocker, reconcile_conf): + obj = _periodics( + mocker, nodes=[_node(mocker, "n1", "r1")], live_router_ids=["r1"] + ) + + obj._reconcile_once() + obj._reconcile_once() + + obj._ironic_ref.release_orphan_node.assert_not_called() + assert obj._orphans_seen == set() + + def test_router_appearing_between_passes_disarms_the_node( + self, mocker, reconcile_conf + ): + # The in-flight create completes between passes: the sighting from the + # first pass must not survive to authorize a release. + obj = _periodics(mocker, nodes=[_node(mocker, "n1", "r1")]) + obj._reconcile_once() + + mocker.patch( + "neutron_understack.maintenance.l3_obj.Router.objects_exist", + return_value=True, + ) + obj._reconcile_once() + + obj._ironic_ref.release_orphan_node.assert_not_called() + assert obj._orphans_seen == set() + + def test_readopted_node_must_be_reconfirmed(self, mocker, reconcile_conf): + # n1 looked orphaned holding r1; by the next pass it has been freed and + # re-adopted by r2, whose router row is still being inserted. Keying the + # sighting on the node alone would release r2's in-flight adoption. + obj = _periodics(mocker, nodes=[_node(mocker, "n1", "r1")]) + obj._reconcile_once() + + obj._ironic_ref.paloalto_reconcile_nodes.return_value = [ + _node(mocker, "n1", "r2") + ] + obj._reconcile_once() + + obj._ironic_ref.release_orphan_node.assert_not_called() + assert obj._orphans_seen == {("n1", "r2")} + + def test_node_without_a_stamp_is_ignored(self, mocker, reconcile_conf): + obj = _periodics(mocker, nodes=[_node(mocker, "n1", None)]) + + obj._reconcile_once() + obj._reconcile_once() + + obj._ironic_ref.release_orphan_node.assert_not_called() + assert obj._orphans_seen == set() + + def test_marker_only_node_is_reconciled(self, mocker, reconcile_conf): + obj = _periodics( + mocker, + nodes=[ + _node( + mocker, + "n1", + None, + extra={"understack_router_release": "r1"}, + ) + ], + ) + + obj._reconcile_once() + obj._reconcile_once() + + obj._ironic_ref.release_orphan_node.assert_called_once_with("n1", "r1") + + def test_conflicting_marker_is_ignored(self, mocker, reconcile_conf): + obj = _periodics( + mocker, + nodes=[ + _node( + mocker, + "n1", + "r2", + extra={"understack_router_release": "r1"}, + ) + ], + ) + + obj._reconcile_once() + obj._reconcile_once() + + obj._ironic_ref.release_orphan_node.assert_not_called() + assert obj._orphans_seen == set() + + def test_only_the_orphans_are_released(self, mocker, reconcile_conf): + obj = _periodics( + mocker, + nodes=[ + _node(mocker, "n1", "live"), + _node(mocker, "n2", "gone"), + _node(mocker, "n3", "also-gone"), + ], + live_router_ids=["live"], + ) + + obj._reconcile_once() + obj._reconcile_once() + + assert sorted(_released_router_ids(obj)) == ["also-gone", "gone"] + + +class TestReleaseFailureHandling: + def test_router_reappearing_before_release_disarms_candidate( + self, mocker, reconcile_conf + ): + obj = _periodics(mocker, nodes=[_node(mocker, "n1", "r1")]) + exists = mocker.patch( + "neutron_understack.maintenance.l3_obj.Router.objects_exist" + ) + exists.side_effect = [False, False, True] + + obj._reconcile_once() + obj._reconcile_once() + + obj._ironic_ref.release_orphan_node.assert_not_called() + assert obj._orphans_seen == set() + + def test_one_failing_node_does_not_stop_the_sweep(self, mocker, reconcile_conf): + obj = _periodics( + mocker, + nodes=[_node(mocker, "n1", "r1"), _node(mocker, "n2", "r2")], + ) + obj._ironic_ref.release_orphan_node.side_effect = [ + RuntimeError("ironic down"), + True, + ] + + obj._reconcile_once() + obj._reconcile_once() + + assert _released_router_ids(obj) == ["r1", "r2"] + + def test_failed_release_stays_a_candidate(self, mocker, reconcile_conf): + obj = _periodics(mocker, nodes=[_node(mocker, "n1", "r1")]) + obj._ironic_ref.release_orphan_node.side_effect = RuntimeError("boom") + + obj._reconcile_once() + obj._reconcile_once() + + # Still orphaned and still armed, so the next pass retries. + assert obj._orphans_seen == {("n1", "r1")} + + def test_incomplete_release_stays_a_candidate(self, mocker, reconcile_conf): + obj = _periodics(mocker, nodes=[_node(mocker, "n1", "r1")]) + obj._ironic_ref.release_orphan_node.return_value = False + obj._ironic_ref.release_orphan_node.side_effect = None + + obj._reconcile_once() + obj._reconcile_once() + + assert _released_router_ids(obj) == ["r1"] + assert obj._orphans_seen == {("n1", "r1")} + obj._reconcile_once() + assert _released_router_ids(obj) == ["r1", "r1"] + + +class TestConfigGates: + def test_dry_run_reports_without_releasing(self, mocker, reconcile_conf): + reconcile_conf.config(group="netdev_router_reconcile", dry_run=True) + obj = _periodics(mocker, nodes=[_node(mocker, "n1", "r1")]) + + obj._reconcile_once() + obj._reconcile_once() + + obj._ironic_ref.release_orphan_node.assert_not_called() + assert obj._orphans_seen == {("n1", "r1")} + + def test_disabled_does_not_even_query_ironic(self, mocker, reconcile_conf): + reconcile_conf.config(group="netdev_router_reconcile", enabled=False) + obj = _periodics(mocker, nodes=[_node(mocker, "n1", "r1")]) + obj._orphans_seen = {("n1", "r1")} + + obj.reconcile_netdev_routers() + + obj._ironic_ref.paloalto_reconcile_nodes.assert_not_called() + assert obj._orphans_seen == set() + + def test_a_failed_pass_never_escapes_the_periodic(self, mocker, reconcile_conf): + # A failed scan is logged and retried on the next scheduled pass. + obj = _periodics(mocker, nodes=[]) + obj._orphans_seen = {("n1", "r1")} + obj._ironic_ref.paloalto_reconcile_nodes.side_effect = RuntimeError( + "ironic down" + ) + + obj.reconcile_netdev_routers() + + assert obj._orphans_seen == set() + + def test_enabled_pass_runs_the_sweep(self, mocker, reconcile_conf): + obj = _periodics(mocker, nodes=[_node(mocker, "n1", "r1")]) + + obj.reconcile_netdev_routers() + + obj._ironic_ref.paloalto_reconcile_nodes.assert_called_once_with() + + def test_lost_lock_before_release_clears_confirmation(self, mocker, reconcile_conf): + obj = _periodics(mocker, nodes=[_node(mocker, "n1", "r1")]) + obj._reconcile_once() + obj._idl.has_lock = False + + obj._reconcile_once() + + obj._ironic_ref.release_orphan_node.assert_not_called() + assert obj._orphans_seen == set() 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 7432ce3a8..1a5a99802 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,4 +1,5 @@ import copy +import logging from types import MethodType from types import SimpleNamespace @@ -15,6 +16,7 @@ from neutron_lib.callbacks import resources from neutron_lib.exceptions import l3 as l3_exc +from neutron_understack.ironic import NodeReleaseResult from neutron_understack.l3_router import palo_alto @@ -310,7 +312,10 @@ def _router(self): def test_releases_adopted_node(self, mocker): ironic = mocker.Mock() - ironic.release_node_for_router.return_value = mocker.Mock(id="node-uuid") + node = mocker.Mock(id="node-uuid") + ironic.release_node_for_router.return_value = NodeReleaseResult( + node=node, released=True + ) provider = _make_provider( mocker, FakeFlavorPlugin(_palo_alto_driver()), ironic=ironic ) @@ -322,7 +327,9 @@ def test_releases_adopted_node(self, mocker): def test_warns_when_no_node_bound(self, mocker): ironic = mocker.Mock() - ironic.release_node_for_router.return_value = None + ironic.release_node_for_router.return_value = NodeReleaseResult( + node=None, released=False + ) provider = _make_provider( mocker, FakeFlavorPlugin(_palo_alto_driver()), ironic=ironic ) @@ -332,6 +339,22 @@ def test_warns_when_no_node_bound(self, mocker): "router", "after_delete", "trigger", FakePayload(self._router()) ) + def test_warns_when_release_is_pending_reconciliation(self, mocker, caplog): + ironic = mocker.Mock() + ironic.release_node_for_router.return_value = NodeReleaseResult( + node=mocker.Mock(id="node-uuid"), released=False + ) + provider = _make_provider( + mocker, FakeFlavorPlugin(_palo_alto_driver()), ironic=ironic + ) + + with caplog.at_level(logging.WARNING): + provider._process_router_delete( + "router", "after_delete", "trigger", FakePayload(self._router()) + ) + + assert "is incomplete; reconciliation will retry it" in caplog.text + def test_skips_non_palo_alto_router(self, mocker): ironic = mocker.Mock() plugin = FakeFlavorPlugin("neutron_understack.l3_router.vrf.Vrf") diff --git a/python/neutron-understack/pyproject.toml b/python/neutron-understack/pyproject.toml index 702dda070..f0423003f 100644 --- a/python/neutron-understack/pyproject.toml +++ b/python/neutron-understack/pyproject.toml @@ -38,6 +38,7 @@ ironic = "neutron_understack.config:list_ironic_opts" understack = "neutron_understack.config:list_understack_opts" cisco-asa = "neutron_understack.config:list_cisco_asa_opts" understack-vni = "neutron_understack.config:list_understack_vni_opts" +netdev-router-reconcile = "neutron_understack.config:list_netdev_reconcile_opts" [project.entry-points."neutron.ml2.mechanism_drivers"] understack = "neutron_understack.neutron_understack_mech:UnderstackDriver"