diff --git a/homeassistant/components/proxmoxve/__init__.py b/homeassistant/components/proxmoxve/__init__.py index 80a781757963..1f39c5d0a6d1 100644 --- a/homeassistant/components/proxmoxve/__init__.py +++ b/homeassistant/components/proxmoxve/__init__.py @@ -14,8 +14,7 @@ from .const import ( CONF_REALM, DEFAULT_REALM, ) -from .coordinator import ProxmoxConfigEntry, ProxmoxCoordinator -from .entity import node_device_info +from .coordinator import ProxmoxConfigEntry, ProxmoxCoordinator, node_device_info PLATFORMS = [ Platform.BINARY_SENSOR, diff --git a/homeassistant/components/proxmoxve/coordinator.py b/homeassistant/components/proxmoxve/coordinator.py index 09b04b21d7a1..d3f47e2d9543 100644 --- a/homeassistant/components/proxmoxve/coordinator.py +++ b/homeassistant/components/proxmoxve/coordinator.py @@ -10,6 +10,7 @@ from proxmoxer import AuthenticationError, ProxmoxAPI from proxmoxer.core import ResourceException import requests from requests.exceptions import ConnectTimeout, SSLError +from yarl import URL from homeassistant.config_entries import ConfigEntry from homeassistant.const import ( @@ -64,6 +65,32 @@ class ProxmoxNodeData: backups: list[dict[str, Any]] = field(default_factory=list) +def proxmox_base_url(coordinator: ProxmoxCoordinator) -> URL: + """Return the base URL for the Proxmox VE.""" + data = coordinator.config_entry.data + return URL.build( + scheme="https", + host=data[CONF_HOST], + port=data[CONF_PORT], + ) + + +def node_device_info( + coordinator: ProxmoxCoordinator, node_data: ProxmoxNodeData +) -> dr.DeviceInfo: + """Return the device info for a Proxmox VE node device.""" + return dr.DeviceInfo( + identifiers={ + (DOMAIN, f"{coordinator.config_entry.entry_id}_node_{node_data.node['id']}") + }, + name=node_data.node.get("node", str(node_data.node["id"])), + model="Node", + configuration_url=proxmox_base_url(coordinator).with_fragment( + f"v1:0:=node/{node_data.node['node']}" + ), + ) + + class ProxmoxCoordinator(DataUpdateCoordinator[dict[str, ProxmoxNodeData]]): """Data Update Coordinator for Proxmox VE integration.""" @@ -268,6 +295,12 @@ class ProxmoxCoordinator(DataUpdateCoordinator[dict[str, ProxmoxNodeData]]): _LOGGER.debug("New nodes found: %s", new_nodes) self.known_nodes.update(new_nodes) new_node_data = [data[node_name] for node_name in new_nodes] + device_registry = dr.async_get(self.hass) + for node_data in new_node_data: + device_registry.async_get_or_create( + config_entry_id=self.config_entry.entry_id, + **node_device_info(self, node_data), + ) for nodes_callback in self.new_nodes_callbacks: nodes_callback(new_node_data) diff --git a/homeassistant/components/proxmoxve/entity.py b/homeassistant/components/proxmoxve/entity.py index d75cf36af002..ec852040e305 100644 --- a/homeassistant/components/proxmoxve/entity.py +++ b/homeassistant/components/proxmoxve/entity.py @@ -2,41 +2,18 @@ from typing import Any, override -from yarl import URL - -from homeassistant.const import CONF_HOST, CONF_PORT from homeassistant.helpers import device_registry as dr from homeassistant.helpers.device_registry import DeviceInfo from homeassistant.helpers.entity import EntityDescription from homeassistant.helpers.update_coordinator import CoordinatorEntity from .const import DOMAIN -from .coordinator import ProxmoxCoordinator, ProxmoxNodeData - - -def _proxmox_base_url(coordinator: ProxmoxCoordinator) -> URL: - """Return the base URL for the Proxmox VE.""" - data = coordinator.config_entry.data - return URL.build( - scheme="https", - host=data[CONF_HOST], - port=data[CONF_PORT], - ) - - -def node_device_info( - coordinator: ProxmoxCoordinator, node_data: ProxmoxNodeData -) -> DeviceInfo: - """Return the device info for a Proxmox VE node device.""" - device_id = node_data.node["id"] - return DeviceInfo( - identifiers={(DOMAIN, f"{coordinator.config_entry.entry_id}_node_{device_id}")}, - name=node_data.node.get("node", str(device_id)), - model="Node", - configuration_url=_proxmox_base_url(coordinator).with_fragment( - f"v1:0:=node/{node_data.node['node']}" - ), - ) +from .coordinator import ( + ProxmoxCoordinator, + ProxmoxNodeData, + node_device_info, + proxmox_base_url, +) class ProxmoxCoordinatorEntity(CoordinatorEntity[ProxmoxCoordinator]): @@ -102,7 +79,7 @@ class ProxmoxStorageEntity(ProxmoxCoordinatorEntity): }, name=f"Storage ({self.device_name})", model="Storage", - configuration_url=_proxmox_base_url(coordinator).with_fragment( + configuration_url=proxmox_base_url(coordinator).with_fragment( f"v1:0:=storage/{self._node_name}/{storage_data['storage']}" ), via_device_id=dr.async_get_device_id_by_identifier( @@ -161,7 +138,7 @@ class ProxmoxVMEntity(ProxmoxCoordinatorEntity): }, name=self.device_name, model="VM", - configuration_url=_proxmox_base_url(coordinator).with_fragment( + configuration_url=proxmox_base_url(coordinator).with_fragment( f"v1:0:=qemu/{vm_data['vmid']}" ), via_device_id=dr.async_get_device_id_by_identifier( @@ -222,7 +199,7 @@ class ProxmoxContainerEntity(ProxmoxCoordinatorEntity): }, name=self.device_name, model="Container", - configuration_url=_proxmox_base_url(coordinator).with_fragment( + configuration_url=proxmox_base_url(coordinator).with_fragment( f"v1:0:=lxc/{container_data['vmid']}" ), via_device_id=dr.async_get_device_id_by_identifier( diff --git a/tests/components/proxmoxve/test_init.py b/tests/components/proxmoxve/test_init.py index 1193bfaf9837..5f3b49e575a7 100644 --- a/tests/components/proxmoxve/test_init.py +++ b/tests/components/proxmoxve/test_init.py @@ -1,5 +1,6 @@ """Tests for the Proxmox VE integration initialization.""" +from typing import Any from unittest.mock import MagicMock from proxmoxer import AuthenticationError @@ -16,6 +17,8 @@ from homeassistant.components.proxmoxve.const import ( DOMAIN, ) from homeassistant.components.proxmoxve.coordinator import ( + ProxmoxCoordinator, + ProxmoxNodeData, ProxmoxNodesNotFoundError, ProxmoxPermissionsError, ) @@ -404,9 +407,9 @@ async def test_new_container_creates_entity( "child_identifier", ["vm_100", "vm_101", "container_200", "container_201", "storage_local"], ) +@pytest.mark.usefixtures("mock_proxmox_client") async def test_child_devices_link_to_node( hass: HomeAssistant, - mock_proxmox_client: MagicMock, mock_config_entry: MockConfigEntry, device_registry: dr.DeviceRegistry, child_identifier: str, @@ -416,18 +419,65 @@ async def test_child_devices_link_to_node( assert mock_config_entry.state is ConfigEntryState.LOADED entry_id = mock_config_entry.entry_id - node_device = device_registry.async_get_device( - identifiers={(DOMAIN, f"{entry_id}_node_node/pve1")} + node_device = device_registry.async_get_device_by_identifier( + (DOMAIN, f"{entry_id}_node_node/pve1"), entry_id ) assert node_device is not None - child_device = device_registry.async_get_device( - identifiers={(DOMAIN, f"{entry_id}_{child_identifier}")} + child_device = device_registry.async_get_device_by_identifier( + (DOMAIN, f"{entry_id}_{child_identifier}"), entry_id ) assert child_device is not None assert child_device.via_device_id == node_device.id +async def test_new_node_device_registered_before_resource_callbacks( + hass: HomeAssistant, + mock_config_entry: MockConfigEntry, + device_registry: dr.DeviceRegistry, +) -> None: + """Test a newly discovered node's device exists before new VM/container/storage callbacks run. + + Regression test for a race where a newly discovered node's VM/container/ + storage entities could be built before the node's own device was + registered, causing via_device_id resolution to raise ValueError. + """ + mock_config_entry.add_to_hass(hass) + coordinator = ProxmoxCoordinator(hass, mock_config_entry) + + entry_id = mock_config_entry.entry_id + resolved_via_device_ids: list[str] = [] + + def _resolve_via_device_on_new_vms( + vms: list[tuple[ProxmoxNodeData, dict[str, Any]]], + ) -> None: + """Mimic what ProxmoxVMEntity.__init__ does when a VM is discovered.""" + for node_data, _vm in vms: + resolved_via_device_ids.append( + dr.async_get_device_id_by_identifier( + hass, + (DOMAIN, f"{entry_id}_node_{node_data.node['id']}"), + config_entry_id=entry_id, + ) + ) + + coordinator.new_vms_callbacks.append(_resolve_via_device_on_new_vms) + + node_data = ProxmoxNodeData( + node={"id": "node/pve2", "node": "pve2"}, + vms={300: {"vmid": 300, "name": "vm-pve2"}}, + ) + coordinator._async_add_remove_nodes({"pve2": node_data}) + + assert resolved_via_device_ids + + node_device = device_registry.async_get_device_by_identifier( + (DOMAIN, f"{entry_id}_node_node/pve2"), entry_id + ) + assert node_device is not None + assert resolved_via_device_ids == [node_device.id] + + async def test_stale_devices_removed( hass: HomeAssistant, mock_proxmox_client: MagicMock,