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.") 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(