From 0d6efef1dbc71ab36e7f2ca2b10dab4632683166 Mon Sep 17 00:00:00 2001 From: Doug Goldstein Date: Mon, 21 Sep 2026 14:12:15 -0500 Subject: [PATCH 1/2] feat(neutron-understack): own trunk status and subport ownership ml2/ovn is about to stop managing trunks whose parent LSP is type=external, which is every baremetal parent (LP #2167105). It was the only thing moving our trunks out of DOWN and claiming their subports, so do both here: ACTIVE once the parent is bound and the undersync driver's handler for the event has run, otherwise DOWN. Subports follow their trunk. A trunk on any other kind of parent stays ml2/ovn's. Undersync does not report back, so ACTIVE means a sync was requested, not that the fabric was confirmed to carry the subports. Correcting status after the fact is left to an out-of-band reconciler. --- .../tests/scenarios/SCENARIOS.md | 39 ++- .../tests/scenarios/base.py | 26 +- .../tests/scenarios/test_trunk_operations.py | 44 ++++ .../neutron_understack/tests/test_trunk.py | 225 +++++++++++++++++- .../tests/test_understack_mech.py | 5 + .../neutron_understack/trunk.py | 146 ++++++++++++ .../neutron_understack/utils.py | 11 + 7 files changed, 483 insertions(+), 13 deletions(-) diff --git a/python/neutron-understack/neutron_understack/tests/scenarios/SCENARIOS.md b/python/neutron-understack/neutron_understack/tests/scenarios/SCENARIOS.md index e1a6fc633..e8e430d52 100644 --- a/python/neutron-understack/neutron_understack/tests/scenarios/SCENARIOS.md +++ b/python/neutron-understack/neutron_understack/tests/scenarios/SCENARIOS.md @@ -233,6 +233,18 @@ These scenarios load the real neutron trunk service plugin adds/removes must reconcile the parent's switch (VLAN group). `utils.fetch_network_node_trunk_id` (live OVN + Ironic discovery) is stubbed. +This driver also owns the **status** of the trunks it serves and the +`device_owner`/`status` of their subports. The trunk plugin creates every trunk +DOWN and forces it back to DOWN on each subport add/remove, and ml2/ovn will not +manage a trunk whose parent LSP is `type=external` — which is what a baremetal +parent gets (LP #2167105). A trunk on any other kind of parent stays ml2/ovn's, +so we leave its status and subports alone. Ours report ACTIVE once the undersync +driver has been asked to reconcile the parent's VLAN group (this driver records +status after that handler, but cannot tell whether the push succeeded), DOWN +while the parent is unbound (nothing carries the subports). Subports follow +their trunk. +Statuses are corrected after the fact, out of band. + ### TRUNK-SUB-ADD — subport attach syncs the parent's physnet - given: a bound baremetal parent port on `physnet1` with a trunk, and a subport port on another network @@ -248,6 +260,8 @@ adds/removes must reconcile the parent's switch (VLAN group). that row is the record that the subport's network is carried on this VLAN of the parent's switchport - `undersync.sync(physnet1)` reconciles the parent port's switch + - the subport is claimed (`device_owner=trunk:subport`, status ACTIVE) and the + trunk reports ACTIVE ### TRUNK-SUB-DEL — subport removal syncs the parent's physnet - given: the TRUNK-SUB-ADD setup with the subport attached @@ -257,7 +271,8 @@ adds/removes must reconcile the parent's switch (VLAN group). **because that row was the last reference to it**, and `undersync.sync(physnet1)` reconciles the parent port's switch. Had another port or subport still been bound to that segment, only the row would go and - the segment would stay. + the segment would stay. The port is released (`device_owner` cleared, status + DOWN) and the trunk reports ACTIVE again. ### TRUNK-PARENT-NOIP — subport add syncs when the parent has no IP - given: a bound baremetal parent on a subnetted network but with no fixed IP, @@ -291,14 +306,23 @@ adds/removes must reconcile the parent's switch (VLAN group). - then: trunk membership is recorded, but no subport binding level row or dynamic VLAN segment is created and no `undersync.sync` occurs — with no parent binding there is no physnet to carry the subport on and nothing to - reconcile + reconcile. The parent is a plain port, not a baremetal one, so the trunk is + not ours to manage either: it stays DOWN and the subport is left unclaimed. ### TRUNK-SEGID-RANGE-01 — subport seg_id outside the allowed range is rejected - given: a bound baremetal parent with a trunk - when: a subport is added with a segmentation_id outside `[1, 3799]` - then: `SubportSegmentationIDError` (raised in the SUBPORTS PRECOMMIT_CREATE callback, surfaced as `CallbackFailure`), with no binding level or dynamic - segment allocated + segment allocated and the port left unclaimed + +### TRUNK-STATUS-PARENT-BIND-01 — a trunk activates when its parent is attached +- given: an *unbound* baremetal parent with a trunk and one subport — the usual + order of events, since a trunk is built before the node is deployed +- when: the parent port is vif-attached to `physnet1` +- then: while unbound, trunk and subport are DOWN with nothing allocated; the + vif-attach then binds the subport (dynamic segment + level-0 row), syncs + `physnet1` and takes both to ACTIVE ### TRUNK-ORDER-01 — subport removal deallocates before it notifies Undersync - given: a bound baremetal parent with a trunk and an attached subport @@ -473,8 +497,13 @@ Port create / bind / delete (no new test doubles): (early return, no sync). Trunk (no new test doubles): -- TRUNK-CREATE-WITH-SUBPORTS-01 — trunk created with subports present - (`trunk_created` path); confirm whether it syncs (it may not — possible gap). +- TRUNK-CREATE-WITH-SUBPORTS-01 — trunk created with subports inline. The driver + handles it (TRUNK PRECOMMIT_CREATE allocates, AFTER_CREATE pushes and sets the + status; unit-tested in `test_trunk.py`), but it is not observable here: the + precommit allocation writes through its own session (`utils.get_admin_session`), + and closing that session rolls the trunk's own insert back on the harness's + shared sqlite connection. Needs the precommit writes to join the caller's + transaction — which would also stop them leaking if the create is rolled back. - TRUNK-PARENT-UNBIND-01 — unbinding a trunked baremetal parent runs `clean_trunk` (subport teardown) alongside BM-BIND-04's segment handling. diff --git a/python/neutron-understack/neutron_understack/tests/scenarios/base.py b/python/neutron-understack/neutron_understack/tests/scenarios/base.py index 3a53a9936..f45dc1d79 100644 --- a/python/neutron-understack/neutron_understack/tests/scenarios/base.py +++ b/python/neutron-understack/neutron_understack/tests/scenarios/base.py @@ -109,12 +109,11 @@ def baremetal_binding_profile(physnet=DEFAULT_PHYSNET, port_id="Ethernet1/1"): profile["physical_network"] = physnet return profile - def _bind_baremetal_port(self, net_id, physnet, host, fixed_ips=None): - """Create a baremetal port on the network and vif-attach it to physnet. + def _create_baremetal_port(self, net_id, fixed_ips=None): + """Create an unbound baremetal port on the network. Returns its id. - Returns the bound port's id. binding:host_id / binding:profile writes - require the service role under secure RBAC. Pass ``fixed_ips=[]`` to force - a port with no IP even on a subnetted network. + Pass ``fixed_ips=[]`` to force a port with no IP even on a subnetted + network. """ create_kwargs = {portbindings.VNIC_TYPE: portbindings.VNIC_BAREMETAL} if fixed_ips is not None: @@ -127,7 +126,14 @@ def _bind_baremetal_port(self, net_id, physnet, host, fixed_ips=None): **create_kwargs, ) assert res.status_int == 201, res.body - port_id = self.deserialize(self.fmt, res)["port"]["id"] + return self.deserialize(self.fmt, res)["port"]["id"] + + def _vif_attach_port(self, port_id, physnet, host): + """vif-attach a port to a switch, as Ironic does on deploy. + + binding:host_id / binding:profile writes require the service role under + secure RBAC. + """ data = { "port": { portbindings.HOST_ID: host, @@ -136,6 +142,14 @@ def _bind_baremetal_port(self, net_id, physnet, host, fixed_ips=None): } req = self.new_update_request("ports", data, port_id, as_service=True) assert req.get_response(self.api).status_int == 200 + + def _bind_baremetal_port(self, net_id, physnet, host, fixed_ips=None): + """Create a baremetal port on the network and vif-attach it to physnet. + + Returns the bound port's id. + """ + port_id = self._create_baremetal_port(net_id, fixed_ips=fixed_ips) + self._vif_attach_port(port_id, physnet, host) return port_id def _trunk_subports(self, trunk_id): diff --git a/python/neutron-understack/neutron_understack/tests/scenarios/test_trunk_operations.py b/python/neutron-understack/neutron_understack/tests/scenarios/test_trunk_operations.py index 0de428899..bb1edb7aa 100644 --- a/python/neutron-understack/neutron_understack/tests/scenarios/test_trunk_operations.py +++ b/python/neutron-understack/neutron_understack/tests/scenarios/test_trunk_operations.py @@ -13,6 +13,7 @@ from neutron.services.trunk import exceptions as trunk_exc from neutron_lib import constants as p_const from neutron_lib.callbacks import exceptions as cb_exc +from neutron_lib.services.trunk import constants as trunk_consts from neutron_understack.tests.scenarios.base import DEFAULT_PHYSNET from neutron_understack.tests.scenarios.base import UnderstackMl2TrunkScenarioBase @@ -74,6 +75,15 @@ def _remove_subport(self, trunk_id, subport_id): self.context, trunk_id, {"sub_ports": [{"port_id": subport_id}]} ) + def _trunk_status(self, trunk_id): + return self.trunk_plugin.get_trunk(self.context, trunk_id)["status"] + + def _assert_subport_claimed(self, subport_id, status): + """The trunk owns the subport, and the subport follows the trunk.""" + port = self._show("ports", subport_id)["port"] + assert port["device_owner"] == trunk_consts.TRUNK_SUBPORT_OWNER + assert port["status"] == status + def _assert_subport_bound(self, trunk_id, subport_id, host, seg_id=SUBPORT_VLAN): assert { subport["port_id"]: subport for subport in self._trunk_subports(trunk_id) @@ -93,12 +103,19 @@ def _assert_subport_bound(self, trunk_id, subport_id, host, seg_id=SUBPORT_VLAN) assert segment[segments_db.PHYSICAL_NETWORK] == DEFAULT_PHYSNET segment_obj = NetworkSegment.get_object(self.context, id=levels[0].segment_id) assert segment_obj.is_dynamic + + self._assert_subport_claimed(subport_id, p_const.PORT_STATUS_ACTIVE) + assert self._trunk_status(trunk_id) == trunk_consts.TRUNK_ACTIVE_STATUS return levels[0].segment_id def _assert_subport_unbound(self, subport_id, host, segment_id): assert not ml2_db.get_binding_level_objs(self.context, subport_id, host) assert segments_db.get_segment_by_id(self.context, segment_id) is None + port = self._show("ports", subport_id)["port"] + assert port["device_owner"] == "" + assert port["status"] == p_const.PORT_STATUS_DOWN + @pytest.mark.scenario("TRUNK-SUB-ADD") def test_subport_add_syncs_parent_physnet(self): parent_net = self._make_network(self.fmt, "parent-net", True)["network"]["id"] @@ -128,6 +145,7 @@ def test_subport_remove_syncs_parent_physnet(self): assert updated_trunk["sub_ports"] == [] self._assert_subport_unbound(subport_id, "host-a", segment_id) + assert self._trunk_status(trunk_id) == trunk_consts.TRUNK_ACTIVE_STATUS self.undersync_mock.sync.assert_any_call(DEFAULT_PHYSNET) @pytest.mark.scenario("TRUNK-ORDER-01") @@ -221,6 +239,8 @@ def test_subport_add_unbound_parent_no_sync(self): self._add_subport(trunk_id, subport_id) assert self._trunk_subports(trunk_id)[0]["port_id"] == subport_id + assert self._trunk_status(trunk_id) == trunk_consts.TRUNK_DOWN_STATUS + assert self._show("ports", subport_id)["port"]["device_owner"] == "" assert not ml2_db.get_binding_level_objs(self.context, subport_id, "") assert ( segments_db.get_dynamic_segment( @@ -274,6 +294,29 @@ def test_multiple_subports_add_syncs(self): assert segment_a != segment_b self.undersync_mock.sync.assert_any_call(DEFAULT_PHYSNET) + @pytest.mark.scenario("TRUNK-STATUS-PARENT-BIND-01") + def test_trunk_activates_when_the_parent_is_vif_attached(self): + parent_net = self._make_network(self.fmt, "parent-net", True)["network"]["id"] + parent_id = self._create_baremetal_port(parent_net) + subport_net = self._make_network(self.fmt, "subport-net", True)["network"]["id"] + subport_id = self._plain_port(subport_net) + trunk_id = self._make_trunk(parent_id) + self._add_subport(trunk_id, subport_id) + + # Nothing is carried yet: the node has not been deployed. + assert self._trunk_status(trunk_id) == trunk_consts.TRUNK_DOWN_STATUS + self._assert_subport_claimed(subport_id, p_const.PORT_STATUS_DOWN) + + self.undersync_mock.reset_mock() + with mock.patch( + "neutron_understack.utils.fetch_network_node_trunk_id", + return_value=_FAKE_NN_TRUNK, + ): + self._vif_attach_port(parent_id, DEFAULT_PHYSNET, "host-a") + + self._assert_subport_bound(trunk_id, subport_id, "host-a") + self.undersync_mock.sync.assert_any_call(DEFAULT_PHYSNET) + @pytest.mark.scenario("TRUNK-SEGID-RANGE-01") def test_subport_segid_out_of_range_rejected(self): parent_net = self._make_network(self.fmt, "parent-net", True)["network"]["id"] @@ -288,6 +331,7 @@ def test_subport_segid_out_of_range_rejected(self): with pytest.raises(cb_exc.CallbackFailure) as exc_info: self._add_subport(trunk_id, subport_id, seg_id=4000) assert "Segmentation ID" in str(exc_info.value) + assert self._show("ports", subport_id)["port"]["device_owner"] == "" assert not ml2_db.get_binding_level_objs(self.context, subport_id, "host-a") assert ( segments_db.get_dynamic_segment( diff --git a/python/neutron-understack/neutron_understack/tests/test_trunk.py b/python/neutron-understack/neutron_understack/tests/test_trunk.py index d7555e7a3..81aab84d4 100644 --- a/python/neutron-understack/neutron_understack/tests/test_trunk.py +++ b/python/neutron-understack/neutron_understack/tests/test_trunk.py @@ -1,11 +1,44 @@ import pytest from neutron.plugins.ml2.driver_context import portbindings +from neutron_lib import constants as p_const from neutron_lib import exceptions as exc +from neutron_lib.services.trunk import constants as trunk_consts from neutron_understack import utils from neutron_understack.trunk import SubportSegmentationIDError +@pytest.fixture +def trunk_status(mocker, trunk): + """Capture the driver's trunk status write, which would hit the DB.""" + return mocker.patch.object(trunk, "update") + + +@pytest.fixture +def subport_fields(mocker): + """Capture the driver's writes to the subport's port row.""" + return mocker.patch("neutron_understack.utils.update_port_fields") + + +def assert_subport_owned(subport_fields, subport, status): + subport_fields.assert_called_once_with( + subport["port_id"], + {"device_owner": trunk_consts.TRUNK_SUBPORT_OWNER, "status": status}, + ) + + +def assert_subport_released(subport_fields, subport): + subport_fields.assert_called_once_with( + subport["port_id"], {"device_owner": "", "status": p_const.PORT_STATUS_DOWN} + ) + + +@pytest.fixture +def _bound_parent(mocker, port_object): + mocker.patch("neutron_understack.utils.fetch_port_object", return_value=port_object) + mocker.patch("neutron_understack.utils.parent_port_is_bound", return_value=True) + + class TestSubportsAdded: def test_that_handler_is_called( self, mocker, understack_trunk_driver, trunk_payload, subport, trunk @@ -100,6 +133,20 @@ def test_when_parent_port_is_bound( ) add_subports_networks.assert_called_once() + @pytest.mark.usefixtures("_bound_parent") + def test_subports_add_post( + self, + understack_trunk_driver, + trunk_payload, + subport, + trunk_status, + subport_fields, + ): + understack_trunk_driver.subports_added_post(None, None, None, trunk_payload) + + trunk_status.assert_called_once_with(status=trunk_consts.TRUNK_ACTIVE_STATUS) + assert_subport_owned(subport_fields, subport, p_const.PORT_STATUS_ACTIVE) + def test_when_parent_port_is_unbound( self, mocker, understack_trunk_driver, trunk, subport, port_object ): @@ -117,9 +164,17 @@ def test_when_parent_port_is_unbound( add_subports_networks.assert_not_called() +@pytest.mark.usefixtures("_bound_parent") class TestSubportsDeleted: def test_that_clean_parent_port_is_triggered( - self, mocker, understack_trunk_driver, trunk_payload, trunk, subport + self, + mocker, + understack_trunk_driver, + trunk_payload, + trunk, + subport, + trunk_status, + subport_fields, ): mocker.patch.object( understack_trunk_driver, "_clean_parent_port_switchport_config" @@ -133,10 +188,35 @@ def test_that_clean_parent_port_is_triggered( ) ) + def test_that_the_subport_is_released( + self, + mocker, + understack_trunk_driver, + trunk_payload, + subport, + trunk_status, + subport_fields, + ): + mocker.patch.object( + understack_trunk_driver, "_clean_parent_port_switchport_config" + ) + + understack_trunk_driver.subports_deleted("", "", "", trunk_payload) + + assert_subport_released(subport_fields, subport) + trunk_status.assert_called_once_with(status=trunk_consts.TRUNK_ACTIVE_STATUS) + +@pytest.mark.usefixtures("_bound_parent") class TestTrunkDeleted: def test_when_subports_are_present( - self, mocker, understack_trunk_driver, trunk_payload, trunk, subport + self, + mocker, + understack_trunk_driver, + trunk_payload, + trunk, + subport, + subport_fields, ): mocker.patch.object( understack_trunk_driver, "_clean_parent_port_switchport_config" @@ -149,6 +229,9 @@ def test_when_subports_are_present( trunk, [subport] ) ) + # The trunk row is gone, so there is no status to record; the ports the + # trunk owned are handed back. + assert_subport_released(subport_fields, subport) def test_when_subports_are_not_present( self, mocker, understack_trunk_driver, trunk_payload, trunk, subport @@ -266,6 +349,144 @@ def test_when_segment_is_used_by_other_ports( port_binding_level.delete.assert_called_once() +class TestTrunkStatus: + """The trunk plugin creates every trunk DOWN; this driver owns the rest.""" + + @pytest.mark.usefixtures("_bound_parent") + def test_created_trunk_on_a_bound_parent_is_active( + self, + understack_trunk_driver, + trunk_payload, + subport, + trunk_status, + subport_fields, + ): + understack_trunk_driver.trunk_created_post(None, None, None, trunk_payload) + + trunk_status.assert_called_once_with(status=trunk_consts.TRUNK_ACTIVE_STATUS) + assert_subport_owned(subport_fields, subport, p_const.PORT_STATUS_ACTIVE) + + def test_created_trunk_on_an_unbound_parent_stays_down( + self, + mocker, + understack_trunk_driver, + trunk_payload, + port_object, + subport, + trunk_status, + subport_fields, + ): + mocker.patch( + "neutron_understack.utils.fetch_port_object", return_value=port_object + ) + mocker.patch( + "neutron_understack.utils.parent_port_is_bound", return_value=False + ) + + understack_trunk_driver.trunk_created_post(None, None, None, trunk_payload) + + trunk_status.assert_called_once_with(status=trunk_consts.TRUNK_DOWN_STATUS) + # Still this trunk's subport, just not carried by any switch yet. + assert_subport_owned(subport_fields, subport, p_const.PORT_STATUS_DOWN) + + def test_a_trunk_on_a_non_baremetal_parent_is_left_to_ovn( + self, + mocker, + understack_trunk_driver, + trunk_payload, + port_object, + trunk_status, + subport_fields, + ): + port_object.bindings[0].vnic_type = portbindings.VNIC_NORMAL + mocker.patch( + "neutron_understack.utils.fetch_port_object", return_value=port_object + ) + + understack_trunk_driver.trunk_created_post(None, None, None, trunk_payload) + + trunk_status.assert_not_called() + subport_fields.assert_not_called() + + +class TestParentPortUpdated: + """A trunk is usually created before its parent port is vif-attached.""" + + @pytest.fixture + def port_update_payload(self, mocker, trunk, port_id): + return mocker.Mock( + context="context", + latest_state={ + "id": str(port_id), + "status": p_const.PORT_STATUS_ACTIVE, + "trunk_details": {"trunk_id": trunk.id}, + }, + states=[{"status": p_const.PORT_STATUS_DOWN}], + ) + + @pytest.fixture + def _trunk_lookup(self, mocker, trunk): + mocker.patch( + "neutron_understack.trunk.trunk_objects.Trunk.get_object", + return_value=trunk, + ) + + @pytest.mark.usefixtures("_bound_parent", "_trunk_lookup") + def test_parent_going_active_activates_the_trunk( + self, + understack_trunk_driver, + port_update_payload, + subport, + trunk_status, + subport_fields, + ): + understack_trunk_driver.parent_port_updated( + None, None, None, port_update_payload + ) + + trunk_status.assert_called_once_with(status=trunk_consts.TRUNK_ACTIVE_STATUS) + assert_subport_owned(subport_fields, subport, p_const.PORT_STATUS_ACTIVE) + + @pytest.mark.usefixtures("_bound_parent", "_trunk_lookup") + def test_a_port_that_was_already_active_is_ignored( + self, understack_trunk_driver, port_update_payload, trunk_status + ): + port_update_payload.states = [{"status": p_const.PORT_STATUS_ACTIVE}] + + understack_trunk_driver.parent_port_updated( + None, None, None, port_update_payload + ) + + trunk_status.assert_not_called() + + @pytest.mark.usefixtures("_trunk_lookup") + def test_a_port_that_is_not_a_trunk_parent_is_ignored( + self, understack_trunk_driver, port_update_payload, trunk_status + ): + del port_update_payload.latest_state["trunk_details"] + + understack_trunk_driver.parent_port_updated( + None, None, None, port_update_payload + ) + + trunk_status.assert_not_called() + + @pytest.mark.usefixtures("_trunk_lookup") + def test_a_non_baremetal_trunk_parent_is_left_to_its_own_driver( + self, mocker, understack_trunk_driver, port_update_payload, trunk_status + ): + mocker.patch("neutron_understack.utils.fetch_port_object") + mocker.patch( + "neutron_understack.utils.parent_port_is_bound", return_value=False + ) + + understack_trunk_driver.parent_port_updated( + None, None, None, port_update_payload + ) + + trunk_status.assert_not_called() + + class TestConfigureTrunk: def test_that_add_subports_networks_is_called( self, diff --git a/python/neutron-understack/neutron_understack/tests/test_understack_mech.py b/python/neutron-understack/neutron_understack/tests/test_understack_mech.py index 5eedca7f0..3b6012bdd 100644 --- a/python/neutron-understack/neutron_understack/tests/test_understack_mech.py +++ b/python/neutron-understack/neutron_understack/tests/test_understack_mech.py @@ -11,6 +11,7 @@ from neutron_understack import trunk from neutron_understack import understack_mech +from neutron_understack import undersync_mech def _port_update_context(original, current): @@ -465,3 +466,7 @@ def test_trunk_module_never_mentions_undersync(self): source = pathlib.Path(trunk.__file__).read_text().lower() assert "undersync" not in source + + def test_trunk_status_is_recorded_after_the_fabric_push(self): + """ACTIVE must only follow the undersync driver's handler for the event.""" + assert trunk.STATUS_EVENT_PRIORITY > undersync_mech.TRUNK_EVENT_PRIORITY diff --git a/python/neutron-understack/neutron_understack/trunk.py b/python/neutron-understack/neutron_understack/trunk.py index d50b162c5..4535ee642 100644 --- a/python/neutron-understack/neutron_understack/trunk.py +++ b/python/neutron-understack/neutron_understack/trunk.py @@ -1,11 +1,14 @@ +from neutron.objects import trunk as trunk_objects from neutron.objects.network import NetworkSegment from neutron.objects.ports import Port from neutron.objects.trunk import SubPort from neutron.services.trunk.drivers import base as trunk_base from neutron.services.trunk.models import Trunk +from neutron_lib import constants as p_const from neutron_lib import exceptions as exc from neutron_lib.api.definitions import portbindings from neutron_lib.callbacks import events +from neutron_lib.callbacks import priority_group from neutron_lib.callbacks import registry from neutron_lib.callbacks import resources from neutron_lib.services.trunk import constants as trunk_consts @@ -20,6 +23,12 @@ SUPPORTED_SEGMENTATION_TYPES = (trunk_consts.SEGMENTATION_TYPE_VLAN,) +# Record status after the handlers that push the trunk's config to the fabric +# (they subscribe at PRIORITY_DEFAULT + 1000), so a trunk is only reported ACTIVE +# once that has been asked for. Its own priority group, since neutron-lib stores +# `cancellable` per group. +STATUS_EVENT_PRIORITY = priority_group.PRIORITY_DEFAULT + 2000 + class SubportSegmentationIDError(exc.NeutronException): message = ( @@ -65,6 +74,13 @@ def register(self, resource, event, trigger, payload=None): events.PRECOMMIT_CREATE, cancellable=True, ) + registry.subscribe( + self.subports_added_post, + resources.SUBPORTS, + events.AFTER_CREATE, + priority=STATUS_EVENT_PRIORITY, + cancellable=True, + ) registry.subscribe( self.subports_deleted, resources.SUBPORTS, @@ -77,12 +93,94 @@ def register(self, resource, event, trigger, payload=None): events.PRECOMMIT_CREATE, cancellable=True, ) + registry.subscribe( + self.trunk_created_post, + resources.TRUNK, + events.AFTER_CREATE, + priority=STATUS_EVENT_PRIORITY, + cancellable=True, + ) registry.subscribe( self.trunk_deleted, resources.TRUNK, events.AFTER_DELETE, cancellable=True, ) + registry.subscribe( + self.parent_port_updated, + resources.PORT, + events.AFTER_UPDATE, + ) + + def _set_trunk_status(self, trunk: Trunk, status: str) -> None: + """Record a trunk's status. Postcommit only. + + This driver owns the status of the trunks it serves: the trunk plugin + forces DOWN on every create/add/remove, and ml2/ovn deliberately does + not manage a trunk whose parent LSP is type=external, which is what a + baremetal parent gets (LP #2167105). + """ + LOG.debug("setting trunk %s status to %s", trunk.id, status) + trunk.update(status=status) + + @staticmethod + def _trunk_status_for_parent(parent_port: Port) -> str: + """The status a trunk on this parent port should report.""" + if utils.parent_port_is_bound(parent_port): + return trunk_consts.TRUNK_ACTIVE_STATUS + return trunk_consts.TRUNK_DOWN_STATUS + + def _record_trunk_state(self, trunk: Trunk, subports: list[SubPort]) -> None: + """Postcommit: record the trunk's status and claim its subports. + + The matching precommit hook has already written the subports' binding + levels; pushing them to the fabric is another driver's job, + and its handler for the same event runs first (STATUS_EVENT_PRIORITY). + """ + parent_port = utils.fetch_port_object(trunk.port_id) + if not utils.is_baremetal_parent_port(parent_port): + return + status = self._trunk_status_for_parent(parent_port) + self._claim_subports(subports, status) + self._set_trunk_status(trunk, status) + + def _claim_subports(self, subports: list[SubPort], trunk_status: str) -> None: + """Mark these ports as subports of a trunk we serve. + + ml2/ovn used to do this as a side effect of managing our trunks + (LP #2167105). device_owner is what stops the rest of Neutron -- ovn's + own update_lsp_host_info() included -- from treating a subport as a + free port, and a subport is only up if the fabric carries it, which is + what the trunk's status says. + """ + self._update_subports( + subports, + device_owner=trunk_consts.TRUNK_SUBPORT_OWNER, + status=( + p_const.PORT_STATUS_ACTIVE + if trunk_status == trunk_consts.TRUNK_ACTIVE_STATUS + else p_const.PORT_STATUS_DOWN + ), + ) + + def _release_subports(self, subports: list[SubPort]) -> None: + """Undo _claim_subports() for ports that are leaving the trunk.""" + self._update_subports( + subports, device_owner="", status=p_const.PORT_STATUS_DOWN + ) + + @staticmethod + def _update_subports( + subports: list[SubPort], device_owner: str, status: str + ) -> None: + # Written straight to the port row, as ml2/ovn did: update_port_status() + # would re-enter the mechanism drivers, and a revision bump would send + # ovn's maintenance worker chasing an LSP that has not changed. + for subport in subports: + utils.update_port_fields( + subport["port_id"], + {"device_owner": device_owner, "status": status}, + ) def _handle_tenant_vlan_id_and_switchport_config( self, subports: list[SubPort], trunk: Trunk @@ -208,19 +306,67 @@ def subports_added(self, resource, event, trunk_plugin, payload): subports = payload.metadata["subports"] self._handle_tenant_vlan_id_and_switchport_config(subports, trunk) + def subports_added_post(self, resource, event, trunk_plugin, payload): + self._record_trunk_state(payload.states[0], payload.metadata["subports"]) + def subports_deleted(self, resource, event, trunk_plugin, payload): trunk = payload.states[0] subports = payload.metadata["subports"] self._clean_parent_port_switchport_config(trunk, subports) + parent_port = utils.fetch_port_object(trunk.port_id) + if not utils.is_baremetal_parent_port(parent_port): + return + self._release_subports(subports) + self._set_trunk_status(trunk, self._trunk_status_for_parent(parent_port)) + def trunk_created(self, resource, event, trunk_plugin, payload): trunk = payload.latest_state subports = trunk.sub_ports if subports: self._handle_tenant_vlan_id_and_switchport_config(subports, trunk) + def trunk_created_post(self, resource, event, trunk_plugin, payload): + """A trunk is born DOWN; record its real state once its rows are committed.""" + trunk = payload.states[0] + self._record_trunk_state(trunk, trunk.sub_ports) + + def parent_port_updated(self, resource, event, trunk_plugin, payload): + """Follow a trunked parent port to ACTIVE. + + A trunk is usually created before the node is deployed, so the parent's + vif-attach is the point where its fabric config first exists. ml2/ovn + does this for its own trunks; ours are not its to manage. + """ + updated_port = payload.latest_state + trunk_details = updated_port.get("trunk_details") + if not trunk_details: + return + if updated_port.get("status") != p_const.PORT_STATUS_ACTIVE: + return + if payload.states[0].get("status") == p_const.PORT_STATUS_ACTIVE: + return + parent_port = utils.fetch_port_object(updated_port["id"]) + if not utils.is_baremetal_parent_port(parent_port): + return + if ( + self._trunk_status_for_parent(parent_port) + != trunk_consts.TRUNK_ACTIVE_STATUS + ): + return + + trunk = trunk_objects.Trunk.get_object( + payload.context, id=trunk_details["trunk_id"] + ) + if not trunk: + return + self._claim_subports(trunk.sub_ports, trunk_consts.TRUNK_ACTIVE_STATUS) + self._set_trunk_status(trunk, trunk_consts.TRUNK_ACTIVE_STATUS) + def trunk_deleted(self, resource, event, trunk_plugin, payload): trunk = payload.states[0] subports = trunk.sub_ports if subports: self._clean_parent_port_switchport_config(trunk, subports) + if utils.is_baremetal_parent_port(utils.fetch_port_object(trunk.port_id)): + self._release_subports(subports) diff --git a/python/neutron-understack/neutron_understack/utils.py b/python/neutron-understack/neutron_understack/utils.py index 83e332bad..bfa768f7a 100644 --- a/python/neutron-understack/neutron_understack/utils.py +++ b/python/neutron-understack/neutron_understack/utils.py @@ -480,6 +480,17 @@ def active_port_binding(port: port_obj.Port) -> port_obj.PortBinding | None: ) +def is_baremetal_parent_port(port: port_obj.Port) -> bool: + """Whether a trunk on this parent port is ours to manage. + + ml2/ovn manages the trunks of every other parent; this is the same + distinction its trunk driver draws on the LSP's type (LP #2167105). + """ + return bool(port.bindings) and ( + port.bindings[0].vnic_type == portbindings.VNIC_BAREMETAL + ) + + def parent_port_is_bound(port: port_obj.Port) -> bool: port_binding = active_port_binding(port) return bool( From 95993e40118cc566f4973b6506e8e6234bda0605 Mon Sep 17 00:00:00 2001 From: Doug Goldstein Date: Mon, 21 Sep 2026 14:25:46 -0500 Subject: [PATCH 2/2] feat(kubectl-us): add net trunk audit and repair ml2/ovn wrote parent_name/tag into every baremetal trunk subport's binding profile and LSP, where they mean nothing: undersync programs the leaf switch. LP #2167105 stops it writing them but leaves the rows it already wrote, since the code that removed them on subport delete now returns early. The binding profile is the source of truth -- ml2/ovn derives the LSP's parent_name/tag from it -- so dropping those keys through Neutron makes it rewrite the LSP itself; ovn-nbctl clear only mops up what it misses. Trunk status is audit-only, as the Neutron API does not accept it. --- docs/operator-guide/kubectl-us.md | 59 ++++ python/kubectl-us/README.md | 30 ++ python/kubectl-us/tests/test_trunk.py | 285 ++++++++++++++++ python/kubectl-us/us_cli/cli.py | 2 + python/kubectl-us/us_cli/commands/trunk.py | 372 +++++++++++++++++++++ 5 files changed, 748 insertions(+) create mode 100644 python/kubectl-us/tests/test_trunk.py create mode 100644 python/kubectl-us/us_cli/commands/trunk.py diff --git a/docs/operator-guide/kubectl-us.md b/docs/operator-guide/kubectl-us.md index 80bdae784..f2dbd8a31 100644 --- a/docs/operator-guide/kubectl-us.md +++ b/docs/operator-guide/kubectl-us.md @@ -310,6 +310,65 @@ refusal in one domain does not block safe work in the other. When `--apply` performs only the safe portion, the command verifies that work and exits `2` to report that part of the router remains unresolved. +### net trunk audit + +```bash +# Every baremetal trunk, or just one. +kubectl us net trunk audit +kubectl us net trunk audit +``` + +Reports the trunk state ml2/ovn left behind on baremetal trunks. ml2/ovn used +to manage any trunk whose parent port had a `Logical_Switch_Port`, baremetal +parents included -- their LSP is `type=external` only so OVN can schedule an HA +chassis group, while the VLANs are programmed on the leaf switch by undersync. +It wrote `parent_name`/`tag` into each subport's binding profile and LSP, where +they mean nothing, and pointed `parent_name` at an LSP on a different logical +switch. [LP #2167105](https://bugs.launchpad.net/neutron/+bug/2167105) stops it +managing these trunks, but the rows it already wrote stay behind: the code that +removed them on subport delete now returns early. + +Trunks whose parent port is not baremetal are skipped -- ml2/ovn still owns +those. For each baremetal trunk the audit verifies: + +- the trunk's status matches its parent port: `ACTIVE` when the parent is + vif-attached with a `physical_network`, `DOWN` when it is unbound. A trunk + last touched before neutron-understack took over the status corrects itself + on its next subport change; +- no subport binding profile carries `parent_name` or `tag`; +- no subport LSP carries `parent_name` or `tag_request` (Neutron converts a + `tag` write into `tag_request`; northd derives the read-only `tag` from it); + and +- each subport is claimed with `device_owner=trunk:subport`. + +Any failed check produces a nonzero exit status. + +### net trunk repair + +```bash +# Plan only; this writes nothing. +kubectl us net trunk repair +kubectl us net trunk repair + +# Apply the displayed plan and verify the result. +kubectl us net trunk repair --apply +``` + +Repairs what the audit reports, except the trunk status, which is not settable +through the Neutron API. + +The subport's binding profile is the source of truth: ml2/ovn derives the LSP's +`parent_name`/`tag` from it (`ovn_client._get_port_options`), so dropping those +keys through Neutron makes ml2/ovn rewrite the LSP itself. The same port update +also resets the subport's stale `vif_type=ovs` binding to `unbound`, and +re-claims a subport whose `device_owner` is missing. Only rows ml2/ovn did not +rewrite are then cleared directly with `ovn-nbctl clear`. + +Nothing else in the binding profile is touched, and the dynamic VLAN segments +and level-0 binding levels that actually carry the subports are never modified. +With `--apply`, the affected trunks are reloaded afterwards and the command +exits nonzero if any repair did not stick. + ### backup local ```bash diff --git a/python/kubectl-us/README.md b/python/kubectl-us/README.md index c5c224497..36993b9a5 100644 --- a/python/kubectl-us/README.md +++ b/python/kubectl-us/README.md @@ -150,6 +150,36 @@ See the [operator guide](../../docs/operator-guide/kubectl-us.md#net-router-repair) for the complete safety contract. +### `net trunk audit` + +``` +kubectl us net trunk audit # every baremetal trunk +kubectl us net trunk audit +``` + +Reports the trunk state ml2/ovn left on baremetal trunks after +[LP #2167105](https://bugs.launchpad.net/neutron/+bug/2167105) stopped it +managing them: `parent_name`/`tag` in a subport's binding profile, the LSP +`parent_name`/`tag_request` derived from those, an unclaimed subport, and a +trunk status that disagrees with its parent port's binding. Trunks whose parent +port is not baremetal are skipped, since ml2/ovn still owns them. Any failed +check produces a nonzero exit status. + +### `net trunk repair` + +``` +kubectl us net trunk repair # dry-run plan +kubectl us net trunk repair --apply # write and verify +``` + +Drops those keys from the subport's binding profile, which makes ml2/ovn +rewrite the LSP without them, and re-claims subports missing +`device_owner=trunk:subport`. Rows ml2/ovn does not rewrite are cleared +directly in the NB. Trunk status is audit-only: the Neutron API does not accept +it. Segments and binding levels are never touched. See the +[operator guide](../../docs/operator-guide/kubectl-us.md#net-trunk-repair) for +the complete contract. + ### `backup local` ``` diff --git a/python/kubectl-us/tests/test_trunk.py b/python/kubectl-us/tests/test_trunk.py new file mode 100644 index 000000000..1e12a05e7 --- /dev/null +++ b/python/kubectl-us/tests/test_trunk.py @@ -0,0 +1,285 @@ +import types + +import pytest +import typer +from typer.testing import CliRunner + +from us_cli.commands import trunk +from us_cli.connection import ConnectionContext + +runner = CliRunner() + + +def make_app(): + app = typer.Typer() + + @app.callback() + def main(ctx: typer.Context) -> None: + ctx.obj = ConnectionContext( + kube_context=None, + namespace="openstack", + nb_pod="ovn-ovsdb-nb-0", + sb_pod="ovn-ovsdb-sb-0", + os_cloud="dev-cloud", + ) + + app.add_typer(trunk.app, name="trunk") + return app + + +def make_parent(vnic_type="baremetal", bound=True): + return types.SimpleNamespace( + id="parent-1", + device_owner="compute:ironic", + binding_vnic_type=vnic_type, + binding_vif_type="other" if bound else "unbound", + binding_profile={"physical_network": "f20-1-network"} if bound else {}, + ) + + +def make_subport(profile=None, device_owner=trunk.SUBPORT_DEVICE_OWNER): + """A subport port, by default already in the state we want.""" + return types.SimpleNamespace( + id="sub-1", + name="subport", + device_owner=device_owner, + binding_vnic_type="normal", + binding_vif_type="unbound", + binding_profile={} if profile is None else profile, + ) + + +def make_trunk(status="ACTIVE"): + return types.SimpleNamespace( + id="trunk-1", + name="bm-trunk", + port_id="parent-1", + status=status, + sub_ports=[ + {"port_id": "sub-1", "segmentation_type": "vlan", "segmentation_id": 1500} + ], + ) + + +class FakeNetworkAPI: + def __init__(self, trunks, ports): + self._trunks = trunks + self._ports = {port.id: port for port in ports} + self.updates: list[tuple[str, dict]] = [] + self.reflect_updates = True + + def trunks(self): + return list(self._trunks) + + def find_trunk(self, name_or_id): + for item in self._trunks: + if name_or_id in (item.id, item.name): + return item + return None + + def get_port(self, port_id): + return self._ports[port_id] + + def update_port(self, port_id, **attrs): + self.updates.append((port_id, attrs)) + port = self._ports[port_id] + if self.reflect_updates: + for key, value in attrs.items(): + setattr(port, key, value) + return port + + +class FakeConfig: + config = {} + + +class FakeConnection: + def __init__(self, trunks, ports): + self.network = FakeNetworkAPI(trunks, ports) + self.config = FakeConfig() + + +class FakeNb: + """The NB Logical_Switch_Port rows, keyed by port id.""" + + def __init__(self, rows): + self.rows = rows + self.calls: list[list[str]] = [] + + def list_records(self, ctx, table, records): + assert table == "Logical_Switch_Port" + return [ + {"name": port_id, **columns} + for port_id, columns in self.rows.items() + if port_id in records + ] + + def raw(self, ctx, args): + self.calls.append(args) + for index, arg in enumerate(args): + # `-- clear Logical_Switch_Port ` + if arg == "clear": + port_id, column = args[index + 2], args[index + 3] + self.rows.get(port_id, {}).pop(column, None) + return "" + + +@pytest.fixture +def setup(monkeypatch): + def _setup(trunks=None, ports=None, nb_rows=None): + trunks = [make_trunk()] if trunks is None else trunks + ports = [make_parent(), make_subport()] if ports is None else ports + conn = FakeConnection(trunks, ports) + nb = FakeNb({} if nb_rows is None else nb_rows) + monkeypatch.setattr(trunk.osclient, "get_connection", lambda os_cloud: conn) + monkeypatch.setattr(trunk.ovn, "nbctl_list_records", nb.list_records) + monkeypatch.setattr(trunk.ovn, "nbctl_raw", nb.raw) + return conn, nb + + return _setup + + +STALE_PROFILE = {"parent_name": "parent-1", "tag": 1500} +STALE_LSP = {"sub-1": {"parent_name": "parent-1", "tag_request": 1500}} + + +class TestAudit: + def test_a_clean_trunk_passes(self, setup): + setup(nb_rows={"sub-1": {"parent_name": "", "tag_request": []}}) + + result = runner.invoke(make_app(), ["trunk", "audit"]) + + assert result.exit_code == 0, result.output + assert "FAIL" not in result.output + assert "Trunk bm-trunk (trunk-1)" in result.output + + def test_stale_profile_and_lsp_rows_fail(self, setup): + setup( + ports=[make_parent(), make_subport(profile=dict(STALE_PROFILE))], + nb_rows={"sub-1": dict(STALE_LSP["sub-1"])}, + ) + + result = runner.invoke(make_app(), ["trunk", "audit"]) + + assert result.exit_code == 1 + assert "stale ml2/ovn keys: parent_name, tag" in result.output + assert "still set: parent_name, tag_request" in result.output + + def test_a_status_that_disagrees_with_the_parent_fails(self, setup): + setup(trunks=[make_trunk(status="DOWN")]) + + result = runner.invoke(make_app(), ["trunk", "audit"]) + + assert result.exit_code == 1 + assert "expected ACTIVE for a bound parent, found DOWN" in result.output + + def test_an_unbound_parent_expects_a_down_trunk(self, setup): + setup( + trunks=[make_trunk(status="DOWN")], + ports=[make_parent(bound=False), make_subport()], + ) + + result = runner.invoke(make_app(), ["trunk", "audit"]) + + assert result.exit_code == 0, result.output + assert "trunk status: DOWN" in result.output + + def test_a_non_baremetal_trunk_is_skipped(self, setup): + setup(ports=[make_parent(vnic_type="normal"), make_subport()]) + + result = runner.invoke(make_app(), ["trunk", "audit"]) + + assert result.exit_code == 0, result.output + assert "so ml2/ovn still owns this trunk" in result.output + assert "(no baremetal trunks found)" in result.output + + def test_an_unclaimed_subport_fails(self, setup): + setup(ports=[make_parent(), make_subport(device_owner="")]) + + result = runner.invoke(make_app(), ["trunk", "audit"]) + + assert result.exit_code == 1 + assert "expected trunk:subport, found (empty)" in result.output + + def test_an_unknown_trunk_errors(self, setup): + setup() + + result = runner.invoke(make_app(), ["trunk", "audit", "nope"]) + + assert result.exit_code == 1 + assert "trunk 'nope' not found" in result.output + + +class TestRepair: + def test_a_dry_run_changes_nothing(self, setup): + conn, nb = setup( + ports=[make_parent(), make_subport(profile=dict(STALE_PROFILE))], + nb_rows={"sub-1": dict(STALE_LSP["sub-1"])}, + ) + + result = runner.invoke(make_app(), ["trunk", "repair"]) + + assert result.exit_code == 0, result.output + assert "binding profile: drop parent_name, tag" in result.output + assert "Dry run only" in result.output + assert conn.network.updates == [] + assert nb.calls == [] + + def test_apply_strips_the_profile_and_clears_the_lsp(self, setup): + conn, nb = setup( + ports=[ + make_parent(), + make_subport(profile={**STALE_PROFILE, "keep": "me"}), + ], + nb_rows={"sub-1": dict(STALE_LSP["sub-1"])}, + ) + + result = runner.invoke(make_app(), ["trunk", "repair", "--apply"]) + + assert result.exit_code == 0, result.output + assert conn.network.updates == [("sub-1", {"binding_profile": {"keep": "me"}})] + assert nb.rows == {"sub-1": {}} + assert "Repaired 1 subport(s); verification passed." in result.output + + def test_apply_reclaims_an_unclaimed_subport(self, setup): + conn, _ = setup(ports=[make_parent(), make_subport(device_owner="")]) + + result = runner.invoke(make_app(), ["trunk", "repair", "--apply"]) + + assert result.exit_code == 0, result.output + assert conn.network.updates == [ + ("sub-1", {"device_owner": trunk.SUBPORT_DEVICE_OWNER}) + ] + + def test_apply_clears_an_lsp_whose_profile_is_already_clean(self, setup): + conn, nb = setup(nb_rows={"sub-1": dict(STALE_LSP["sub-1"])}) + + result = runner.invoke(make_app(), ["trunk", "repair", "--apply"]) + + assert result.exit_code == 0, result.output + assert conn.network.updates == [] + assert nb.rows == {"sub-1": {}} + assert "cleared in the NB directly: sub-1: parent_name" in result.output + + def test_a_clean_trunk_needs_no_repair(self, setup): + conn, nb = setup() + + result = runner.invoke(make_app(), ["trunk", "repair", "--apply"]) + + assert result.exit_code == 0, result.output + assert "No leftover ml2/ovn trunk state found." in result.output + assert conn.network.updates == [] + assert nb.calls == [] + + def test_verification_failure_exits_nonzero(self, setup): + conn, _ = setup( + ports=[make_parent(), make_subport(profile=dict(STALE_PROFILE))] + ) + # A Neutron update that silently does not stick. + conn.network.reflect_updates = False + + result = runner.invoke(make_app(), ["trunk", "repair", "--apply"]) + + assert result.exit_code == 1 + assert "repair verification failed" in result.output + assert "sub-1: binding profile: drop parent_name, tag" in result.output diff --git a/python/kubectl-us/us_cli/cli.py b/python/kubectl-us/us_cli/cli.py index e3c8bcac1..b6f5d4e1b 100644 --- a/python/kubectl-us/us_cli/cli.py +++ b/python/kubectl-us/us_cli/cli.py @@ -7,6 +7,7 @@ from us_cli.commands import backup from us_cli.commands import raw from us_cli.commands import router +from us_cli.commands import trunk from us_cli.connection import ConnectionContext app = typer.Typer( @@ -58,6 +59,7 @@ def main( raw.register(net_app) net_app.add_typer(router.app, name="router") +net_app.add_typer(trunk.app, name="trunk") app.add_typer(net_app, name="net") backup.register(app) diff --git a/python/kubectl-us/us_cli/commands/trunk.py b/python/kubectl-us/us_cli/commands/trunk.py new file mode 100644 index 000000000..d432424c9 --- /dev/null +++ b/python/kubectl-us/us_cli/commands/trunk.py @@ -0,0 +1,372 @@ +"""Audit and clean up the trunk state ml2/ovn left behind on baremetal trunks. + +ml2/ovn used to manage any trunk whose parent port had a Logical_Switch_Port. +A baremetal parent has one -- type=external, purely so OVN can schedule an HA +chassis group -- while the VLANs are programmed on the leaf switch by undersync. +So it wrote parent_name/tag into every subport's binding profile and LSP, which +mean nothing outside OVN, and pointed parent_name at an LSP on a different +logical switch. LP #2167105 stops it managing these trunks, but the rows it +already wrote stay: the code that used to remove them on subport delete now +returns early. + +The subport's binding profile is the source of truth here, because ml2/ovn +derives the LSP's parent_name/tag from it (ovn_client._get_port_options). So +dropping those keys via Neutron makes ml2/ovn rewrite the LSP itself; the direct +NB clear is only for rows it did not catch. +""" + +from __future__ import annotations + +from dataclasses import dataclass +from typing import Any + +import typer + +from us_cli import osclient +from us_cli import ovn +from us_cli.connection import ConnectionContext +from us_cli.connection import print_connection_banner + +BAREMETAL_VNIC_TYPE = "baremetal" +BAREMETAL_VIF_TYPE = "other" +SUBPORT_DEVICE_OWNER = "trunk:subport" + +#: binding:profile keys ml2/ovn's trunk driver wrote on subports. +STALE_PROFILE_KEYS = ("parent_name", "tag") +#: The LSP columns those keys end up in. Neutron converts a `tag` write into +#: `tag_request` (commands._tag_column_to_tag_request); northd then derives the +#: read-only `tag` column, and drops it once parent_name/tag_request are gone. +STALE_LSP_COLUMNS = ("parent_name", "tag_request") + +app = typer.Typer( + no_args_is_help=True, + help="Audit and clean up baremetal trunk state left behind by ml2/ovn.", +) + + +@dataclass(frozen=True) +class Finding: + """One audit assertion.""" + + status: str + check: str + detail: str + + +@dataclass(frozen=True) +class SubportRepair: + """The cleanup for one subport: a Neutron port update, then an NB clear.""" + + port_id: str + port_updates: dict[str, Any] + clear_columns: tuple[str, ...] + changes: tuple[str, ...] + + +@dataclass +class TrunkState: + """A baremetal trunk with the Neutron and NB rows the audit compares.""" + + trunk: Any + parent: Any + subports: list[Any] + lsps: dict[str, dict] + + +def _value(resource: Any, key: str, default=None) -> Any: + """Read an SDK resource or dict without depending on its concrete type.""" + value = getattr(resource, key, None) + if value is not None: + return value + getter = getattr(resource, "get", None) + return getter(key, default) if getter else default + + +def _check( + findings: list[Finding], condition: bool, name: str, good: str, bad: str +) -> None: + findings.append( + Finding("PASS" if condition else "FAIL", name, good if condition else bad) + ) + + +def _label(trunk) -> str: + return f"{_value(trunk, 'name') or '(unnamed)'} ({_value(trunk, 'id')})" + + +def _parent_is_bound(parent) -> bool: + """The same predicate neutron-understack's trunk driver binds on.""" + profile = _value(parent, "binding_profile") or {} + return _value(parent, "binding_vif_type") == BAREMETAL_VIF_TYPE and bool( + profile.get("physical_network") + ) + + +def _lsp_rows(conn_ctx: ConnectionContext, port_ids: list[str]) -> dict[str, dict]: + rows = ovn.nbctl_list_records(conn_ctx, "Logical_Switch_Port", port_ids) + return {row["name"]: row for row in rows if row.get("name")} + + +def _stale_profile_keys(port) -> list[str]: + profile = _value(port, "binding_profile") or {} + return [key for key in STALE_PROFILE_KEYS if key in profile] + + +def _stale_lsp_columns(lsp: dict | None) -> tuple[str, ...]: + if lsp is None: + return () + return tuple(column for column in STALE_LSP_COLUMNS if ovn.as_list(lsp.get(column))) + + +def _resolve_trunks(conn, name_or_id: str | None) -> list[Any]: + if not name_or_id: + return list(conn.network.trunks()) + trunk = conn.network.find_trunk(name_or_id) + if trunk is None: + raise LookupError(f"trunk {name_or_id!r} not found") + return [trunk] + + +def _load_states( + conn, conn_ctx: ConnectionContext, trunks: list[Any] +) -> tuple[list[TrunkState], list[str]]: + """Load each trunk's parent, subports and NB rows, skipping ovn-owned trunks.""" + states: list[TrunkState] = [] + skipped: list[str] = [] + for trunk in trunks: + parent = conn.network.get_port(_value(trunk, "port_id")) + if _value(parent, "binding_vnic_type") != BAREMETAL_VNIC_TYPE: + skipped.append( + f"trunk {_label(trunk)}: parent port is not baremetal, " + "so ml2/ovn still owns this trunk" + ) + continue + subports = [ + conn.network.get_port(subport["port_id"]) + for subport in _value(trunk, "sub_ports") or [] + ] + port_ids = [_value(port, "id") for port in subports] + states.append( + TrunkState(trunk, parent, subports, _lsp_rows(conn_ctx, port_ids)) + ) + return states, skipped + + +def _audit_trunk(state: TrunkState) -> list[Finding]: + findings: list[Finding] = [] + bound = _parent_is_bound(state.parent) + expected_status = "ACTIVE" if bound else "DOWN" + status = _value(state.trunk, "status") + _check( + findings, + status == expected_status, + "trunk status", + expected_status, + f"expected {expected_status} for a " + f"{'bound' if bound else 'unbound'} parent, found {status}; a trunk " + "last touched before the status fix corrects itself on its next " + "subport change", + ) + for port in state.subports: + port_id = _value(port, "id") + label = f"subport {port_id}" + stale_keys = _stale_profile_keys(port) + _check( + findings, + not stale_keys, + f"{label}: binding profile", + "no ml2/ovn trunk keys", + f"stale ml2/ovn keys: {', '.join(stale_keys)}", + ) + lsp = state.lsps.get(port_id) + if lsp is None: + findings.append( + Finding("SKIP", f"{label}: LSP", "no Logical_Switch_Port in the NB") + ) + else: + stale_columns = _stale_lsp_columns(lsp) + _check( + findings, + not stale_columns, + f"{label}: LSP columns", + "parent_name and tag_request unset", + f"still set: {', '.join(stale_columns)}", + ) + owner = _value(port, "device_owner") + _check( + findings, + owner == SUBPORT_DEVICE_OWNER, + f"{label}: device_owner", + SUBPORT_DEVICE_OWNER, + f"expected {SUBPORT_DEVICE_OWNER}, found {owner or '(empty)'}", + ) + return findings + + +def _repair_operations(state: TrunkState) -> list[SubportRepair]: + operations: list[SubportRepair] = [] + for port in state.subports: + port_id = _value(port, "id") + updates: dict[str, Any] = {} + changes: list[str] = [] + + stale_keys = _stale_profile_keys(port) + if stale_keys: + profile = _value(port, "binding_profile") or {} + updates["binding_profile"] = { + key: value + for key, value in profile.items() + if key not in STALE_PROFILE_KEYS + } + changes.append(f"binding profile: drop {', '.join(stale_keys)}") + owner = _value(port, "device_owner") + if owner != SUBPORT_DEVICE_OWNER: + updates["device_owner"] = SUBPORT_DEVICE_OWNER + changes.append( + f"device_owner: {owner or '(empty)'} -> {SUBPORT_DEVICE_OWNER}" + ) + clear_columns = _stale_lsp_columns(state.lsps.get(port_id)) + if clear_columns: + changes.append(f"LSP: clear {', '.join(clear_columns)}") + + if updates or clear_columns: + operations.append( + SubportRepair(port_id, updates, clear_columns, tuple(changes)) + ) + return operations + + +def _clear_stale_lsp_columns( + conn_ctx: ConnectionContext, port_ids: list[str] +) -> list[str]: + """Clear whatever ml2/ovn's own rewrite left behind. Returns what was cleared.""" + args: list[str] = [] + cleared: list[str] = [] + for port_id, row in _lsp_rows(conn_ctx, port_ids).items(): + for column in _stale_lsp_columns(row): + args += ["--", "clear", "Logical_Switch_Port", port_id, column] + cleared.append(f"{port_id}: {column}") + if args: + ovn.nbctl_raw(conn_ctx, args) + return cleared + + +@app.command("audit") +def audit( + ctx: typer.Context, + name_or_id: str = typer.Argument( + None, help="Neutron trunk name or ID (default: every baremetal trunk)" + ), +) -> None: + """Report ml2/ovn trunk state left on baremetal trunks and their subports.""" + conn_ctx: ConnectionContext = ctx.obj + print_connection_banner(conn_ctx, include_openstack=True) + try: + conn = osclient.get_connection(conn_ctx.os_cloud) + states, skipped = _load_states( + conn, conn_ctx, _resolve_trunks(conn, name_or_id) + ) + except Exception as exc: + typer.echo(f"ERROR: {exc}", err=True) + raise typer.Exit(1) from exc + + print() + for reason in skipped: + print(f"SKIP {reason}") + if not states: + print("(no baremetal trunks found)") + return + + failed = False + for state in states: + print(f"\nTrunk {_label(state.trunk)}") + parent_state = "bound" if _parent_is_bound(state.parent) else "unbound" + print(f"Parent port: {_value(state.parent, 'id')} (baremetal, {parent_state})") + findings = _audit_trunk(state) + for finding in findings: + print(f" {finding.status:<4} {finding.check}: {finding.detail}") + failed = failed or any(finding.status == "FAIL" for finding in findings) + + if failed: + raise typer.Exit(1) + + +@app.command("repair") +def repair( + ctx: typer.Context, + name_or_id: str = typer.Argument( + None, help="Neutron trunk name or ID (default: every baremetal trunk)" + ), + apply: bool = typer.Option( + False, + "--apply", + help="Apply the displayed plan; without this flag no changes are made", + ), +) -> None: + """Strip ml2/ovn's leftover parent_name/tag from baremetal trunk subports.""" + conn_ctx: ConnectionContext = ctx.obj + print_connection_banner(conn_ctx, include_openstack=True) + try: + conn = osclient.get_connection(conn_ctx.os_cloud) + trunks = _resolve_trunks(conn, name_or_id) + states, skipped = _load_states(conn, conn_ctx, trunks) + except Exception as exc: + typer.echo(f"ERROR: {exc}", err=True) + raise typer.Exit(1) from exc + + print() + for reason in skipped: + print(f"SKIP {reason}") + print( + "Repair scope: subport binding profiles, their trunk device_owner, and " + "the LSP columns derived from them" + ) + plan = [(state, _repair_operations(state)) for state in states] + for state, operations in plan: + if not operations: + continue + print(f"\nTrunk {_label(state.trunk)}") + for operation in operations: + print(f" subport {operation.port_id}") + for change in operation.changes: + print(f" {change}") + + total = sum(len(operations) for _, operations in plan) + if not total: + print("\nNo leftover ml2/ovn trunk state found.") + return + if not apply: + print("\nDry run only. Re-run with --apply to make these changes.") + return + + try: + for _, operations in plan: + for operation in operations: + if operation.port_updates: + conn.network.update_port( + operation.port_id, **operation.port_updates + ) + # ml2/ovn rewrites the LSP from the binding profile we just changed, so + # only clear directly what it left behind. + port_ids = [ + operation.port_id for _, operations in plan for operation in operations + ] + cleared = _clear_stale_lsp_columns(conn_ctx, port_ids) + refreshed, _ = _load_states(conn, conn_ctx, trunks) + remaining = [ + operation for state in refreshed for operation in _repair_operations(state) + ] + except Exception as exc: + typer.echo(f"ERROR: {exc}", err=True) + raise typer.Exit(1) from exc + + for entry in cleared: + print(f" cleared in the NB directly: {entry}") + if remaining: + typer.echo("ERROR: repair verification failed", err=True) + for operation in remaining: + typer.echo( + f" {operation.port_id}: {', '.join(operation.changes)}", err=True + ) + raise typer.Exit(1) + print(f"\nRepaired {total} subport(s); verification passed.")