fix(vm_provision_mixin): write Cloud-Init snippet via SSH, not the upload API
Real Proxmox's POST /nodes/{node}/storage/{storage}/upload only accepts
content in {iso, vztmpl, import} — content='snippets' is rejected
outright with a 400 ("does not have a value in the enumeration").
Snippets can only be written directly to the storage's filesystem path.
Found live, right after the previous multipart-upload fix: the VM
shell, disk import, and node-scoped storage selection all succeeded,
then create_vm_from_cloud_init failed with a 400 at the snippet write
step. Resolves the storage's path via the cluster storage config and
writes the file over SSH (base64-piped, to survive arbitrary YAML
content safely).
This commit is contained in:
Christian Manivong
@@ -2,7 +2,7 @@
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import io
|
||||
import base64
|
||||
|
||||
import pytest
|
||||
from unittest.mock import MagicMock, patch
|
||||
@@ -76,6 +76,7 @@ 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.return_value.get.return_value = {"path": "/var/lib/vz"}
|
||||
|
||||
mock_node = MagicMock()
|
||||
mock_node.storage.get.return_value = [
|
||||
@@ -103,11 +104,6 @@ def test_create_vm_from_cloud_init_single_nic():
|
||||
mock_task.status.get.return_value = {"status": "stopped", "exitstatus": "OK"}
|
||||
mock_node.tasks.return_value = mock_task
|
||||
|
||||
# Mock storage upload
|
||||
mock_storage = MagicMock()
|
||||
mock_storage.upload.post.return_value = {"filename": "snippets:snippets/101-user-data.yaml"}
|
||||
mock_node.storage.return_value = mock_storage
|
||||
|
||||
with patch("time.sleep"):
|
||||
result = mixin.create_vm_from_cloud_init(
|
||||
name="test-vm",
|
||||
@@ -132,14 +128,15 @@ def test_create_vm_from_cloud_init_single_nic():
|
||||
assert "tag=10" in net_call_args[1]["net0"]
|
||||
assert "vmbr0" in net_call_args[1]["net0"]
|
||||
|
||||
# Regression: the snippet must be uploaded as an actual file (io.IOBase),
|
||||
# not a plain filename string with a separate "data" field — proxmoxer
|
||||
# only builds a real multipart request for io.IOBase values, and real
|
||||
# Proxmox drops the connection outright for anything else (see
|
||||
# test_create_vm_uploads_snippet_as_file_object for the dedicated check).
|
||||
upload_kwargs = mock_storage.upload.post.call_args[1]
|
||||
assert "data" not in upload_kwargs
|
||||
assert isinstance(upload_kwargs["filename"], io.IOBase)
|
||||
# Regression: the snippet must be written directly to the filesystem via
|
||||
# SSH, not uploaded via the /storage/upload API — real Proxmox rejects
|
||||
# content='snippets' on that endpoint outright (see
|
||||
# test_create_vm_writes_snippet_via_ssh_with_correct_content for the
|
||||
# dedicated check).
|
||||
write_cmd = next(
|
||||
c[0][0] for c in mixin._run_node_command.call_args_list if "snippets/101-user-data.yaml" in c[0][0]
|
||||
)
|
||||
assert "/var/lib/vz/snippets" in write_cmd
|
||||
|
||||
|
||||
def test_create_vm_from_cloud_init_dual_nic_trunk():
|
||||
@@ -150,6 +147,7 @@ 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.return_value.get.return_value = {"path": "/var/lib/vz"}
|
||||
|
||||
mock_node = MagicMock()
|
||||
mock_node.storage.get.return_value = [
|
||||
@@ -177,11 +175,6 @@ def test_create_vm_from_cloud_init_dual_nic_trunk():
|
||||
mock_task.status.get.return_value = {"status": "stopped", "exitstatus": "OK"}
|
||||
mock_node.tasks.return_value = mock_task
|
||||
|
||||
# Mock storage upload
|
||||
mock_storage = MagicMock()
|
||||
mock_storage.upload.post.return_value = {"filename": "snippets:snippets/102-user-data.yaml"}
|
||||
mock_node.storage.return_value = mock_storage
|
||||
|
||||
with patch("time.sleep"):
|
||||
result = mixin.create_vm_from_cloud_init(
|
||||
name="wireshark-sat-1",
|
||||
@@ -265,6 +258,7 @@ def test_create_vm_with_disk_resize():
|
||||
# Mock API hierarchy
|
||||
mock_api = MagicMock()
|
||||
mock_api.cluster.nextid.get.return_value = 103
|
||||
mock_api.storage.return_value.get.return_value = {"path": "/var/lib/vz"}
|
||||
|
||||
mock_node = MagicMock()
|
||||
mock_node.storage.get.return_value = [
|
||||
@@ -293,11 +287,6 @@ def test_create_vm_with_disk_resize():
|
||||
mock_task.status.get.return_value = {"status": "stopped", "exitstatus": "OK"}
|
||||
mock_node.tasks.return_value = mock_task
|
||||
|
||||
# Mock storage upload
|
||||
mock_storage = MagicMock()
|
||||
mock_storage.upload.post.return_value = {"filename": "snippets:snippets/103-user-data.yaml"}
|
||||
mock_node.storage.return_value = mock_storage
|
||||
|
||||
with patch("time.sleep"):
|
||||
result = mixin.create_vm_from_cloud_init(
|
||||
name="big-vm",
|
||||
@@ -722,6 +711,42 @@ def test_find_default_image_storage_excludes_storage_restricted_to_other_nodes()
|
||||
assert storage == "local-zfs"
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# _get_storage_path
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_get_storage_path_returns_path_from_cluster_config():
|
||||
mixin = ProxmoxVMProvisionMixin()
|
||||
mixin._api = MagicMock()
|
||||
mixin._api.storage.return_value.get.return_value = {
|
||||
"storage": "local",
|
||||
"type": "dir",
|
||||
"path": "/var/lib/vz",
|
||||
}
|
||||
|
||||
path = mixin._get_storage_path("local")
|
||||
|
||||
assert path == "/var/lib/vz"
|
||||
mixin._api.storage.assert_called_with("local")
|
||||
|
||||
|
||||
def test_get_storage_path_raises_when_storage_has_no_path():
|
||||
"""A storage type without a filesystem path (e.g. lvmthin, zfspool) can't
|
||||
back content='snippets' at all — Proxmox itself only allows that content
|
||||
type on dir/nfs/cifs/cephfs storages, so this should never actually be
|
||||
reached for a real snippet storage, but must fail clearly if it is."""
|
||||
mixin = ProxmoxVMProvisionMixin()
|
||||
mixin._api = MagicMock()
|
||||
mixin._api.storage.return_value.get.return_value = {
|
||||
"storage": "local-lvm",
|
||||
"type": "lvmthin",
|
||||
}
|
||||
|
||||
with pytest.raises(ValueError, match="path"):
|
||||
mixin._get_storage_path("local-lvm")
|
||||
|
||||
|
||||
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
|
||||
@@ -733,6 +758,7 @@ 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.return_value.get.return_value = {"path": "/var/lib/vz"}
|
||||
|
||||
mock_node = MagicMock()
|
||||
mock_node.storage.get.return_value = [
|
||||
@@ -760,10 +786,6 @@ def test_create_vm_from_cloud_init_with_real_world_storage_shape():
|
||||
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",
|
||||
@@ -786,6 +808,7 @@ def test_create_vm_from_cloud_init_explicit_storage_skips_auto_detect():
|
||||
|
||||
mock_api = MagicMock()
|
||||
mock_api.cluster.nextid.get.return_value = 105
|
||||
mock_api.storage.return_value.get.return_value = {"path": "/var/lib/vz"}
|
||||
|
||||
mock_node = MagicMock()
|
||||
mock_node.storage.get.return_value = [
|
||||
@@ -813,10 +836,6 @@ def test_create_vm_from_cloud_init_explicit_storage_skips_auto_detect():
|
||||
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",
|
||||
@@ -901,21 +920,22 @@ def test_get_image_storages_excludes_storage_restricted_to_other_nodes():
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_create_vm_uploads_snippet_as_file_object_with_correct_content():
|
||||
"""Regression: real Proxmox's /storage/{s}/upload endpoint expects the
|
||||
"filename" parameter to be the file itself (multipart). proxmoxer only
|
||||
builds a multipart request when the value is an io.IOBase instance —
|
||||
passing a plain string (with a separate, nonexistent "data" field, as the
|
||||
old code did) makes proxmoxer send a normal form-urlencoded POST instead,
|
||||
which real Proxmox responds to by closing the connection outright
|
||||
(observed live: requests.exceptions.ConnectionError /
|
||||
RemoteDisconnected('Remote end closed connection without response'),
|
||||
after the VM shell and disk import had already succeeded)."""
|
||||
def test_create_vm_writes_snippet_via_ssh_with_correct_content():
|
||||
"""Regression: real Proxmox's /storage/{s}/upload endpoint rejects
|
||||
content='snippets' outright ("value 'snippets' does not have a value in
|
||||
the enumeration 'iso, vztmpl, import'") — that endpoint only handles
|
||||
ISOs, container templates, and imports. Snippets can only be written
|
||||
directly to the storage's filesystem path, so this must go through SSH
|
||||
(_run_node_command), not the upload API.
|
||||
(observed live: 400 Bad Request from Proxmox, right after the earlier
|
||||
multipart-upload fix had already gotten past a prior RemoteDisconnected
|
||||
bug at the same step — two distinct real-world failures at this line)."""
|
||||
mixin = ProxmoxVMProvisionMixin()
|
||||
mixin._node_name = "pve1"
|
||||
|
||||
mock_api = MagicMock()
|
||||
mock_api.cluster.nextid.get.return_value = 106
|
||||
mock_api.storage.return_value.get.return_value = {"path": "/mnt/pve/snippet-nfs"}
|
||||
|
||||
mock_node = MagicMock()
|
||||
mock_node.storage.get.return_value = [
|
||||
@@ -941,28 +961,29 @@ def test_create_vm_uploads_snippet_as_file_object_with_correct_content():
|
||||
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": "local:snippets/106-user-data.yaml"}
|
||||
mock_node.storage.return_value = mock_storage
|
||||
|
||||
with patch("time.sleep"):
|
||||
mixin.create_vm_from_cloud_init(
|
||||
name="upload-shape-vm",
|
||||
name="write-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": "upload-shape-vm", "chpasswd": {"expire": False}},
|
||||
cloud_init_config={"hostname": "write-shape-vm", "chpasswd": {"expire": False}},
|
||||
)
|
||||
|
||||
upload_call = mock_storage.upload.post.call_args
|
||||
assert upload_call[1]["content"] == "snippets"
|
||||
assert "data" not in upload_call[1]
|
||||
# No call to the upload API at all — snippets aren't a valid content
|
||||
# type there.
|
||||
mock_node.storage.assert_not_called()
|
||||
|
||||
file_obj = upload_call[1]["filename"]
|
||||
assert isinstance(file_obj, io.IOBase)
|
||||
assert file_obj.name == "106-user-data.yaml"
|
||||
file_obj.seek(0)
|
||||
content = file_obj.read().decode("utf-8")
|
||||
write_cmd = next(
|
||||
c[0][0]
|
||||
for c in mixin._run_node_command.call_args_list
|
||||
if "snippets/106-user-data.yaml" in c[0][0]
|
||||
)
|
||||
assert "mkdir -p /mnt/pve/snippet-nfs/snippets" in write_cmd
|
||||
assert "/mnt/pve/snippet-nfs/snippets/106-user-data.yaml" in write_cmd
|
||||
|
||||
encoded = write_cmd.split("echo ", 1)[1].split(" | base64 -d")[0]
|
||||
content = base64.b64decode(encoded).decode("utf-8")
|
||||
assert content.startswith("#cloud-config\n")
|
||||
assert "hostname: upload-shape-vm" in content
|
||||
assert "hostname: write-shape-vm" in content
|
||||
|
||||
Reference in New Issue
Block a user