[pve-devel] [PATCH storage 17/26] plugins: add vtype parameter to alloc_image

[Max R. Carrara]

On Wed Jul 30, 2025 at 4:26 PM CEST, Fabian Grünbichler wrote:
> On July 30, 2025 4:05 pm, Max R. Carrara wrote:
> > On Wed Jul 30, 2025 at 4:00 PM CEST, Max R. Carrara wrote:
> >> On Tue Jul 29, 2025 at 1:15 PM CEST, Wolfgang Bumiller wrote:
> >> > Signed-off-by: Wolfgang Bumiller <w.bumiller at proxmox.com>
> >> > --- a/src/PVE/Storage/LvmThinPlugin.pm
> >> > +++ b/src/PVE/Storage/LvmThinPlugin.pm
> >> > @@ -122,12 +122,23 @@ my $set_lv_autoactivation = sub {
> >> >  };
> >> >  
> >> >  sub alloc_image {
> >> > -    my ($class, $storeid, $scfg, $vmid, $fmt, $name, $size) = @_;
> >> > +    my ($class, $storeid, $scfg, $vmid, $fmt, $name, $size, $vtype) = @_;
> >> >  
> >> >      die "unsupported format '$fmt'" if $fmt ne 'raw';
> >> >  
> >> > -    die "illegal name '$name' - should be 'vm-$vmid-*'\n"
> >> > -        if $name && $name !~ m/^vm-$vmid-/;
> >> > +    if ($name) {
> >> > +        if (defined($vtype) && $vtype eq 'vm-vol') {
> >> > +            die "illegal name '$name' - should be 'vol-vm-$vmid-*'\n"
> >> > +                if $name !~ m/^vol-vm-$vmid-/;
> >> > +        } elsif (defined($vtype) && $vtype eq 'ct-vol') {
> >> > +            die "illegal name '$name' - should be 'vol-ct-$vmid-*'\n"
> >> > +                if $name !~ m/^vol-ct-$vmid-/;
> >> > +        } else {
> >> > +            die "illegal name '$name'"
> >> > +                . " - should be 'vm-$vmid-*', 'vol-vm-$vmid-*' or 'vol-ct-$vmid-*'\n"
> >> > +                if $name !~ m/^(?:vol-vm|vol-ct|vm)-$vmid-/;
> >> > +        }
> >> > +    }
> >>
> >> ^ This currently trips up when you try to make a snapshot on a VM disk
> >> following the new naming scheme:
> >>
> >> TASK ERROR: illegal name 'vm-200-state-foo' - should be 'vol-vm-200-*'
> >>
> >> Did some debugging and stacktrace-diving--turns out that
> >> `PVE::QemuConfig::__snapshot_save_vmstate()` passes the wrong name for
> >> the snapshot.
> >>
> >> Should we keep the old snapshot naming scheme for 'vm-$vmid-*' volumes
> >> or also use the new one from now on?
> >>
> >> With that being said, perhaps this could be a good opportunity to let
> >> `PVE::Storage::vdisk_alloc()` decide on the snapshot's name instead?
> >> As in, have `__snapshot_save_vmstate()` just pass on the plain name,
> >> that is "foo" instead of e.g. "vm-666-state-foo" since the $vmid is
> >> passed along anyway (and the vtype now is, too).
> >>
> >> NOTE: This also happens for directory storage too, and I'm assuming
> >> others as well. However, containers seem to be fine..?
> > 
> > I forgot to mention: VM disks with the legacy naming scheme work fine.
> > Just double checked for CTs--CT disks with both the legacy naming and
> > new naming scheme work fine (on lvm-thin).
>
> containers don't have state volumes in the first place, so it's not
> really surprising they don't break ;)

Yeah I realized that after I sent my response 🤦

>
> this is a bit of a conundrum - if a plugin doesn't yet support vtypes,
> it will potentially only handle the old naming scheme. if it does
> support vtypes, it might only handle the new naming scheme if we pass
> the proper vtype..
>
> we discussed introducing sub types for such things, but that would also
> require some query or fallback mode..

Yeah okay I see, that's tricky...

>
> > 
> >>
> >> >  
> >> >      my $vgs = PVE::Storage::LVMPlugin::lvm_vgs();
> >> >  
> >> > @@ -135,7 +146,7 @@ sub alloc_image {
> >> >  
> >> >      die "no such volume group '$vg'\n" if !defined($vgs->{$vg});
> >> >  
> >> > -    $name = $class->find_free_diskname($storeid, $scfg, $vmid)
> >> > +    $name = $class->find_free_diskname($storeid, $scfg, $vmid, undef, 0, $vtype)
> >> >          if !$name;
> >> >  
> >> >      my $cmd = [
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel at lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel