[pve-devel] [RFC qemu-server 0/9] consistent checks for storage content types on volume disk allocation

[pve-devel] [RFC qemu-server 0/9] consistent checks for storage content types on volume disk allocation

[Daniel Kral]

There currently is some inconsistency in how storages are checked when
they are used to allocate new VM disk images on them at different API
endpoints and internal subroutines. The usual checks that are done in
these contexts are:

- check whether the storage is enabled,
- check whether the content type 'images' (or the volume-specific
  content type) is supported on the storage, and
- check whether the authenticated user has the necessary permissions to
  allocate space on the storage;

The origin of this patch series is bug report #5284, where a PVE user
indicated that when moving VM disks from one storage to another, this
process also works for storages that do not support VM images, but
causes the VM to fail when started afterwards. This also happens in the
case when a VM is cloned to a storage without VM images support, which
will deliver the same result.

As there are many similar checks throughout the qemu-server package,
this patch series refactors the common denominator of these checks into
universal helper functions to have predictable error messages throughout
the repository for the same error and improve parameter context
consistency (`raise_param_exc`) for error messages on API endpoints to
make it easier for users to find the error more quickly.

As I'm still new to the codebase I am unsure about some changes in the
codebase, as the indent was to find places where the checks are missing
and adding consistency for storing VM images only on storages that
support them. I have tested the changes to my current best ability, but
as the package is functionally quite fundamental and large, I'm still
putting this out as a RFC and added notes throughout the patches.

I'm particular unsure about the changes made to fleecing images,
snapshot statefiles and the cloudinit allocation, which I've pointed
out in the patches #6 and #9 respectively. Also, my goal was in making
it harder for the user to get into 'invalid' states, but not to move out
of them. So it's still possible to move disks from non-supporting to
supporting storages and update the VM config with unsupported storages.
Are there any cases that I'm missing?

Daniel Kral (9):
  test: cfg2cmd: expect error for invalid volume's storage content type
  cfg2cmd: improve error message for invalid volume storage content type
  fix #5284: move_vm: add check if target storage supports vm images
  api: clone_vm: add check if storage supports vm images
  api: create_vm: improve checks if storages for disks support vm images
  cloudinit: add check if storage for cloudinit disk supports vm images
  api: migrate_vm: improve check if target storages support vm images
  api: importdisk: improve check if storage supports vm images
  restore_vm: improve checks if storage supports vm images

 PVE/API2/Qemu.pm                              |  70 +++++------
 PVE/CLI/qm.pm                                 |  12 +-
 PVE/QemuConfig.pm                             |   4 +-
 PVE/QemuMigrate.pm                            |  13 +-
 PVE/QemuServer.pm                             |  55 +++------
 PVE/QemuServer/Cloudinit.pm                   |   4 +-
 PVE/QemuServer/Helpers.pm                     | 111 ++++++++++++++++++
 PVE/QemuServer/ImportDisk.pm                  |   4 +-
 PVE/VZDump/QemuServer.pm                      |   4 +-
 .../unsupported-storage-content-type.conf     |   3 +
 test/run_config2command_tests.pl              |   6 +-
 11 files changed, 188 insertions(+), 98 deletions(-)
 create mode 100644 test/cfg2cmd/unsupported-storage-content-type.conf

-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC qemu-server 1/9] test: cfg2cmd: expect error for invalid volume's storage content type

[Daniel Kral]

Tests whether when running `config_to_command` it will correctly fail
with an error message that a volume cannot be used if the underlying
storage does not support its content type.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
 test/cfg2cmd/unsupported-storage-content-type.conf | 3 +++
 test/run_config2command_tests.pl                   | 6 +++++-
 2 files changed, 8 insertions(+), 1 deletion(-)
 create mode 100644 test/cfg2cmd/unsupported-storage-content-type.conf

diff --git a/test/cfg2cmd/unsupported-storage-content-type.conf b/test/cfg2cmd/unsupported-storage-content-type.conf
new file mode 100644
index 00000000..35e789b2
--- /dev/null
+++ b/test/cfg2cmd/unsupported-storage-content-type.conf
@@ -0,0 +1,3 @@
+# TEST: Unsupported storage content type in a volume disk
+# EXPECT_ERROR: storage 'no-images' does not support content-type 'images'
+scsi0: no-images:8006/vm-8006-disk-0.raw,iothread=1,size=32G
diff --git a/test/run_config2command_tests.pl b/test/run_config2command_tests.pl
index d48ef562..32d645f2 100755
--- a/test/run_config2command_tests.pl
+++ b/test/run_config2command_tests.pl
@@ -30,6 +30,10 @@ my $base_env = {
 		type => 'dir',
 		shared => 0,
 	    },
+	    'no-images' => {
+		path => '/var/lib/vz',
+		type => 'dir',
+	    },
 	    'btrfs-store' => {
 		content => {
 		    images => 1,
@@ -68,7 +72,7 @@ my $base_env = {
 		content => {
 		    images => 1,
 		}
-	    }
+	    },
 	}
     },
     vmid => 8006,
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC qemu-server 2/9] cfg2cmd: improve error message for invalid volume storage content type

[Daniel Kral]

Improve the error message when a VM start fails due to a volume storage
not supporting the volume's required content type by prefixing it with
the disk handle (e.g. scsi0).

This moves and splits the subroutine for checking the volume storage's
content type to increase its reusability.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
This introduces the check_storage_content_type and
check_volume_content_type subroutines, which are later used throughout
the refactoring of the other locations with similar checks.

 PVE/QemuServer.pm                             | 16 +------
 PVE/QemuServer/Helpers.pm                     | 45 +++++++++++++++++++
 .../unsupported-storage-content-type.conf     |  2 +-
 3 files changed, 48 insertions(+), 15 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index b26da505..e24c741c 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -3961,7 +3961,8 @@ sub config_to_command {
 	my ($ds, $drive) = @_;
 
 	if (PVE::Storage::parse_volume_id($drive->{file}, 1)) {
-	    check_volume_storage_type($storecfg, $drive->{file});
+	    eval { PVE::QemuServer::Helpers::check_volume_content_type($storecfg, $drive->{file}) };
+	    die "$ds: $@" if $@;
 	    push @$vollist, $drive->{file};
 	}
 
@@ -8794,19 +8795,6 @@ sub vm_is_paused {
     );
 }
 
