NAPALM/napalm-proxmox
1
0
Fork 0
Code Issues 1 Pull Requests Actions Packages Projects Releases Wiki Activity

Merge feature/node-scoped-image-storage: fix node-scoped storage query + selectable storage

Browse Source
This commit is contained in:
Christian Manivong
2026-07-07 22:36:16 +02:00
parent 12135735cb 4d568bc6dc
commit fe4f5e84d8
2 changed files with 208 additions and 26 deletions
Download Patch File Download Diff File Expand all files Collapse all files
napalm_proxmox/vm_provision_mixin.py
+43 -5
View File
@@ -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", "")
tests/test_vm_provision_mixin.py
+165 -21
View File
@@ -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"]