fix(vm_provision_mixin): storage 'enabled' absent means enabled, not disabled
Proxmox's /storage API omits the "enabled" key entirely for storages that
were never explicitly toggled, rather than defaulting it to 1 — it isn't
present-and-falsy, it's just absent. Both _find_default_image_storage and
the snippet-storage discovery treated storage.get("enabled") as truthy-check,
so every storage without an explicit "enabled": 1 was silently excluded.
Confirmed live against a real Proxmox test server: local-lvm, local-zfs, and
fast-zfs all had content=images with no "enabled" key at all, causing
create_vm_from_cloud_init to always fail with "No storage with
content='images' found" despite multiple valid storages existing. All prior
tests used "enabled": 1 explicitly in their fixtures, masking the bug.
Fix: storage.get("enabled", 1) != 0 — absent or truthy means enabled, only
an explicit 0 excludes it. 4 new regression tests, 28 total pass.
This commit is contained in:
@@ -102,10 +102,15 @@ class ProxmoxVMProvisionMixin:
|
|||||||
return local_path
|
return local_path
|
||||||
|
|
||||||
def _find_default_image_storage(self) -> str:
|
def _find_default_image_storage(self) -> str:
|
||||||
"""Find a storage suitable for VM root disks (content includes 'images')."""
|
"""Find a storage suitable for VM root disks (content includes 'images').
|
||||||
|
|
||||||
|
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.
|
||||||
|
"""
|
||||||
for storage in self._api.storage.get():
|
for storage in self._api.storage.get():
|
||||||
content = storage.get("content", "")
|
content = storage.get("content", "")
|
||||||
if "images" in content and storage.get("enabled"):
|
if "images" in content and storage.get("enabled", 1) != 0:
|
||||||
return storage["storage"]
|
return storage["storage"]
|
||||||
raise ValueError(
|
raise ValueError(
|
||||||
"No storage with content='images' found. Configure a storage for VM disks."
|
"No storage with content='images' found. Configure a storage for VM disks."
|
||||||
@@ -266,12 +271,13 @@ class ProxmoxVMProvisionMixin:
|
|||||||
self._node_api().qemu(vmid).config.post(**config_args)
|
self._node_api().qemu(vmid).config.post(**config_args)
|
||||||
|
|
||||||
# Step 5: Verify snippet storage exists
|
# Step 5: Verify snippet storage exists
|
||||||
|
# (enabled is absent-not-falsy on Proxmox — see _find_default_image_storage)
|
||||||
_logger.info("Checking for snippet storage...")
|
_logger.info("Checking for snippet storage...")
|
||||||
storages = self._api.storage.get()
|
storages = self._api.storage.get()
|
||||||
snippet_storage = None
|
snippet_storage = None
|
||||||
for storage in storages:
|
for storage in storages:
|
||||||
content = storage.get("content", "")
|
content = storage.get("content", "")
|
||||||
if "snippets" in content and storage.get("enabled"):
|
if "snippets" in content and storage.get("enabled", 1) != 0:
|
||||||
snippet_storage = storage["storage"]
|
snippet_storage = storage["storage"]
|
||||||
break
|
break
|
||||||
|
|
||||||
|
|||||||
@@ -639,3 +639,103 @@ def test_download_cloud_image_checksum_mismatch_raises_and_removes_file():
|
|||||||
|
|
||||||
commands = [c[0][0] for c in mixin._run_node_command.call_args_list]
|
commands = [c[0][0] for c in mixin._run_node_command.call_args_list]
|
||||||
assert any(cmd.startswith("rm -f") for cmd in commands)
|
assert any(cmd.startswith("rm -f") for cmd in commands)
|
||||||
|
|
||||||
|
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
# _find_default_image_storage
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
|
||||||
|
|
||||||
|
def test_find_default_image_storage_treats_absent_enabled_as_enabled():
|
||||||
|
"""Proxmox omits "enabled" entirely for storages never explicitly toggled —
|
||||||
|
absent must mean enabled, not disabled. Regression: this was previously
|
||||||
|
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 = [
|
||||||
|
{"storage": "local-lvm", "type": "lvmthin", "content": "images,rootdir"},
|
||||||
|
]
|
||||||
|
|
||||||
|
storage = mixin._find_default_image_storage()
|
||||||
|
|
||||||
|
assert storage == "local-lvm"
|
||||||
|
|
||||||
|
|
||||||
|
def test_find_default_image_storage_excludes_explicitly_disabled():
|
||||||
|
mixin = ProxmoxVMProvisionMixin()
|
||||||
|
mixin._api = MagicMock()
|
||||||
|
mixin._api.storage.get.return_value = [
|
||||||
|
{"storage": "old-storage", "type": "dir", "content": "images", "enabled": 0},
|
||||||
|
{"storage": "local-lvm", "type": "lvmthin", "content": "images,rootdir", "enabled": 1},
|
||||||
|
]
|
||||||
|
|
||||||
|
storage = mixin._find_default_image_storage()
|
||||||
|
|
||||||
|
assert storage == "local-lvm"
|
||||||
|
|
||||||
|
|
||||||
|
def test_find_default_image_storage_raises_when_none_found():
|
||||||
|
mixin = ProxmoxVMProvisionMixin()
|
||||||
|
mixin._api = MagicMock()
|
||||||
|
mixin._api.storage.get.return_value = [
|
||||||
|
{"storage": "local", "type": "dir", "content": "backup,iso,vztmpl"},
|
||||||
|
]
|
||||||
|
|
||||||
|
with pytest.raises(ValueError, match="images"):
|
||||||
|
mixin._find_default_image_storage()
|
||||||
|
|
||||||
|
|
||||||
|
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
|
||||||
|
"enabled" key at all) — this used to make create_vm_from_cloud_init fail
|
||||||
|
with "No storage with content='images' found" even though usable storage
|
||||||
|
existed."""
|
||||||
|
mixin = ProxmoxVMProvisionMixin()
|
||||||
|
mixin._node_name = "pve1"
|
||||||
|
|
||||||
|
mock_api = MagicMock()
|
||||||
|
mock_api.cluster.nextid.get.return_value = 104
|
||||||
|
mock_api.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")
|
||||||
|
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": "local-lvm:vm-104-disk-0",
|
||||||
|
"scsi0": "local-lvm:vm-104-disk-0",
|
||||||
|
}
|
||||||
|
mock_vm.status.start.post.return_value = "UPID:pve1:130: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/104-user-data.yaml"}
|
||||||
|
mock_node.storage.return_value = mock_storage
|
||||||
|
|
||||||
|
with patch("time.sleep"):
|
||||||
|
result = mixin.create_vm_from_cloud_init(
|
||||||
|
name="real-shape-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": "real-shape-vm"},
|
||||||
|
)
|
||||||
|
|
||||||
|
assert result["vmid"] == "104"
|
||||||
|
|||||||
Reference in New Issue
Block a user