== Changelog since v2 == Thanks @Fiona for the feedback and help on this!
v1: https://lore.proxmox.com/pve-devel/20240916163839.236908-1-d.k...@proxmox.com/ v2: https://lore.proxmox.com/pve-devel/20250211160825.254167-1-d.k...@proxmox.com/ - improve some commit messages and clarify some non-trivial changes - rename helpers from `assert_*_supported` to `assert_*_available` - make helpers also assert whether storages are enabled (local/remote) - make $fmt a required parameter for vdisk_alloc again - make $vtype a required parameter for vdisk_alloc - other smaller changes are inline in patches == 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 importing OVF manifests (e.g. `qm importovf ... --storage ...`) - when adding cloudinit images (e.g. `qm set <vmid> -ide0 "noimages:cloudinit"`), and The first two were already merged for v2, while the latter two are still in this revision. Otherwise, this patch series 1) fixes these situations (qemu-server #1-#3) 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: - qemu-server #1-#3, and - pve-container #1-#5. == Breaking changes == As this patch series changes the signature of vdisk_alloc, there needs to be a versioned break between them, since else the other packages will fail to call vdisk_alloc with the older parameters. This seemed like the best option as this already forces any caller to pass the $vtype now and will only require remaining 'any's to be changed to the correct content type in PVE 9. == 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/two commit in each repository to make them compatible with each other, which is pve-storage #3 and $4, qemu-server #4 and #5, and pve-container #6 and $7. These two patches each can be squashed in all cases as I only wanted to separate them for review, but either way is fine for me here. The first always moves $name to the optional hash and the second always adds the new required parameter $vtype. == Testing == I tested the same (and some more) scenarios unpatched and patched as for v2: - 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, - it is irrelevant if there is a 'media=cdrom' appended or not now, - this needs the "Datastore.AllocateSpace" permission 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, - migrating a VM to another node from a unsupported source 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. == Diffstat == pve-storage: Daniel Kral (4): 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 assertion for volume's content type src/PVE/API2/Storage/Content.pm | 16 +++++- src/PVE/API2/Storage/Status.pm | 10 ++-- src/PVE/GuestImport.pm | 3 +- src/PVE/Storage.pm | 92 +++++++++++++++++++++++++++++- src/test/run_test_zfspoolplugin.pl | 12 ++-- 5 files changed, 116 insertions(+), 17 deletions(-) qemu-server: Daniel Kral (12): fix #5284: cli: importovf: assert content type support for target storage api: remove unusable default storage parameter in check_storage_access fix #5284: api: update-vm: assert content type support for cloudinit images tree-wide: update vdisk_alloc optional arguments signature tree-wide: update vdisk_alloc vtype argument 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 | 9 ++-- PVE/QemuConfig.pm | 4 +- PVE/QemuMigrate.pm | 17 +++--- PVE/QemuServer.pm | 52 +++++++------------ PVE/QemuServer/Cloudinit.pm | 3 +- PVE/QemuServer/ImportDisk.pm | 3 +- PVE/VZDump/QemuServer.pm | 10 +++- qmextract | 4 +- test/MigrationTest/QmMock.pm | 7 ++- .../unsupported-storage-content-type.conf | 2 +- 11 files changed, 78 insertions(+), 85 deletions(-) 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: fix content type check for ZFS storages 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 | 47 ++++----- src/PVE/LXC/Config.pm | 4 +- src/PVE/LXC/Migrate.pm | 15 ++- src/test/Makefile | 5 +- src/test/run_alloc_disk_tests.pl | 157 +++++++++++++++++++++++++++++++ 6 files changed, 198 insertions(+), 45 deletions(-) create mode 100755 src/test/run_alloc_disk_tests.pl Summary over all repositories: 22 files changed, 392 insertions(+), 147 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
