Re: [pve-devel] [PATCH storage 08/26] prepare for vm-vol and ct-vol content and vtypes

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

On Wed, Jul 30, 2025 at 10:38:19AM +0200, Fabian Grünbichler wrote:
> 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.bumiller@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..

Given that that's in the same repository and you suggested removing the
ct variant, I'll just make this a private helper inside the guest import
module.

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

I'm generally not a fan of this set of functions as they're limited to
'path' based storages. I'm fine with removing this.
Given the one above is not meant for general use either...

But that probably goes for the entire set of `get_*_dir` functions, no?

> 
> > @@ -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?

Yeah. Probably also shouldn't pass 'rootdir'.

> 
> >      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 ;)

Probably.

> 
> > +
> > +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'

Fixed.

> 
> > +}
> > +
> >  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?

Right, I forgot we'd be filtering them out a few lines below.


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