-sub check_volume_storage_type {
-    my ($storecfg, $vol) = @_;
-
-    my ($storeid, $volname) = PVE::Storage::parse_volume_id($vol);
-    my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
-    my ($vtype) = PVE::Storage::parse_volname($storecfg, $vol);
-
-    die "storage '$storeid' does not support content-type '$vtype'\n"
-	if !$scfg->{content}->{$vtype};
-
-    return 1;
-}
-
 sub add_nets_bridge_fdb {
     my ($conf, $vmid) = @_;
 
diff --git a/PVE/QemuServer/Helpers.pm b/PVE/QemuServer/Helpers.pm
index 0afb6317..9d0f24aa 100644
--- a/PVE/QemuServer/Helpers.pm
+++ b/PVE/QemuServer/Helpers.pm
@@ -106,6 +106,51 @@ sub vm_running_locally {
     return;
 }
 
+=head3 check_storage_content_type($storecfg, $storeid [, $content_type])
+
+Checks whether the storage with the identifier C<$storeid>, that is defined in C<$storecfg>,
+supports the content type C<$content_type>. If the C<$content_type> is undefined, it will default
+to the value C<"images">.
+
+If the check fails, the subroutine will C<die> with an error message containing the storage and
+content type that is not supported.
+
+Returns C<1> if the check is successful.
+
+=cut
+
+sub check_storage_content_type : prototype($$;$) {
+    my ($storecfg, $storeid, $content_type) = @_;
+
+    $content_type = "images" if !defined($content_type);
+    my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
+    die "storage '$storeid' does not support content type '$content_type'\n"
+	if !$scfg->{content}->{$content_type};
+
+    return 1;
+}
+
+=head3 check_volume_content_type($storecfg, $volid)
+
+Checks whether the volume with the identifier C<$volid>, that is defined in C<$storecfg>, supports
+the content type, which is determined by L<PVE::Storage::parse_volname>.
+
+If the check fails, the subroutine will C<die> with an error message containing the storage and
+content type that is not supported.
+
+Returns C<1> if the check is successful.
+
+=cut
+
+sub check_volume_content_type : prototype($$) {
+    my ($storecfg, $volid) = @_;
+
+    my ($storeid) = PVE::Storage::parse_volume_id($volid);
+    my ($vtype) = PVE::Storage::parse_volname($storecfg, $volid);
+
+    return check_storage_content_type($storecfg, $storeid, $vtype);
+}
+
 sub min_version {
     my ($verstr, $major, $minor, $pve) = @_;
 
diff --git a/test/cfg2cmd/unsupported-storage-content-type.conf b/test/cfg2cmd/unsupported-storage-content-type.conf
index 35e789b2..7e7952aa 100644
--- a/test/cfg2cmd/unsupported-storage-content-type.conf
+++ b/test/cfg2cmd/unsupported-storage-content-type.conf
@@ -1,3 +1,3 @@
 # TEST: Unsupported storage content type in a volume disk
-# EXPECT_ERROR: storage 'no-images' does not support content-type 'images'
+# EXPECT_ERROR: scsi0: storage 'no-images' does not support content type 'images'
 scsi0: no-images:8006/vm-8006-disk-0.raw,iothread=1,size=32G
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC qemu-server 3/9] fix #5284: move_vm: add check if target storage supports vm images

[Daniel Kral]

Adds a check if the target storage when moving a VM disk images between
two different storages supports the content type 'images'. Without the
check in place, it will move the volume image to the target storage even
though vm images are not supported there. This will result in the VM
failing to start for any non-unused disk volumes.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
 PVE/API2/Qemu.pm          |  8 ++++---
 PVE/QemuServer/Helpers.pm | 46 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+), 3 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index d25a79fe..0cb4af89 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -29,7 +29,7 @@ use PVE::QemuServer;
 use PVE::QemuServer::Cloudinit;
 use PVE::QemuServer::CPUConfig;
 use PVE::QemuServer::Drive;
-use PVE::QemuServer::Helpers;
+use PVE::QemuServer::Helpers qw(check_storage_alloc check_volume_alloc);
 use PVE::QemuServer::ImportDisk;
 use PVE::QemuServer::Monitor qw(mon_cmd);
 use PVE::QemuServer::Machine;
