On July 29, 2025 1:15 pm, Wolfgang Bumiller wrote:
> Prepares the stoplevel PVE::Storage API updates as well as adding the
> new vtype subdirs to the base plugin's vtype subdir hash.
>
> The new types are "vm-vol" and "ct-vol". They represent VM and
> container volumes, respectively. The "images" and "rootdir" types are
> considered legacy/deprecated, as the "rootdir" type was not properly
> used, and container volumes were technically of type "images", with
> the "rootdir" case "hacked in" by checking the existing VMs.
>
> To more easily transition, the "images" type is now also a "supertype"
> of "vm-vol", and the "rootdir" type a "supertype" of "ct-vol".
>
> - `get_images_dir()` is replaced by `get_vm_volume_dir()`
> - `get_private_dir()` is dropped as it is an openvz leftover
> - `get_ct_volume_dir()` is added its stead
>
> We now also unify the vtypes and content types. As such,
> `content-dirs` can now include separate dirs for `vm-vol` and
> `ct-vol`.
> This is now also taken into account in `path_to_volume_id()` which
> tries to match file system paths to a storage and content type.
>
> The following subs also get a $vtype parameter:
> - `vdisk_alloc()`
> - `vdisk_clone()`
> - `volume_import()`
>
> `volume_list()`'s `$content` parameter allows `vm-vol` and
> `ct-vol` as type.
>
> New helpers are added:
> - is_content_type_covered($content_hash, $content_type)
> Check if the contents listed in the hash(set) allows the provided
> $content_type.
> Meaning the content type is in the set *or* as supertype is in the
> set (see above).
> - storage_check_type_allowed($cfg, $storeid, $type [, $noerr])
> Access to the above in our usual public storag API signature.
>
> Finally: the content type completion also gets the new content types.
>
> Signed-off-by: Wolfgang Bumiller <w.bumil...@proxmox.com>
> ---
> src/PVE/GuestImport.pm | 2 +-
> src/PVE/Storage.pm | 113 ++++++++++++++++++++++++++++++--------
> src/PVE/Storage/Plugin.pm | 4 +-
> 3 files changed, 95 insertions(+), 24 deletions(-)
>
> diff --git a/src/PVE/GuestImport.pm b/src/PVE/GuestImport.pm
> index 3d59dcd..ec2f09d 100644
> --- a/src/PVE/GuestImport.pm
> +++ b/src/PVE/GuestImport.pm
> @@ -40,7 +40,7 @@ sub extract_disk_from_import_file {
>
> my $ova_path = PVE::Storage::path($cfg, $archive_volid);
>
> - my $tmpdir = PVE::Storage::get_image_dir($cfg, $target_storeid, $vmid);
> + my $tmpdir = PVE::Storage::get_vm_volume_dir($cfg, $target_storeid,
> $vmid);
> my $pid = $$;
> $tmpdir .= "/tmp_${pid}_${vmid}";
> mkpath $tmpdir;
> diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
> index da53beb..91a0278 100755
> --- a/src/PVE/Storage.pm
> +++ b/src/PVE/Storage.pm
> @@ -547,24 +547,24 @@ sub volume_snapshot_info {
> return $plugin->volume_snapshot_info($scfg, $storeid, $volname);
> }
>
> -sub get_image_dir {
> +sub get_vm_volume_dir {
> my ($cfg, $storeid, $vmid) = @_;
>
> my $scfg = storage_config($cfg, $storeid);
> my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
>
> - my $path = $plugin->get_subdir($scfg, 'images');
> + my $path = $plugin->get_subdir($scfg, 'vm-vol');
>
> return $vmid ? "$path/$vmid" : $path;
> }
this one is only used by GuestImport above AFAICT, should we use this
opportunity and give it a better name to make it clear that this should
not be used in general? we use it there to get a base path for creating
a tmpdir inside for extracting the OVA, and might want to switch to some
other subdir in the future..
>
> -sub get_private_dir {
> +sub get_ct_volume_dir {
> my ($cfg, $storeid, $vmid) = @_;
>
> my $scfg = storage_config($cfg, $storeid);
> my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
>
> - my $path = $plugin->get_subdir($scfg, 'rootdir');
> + my $path = $plugin->get_subdir($scfg, 'ct-vol');
>
> return $vmid ? "$path/$vmid" : $path;
> }
this one is not used anywhere, should we drop it?
> @@ -737,22 +737,26 @@ sub path_to_volume_id {
> next if !$scfg->{path};
> my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
> my $imagedir = $plugin->get_subdir($scfg, 'images');
> + my $vmdir = $plugin->get_subdir($scfg, 'vm-vol');
> + my $ctdir = $plugin->get_subdir($scfg, 'ct-vol');
> my $isodir = $plugin->get_subdir($scfg, 'iso');
> my $tmpldir = $plugin->get_subdir($scfg, 'vztmpl');
> my $backupdir = $plugin->get_subdir($scfg, 'backup');
> my $snippetsdir = $plugin->get_subdir($scfg, 'snippets');
> my $importdir = $plugin->get_subdir($scfg, 'import');
>
> - if ($path =~ m!^\Q$imagedir\E/(\d+)/([^/\s]+)$!) {
> - my $vmid = $1;
> - my $name = $2;
> + if ($path =~
> m!^(\Q$imagedir\E|\Q$vmdir\E|\Q$ctdir\E)/(\d+)/([^/\s]+)$!) {
> + my $subdir = $1;
> + my $vmid = $2;
> + my $name = $3;
>
> my $vollist = $plugin->list_images($sid, $scfg, $vmid);
> foreach my $info (@$vollist) {
> my ($storeid, $volname) = parse_volume_id($info->{volid});
> + my ($vtype) = parse_volname($cfg, $info->{volid});
> my $volpath = $plugin->path($scfg, $volname, $storeid);
> if ($volpath eq $path) {
> - return ('images', $info->{volid});
> + return ($vtype, $info->{volid});
> }
> }
> } elsif ($path =~ m!^\Q$isodir\E/([^/]+$ISO_EXT_RE_0)$!) {
> @@ -822,7 +826,7 @@ sub qemu_blockdev_options {
>
> my ($vtype) = $plugin->parse_volname($volname);
> die "cannot use volume of type '$vtype' as a QEMU blockdevice\n"
> - if $vtype ne 'images' && $vtype ne 'iso' && $vtype ne 'import';
> + if $vtype ne 'vm-vol' && $vtype ne 'images' && $vtype ne 'iso' &&
> $vtype ne 'import';
>
> my $blockdev =
> $plugin->qemu_blockdev_options($scfg, $storeid, $volname,
> $machine_version, $options);
> @@ -884,6 +888,7 @@ my $volume_import_prepare = sub {
> my $with_snapshots = $opts->{with_snapshots} ? 1 : 0;
> my $migration_snapshot = $opts->{migration_snapshot} ? 1 : 0;
> my $allow_rename = $opts->{allow_rename} ? 1 : 0;
> + my $vtype = $opts->{vtype};
>
> my $recv = ['pvesm', 'import', $volid, $format, $path,
> '-with-snapshots', $with_snapshots];
> if (defined($snapshot)) {
> @@ -892,6 +897,9 @@ my $volume_import_prepare = sub {
> if ($migration_snapshot) {
> push @$recv, '-delete-snapshot', $snapshot;
> }
> + if ($vtype) {
> + push @$recv, '-vtype', $vtype;
> + }
should we skip this if `$vtype` is `images`, to improve backwards
compat? otherwise this would break migrating back from new to old nodes,
right?
> push @$recv, '-allow-rename', $allow_rename;
>
> if (defined($base_snapshot)) {
> @@ -1099,7 +1107,7 @@ sub storage_migrate {
> }
>
> sub vdisk_clone {
> - my ($cfg, $volid, $vmid, $snap) = @_;
> + my ($cfg, $volid, $vmid, $snap, $vtype) = @_;
>
> my ($storeid, $volname) = parse_volume_id($volid);
>
> @@ -1115,7 +1123,7 @@ sub vdisk_clone {
> $scfg->{shared},
> undef,
> sub {
> - my $volname = $plugin->clone_image($scfg, $storeid, $volname,
> $vmid, $snap);
> + my $volname = $plugin->clone_image($scfg, $storeid, $volname,
> $vmid, $snap, $vtype);
> return "$storeid:$volname";
> },
> );
> @@ -1168,8 +1176,52 @@ sub unmap_volume {
> return $plugin->unmap_volume($storeid, $scfg, $volname, $snapname);
> }
>
> +=head3 is_content_type_covered($content_hash, $content_type)
> +
> +Check if the C<$content_hash> allows content of type C<$content_type>.
> +
> +Note that the legacy types C<images> and C<rootdir> will allow content of
> type C<vm-vol> and
> +C<ct-vol> respectively, but not the other way round.
> +
> +For anything else this just checks whether C<$content_hash->{$content_type}>
> is set.
> +
> +=cut
> +
> +my sub is_content_type_covered : prototype($$) {
> + my ($content_hash, $content_type) = @_;
> +
> + return
> + $content_hash->{$content_type}
> + || ($content_type eq 'vm-vol' && $content_hash->{images})
> + || ($content_type eq 'ct-vol' && $content_hash->{rootdir});
> +}
> +
> +=head3 storage_check_type_allowed($cfg, $storeid, $type [, $noerr])
should this be called by qemu-server? see comments there ;)
> +
> +Check if a storage allows content of type C<$type>.
> +
> +Note that the legacy types C<images> and C<rootdir> will allow content of
> type C<vm-vol> and
> +C<ct-vol> respectively, but not the other way round.
> +
> +If C<$noerr> is true, returns true if the type is allowed, false otherwise.
> +If C<$noerr> is false, dies if the type is not allowed, returns true
> otherwise.
> +
> +=cut
> +
> +sub storage_check_type_allowed : prototype($$$;$) {
> + my ($cfg, $storeid, $content_type, $noerr) = @_;
> +
> + my $scfg = storage_config($cfg, $storeid);
> +
> + return !!1 if is_content_type_covered($scfg->{content}, $content_type);
> + return !!0 if $noerr;
> + die "storage '$storeid' doees not allow content of type
> '$content_type'\n";
typo 'doees'
> +}
> +
> sub vdisk_alloc {
> - my ($cfg, $storeid, $vmid, $fmt, $name, $size) = @_;
> + my ($cfg, $storeid, $vmid, $fmt, $name, $size, $vtype) = @_;
> +
> + die "vdisk_alloc without vtype not allowed anymore\n" if
> !defined($vtype);
>
> die "no storage ID specified\n" if !$storeid;
>
> @@ -1187,6 +1239,8 @@ sub vdisk_alloc {
>
> activate_storage($cfg, $storeid);
>
> + storage_check_type_allowed($cfg, $storeid, $vtype);
> +
> # lock shared storage
> return $plugin->cluster_lock_storage(
> $storeid,
> @@ -1195,7 +1249,7 @@ sub vdisk_alloc {
> sub {
> my $old_umask = umask(umask | 0037);
> my $volname =
> - eval { $plugin->alloc_image($storeid, $scfg, $vmid, $fmt,
> $name, $size) };
> + eval { $plugin->alloc_image($storeid, $scfg, $vmid, $fmt,
> $name, $size, $vtype) };
> my $err = $@;
> umask $old_umask;
> die $err if $err;
> @@ -1263,8 +1317,10 @@ sub vdisk_list {
> next if $storeid && $storeid ne $sid;
> next if !storage_check_enabled($cfg, $sid, undef, 1);
> my $content = $ids->{$sid}->{content};
> - next if defined($ctype) && !$content->{$ctype};
> - next if !($content->{rootdir} || $content->{images});
> + next if defined($ctype) && !is_content_type_covered($content,
> $ctype);
> + next
> + if !(is_content_type_covered($content, 'vm-vol')
> + || is_content_type_covered($content, 'ct-vol'));
> push @$storage_list, $sid;
> }
> }
> @@ -1278,7 +1334,7 @@ sub vdisk_list {
>
> my $scfg = $ids->{$sid};
> my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
> - $res->{$sid} = $plugin->list_images($sid, $scfg, $vmid, $vollist,
> $cache);
> + $res->{$sid} = $plugin->list_images($sid, $scfg, $vmid, $vollist,
> $cache, $ctype);
> @{ $res->{$sid} } = sort { lc($a->{volid}) cmp lc($b->{volid}) } @{
> $res->{$sid} }
> if $res->{$sid};
> }
> @@ -1318,13 +1374,15 @@ sub template_list {
> sub volume_list {
> my ($cfg, $storeid, $vmid, $content) = @_;
>
> + # We don't need to extend this by 'vm-vol' and 'ct-vol' as they are
> included by 'images' and
> + # 'rootdir'.
> my @ctypes = qw(rootdir images vztmpl iso backup snippets import);
I am not sure that's a good approach - we want to sunset 'images' and
'rootdir' at some point, and we don't allow users to opt-into not
allowing the legacy types if we do this?
>
> my $cts = $content ? [$content] : [@ctypes];
>
> my $scfg = PVE::Storage::storage_config($cfg, $storeid);
>
> - $cts = [grep { defined($scfg->{content}->{$_}) } @$cts];
> + $cts = [grep { is_content_type_covered($scfg->{content}, $_) } @$cts];
>
> my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
>
> @@ -2121,8 +2179,18 @@ sub volume_export : prototype($$$$$$$) {
> );
> }
>
> -sub volume_import : prototype($$$$$$$$) {
> - my ($cfg, $fh, $volid, $format, $snapshot, $base_snapshot,
> $with_snapshots, $allow_rename) = @_;
> +sub volume_import : prototype($$$$$$$$$) {
> + my (
> + $cfg,
> + $fh,
> + $volid,
> + $format,
> + $snapshot,
> + $base_snapshot,
> + $with_snapshots,
> + $allow_rename,
> + $import_vtype,
> + ) = @_;
>
> my ($storeid, $volname) = parse_volume_id($volid, 1);
> die "cannot import into volume '$volid'\n" if !$storeid;
> @@ -2138,6 +2206,7 @@ sub volume_import : prototype($$$$$$$$) {
> $base_snapshot,
> $with_snapshots,
> $allow_rename,
> + $import_vtype,
> ) // $volid;
> }
>
> @@ -2288,7 +2357,7 @@ sub complete_storage_enabled {
> sub complete_content_type {
> my ($cmdname, $pname, $cvalue) = @_;
>
> - return [qw(rootdir images vztmpl iso backup snippets)];
> + return [qw(vm-vol ct-vol rootdir images vztmpl iso backup snippets)];
> }
>
> sub complete_volume {
> @@ -2326,7 +2395,7 @@ sub complete_volume {
> }
>
> sub rename_volume {
> - my ($cfg, $source_volid, $target_vmid, $target_volname) = @_;
> + my ($cfg, $source_volid, $target_vmid, $target_volname, $vtype) = @_;
>
> die "no source volid provided\n" if !$source_volid;
> die "no target VMID or target volname provided\n" if !$target_vmid &&
> !$target_volname;
> @@ -2346,7 +2415,7 @@ sub rename_volume {
> undef,
> sub {
> return $plugin->rename_volume(
> - $scfg, $storeid, $source_volname, $target_vmid,
> $target_volname,
> + $scfg, $storeid, $source_volname, $target_vmid,
> $target_volname, $vtype,
> );
> },
> );
> diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
> index 0107976..ae9d673 100644
> --- a/src/PVE/Storage/Plugin.pm
> +++ b/src/PVE/Storage/Plugin.pm
> @@ -771,7 +771,9 @@ sub parse_volname {
>
> my $vtype_subdirs = {
> images => 'images',
> - rootdir => 'private',
> + rootdir => 'images',
> + 'vm-vol' => 'vms',
> + 'ct-vol' => 'cts',
> iso => 'template/iso',
> vztmpl => 'template/cache',
> backup => 'dump',
> --
> 2.47.2
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
