From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <pve-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9])
by lore.proxmox.com (Postfix) with ESMTPS id 1DB511FF15E
for <inbox@lore.proxmox.com>; Tue, 11 Feb 2025 17:09:21 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 0F664336F;
Tue, 11 Feb 2025 17:08:42 +0100 (CET)
From: Daniel Kral <d.kral@proxmox.com>
To: pve-devel@lists.proxmox.com
Date: Tue, 11 Feb 2025 17:07:54 +0100
Message-Id: <20250211160825.254167-1-d.kral@proxmox.com>
X-Mailer: git-send-email 2.39.5
MIME-Version: 1.0
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.011 Adjusted score from AWL reputation of From: address
BAYES_00 -1.9 Bayes spam probability is 0 to 1%
DMARC_MISSING 0.1 Missing DMARC policy
KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record
SPF_PASS -0.001 SPF: sender matches SPF record
Subject: [pve-devel] [PATCH container/qemu-server/storage v2 00/31]
consistent assertions for volume's content types
X-BeenThere: pve-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=subscribe>
Reply-To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: pve-devel-bounces@lists.proxmox.com
Sender: "pve-devel" <pve-devel-bounces@lists.proxmox.com>
== Description ==
There were (at least) four missing assertions whether the underlying
storage supports the content type that is about the be allocated, but
will fail to start with a volume not being on a storage, which supports
VM images (see `config_to_command` for the existing check):
- when moving disks (e.g. `qm disk move <vmid> --storage ...`)
- when cloning vms (e.g. `qm clone <vmid> <new-vmid> --storage ...`)
- when adding cloudinit images (e.g. `qm set <vmid> -ide0
"noimages:cloudinit"`), and
- when importing OVF manifests (e.g. `qm importovf ... --storage ...`)
This patch series 1) fixes these situations (qemu-server #2-#7) and
introduces 2) assertion helpers and 3) an optional additional assertion
in `vdisk_alloc` in the rest of the patch series to make the error
messages of content type assertions more consistent and allow for easier
grepping where these checks are done.
This patch series also introduces test cases for the `alloc_disk` helper
in pve-container to make the existing function easier to follow and
allow a single call to `vdisk_alloc` instead of four. Where applicable
(e.g. qemu-server #4-#6 for cloudinit images), there are also some
smaller refactorings so that the bug fix ends up not duplicating code.
The only content type assertions at `vdisk_alloc`, which are not
possible are for fleecing backup images and snapshot vm state files,
since both can be currently created without any error, so adding the
assertion would be an API breakage. Therefore, I propose to add this
check for PVE 9.0.
== Standalone patches ==
I also made sure that the bug fixes and introduction of test cases a
factored out at the top, so they can be applied before the refactorings:
- pve-storage #1-#2,
- qemu-server #1-#7, and
- pve-container #1-#5.
== Possible squash ==
I was unsure how granular the patches here should be, so I left it at
the finest granularity to make reviewing easier, but there's room for
squashing patches together:
- qemu-server #10-#14,
- pve-container #3-#5, and
- pve-container #7-#11.
== Building ==
To build this, pve-storage must be built first, as the API of
`vdisk_alloc` is broken. I made sure that there's one commit in each
repository to make them compatible with each other, which is pve-storage
#4 (moving `$name` param to optional hash), qemu-server #8 (updating to
new vdisk_alloc signature), and pve-container #6 (updating single call
of vdisk_alloc to new signature)
== Testing ==
I tested the following scenarios unpatched and patched:
- moving disks will only work to supported storages now,
- cloning VMs will only work to supported storages now,
- creating cloudinit images will only work on supported storages now,
- importing OVF manifests will only work on supported storages now,
- creating VMs on supported storages succeed for harddisks, efidisks,
tpm disks, and cloudinit images,
- creating VMs on unsupported storages (for the above) fails,
- cloning VMs between supported storages locally works as expected,
- cloning VMs between supported storages between nodes works,
- suspending and resuming a VM works fine,
- backups and qmrestores of VMs work fine,
- migrating a VM between two nodes on a supported target storage works,
- migrating a VM to another node on a unsupported target storage fails,
- updating the VM config in such a way that a disk needs to be allocated
works fine as long as the disk to allocate is on a supported storage,
- importing disks works as expected to {un,}supported storages,
- allocating a disk with `pvesm` works as expected,
- backing up a VM with a disk that is on an unsupported storage fails,
- restoring a VM with to an unsupported storage fails.
== Changelog ==
Thanks @Fiona for the feedback and help on this.
v1: https://lore.proxmox.com/pve-devel/20240916163839.236908-1-d.kral@proxmox.com/
- rename helpers from `check_*_type` to `assert_*_type_supported`
- remove helpers `check_{storage,volume}_alloc`
- remove helper `alloc_volume_disk` in favor of `vdisk_alloc`
- move content type helpers to `PVE::Storage`
- make the optional `$name` parameter in `vdisk_alloc` optional by
moving it to a newly introduced hash at the end
- introduce assertion also in `vdisk_alloc` by allowing a new optional
parameter `$vtype` to make the assertion opt-in for now
- keeping `check_storage_access_migrate` as a wrapper for the helper
- no functional changes to fleecing images and vm state files
== Diffstat ==
pve-storage:
Daniel Kral (5):
api: {upload,download}_url: factor out common parameter hash accesses
introduce helpers for content type assertions of storages and volumes
tree-wide: make use of content type assertion helper
vdisk_alloc: factor out optional parameters in options hash argument
vdisk_alloc: add optional assertion for volume's content type
src/PVE/API2/Storage/Content.pm | 6 +--
src/PVE/API2/Storage/Status.pm | 24 ++++-----
src/PVE/GuestImport.pm | 2 +-
src/PVE/Storage.pm | 83 ++++++++++++++++++++++++++++--
src/test/run_test_zfspoolplugin.pl | 8 +--
5 files changed, 99 insertions(+), 24 deletions(-)
qemu-server:
Daniel Kral (15):
test: cfg2cmd: expect error for invalid volume's storage content type
fix #5284: api: move-disk: assert content type support for target storage
fix #5284: api: clone_vm: assert content type support for target storage
api: remove unused size variable in check_storage_access
api: remove unusable default storage parameter in check_storage_access
fix #5284: api: update-vm: assert content type support for cloudinit images
fix #5284: cli: importovf: assert content type support for target storage
tree-wide: update vdisk_alloc optional arguments signature
cfg2cmd: improve error message for invalid volume content type
api: {clone,move}_vm: use volume content type assertion helpers
api: {create,update}_vm: use volume content type assertion helpers
api: import{disk,ovf}: use volume content type assertion helpers
api: qmrestore: use volume content type assertion helpers
api: migrate_vm: use volume content type assertion helpers
tree-wide: add todos for breaking content type assertions
PVE/API2/Qemu.pm | 52 ++++++++-------
PVE/CLI/qm.pm | 6 +-
PVE/QemuConfig.pm | 5 +-
PVE/QemuMigrate.pm | 16 ++---
PVE/QemuServer.pm | 64 ++++++++-----------
PVE/QemuServer/Cloudinit.pm | 5 +-
PVE/QemuServer/ImportDisk.pm | 4 +-
PVE/VZDump/QemuServer.pm | 5 +-
qmextract | 5 +-
test/MigrationTest/QmMock.pm | 4 +-
.../unsupported-storage-content-type.conf | 3 +
test/run_config2command_tests.pl | 4 ++
12 files changed, 93 insertions(+), 80 deletions(-)
create mode 100644 test/cfg2cmd/unsupported-storage-content-type.conf
pve-container:
Daniel Kral (11):
migration: prepare: factor out common read-only variables
tests: add tests for expected behavior of alloc_disk wrapper
alloc_disk: fail fast if storage does not support content type rootdir
alloc_disk: factor out common arguments for call to vdisk_alloc
alloc_disk: simplify storage-specific logic for vdisk_alloc arguments
alloc_disk: update vdisk_alloc optional arguments signature
alloc_disk: use volume content type assertion helpers
api: create: use volume content type assertion helpers
migration: prepare: use volume content type assertion helpers
api: update_vm: use volume content type assertion helpers
mount: raw/iso: use volume content type assertion helpers
src/PVE/API2/LXC.pm | 15 ++--
src/PVE/LXC.pm | 48 +++++-----
src/PVE/LXC/Config.pm | 4 +-
src/PVE/LXC/Migrate.pm | 16 ++--
src/test/Makefile | 5 +-
src/test/run_alloc_disk_tests.pl | 149 +++++++++++++++++++++++++++++++
6 files changed, 192 insertions(+), 45 deletions(-)
create mode 100755 src/test/run_alloc_disk_tests.pl
Summary over all repositories:
23 files changed, 384 insertions(+), 149 deletions(-)
--
Generated by git-murpp 0.8.0
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel