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

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On July 30, 2025 4:49 pm, Max R. Carrara wrote:
> 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@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...

we could parse the new name and see if the plugin says it's the new
vtype -> if it does, we can use the new name. if it fails parsing, we
can fallback to the old name?


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