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

[Fabian Grünbichler]

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

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

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