[pve-devel] [PATCH storage/qemu-server/container v3 00/27] consistent assertions for volume's content types

[Daniel Kral]

== Changelog since v2 ==

Thanks @Fiona for the feedback and help on this!

v1: https://lore.proxmox.com/pve-devel/20240916163839.236908-1-d.kral@proxmox.com/
v2: https://lore.proxmox.com/pve-devel/20250211160825.254167-1-d.kral@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