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

[Daniel Kral]

== 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