diff --git a/napalm_proxmox/vm_provision_mixin.py b/napalm_proxmox/vm_provision_mixin.py index 3bed3ab..e5fb2f5 100644 --- a/napalm_proxmox/vm_provision_mixin.py +++ b/napalm_proxmox/vm_provision_mixin.py @@ -8,7 +8,12 @@ import yaml from typing import Any, Dict, List from urllib.parse import quote -from napalm_device_types.models import NetworkTargetDict, VMProvisionResultDict, VMStatusDict +from napalm_device_types.models import ( + NetworkTargetDict, + StorageTargetDict, + VMProvisionResultDict, + VMStatusDict, +) _logger = logging.getLogger(__name__) @@ -107,8 +112,16 @@ class ProxmoxVMProvisionMixin: Proxmox's /storage API omits the "enabled" field entirely for storages that were never explicitly toggled — it is not present-and-falsy, it is just absent, defaulting to enabled. Only an explicit 0 means disabled. + + Queries the node-scoped /nodes/{node}/storage endpoint, not the + cluster-wide /storage one: a storage can be configured with a "nodes" + restriction limiting it to other cluster members, and the cluster-wide + list doesn't reflect that — it would happily return a storage this + node can't actually see, and "qm importdisk" would fail with + "storage 'X' is not available on node 'Y'" after the VM shell was + already created. """ - for storage in self._api.storage.get(): + for storage in self._node_api().storage.get(): content = storage.get("content", "") if "images" in content and storage.get("enabled", 1) != 0: return storage["storage"] @@ -116,6 +129,27 @@ class ProxmoxVMProvisionMixin: "No storage with content='images' found. Configure a storage for VM disks." ) + def get_image_storages(self) -> List[StorageTargetDict]: + """List node-available storage pools suitable for a new VM's root disk.""" + targets: List[StorageTargetDict] = [] + for storage in self._node_api().storage.get(): + content = storage.get("content", "") + if "images" not in content or storage.get("enabled", 1) == 0: + continue + if storage.get("active", 1) == 0: + continue + total = storage.get("total") or 0 + avail = storage.get("avail") or 0 + targets.append( + { + "name": storage["storage"], + "type": storage.get("type", ""), + "total_gb": round(total / (1024**3), 1), + "available_gb": round(avail / (1024**3), 1), + } + ) + return targets + def _wait_for_task(self, upid: str, timeout: int = 120) -> None: """ Poll a Proxmox task until completion. @@ -159,6 +193,7 @@ class ProxmoxVMProvisionMixin: image_checksum: str | None = None, ssh_public_keys: List[str] | None = None, disk_resize_gb: int | None = None, + storage: str | None = None, download_timeout: int = 300, timeout: int = 180, ) -> VMProvisionResultDict: @@ -189,6 +224,8 @@ class ProxmoxVMProvisionMixin: after download (None = no verification) ssh_public_keys: SSH public keys to inject disk_resize_gb: resize root disk to this size (None = no resize) + storage: storage pool for the root disk (None = auto-detect first + enabled, node-available storage with content='images') download_timeout: max seconds for the image download (skipped if cached) timeout: max seconds for the remaining provisioning steps @@ -222,7 +259,7 @@ class ProxmoxVMProvisionMixin: local_path = self._download_cloud_image( image_url, image_checksum, timeout=download_timeout ) - image_storage = self._find_default_image_storage() + image_storage = storage or self._find_default_image_storage() _logger.info(f"Importing {local_path} into VM {vmid} on storage {image_storage}") self._run_node_command( @@ -271,9 +308,10 @@ class ProxmoxVMProvisionMixin: self._node_api().qemu(vmid).config.post(**config_args) # Step 5: Verify snippet storage exists - # (enabled is absent-not-falsy on Proxmox — see _find_default_image_storage) + # (enabled is absent-not-falsy, and node-scoping matters — see + # _find_default_image_storage) _logger.info("Checking for snippet storage...") - storages = self._api.storage.get() + storages = self._node_api().storage.get() snippet_storage = None for storage in storages: content = storage.get("content", "") diff --git a/tests/test_vm_provision_mixin.py b/tests/test_vm_provision_mixin.py index 7da81b5..106c77c 100644 --- a/tests/test_vm_provision_mixin.py +++ b/tests/test_vm_provision_mixin.py @@ -74,12 +74,12 @@ def test_create_vm_from_cloud_init_single_nic(): # Mock API hierarchy mock_api = MagicMock() mock_api.cluster.nextid.get.return_value = 101 - mock_api.storage.get.return_value = [ + + mock_node = MagicMock() + mock_node.storage.get.return_value = [ {"storage": "local-lvm", "type": "lvmthin", "content": "images,rootdir", "enabled": 1}, {"storage": "snippets", "type": "dir", "content": "snippets", "enabled": 1}, ] - - mock_node = MagicMock() mixin._api = mock_api mixin._node_api = MagicMock(return_value=mock_node) mixin._download_cloud_image = MagicMock(return_value="/var/lib/vz/template/netork-images/debian-12.qcow2") @@ -139,12 +139,12 @@ def test_create_vm_from_cloud_init_dual_nic_trunk(): # Mock API hierarchy mock_api = MagicMock() mock_api.cluster.nextid.get.return_value = 102 - mock_api.storage.get.return_value = [ + + mock_node = MagicMock() + mock_node.storage.get.return_value = [ {"storage": "local-lvm", "type": "lvmthin", "content": "images,rootdir", "enabled": 1}, {"storage": "snippets", "type": "dir", "content": "snippets", "enabled": 1}, ] - - mock_node = MagicMock() mixin._api = mock_api mixin._node_api = MagicMock(return_value=mock_node) mixin._download_cloud_image = MagicMock(return_value="/var/lib/vz/template/netork-images/debian-12.qcow2") @@ -213,13 +213,13 @@ def test_create_vm_missing_snippet_storage(): # Mock API with images storage but no snippet storage mock_api = MagicMock() mock_api.cluster.nextid.get.return_value = 101 - mock_api.storage.get.return_value = [ - {"storage": "local-lvm", "type": "lvmthin", "content": "images,rootdir", "enabled": 1} - ] mixin._api = mock_api mock_node = MagicMock() + mock_node.storage.get.return_value = [ + {"storage": "local-lvm", "type": "lvmthin", "content": "images,rootdir", "enabled": 1} + ] mixin._node_api = MagicMock(return_value=mock_node) mixin._download_cloud_image = MagicMock(return_value="/var/lib/vz/template/netork-images/debian-12.qcow2") mixin._run_node_command = MagicMock(return_value="") @@ -254,12 +254,12 @@ def test_create_vm_with_disk_resize(): # Mock API hierarchy mock_api = MagicMock() mock_api.cluster.nextid.get.return_value = 103 - mock_api.storage.get.return_value = [ + + mock_node = MagicMock() + mock_node.storage.get.return_value = [ {"storage": "local-lvm", "type": "lvmthin", "content": "images,rootdir", "enabled": 1}, {"storage": "snippets", "type": "dir", "content": "snippets", "enabled": 1}, ] - - mock_node = MagicMock() mixin._api = mock_api mixin._node_api = MagicMock(return_value=mock_node) mixin._download_cloud_image = MagicMock(return_value="/var/lib/vz/template/netork-images/debian-12.qcow2") @@ -652,10 +652,11 @@ def test_find_default_image_storage_treats_absent_enabled_as_enabled(): treated as falsy, causing every real Proxmox server to report no usable image storage even when several existed.""" mixin = ProxmoxVMProvisionMixin() - mixin._api = MagicMock() - mixin._api.storage.get.return_value = [ + mock_node = MagicMock() + mock_node.storage.get.return_value = [ {"storage": "local-lvm", "type": "lvmthin", "content": "images,rootdir"}, ] + mixin._node_api = MagicMock(return_value=mock_node) storage = mixin._find_default_image_storage() @@ -664,11 +665,12 @@ def test_find_default_image_storage_treats_absent_enabled_as_enabled(): def test_find_default_image_storage_excludes_explicitly_disabled(): mixin = ProxmoxVMProvisionMixin() - mixin._api = MagicMock() - mixin._api.storage.get.return_value = [ + mock_node = MagicMock() + mock_node.storage.get.return_value = [ {"storage": "old-storage", "type": "dir", "content": "images", "enabled": 0}, {"storage": "local-lvm", "type": "lvmthin", "content": "images,rootdir", "enabled": 1}, ] + mixin._node_api = MagicMock(return_value=mock_node) storage = mixin._find_default_image_storage() @@ -677,15 +679,38 @@ def test_find_default_image_storage_excludes_explicitly_disabled(): def test_find_default_image_storage_raises_when_none_found(): mixin = ProxmoxVMProvisionMixin() - mixin._api = MagicMock() - mixin._api.storage.get.return_value = [ + mock_node = MagicMock() + mock_node.storage.get.return_value = [ {"storage": "local", "type": "dir", "content": "backup,iso,vztmpl"}, ] + mixin._node_api = MagicMock(return_value=mock_node) with pytest.raises(ValueError, match="images"): mixin._find_default_image_storage() +def test_find_default_image_storage_excludes_storage_restricted_to_other_nodes(): + """Regression: a storage configured cluster-wide but restricted via 'nodes' + to other cluster members must not be picked — /nodes/{node}/storage (unlike + the cluster-wide /storage endpoint) only lists what's actually available on + this node, so querying it is what makes this exclusion happen naturally. + This was the real-world bug: local-lvm (nodes=pve-garden) was picked for a + VM on pve-02, and 'qm importdisk' failed with 'storage not available on + node', leaving a VM shell with no disk attached.""" + mixin = ProxmoxVMProvisionMixin() + mock_node = MagicMock() + # /nodes/pve-02/storage never even lists local-lvm — Proxmox itself filters + # node-restricted storages out of this endpoint. + mock_node.storage.get.return_value = [ + {"storage": "local-zfs", "type": "zfspool", "content": "images,rootdir"}, + ] + mixin._node_api = MagicMock(return_value=mock_node) + + storage = mixin._find_default_image_storage() + + assert storage == "local-zfs" + + def test_create_vm_from_cloud_init_with_real_world_storage_shape(): """Regression: real Proxmox servers omit "enabled" for never-toggled storages (observed live: local-lvm/local-zfs/fast-zfs all had content=images but no @@ -697,14 +722,14 @@ def test_create_vm_from_cloud_init_with_real_world_storage_shape(): mock_api = MagicMock() mock_api.cluster.nextid.get.return_value = 104 - mock_api.storage.get.return_value = [ + + mock_node = MagicMock() + mock_node.storage.get.return_value = [ {"storage": "local-lvm", "type": "lvmthin", "content": "images,rootdir"}, {"storage": "local-zfs", "type": "zfspool", "content": "rootdir,images"}, {"storage": "local", "type": "dir", "content": "backup,iso,vztmpl"}, {"storage": "snippets", "type": "dir", "content": "snippets"}, ] - - mock_node = MagicMock() mixin._api = mock_api mixin._node_api = MagicMock(return_value=mock_node) mixin._download_cloud_image = MagicMock(return_value="/var/lib/vz/template/netork-images/debian-12.qcow2") @@ -739,3 +764,122 @@ def test_create_vm_from_cloud_init_with_real_world_storage_shape(): ) assert result["vmid"] == "104" + + +def test_create_vm_from_cloud_init_explicit_storage_skips_auto_detect(): + """When storage= is given explicitly, it's used directly and + _find_default_image_storage is never consulted (so an explicit choice + always wins, even if auto-detect would have picked something else).""" + mixin = ProxmoxVMProvisionMixin() + mixin._node_name = "pve1" + + mock_api = MagicMock() + mock_api.cluster.nextid.get.return_value = 105 + + mock_node = MagicMock() + mock_node.storage.get.return_value = [ + {"storage": "local-lvm", "type": "lvmthin", "content": "images,rootdir", "enabled": 1}, + {"storage": "fast-zfs", "type": "zfspool", "content": "images,rootdir", "enabled": 1}, + {"storage": "snippets", "type": "dir", "content": "snippets", "enabled": 1}, + ] + mixin._api = mock_api + mixin._node_api = MagicMock(return_value=mock_node) + mixin._download_cloud_image = MagicMock(return_value="/var/lib/vz/template/netork-images/debian-12.qcow2") + mixin._find_default_image_storage = MagicMock(side_effect=AssertionError("should not be called")) + mixin._run_node_command = MagicMock(return_value="") + + mock_vm = MagicMock() + mock_node.qemu.return_value = mock_vm + mock_node.qemu.post.return_value = None + mock_vm.config.post.return_value = None + mock_vm.config.get.return_value = { + "unused0": "fast-zfs:vm-105-disk-0", + "scsi0": "fast-zfs:vm-105-disk-0", + } + mock_vm.status.start.post.return_value = "UPID:pve1:131:start" + + mock_task = MagicMock() + mock_task.status.get.return_value = {"status": "stopped", "exitstatus": "OK"} + mock_node.tasks.return_value = mock_task + + mock_storage = MagicMock() + mock_storage.upload.post.return_value = {"filename": "snippets:snippets/105-user-data.yaml"} + mock_node.storage.return_value = mock_storage + + with patch("time.sleep"): + result = mixin.create_vm_from_cloud_init( + name="explicit-storage-vm", + image_url="https://cloud.debian.org/images/cloud/bookworm/latest/debian-12-genericcloud-amd64.qcow2", + cpu=2, + memory=2048, + nics=[{"bridge": "vmbr0"}], + cloud_init_config={"hostname": "explicit-storage-vm"}, + storage="fast-zfs", + ) + + assert result["vmid"] == "105" + mixin._find_default_image_storage.assert_not_called() + import_cmd = next( + c[0][0] for c in mixin._run_node_command.call_args_list if "qm importdisk" in c[0][0] + ) + assert "fast-zfs" in import_cmd + assert "local-lvm" not in import_cmd + + +# --------------------------------------------------------------------------- +# get_image_storages +# --------------------------------------------------------------------------- + + +def test_get_image_storages_filters_to_images_content(): + mixin = ProxmoxVMProvisionMixin() + mock_node = MagicMock() + mock_node.storage.get.return_value = [ + { + "storage": "local-zfs", + "type": "zfspool", + "content": "images,rootdir", + "total": 100 * 1024**3, + "avail": 40 * 1024**3, + }, + {"storage": "local", "type": "dir", "content": "backup,iso,vztmpl,snippets"}, + ] + mixin._node_api = MagicMock(return_value=mock_node) + + targets = mixin.get_image_storages() + + assert len(targets) == 1 + assert targets[0]["name"] == "local-zfs" + assert targets[0]["type"] == "zfspool" + assert targets[0]["total_gb"] == 100.0 + assert targets[0]["available_gb"] == 40.0 + + +def test_get_image_storages_excludes_disabled_and_inactive(): + mixin = ProxmoxVMProvisionMixin() + mock_node = MagicMock() + mock_node.storage.get.return_value = [ + {"storage": "disabled-store", "type": "dir", "content": "images", "enabled": 0}, + {"storage": "inactive-store", "type": "nfs", "content": "images", "active": 0}, + {"storage": "ok-store", "type": "dir", "content": "images"}, + ] + mixin._node_api = MagicMock(return_value=mock_node) + + targets = mixin.get_image_storages() + + assert [t["name"] for t in targets] == ["ok-store"] + + +def test_get_image_storages_excludes_storage_restricted_to_other_nodes(): + """Node-scoped query naturally excludes storages Proxmox itself doesn't + list for this node (e.g. restricted via 'nodes' to other cluster members).""" + mixin = ProxmoxVMProvisionMixin() + mock_node = MagicMock() + mock_node.storage.get.return_value = [ + {"storage": "local-zfs", "type": "zfspool", "content": "images,rootdir"}, + ] + mixin._node_api = MagicMock(return_value=mock_node) + + targets = mixin.get_image_storages() + + assert [t["name"] for t in targets] == ["local-zfs"]