@@ -4115,6 +4115,10 @@ __PACKAGE__->register_method({
 	    die "you can't move to the same storage with same format\n"
 		if $oldstoreid eq $storeid && (!$format || !$oldfmt || $oldfmt eq $format);
 
+	    check_storage_alloc($rpcenv, $authuser, $storeid);
+	    eval { check_volume_alloc($storecfg, $storeid) };
+	    raise_param_exc({ storage => "$@" }) if $@;
+
 	    # this only checks snapshots because $disk is passed!
 	    my $snapshotted = PVE::QemuServer::Drive::is_volume_in_use(
 		$storecfg,
@@ -4408,8 +4412,6 @@ __PACKAGE__->register_method({
 		$disk_reassignfn
 	    );
 	} elsif ($storeid) {
-	    $rpcenv->check($authuser, "/storage/$storeid", ['Datastore.AllocateSpace']);
-
 	    $load_and_check_move->(); # early checks before forking/locking
 
 	    my $realcmd = sub {
diff --git a/PVE/QemuServer/Helpers.pm b/PVE/QemuServer/Helpers.pm
index 9d0f24aa..a5f6b328 100644
--- a/PVE/QemuServer/Helpers.pm
+++ b/PVE/QemuServer/Helpers.pm
@@ -11,6 +11,8 @@ use PVE::ProcFSTools;
 
 use base 'Exporter';
 our @EXPORT_OK = qw(
+check_storage_alloc
+check_volume_alloc
 min_version
 config_aware_timeout
 parse_number_sets
@@ -151,6 +153,50 @@ sub check_volume_content_type : prototype($$) {
     return check_storage_content_type($storecfg, $storeid, $vtype);
 }
 
+=head3 check_storage_alloc($rpcenv, $user, $storeid)
+
+Checks whether the C<$user> has the permissions in the C<$rpcenv> to allocate space in the storage
+with the identifier C<$storeid>.
+
+
+If the check fails, the subroutine will C<die> with a permission exception inside the subroutine
+L<PVE::RPCEnvironment::check>.
+
+Returns C<1> if the check is successful.
+
+=cut
+
+sub check_storage_alloc : prototype($$$) {
+    my ($rpcenv, $user, $storeid) = @_;
+
+    if (defined($rpcenv) && defined($user)) {
+	$rpcenv->check($user, "/storage/$storeid", ['Datastore.AllocateSpace'])
+	    if $user ne 'root@pam';
+    }
+
+    return 1;
+}
+
+=head3 check_volume_alloc($storecfg, $storeid, $node)
+
+Checks whether the volume with the identifier C<$volid>, that is defined in C<$storecfg> (which
+is typically retrieved with L<PVE::Storage::config>), is enabled an supports volume images.
+
+If the check fails, it will C<die> with an error message.
+
+Returns C<1> if the check is successful.
+
+=cut
+
+sub check_volume_alloc : prototype($$;$) {
+    my ($storecfg, $storeid, $node) = @_;
+
+    PVE::Storage::storage_check_enabled($storecfg, $storeid, $node);
+    check_storage_content_type($storecfg, $storeid);
+
+    return 1;
+}
+
 sub min_version {
     my ($verstr, $major, $minor, $pve) = @_;
 
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC qemu-server 4/9] api: clone_vm: add check if storage supports vm images

[Daniel Kral]

Adds a check when cloning a disk if the underlying storage, where the
disk should be cloned to, supports the content type 'images'. This
happens first when using the clone vm api and right before doing the
disk allocation.

Without this check, a vm could be cloned to an arbitrary storage and
then failed to start if the storage does not support vm images. This
will restrict the allocation of disk images when moving between storages
(api call `move_vm_disk`), cloning a vm (api call `clone_vm`) and
importint EFI disks and/or non-live disk imports (`create_disks`).

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
 PVE/API2/Qemu.pm          |  5 +++--
 PVE/QemuServer.pm         |  6 ++----
 PVE/QemuServer/Helpers.pm | 20 ++++++++++++++++++++
 3 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 0cb4af89..a7931d98 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -3737,8 +3737,9 @@ __PACKAGE__->register_method({
 	    my $storecfg = PVE::Storage::config();
 
 	    if ($storage) {
-		# check if storage is enabled on local node
-		PVE::Storage::storage_check_enabled($storecfg, $storage);
+		# check if storage is enabled and supports vm images on local node
+		check_volume_alloc($storecfg, $storage);
+
 		if ($target) {
 		    # check if storage is available on target node
 		    PVE::Storage::storage_check_enabled($storecfg, $storage, $target);
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index e24c741c..507932d3 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -50,7 +50,7 @@ use PVE::Tools qw(run_command file_read_firstline file_get_contents dir_glob_for
 
 use PVE::QMPClient;
 use PVE::QemuConfig;
-use PVE::QemuServer::Helpers qw(config_aware_timeout min_version windows_version);
+use PVE::QemuServer::Helpers qw(config_aware_timeout min_version windows_version alloc_volume_disk);
 use PVE::QemuServer::Cloudinit;
 use PVE::QemuServer::CGroup;
 use PVE::QemuServer::CPUConfig qw(print_cpu_device get_cpu_options get_cpu_bitness is_native_arch);
@@ -8326,9 +8326,7 @@ sub clone_disk {
 
 	    $size = PVE::Storage::volume_size_info($storecfg, $drive->{file}, 10);
 	}
-	$newvolid = PVE::Storage::vdisk_alloc(
-	    $storecfg, $storeid, $newvmid, $dst_format, $name, ($size/1024)
-	);
+	$newvolid = alloc_volume_disk($storecfg, $storeid, $newvmid, $dst_format, $name, $size/1024);
 	push @$newvollist, $newvolid;
 
 	PVE::Storage::activate_volumes($storecfg, [$newvolid]);
diff --git a/PVE/QemuServer/Helpers.pm b/PVE/QemuServer/Helpers.pm
index a5f6b328..937e32bf 100644
--- a/PVE/QemuServer/Helpers.pm
+++ b/PVE/QemuServer/Helpers.pm
@@ -13,6 +13,7 @@ use base 'Exporter';
 our @EXPORT_OK = qw(
 check_storage_alloc
 check_volume_alloc
+alloc_volume_disk
 min_version
 config_aware_timeout
 parse_number_sets
@@ -197,6 +198,25 @@ sub check_volume_alloc : prototype($$;$) {
     return 1;
 }
 
+=head3 alloc_volume_disk($storecfg, $storeid, $vmid, $format, $name, $size_kb)
+
+Allocates a volume disk image on C<$storeid>, that is defined in C<$storecfg> (which is typically
+retrieved with L<PVE::Storage::config>), with the VM id C<$vmid>, the format C<$format> (e.g.
+C<"raw">), the name C<$name> and the image size in kilobytes C<$size_kb>.
+
+This subroutine will check whether the storage, where the volume disk image should be allocated,
+supports the allocation beforehand with L<check_volume_alloc>.
+
+=cut
+
+sub alloc_volume_disk : prototype($$$$$$) {
+    my ($storecfg, $storeid, $vmid, $format, $name, $size_kb) = @_;
+
+    check_volume_alloc($storecfg, $storeid);
+
+    return PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $format, $name, $size_kb);
+}
+
 sub min_version {
     my ($verstr, $major, $minor, $pve) = @_;
 
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC qemu-server 5/9] api: create_vm: improve checks if storages for disks support vm images

[Daniel Kral]

Adds checks when creating regular volume disks, the EFI disk and the TPM
state disk if the underlying storage for these disks support the content
type 'images'. This adds parameter context to the error message and
checks right before allocating the disk with `alloc_volume_disk`.

This affects the behavior when creating disks through the create vm api
(`create_disks`) and inadvertently when updating the VM configuration
(`update_vm_api`).

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
 PVE/API2/Qemu.pm  | 18 +++++++++---------
 PVE/QemuServer.pm |  2 +-
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index a7931d98..b6bfd68e 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -29,7 +29,7 @@ use PVE::QemuServer;
 use PVE::QemuServer::Cloudinit;
 use PVE::QemuServer::CPUConfig;
 use PVE::QemuServer::Drive;
-use PVE::QemuServer::Helpers qw(check_storage_alloc check_volume_alloc);
+use PVE::QemuServer::Helpers qw(check_storage_alloc check_volume_alloc alloc_volume_disk);
 use PVE::QemuServer::ImportDisk;
 use PVE::QemuServer::Monitor qw(mon_cmd);
 use PVE::QemuServer::Machine;
@@ -147,10 +147,10 @@ my $check_storage_access = sub {
 	} elsif (!$isCDROM && ($volid =~ $PVE::QemuServer::Drive::NEW_DISK_RE)) {
 	    my ($storeid, $size) = ($2 || $default_storage, $3);
 	    die "no storage ID specified (and no default storage)\n" if !$storeid;
-	    $rpcenv->check($authuser, "/storage/$storeid", ['Datastore.AllocateSpace']);
-	    my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
-	    raise_param_exc({ storage => "storage '$storeid' does not support vm images"})
-		if !$scfg->{content}->{images};
+
+	    check_storage_alloc($rpcenv, $authuser, $storeid);
+	    eval { check_volume_alloc($storecfg, $storeid) };
+	    raise_param_exc({ storage => "$@" }) if $@;
 	} else {
 	    PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $vmid, $volid);
 	    if ($storeid) {
@@ -402,7 +402,7 @@ my sub create_disks : prototype($$$$$$$$$$) {
 	    my ($storeid, $size) = ($2 || $default_storage, $3);
 	    die "no storage ID specified (and no default storage)\n" if !$storeid;
 
-	    $size = PVE::Tools::convert_size($size, 'gb' => 'kb'); # vdisk_alloc uses kb
+	    $size = PVE::Tools::convert_size($size, 'gb' => 'kb'); # alloc_volume_disk uses kb
 
 	    my $live_import = $is_live_import && $ds ne 'efidisk0';
 	    my $needs_creation = 1;
@@ -465,7 +465,7 @@ my sub create_disks : prototype($$$$$$$$$$) {
 		}
 
 		if ($needs_creation) {
-		    $size = PVE::Tools::convert_size($size, 'b' => 'kb'); # vdisk_alloc uses kb
+		    $size = PVE::Tools::convert_size($size, 'b' => 'kb'); # alloc_volume_disk uses kb
 		} else {
 		    $disk->{file} = $dst_volid;
 		    $disk->{size} = $size;
@@ -486,9 +486,9 @@ my sub create_disks : prototype($$$$$$$$$$) {
 		} elsif ($ds eq 'tpmstate0') {
 		    # swtpm can only use raw volumes, and uses a fixed size
 		    $size = PVE::Tools::convert_size(PVE::QemuServer::Drive::TPMSTATE_DISK_SIZE, 'b' => 'kb');
-		    $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, "raw", undef, $size);
+		    $volid = alloc_volume_disk($storecfg, $storeid, $vmid, "raw", undef, $size);
 		} else {
-		    $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $fmt, undef, $size);
+		    $volid = alloc_volume_disk($storecfg, $storeid, $vmid, $fmt, undef, $size);
 		}
 		push @$vollist, $volid;
 		$disk->{file} = $volid;
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 507932d3..b40f6f6e 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -8449,7 +8449,7 @@ sub create_efidisk($$$$$$$) {
 
     my $vars_size_b = -s $ovmf_vars;
     my $vars_size = PVE::Tools::convert_size($vars_size_b, 'b' => 'kb');
-    my $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $fmt, undef, $vars_size);
+    my $volid = alloc_volume_disk($storecfg, $storeid, $vmid, $fmt, undef, $vars_size);
     PVE::Storage::activate_volumes($storecfg, [$volid]);
 
     qemu_img_convert($ovmf_vars, $volid, $vars_size_b, undef, 0);
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC qemu-server 6/9] cloudinit: add check if storage for cloudinit disk supports vm images

[Daniel Kral]

Adds a check when a cloudinit disk is created or committed if the
underlying storage supports the content type 'images'. The check will be
done right before allocating the disk image.

This has not been done before, but as the cloudinit disk is recognized
as a disk image in itself, this check should be done to stay consistent.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
I am unsure about these changes as this aligns with the consistency that
disk images should only be allocated on storages that support them, but
this case has not caused the VM to fail to run before.

Are there downsides to adding this? How many people will have VMs that
have a now misconfigured VM as the PVE WebGUI doesn't show any storages
without the content type 'images'.

 PVE/API2/Qemu.pm            | 2 +-
 PVE/QemuServer/Cloudinit.pm | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index b6bfd68e..a0abe44f 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -391,7 +391,7 @@ my sub create_disks : prototype($$$$$$$$$$) {
 
 	    # Initial disk created with 4 MB and aligned to 4MB on regeneration
 	    my $ci_size = PVE::QemuServer::Cloudinit::CLOUDINIT_DISK_SIZE;
-	    my $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $fmt, $name, $ci_size/1024);
+	    my $volid = alloc_volume_disk($storecfg, $storeid, $vmid, $fmt, $name, $ci_size/1024);
 	    $disk->{file} = $volid;
 	    $disk->{media} = 'cdrom';
 	    push @$vollist, $volid;
diff --git a/PVE/QemuServer/Cloudinit.pm b/PVE/QemuServer/Cloudinit.pm
index 4efbdf53..573dd870 100644
--- a/PVE/QemuServer/Cloudinit.pm
+++ b/PVE/QemuServer/Cloudinit.pm
@@ -13,7 +13,7 @@ use JSON;
 use PVE::Tools qw(run_command file_set_contents);
 use PVE::Storage;
 use PVE::QemuServer;
-use PVE::QemuServer::Helpers;
+use PVE::QemuServer::Helpers qw(alloc_volume_disk);
 
 use constant CLOUDINIT_DISK_SIZE => 4 * 1024 * 1024; # 4MiB in bytes
 
@@ -43,7 +43,7 @@ sub commit_cloudinit_disk {
 	$volname =~ m/(vm-$vmid-cloudinit(.\Q$format\E)?)/;
 	my $name = $1;
 	$size = 4 * 1024;
-	PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $format, $name, $size);
+	alloc_volume_disk($storecfg, $storeid, $vmid, $format, $name, $size);
 	$size *= 1024; # vdisk alloc takes KB, qemu-img dd's osize takes byte
     }
     my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC qemu-server 7/9] api: migrate_vm: improve check if target storages support vm images

[Daniel Kral]

Improve the checks when migrating a VM if the target storage supports
the content type 'images' by refactoring it to use `check_storage_alloc`
and `check_volume_alloc` and adding parameter context to the error
message, where it is sensible to do so.

This doesn't change a lot in functionality, except for the allocation of
NBD disks while migrating, which will now do a check if the storage is
enabled and supports vm images before allocating the volume disk image.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
I have only tested migration and remote migration on a surface level for
now, but will follow up with more thorough testing the internal
`mtunnel` API endpoint and setup `qemu-nbd` to test the NBD disk
allocation, as I haven't worked with them before.

 PVE/API2/Qemu.pm   | 39 +++++++++++++++------------------------
 PVE/QemuMigrate.pm | 13 +++++--------
 PVE/QemuServer.pm  | 11 +++++------
 3 files changed, 25 insertions(+), 38 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index a0abe44f..13c1c4e0 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -220,18 +220,6 @@ my $check_storage_access_clone = sub {
    return $sharedvm;
 };
 
-my $check_storage_access_migrate = sub {
-    my ($rpcenv, $authuser, $storecfg, $storage, $node) = @_;
-
-    PVE::Storage::storage_check_enabled($storecfg, $storage, $node);
-
-    $rpcenv->check($authuser, "/storage/$storage", ['Datastore.AllocateSpace']);
-
-    my $scfg = PVE::Storage::storage_config($storecfg, $storage);
-    die "storage '$storage' does not support vm images\n"
-	if !$scfg->{content}->{images};
-};
-
 my $import_from_volid = sub {
     my ($storecfg, $src_volid, $dest_info, $vollist) = @_;
 
@@ -4697,11 +4685,14 @@ __PACKAGE__->register_method({
 		if !defined($storagemap->{identity});
 
 	    foreach my $target_sid (values %{$storagemap->{entries}}) {
-		$check_storage_access_migrate->($rpcenv, $authuser, $storecfg, $target_sid, $target);
+		check_storage_alloc($rpcenv, $authuser, $target_sid);
+		check_volume_alloc($storecfg, $target_sid, $target);
 	    }
 
-	    $check_storage_access_migrate->($rpcenv, $authuser, $storecfg, $storagemap->{default}, $target)
-		if $storagemap->{default};
+	    if ($storagemap->{default}) {
+		check_storage_alloc($rpcenv, $authuser, $storagemap->{default});
+		check_volume_alloc($storecfg, $storagemap->{default}, $target);
+	    }
 
 	    PVE::QemuServer::check_storage_availability($storecfg, $conf, $target)
 		if $storagemap->{identity};
@@ -5652,7 +5643,9 @@ __PACKAGE__->register_method({
 
 	my $storecfg = PVE::Storage::config();
 	foreach my $storeid (PVE::Tools::split_list($storages)) {
-	    $check_storage_access_migrate->($rpcenv, $authuser, $storecfg, $storeid, $node);
+	    check_storage_alloc($rpcenv, $authuser, $storeid);
+	    eval { check_volume_alloc($storecfg, $storeid, $node) };
+	    raise_param_exc({ storages => "$@" }) if $@;
 	}
 
 	foreach my $bridge (PVE::Tools::split_list($bridges)) {
@@ -5829,7 +5822,9 @@ __PACKAGE__->register_method({
 		    my $storeid = $params->{storage};
 		    my $drive = $params->{drive};
 
-		    $check_storage_access_migrate->($rpcenv, $authuser, $state->{storecfg}, $storeid, $node);
+		    check_storage_alloc($rpcenv, $authuser, $storeid);
+		    eval { check_volume_alloc($state->{storecfg}, $storeid, $node) };
+		    raise_param_exc({ storage => "$@" }) if $@;
 
 		    my $storagemap = {
 			default => $storeid,
@@ -5856,13 +5851,9 @@ __PACKAGE__->register_method({
 		'disk-import' => sub {
 		    my ($params) = @_;
 
-		    $check_storage_access_migrate->(
-			$rpcenv,
-			$authuser,
-			$state->{storecfg},
-			$params->{storage},
-			$node
-		    );
+		    check_storage_alloc($rpcenv, $authuser, $params->{storage});
+		    eval { check_volume_alloc($state->{storecfg}, $params->{storage}, $node) };
+		    raise_param_exc({ storage => "$@" }) if $@;
 
 		    $params->{unix} = "/run/qemu-server/$state->{vmid}.storage";
 
diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
index 6591f3f7..0cc582eb 100644
--- a/PVE/QemuMigrate.pm
+++ b/PVE/QemuMigrate.pm
@@ -159,14 +159,11 @@ sub target_storage_check_available {
 
     if (!$self->{opts}->{remote}) {
 	# check if storage is available on target node
-	my $target_scfg = PVE::Storage::storage_check_enabled(
-	    $storecfg,
-	    $targetsid,
-	    $self->{node},
-	);
-	my ($vtype) = PVE::Storage::parse_volname($storecfg, $volid);
-	die "$volid: content type '$vtype' is not available on storage '$targetsid'\n"
-	    if !$target_scfg->{content}->{$vtype};
+	PVE::Storage::storage_check_enabled($storecfg, $targetsid, $self->{node});
+
+	# check if storage supports the volume's content type
+	eval { PVE::QemuServer::Helpers::check_volume_content_type($storecfg, $volid) };
+	die "$volid: $@" if $@;
     }
 }
 
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index b40f6f6e..c07dd7aa 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -2630,13 +2630,12 @@ sub check_storage_availability {
 	return if !$sid;
 
 	# check if storage is available on both nodes
-	my $scfg = PVE::Storage::storage_check_enabled($storecfg, $sid);
+	PVE::Storage::storage_check_enabled($storecfg, $sid);
 	PVE::Storage::storage_check_enabled($storecfg, $sid, $node);
 
-	my ($vtype) = PVE::Storage::parse_volname($storecfg, $volid);
-
-	die "$volid: content type '$vtype' is not available on storage '$sid'\n"
-	    if !$scfg->{content}->{$vtype};
+	# check if storage supports the volume's content type
+	eval { PVE::QemuServer::Helpers::check_volume_content_type($storecfg, $volid) };
+	die "$volid: $@" if $@;
    });
 }
 
@@ -5590,7 +5589,7 @@ sub vm_migrate_alloc_nbd_disks {
 	$format = $defFormat if !$format || !grep { $format eq $_ } $validFormats->@*;
 
 	my $size = $drive->{size} / 1024;
-	my $newvolid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $format, undef, $size);
+	my $newvolid = alloc_volume_disk($storecfg, $storeid, $vmid, $format, undef, $size);
 	my $newdrive = $drive;
 	$newdrive->{format} = $format;
 	$newdrive->{file} = $newvolid;
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC qemu-server 8/9] api: importdisk: improve check if storage supports vm images

[Daniel Kral]

Adds a check in the API call `importovf` and improves the check in
`importdisk` if the target storage for the disk import supports the
content type 'images'. This will add parameter context when the check
fails and also adds a check right before allocating the disk that is
being imported.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
 PVE/CLI/qm.pm                | 12 ++++++------
 PVE/QemuServer/ImportDisk.pm |  4 +++-
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/PVE/CLI/qm.pm b/PVE/CLI/qm.pm
index d3dbf7b4..0f62cece 100755
--- a/PVE/CLI/qm.pm
+++ b/PVE/CLI/qm.pm
@@ -30,7 +30,7 @@ use PVE::API2::Qemu::Agent;
 use PVE::API2::Qemu;
 use PVE::QemuConfig;
 use PVE::QemuServer::Drive;
-use PVE::QemuServer::Helpers;
+use PVE::QemuServer::Helpers qw(check_storage_alloc check_volume_alloc);
 use PVE::QemuServer::Agent qw(agent_available);
 use PVE::QemuServer::ImportDisk;
 use PVE::QemuServer::Monitor qw(mon_cmd);
@@ -595,11 +595,9 @@ __PACKAGE__->register_method ({
 	die "$source: non-existent or non-regular file\n" if (! -f $source);
 
 	my $storecfg = PVE::Storage::config();
-	PVE::Storage::storage_check_enabled($storecfg, $storeid);
 
-	my $target_storage_config =  PVE::Storage::storage_config($storecfg, $storeid);
-	die "storage $storeid does not support vm images\n"
-	    if !$target_storage_config->{content}->{images};
+	eval { check_volume_alloc($storecfg, $storeid) };
+	raise_param_exc({ storage => "$@" }) if $@;
 
 	print "importing disk '$source' to VM $vmid ...\n";
 	my ($drive_id, $volid) = PVE::QemuServer::ImportDisk::do_import($source, $vmid, $storeid, { format => $format });
@@ -727,7 +725,9 @@ __PACKAGE__->register_method ({
 
 	die "$ovf_file: non-existent or non-regular file\n" if (! -f $ovf_file);
 	my $storecfg = PVE::Storage::config();
-	PVE::Storage::storage_check_enabled($storecfg, $storeid);
+
+	eval { check_volume_alloc($storecfg, $storeid) };
+	raise_param_exc({ storage => "$@" }) if $@;
 
 	my $parsed = PVE::QemuServer::OVF::parse_ovf($ovf_file);
 
diff --git a/PVE/QemuServer/ImportDisk.pm b/PVE/QemuServer/ImportDisk.pm
index 132932ae..e32b7155 100755
--- a/PVE/QemuServer/ImportDisk.pm
+++ b/PVE/QemuServer/ImportDisk.pm
@@ -5,6 +5,7 @@ use warnings;
 
 use PVE::Storage;
 use PVE::QemuServer;
+use PVE::QemuServer::Helpers qw(alloc_volume_disk);
 use PVE::Tools qw(run_command extract_param);
 
 # imports an external disk image to an existing VM
@@ -31,7 +32,8 @@ sub do_import {
     warn "format '$format' is not supported by the target storage - using '$dst_format' instead\n"
 	if $format && $format ne $dst_format;
 
-    my $dst_volid = PVE::Storage::vdisk_alloc($storecfg, $storage_id, $vmid, $dst_format, undef, $src_size / 1024);
+    my $dst_volid = alloc_volume_disk(
+	$storecfg, $storage_id, $vmid, $dst_format, undef, $src_size/1024);
 
     my $zeroinit = PVE::Storage::volume_has_feature($storecfg, 'sparseinit', $dst_volid);
 
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC qemu-server 9/9] restore_vm: improve checks if storage supports vm images

[Daniel Kral]

Improves checks if the underlying storage, where VMs are restored to,
support the content type 'images'. This has been already the case for
backup restores, but is refactored to use `check_storage_alloc` and
`check_volume_alloc`.

Adds a check right before allocating a snapshot statefile and a
fleecing VM disk image for backups for consistency with the storage
content type system.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
I am not sure about the changes to the statefile and fleecing images
allocation as I've done them for consistency as well, but could cause
sudden failures, especially if the logic in
`PVE::QemuServer::find_vmstate_storage` could default to the hardcoded
`local` storage, which fails on system where said storage does not
support vm images (which is the default when installing PVE). Also the
fleecing disk image storage is specified when starting the backup job
with the `storeid` parameter in the PVE::VZDump::Plugin, where I'm not
totally sure yet how it is used across the repositories.

 PVE/QemuConfig.pm        |  4 ++--
 PVE/QemuServer.pm        | 22 ++++++++--------------
 PVE/VZDump/QemuServer.pm |  4 ++--
 3 files changed, 12 insertions(+), 18 deletions(-)

diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
index 8e8a7828..d502b41f 100644
--- a/PVE/QemuConfig.pm
+++ b/PVE/QemuConfig.pm
@@ -8,7 +8,7 @@ use PVE::INotify;
 use PVE::JSONSchema;
 use PVE::QemuServer::CPUConfig;
 use PVE::QemuServer::Drive;
-use PVE::QemuServer::Helpers;
+use PVE::QemuServer::Helpers qw(alloc_volume_disk);
 use PVE::QemuServer::Monitor qw(mon_cmd);
 use PVE::QemuServer;
 use PVE::QemuServer::Machine;
@@ -221,7 +221,7 @@ sub __snapshot_save_vmstate {
     my $name = "vm-$vmid-state-$snapname";
     $name .= ".raw" if $scfg->{path}; # add filename extension for file base storage
 
-    my $statefile = PVE::Storage::vdisk_alloc($storecfg, $target, $vmid, 'raw', $name, $size*1024);
+    my $statefile = alloc_volume_disk($storecfg, $target, $vmid, 'raw', $name, $size*1024);
     my $runningmachine = PVE::QemuServer::Machine::get_current_qemu_machine($vmid);
 
     # get current QEMU -cpu argument to ensure consistency of custom CPU models
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index c07dd7aa..fb70caee 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -50,7 +50,7 @@ use PVE::Tools qw(run_command file_read_firstline file_get_contents dir_glob_for
 
 use PVE::QMPClient;
 use PVE::QemuConfig;
-use PVE::QemuServer::Helpers qw(config_aware_timeout min_version windows_version alloc_volume_disk);
+use PVE::QemuServer::Helpers qw(config_aware_timeout min_version windows_version check_storage_alloc check_volume_alloc alloc_volume_disk);
 use PVE::QemuServer::Cloudinit;
 use PVE::QemuServer::CGroup;
 use PVE::QemuServer::CPUConfig qw(print_cpu_device get_cpu_options get_cpu_bitness is_native_arch);
@@ -6688,14 +6688,6 @@ my $restore_cleanup_oldconf = sub {
 my $parse_backup_hints = sub {
     my ($rpcenv, $user, $storecfg, $fh, $devinfo, $options) = @_;
 
-    my $check_storage = sub { # assert if an image can be allocate
-	my ($storeid, $scfg) = @_;
-	die "Content type 'images' is not available on storage '$storeid'\n"
-	    if !$scfg->{content}->{images};
-	$rpcenv->check($user, "/storage/$storeid", ['Datastore.AllocateSpace'])
-	    if $user ne 'root@pam';
-    };
-
     my $virtdev_hash = {};
     while (defined(my $line = <$fh>)) {
 	if ($line =~ m/^\#qmdump\#map:(\S+):(\S+):(\S*):(\S*):$/) {
@@ -6714,8 +6706,9 @@ my $parse_backup_hints = sub {
 	    $devinfo->{$devname}->{format} = $format;
 	    $devinfo->{$devname}->{storeid} = $storeid;
 
-	    my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
-	    $check_storage->($storeid, $scfg); # permission and content type check
+	    # permission and content type check
+	    check_storage_alloc($rpcenv, $user, $storeid);
+	    check_volume_alloc($storecfg, $storeid);
 
 	    $virtdev_hash->{$virtdev} = $devinfo->{$devname};
 	} elsif ($line =~ m/^((?:ide|sata|scsi)\d+):\s*(.*)\s*$/) {
@@ -6728,7 +6721,9 @@ my $parse_backup_hints = sub {
 		my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
 		my $format = qemu_img_format($scfg, $volname); # has 'raw' fallback
 
-		$check_storage->($storeid, $scfg); # permission and content type check
+		# permission and content type check
+		check_storage_alloc($rpcenv, $user, $storeid);
+		check_volume_alloc($storecfg, $storeid);
 
 		$virtdev_hash->{$virtdev} = {
 		    format => $format,
@@ -6773,8 +6768,7 @@ my $restore_allocate_devices = sub {
 	    }
 	}
 
-	my $volid = PVE::Storage::vdisk_alloc(
-	    $storecfg, $storeid, $vmid, $d->{format}, $name, $alloc_size);
+	my $volid = alloc_volume_disk($storecfg, $storeid, $vmid, $d->{format}, $name, $alloc_size);
 
 	print STDERR "new volume ID is '$volid'\n";
 	$d->{volid} = $volid;
diff --git a/PVE/VZDump/QemuServer.pm b/PVE/VZDump/QemuServer.pm
index 012c9210..0037f648 100644
--- a/PVE/VZDump/QemuServer.pm
+++ b/PVE/VZDump/QemuServer.pm
@@ -26,7 +26,7 @@ use PVE::Format qw(render_duration render_bytes);
 
 use PVE::QemuConfig;
 use PVE::QemuServer;
-use PVE::QemuServer::Helpers;
+use PVE::QemuServer::Helpers qw(alloc_volume_disk);
 use PVE::QemuServer::Machine;
 use PVE::QemuServer::Monitor qw(mon_cmd);
 
@@ -553,7 +553,7 @@ my sub allocate_fleecing_images {
 
 		my $size = PVE::Tools::convert_size($di->{size}, 'b' => 'kb');
 
-		$di->{'fleece-volid'} = PVE::Storage::vdisk_alloc(
+		$di->{'fleece-volid'} = alloc_volume_disk(
 		    $self->{storecfg}, $fleecing_storeid, $vmid, $format, $name, $size);
 
 		$n++;
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [RFC qemu-server 1/9] test: cfg2cmd: expect error for invalid volume's storage content type

[Fiona Ebner]

Am 16.09.24 um 18:38 schrieb Daniel Kral:
> @@ -68,7 +72,7 @@ my $base_env = {
>  		content => {
>  		    images => 1,
>  		}
> -	    }
> +	    },

Nit: unrelated hunk should be it's own patch

>  	}
>      },
>      vmid => 8006,



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [RFC qemu-server 2/9] cfg2cmd: improve error message for invalid volume storage content type

[Fiona Ebner]

Am 16.09.24 um 18:38 schrieb Daniel Kral:
> diff --git a/PVE/QemuServer/Helpers.pm b/PVE/QemuServer/Helpers.pm
> index 0afb6317..9d0f24aa 100644
> --- a/PVE/QemuServer/Helpers.pm
> +++ b/PVE/QemuServer/Helpers.pm
> @@ -106,6 +106,51 @@ sub vm_running_locally {
>      return;
>  }
>  
> +=head3 check_storage_content_type($storecfg, $storeid [, $content_type])

Usually, "assert_" rather than "check_" is used for such helpers that
die upon failing the check, so maybe "assert_content_type_supported"

Nit: there is no blank line below the header when viewed with perldoc

I'd rather have the content type be non-optional. And actually, the
helper should live in the storage library, because it doesn't depend on
anything outside it.

> +
> +Checks whether the storage with the identifier C<$storeid>, that is defined in C<$storecfg>,
> +supports the content type C<$content_type>. If the C<$content_type> is undefined, it will default
> +to the value C<"images">.
> +
> +If the check fails, the subroutine will C<die> with an error message containing the storage and
> +content type that is not supported.
> +
> +Returns C<1> if the check is successful.
> +
> +=cut
> +
> +sub check_storage_content_type : prototype($$;$) {
> +    my ($storecfg, $storeid, $content_type) = @_;
> +
> +    $content_type = "images" if !defined($content_type);
> +    my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
> +    die "storage '$storeid' does not support content type '$content_type'\n"
> +	if !$scfg->{content}->{$content_type};
> +
> +    return 1;
> +}
> +
> +=head3 check_volume_content_type($storecfg, $volid)

assert_volume_content_type_supported

should/could also live in the storage library

> +
> +Checks whether the volume with the identifier C<$volid>, that is defined in C<$storecfg>, supports
> +the content type, which is determined by L<PVE::Storage::parse_volname>.
> +
> +If the check fails, the subroutine will C<die> with an error message containing the storage and
> +content type that is not supported.
> +
> +Returns C<1> if the check is successful.
> +
> +=cut
> +
> +sub check_volume_content_type : prototype($$) {
> +    my ($storecfg, $volid) = @_;
> +
> +    my ($storeid) = PVE::Storage::parse_volume_id($volid);
> +    my ($vtype) = PVE::Storage::parse_volname($storecfg, $volid);
> +
> +    return check_storage_content_type($storecfg, $storeid, $vtype);
> +}
> +
>  sub min_version {
>      my ($verstr, $major, $minor, $pve) = @_;
>  


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [RFC qemu-server 3/9] fix #5284: move_vm: add check if target storage supports vm images

[Fiona Ebner]

For issues like these, it's often nice to start out with the fix and put
bigger refactorings later. Then the fix can already be applied up-front
while discussing the bigger changes.

Am 16.09.24 um 18:38 schrieb Daniel Kral:
> diff --git a/PVE/QemuServer/Helpers.pm b/PVE/QemuServer/Helpers.pm
> index 9d0f24aa..a5f6b328 100644
> --- a/PVE/QemuServer/Helpers.pm
> +++ b/PVE/QemuServer/Helpers.pm
> @@ -11,6 +11,8 @@ use PVE::ProcFSTools;
>  
>  use base 'Exporter';
>  our @EXPORT_OK = qw(
> +check_storage_alloc
> +check_volume_alloc
>  min_version
>  config_aware_timeout
>  parse_number_sets
> @@ -151,6 +153,50 @@ sub check_volume_content_type : prototype($$) {
>      return check_storage_content_type($storecfg, $storeid, $vtype);
>  }
>  
> +=head3 check_storage_alloc($rpcenv, $user, $storeid)
> +
> +Checks whether the C<$user> has the permissions in the C<$rpcenv> to allocate space in the storage
> +with the identifier C<$storeid>.
> +
> +
> +If the check fails, the subroutine will C<die> with a permission exception inside the subroutine
> +L<PVE::RPCEnvironment::check>.
> +
> +Returns C<1> if the check is successful.
> +
> +=cut
> +
> +sub check_storage_alloc : prototype($$$) {

I'd rather call it assert_storage_alloc_permission

> +    my ($rpcenv, $user, $storeid) = @_;
> +
> +    if (defined($rpcenv) && defined($user)) {

Should we rather assert these? It should not be called in a context
where we don't have them. In fact, I'd prefer this to be a private
helper in the API module directly. But I'm not fully convinced we need a
helper for this to begin with, the actual code is just two lines (or one
statement).

> +	$rpcenv->check($user, "/storage/$storeid", ['Datastore.AllocateSpace'])
> +	    if $user ne 'root@pam';
> +    }
> +
> +    return 1;
> +}
> +
> +=head3 check_volume_alloc($storecfg, $storeid, $node)
> +
> +Checks whether the volume with the identifier C<$volid>, that is defined in C<$storecfg> (which
> +is typically retrieved with L<PVE::Storage::config>), is enabled an supports volume images.
> +
> +If the check fails, it will C<die> with an error message.
> +
> +Returns C<1> if the check is successful.
> +
> +=cut
> +
> +sub check_volume_alloc : prototype($$;$) {

Again, "assert_" and "_permission"

should/could also live in the storage library as it does not depend on
anything else

> +    my ($storecfg, $storeid, $node) = @_;
> +
> +    PVE::Storage::storage_check_enabled($storecfg, $storeid, $node);
> +    check_storage_content_type($storecfg, $storeid);
> +
> +    return 1;
> +}
> +
>  sub min_version {
>      my ($verstr, $major, $minor, $pve) = @_;
>  



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [RFC qemu-server 4/9] api: clone_vm: add check if storage supports vm images

[Fiona Ebner]

Am 16.09.24 um 18:38 schrieb Daniel Kral:
> @@ -197,6 +198,25 @@ sub check_volume_alloc : prototype($$;$) {
>      return 1;
>  }
>  
> +=head3 alloc_volume_disk($storecfg, $storeid, $vmid, $format, $name, $size_kb)
> +
> +Allocates a volume disk image on C<$storeid>, that is defined in C<$storecfg> (which is typically
> +retrieved with L<PVE::Storage::config>), with the VM id C<$vmid>, the format C<$format> (e.g.
> +C<"raw">), the name C<$name> and the image size in kilobytes C<$size_kb>.
> +
> +This subroutine will check whether the storage, where the volume disk image should be allocated,
> +supports the allocation beforehand with L<check_volume_alloc>.
> +
> +=cut
> +
> +sub alloc_volume_disk : prototype($$$$$$) {

I think the volume+disk is redundant. Maybe simply allocate_image or
allocate_volume?

Thinking about this, are there any cases where we do not want to have
the checks done first? I.e. can we simply add the checks as part of
vdisk_alloc itself (would require passing along the content type for the
checks but would avoid the need for this helper)?

> +    my ($storecfg, $storeid, $vmid, $format, $name, $size_kb) = @_;
> +
> +    check_volume_alloc($storecfg, $storeid);
> +
> +    return PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $format, $name, $size_kb);
> +}
> +
>  sub min_version {
>      my ($verstr, $major, $minor, $pve) = @_;
>  



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [RFC qemu-server 7/9] api: migrate_vm: improve check if target storages support vm images

[Fiona Ebner]

Am 16.09.24 um 18:38 schrieb Daniel Kral:
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index a0abe44f..13c1c4e0 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -220,18 +220,6 @@ my $check_storage_access_clone = sub {
>     return $sharedvm;
>  };
>  
> -my $check_storage_access_migrate = sub {
> -    my ($rpcenv, $authuser, $storecfg, $storage, $node) = @_;
> -
> -    PVE::Storage::storage_check_enabled($storecfg, $storage, $node);
> -
> -    $rpcenv->check($authuser, "/storage/$storage", ['Datastore.AllocateSpace']);
> -
> -    my $scfg = PVE::Storage::storage_config($storecfg, $storage);
> -    die "storage '$storage' does not support vm images\n"
> -	if !$scfg->{content}->{images};
> -};

I'd prefer to keep this helper and have it call the new ones.

> -
>  my $import_from_volid = sub {
>      my ($storecfg, $src_volid, $dest_info, $vollist) = @_;
>  


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [RFC qemu-server 9/9] restore_vm: improve checks if storage supports vm images

[Fiona Ebner]

Am 16.09.24 um 18:38 schrieb Daniel Kral:
> Improves checks if the underlying storage, where VMs are restored to,
> support the content type 'images'. This has been already the case for
> backup restores, but is refactored to use `check_storage_alloc` and
> `check_volume_alloc`.
> 
> Adds a check right before allocating a snapshot statefile and a
> fleecing VM disk image for backups for consistency with the storage
> content type system.
> 
> Signed-off-by: Daniel Kral <d.kral@proxmox.com>
> ---
> I am not sure about the changes to the statefile and fleecing images
> allocation as I've done them for consistency as well, but could cause
> sudden failures, especially if the logic in
> `PVE::QemuServer::find_vmstate_storage` could default to the hardcoded
> `local` storage, which fails on system where said storage does not
> support vm images (which is the default when installing PVE). Also the
> fleecing disk image storage is specified when starting the backup job
> with the `storeid` parameter in the PVE::VZDump::Plugin, where I'm not
> totally sure yet how it is used across the repositories.
> 

The part about the vmstate images should be part of the commit message,
but is also the reason we can't go for it right now. I do think the
fallback to 'local' can trigger for a VM without disks. We can add a
comment there to fix it up for Proxmox VE 9.0 (i.e. don't default to
local storage anymore, but require an explicit vmstate storage for such
VMs) where we can do such breaking changes.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel