diff --git a/backend/app/api/routes/proxmox.py b/backend/app/api/routes/proxmox.py index 078f914..fab786a 100644 --- a/backend/app/api/routes/proxmox.py +++ b/backend/app/api/routes/proxmox.py @@ -339,15 +339,22 @@ async def _find_pending( 1. The synthetic ``ieee_address`` (``pve-{host}-{vmid}``). Unique per guest and the only key that identifies one. - 2. Else MAC, else IP — merging into a row an IP/ARP scan found first. + 2. Else the NIC MAC, else the IP — merging into a row an IP/ARP scan found + first, or into this same guest under its previous host name. An IP is not an identity: a duplicated static lease, a re-used DHCP address, two guests behind one NAT, or a container-bridge address several guests report to the agent all make one address describe several machines. So the - fallback skips any row already claimed by a *different* Proxmox guest — - matching one used to overwrite that guest's hostname, VMID and specs, and + IP fallback skips any row already claimed by a Proxmox guest — matching one + used to overwrite that guest's hostname, VMID and specs, and non-deterministically, since one flat OR with no ordering returned whichever row the database yielded first. Oldest row wins, so a re-import is stable. + + A MAC *is* an identity, so its fallback matches a claimed row too. It has to: + the synthetic ieee embeds the host name, and a live migration or a node + rename changes it for what is the same guest (``pve-pve1-802`` → + ``pve-pve2-802``). Excluding claimed rows there would file the migrated + guest as a new device and orphan its old row. See ``_adopted_ieee``. """ exact = ( await db.execute( @@ -361,13 +368,17 @@ async def _find_pending( InventoryDevice.ieee_address.is_(None), ~InventoryDevice.ieee_address.startswith(_PVE_IEEE_PREFIX), ) - for column, value in ((InventoryDevice.mac, mac), (InventoryDevice.ip, ip)): + for column, value, claimed_ok in ( + (InventoryDevice.mac, mac, True), + (InventoryDevice.ip, ip, False), + ): if not value: continue + where = [column == value] if claimed_ok else [column == value, unclaimed] row = ( await db.execute( select(InventoryDevice) - .where(column == value, unclaimed) + .where(*where) .order_by(InventoryDevice.discovered_at, InventoryDevice.id) ) ).scalars().first() @@ -376,6 +387,20 @@ async def _find_pending( return None +def _adopted_ieee(current: str | None, ieee: str) -> str: + """The ``ieee_address`` a row should carry once this guest merges into it. + + Re-point a row that already belongs to a Proxmox guest: matched by NIC MAC, + it is this same guest under a stale host name (migrated, or the node was + renamed). Links are rebuilt keyed on the ieee, so a row left on the old one + stops resolving as a host→guest endpoint. A mesh ieee is a real hardware + address and is never overwritten. + """ + if not current or current.startswith(_PVE_IEEE_PREFIX): + return ieee + return current + + def _new_pending( ieee: str, ip: str | None, @@ -432,7 +457,7 @@ def _refresh_pending( ) -> None: # Compute sources before adopting the pve ieee (needs the pre-merge origin). pending.discovery_sources = _sources_after_merge(pending) - pending.ieee_address = pending.ieee_address or ieee + pending.ieee_address = _adopted_ieee(pending.ieee_address, ieee) pending.ip = ip or pending.ip pending.mac = pending.mac or mac pending.hostname = n.get("hostname") or pending.hostname @@ -464,7 +489,7 @@ async def _ensure_inventory_row( else: # Compute sources before adopting the pve ieee (needs the pre-merge origin). inv.discovery_sources = _sources_after_merge(inv) - inv.ieee_address = inv.ieee_address or ieee + inv.ieee_address = _adopted_ieee(inv.ieee_address, ieee) inv.ip = ip or inv.ip inv.mac = inv.mac or mac inv.hostname = n.get("hostname") or inv.hostname diff --git a/backend/tests/test_proxmox_router.py b/backend/tests/test_proxmox_router.py index c3eafd3..465f76f 100644 --- a/backend/tests/test_proxmox_router.py +++ b/backend/tests/test_proxmox_router.py @@ -40,14 +40,17 @@ def _host_node() -> dict: } -def _guest_node(vmid: int, ip: str | None, status: str = "online", mac: str | None = None) -> dict: +def _guest_node( + vmid: int, ip: str | None, status: str = "online", mac: str | None = None, + host: str = "pve1", +) -> dict: return { - "id": f"pve-pve1-{vmid}", "label": f"vm{vmid}", "type": "vm", - "ieee_address": f"pve-pve1-{vmid}", "hostname": f"vm{vmid}", "ip": ip, + "id": f"pve-{host}-{vmid}", "label": f"vm{vmid}", "type": "vm", + "ieee_address": f"pve-{host}-{vmid}", "hostname": f"vm{vmid}", "ip": ip, "mac": mac, "status": status, "cpu_count": 2, "ram_gb": 4.0, "disk_gb": 32.0, "vendor": "Proxmox VE", "model": "QEMU", "vmid": vmid, - "parent_ieee": "pve-node-pve1", + "parent_ieee": f"pve-node-{host}", } @@ -341,6 +344,67 @@ async def test_persist_merges_the_oldest_scan_row_when_two_share_an_ip(db_sessio assert merged.mac == "aa:bb:cc:00:00:02" # the older row +@pytest.mark.asyncio +async def test_persist_merges_a_guest_that_moved_to_another_host(db_session) -> None: + # The synthetic ieee embeds the host name, so a live migration or a node + # rename changes it for what is the same guest. The NIC MAC survives both: + # merge and re-point the row, rather than filing a new device and orphaning + # the old row (which would then never resolve as a host→guest link end). + await _persist_pending_import( + db_session, [_guest_node(802, "10.0.0.5", mac="bc:24:11:aa:bb:cc")], [] + ) + row = (await db_session.execute(select(InventoryDevice))).scalars().one() + row.status = "approved" + await db_session.commit() + + await _persist_pending_import( + db_session, + [_guest_node(802, "10.0.0.5", mac="bc:24:11:aa:bb:cc", host="pve2")], + [], + ) + + rows = (await db_session.execute(select(InventoryDevice))).scalars().all() + assert len(rows) == 1 # no duplicate, no orphan + assert rows[0].ieee_address == "pve-pve2-802" # re-pointed to the new host + + +@pytest.mark.asyncio +async def test_persist_never_repoints_a_mesh_ieee(db_session) -> None: + # A zigbee/zwave ieee is a real hardware address. A Proxmox import that + # merges into such a row by MAC must not overwrite it. + db_session.add(InventoryDevice( + id=str(uuid.uuid4()), ieee_address="0x00124b0022a1b2c3", + mac="bc:24:11:aa:bb:cc", suggested_type="zigbee_device", status="pending", + discovery_source="zigbee", discovery_sources=["zigbee"], + )) + await db_session.commit() + + await _persist_pending_import( + db_session, [_guest_node(101, None, mac="bc:24:11:aa:bb:cc")], [] + ) + + rows = (await db_session.execute(select(InventoryDevice))).scalars().all() + assert len(rows) == 1 + assert rows[0].ieee_address == "0x00124b0022a1b2c3" + + +@pytest.mark.asyncio +async def test_persist_merges_two_guests_that_share_a_nic_mac(db_session) -> None: + # Accepted trade-off of matching on MAC: two guests configured with the same + # NIC MAC collapse into one row. That is a misconfiguration which already + # breaks their networking, and the alternative — excluding claimed rows from + # the MAC fallback — orphans every migrated guest. Shared *IPs* stay split; + # see test_persist_never_overwrites_another_guest_row_on_a_shared_ip. + nodes = [ + _guest_node(802, "10.0.0.5", mac="bc:24:11:aa:bb:cc"), + _guest_node(812, "10.0.0.6", mac="bc:24:11:aa:bb:cc"), + ] + await _persist_pending_import(db_session, nodes, []) + + rows = (await db_session.execute(select(InventoryDevice))).scalars().all() + assert len(rows) == 1 + + @pytest.mark.asyncio async def test_persist_preserves_ip_tag_for_legacy_null_source_row(db_session) -> None: # Legacy inventory row from an old IP scan, before discovery_source(s) were