Re: [pve-devel] [PATCH storage 10/26] plugins: update volname parsing for new naming convention

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

On July 30, 2025 10:53 am, Wolfgang Bumiller wrote:
> On Wed, Jul 30, 2025 at 10:37:27AM +0200, Fabian Grünbichler wrote:
>> On July 29, 2025 1:15 pm, Wolfgang Bumiller wrote:
>> > - ESXi, ISCSIDirect, ISCSI:
>> >   Volumes are always vm volumes.
>> > - LVM:
>> >   New volumes use a `vol-vm-` or `vol-ct-` prefix.
>> > - Dir based, LvmThin, RBD:
>> >   Like LVM, but for base images a `base-` prefix is added
>> >   *additionally*, instead of *replacing* the `vm-` portion like we
>> >   used to.
>> > - ZFS:
>> >   VMs: `vol-vm-` prefix.
>> >   Containers: `subvol-ct-` prefix.
>> 
>> we could get rid of this historic anomaly while we're at it, and make
>> the new vtype just encoded as vol-ct for consistency - that those are
>> always filesystem datasets, and the vol-vm ones always zvols is an
>> implementation detail then?
> 
> But this would also make it difficult if we ever want to allow zvols
> with file systems. And given that the code falls through to Plugin.pm in
> some places (and dir plugins can theoretically have size=0 containers
> dirs), it seemed much safer to stick with a scheme that works across
> more storage types. Eg. we fall through to Plugin.pm via
> `get_next_vm_diskname()` which is the thing that causes the "subvol-ct-"
> prefix for "format == subvol". And unlike dir plugins, our ZFS plugin
> does not have a distinguishing format suffix for this...

using zvols there doesn't seem like a good idea, but point taken ;)

> 
>> 
>> >   Both also get an optional additional `base-` prefix for base
>> >   volumes.
>> > 
>> > Note: all new base- prefix come in front of the `(sub)vol-*` prefixes.
>> > 
>> > Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
>> > ---
>> >  src/PVE/Storage/ESXiPlugin.pm        |  2 +-
>> >  src/PVE/Storage/ISCSIDirectPlugin.pm |  2 +-
>> >  src/PVE/Storage/ISCSIPlugin.pm       |  2 +-
>> >  src/PVE/Storage/LVMPlugin.pm         | 19 ++++++++---
>> >  src/PVE/Storage/LvmThinPlugin.pm     | 13 ++++++++
>> >  src/PVE/Storage/Plugin.pm            | 48 +++++++++++++++++++++-------
>> >  src/PVE/Storage/RBDPlugin.pm         | 45 ++++++++++++++++++++++++--
>> >  src/PVE/Storage/ZFSPoolPlugin.pm     | 34 ++++++++++++++++----
>> >  8 files changed, 138 insertions(+), 27 deletions(-)
>> > 
>> > diff --git a/src/PVE/Storage/ESXiPlugin.pm b/src/PVE/Storage/ESXiPlugin.pm
>> > index eeb6a48..ea2f8f9 100644
>> > --- a/src/PVE/Storage/ESXiPlugin.pm
>> > +++ b/src/PVE/Storage/ESXiPlugin.pm
>> > @@ -421,7 +421,7 @@ sub parse_volname {
>> >  
>> >      my $format = 'raw';
>> >      $format = 'vmdk' if $volname =~ /\.vmdk$/;
>> > -    return ('images', $volname, 0, undef, undef, undef, $format);
>> > +    return ('vm-vol', $volname, 0, undef, undef, undef, $format);
>> >  }
>> >  
>> >  sub list_images {
>> > diff --git a/src/PVE/Storage/ISCSIDirectPlugin.pm b/src/PVE/Storage/ISCSIDirectPlugin.pm
>> > index 069a41f..f5b466e 100644
>> > --- a/src/PVE/Storage/ISCSIDirectPlugin.pm
>> > +++ b/src/PVE/Storage/ISCSIDirectPlugin.pm
>> > @@ -87,7 +87,7 @@ sub parse_volname {
>> >      my ($class, $volname) = @_;
>> >  
>> >      if ($volname =~ m/^lun(\d+)$/) {
>> > -        return ('images', $1, undef, undef, undef, undef, 'raw');
>> > +        return ('vm-vol', $1, undef, undef, undef, undef, 'raw');
>> >      }
>> >  
>> >      die "unable to parse iscsi volume name '$volname'\n";
>> > diff --git a/src/PVE/Storage/ISCSIPlugin.pm b/src/PVE/Storage/ISCSIPlugin.pm
>> > index 7b30955..4875a1f 100644
>> > --- a/src/PVE/Storage/ISCSIPlugin.pm
>> > +++ b/src/PVE/Storage/ISCSIPlugin.pm
>> > @@ -371,7 +371,7 @@ sub parse_volname {
>> >      my ($class, $volname) = @_;
>> >  
>> >      if ($volname =~ m!^\d+\.\d+\.\d+\.([^/\s]+)$!) {
>> > -        return ('images', $1, undef, undef, undef, undef, 'raw');
>> > +        return ('vm-vol', $1, undef, undef, undef, undef, 'raw');
>> >      }
>> >  
>> >      die "unable to parse iscsi volume name '$volname'\n";
>> > diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
>> > index 6694cf2..9b88c6a 100644
>> > --- a/src/PVE/Storage/LVMPlugin.pm
>> > +++ b/src/PVE/Storage/LVMPlugin.pm
>> > @@ -452,11 +452,22 @@ sub parse_volname {
>> >  
>> >      PVE::Storage::Plugin::parse_lvm_name($volname);
>> >  
>> > -    if ($volname =~ m/^(vm-(\d+)-\S+)$/) {
>> > -        my $name = $1;
>> > -        my $vmid = $2;
>> > +    if (
>> > +        $volname =~ m!^(?<name>
>> > +        (
>> > +            # New style volumes have a vtype:
>> > +              vol-(?<vtype>vm|ct)
>> > +
>> > +            # Old style:
>> > +            | vm
>> > +        )
>> > +        -(?<vmid>\d+)-\S+
>> > +        )$!xn
>> > +    ) {
>> > +        my ($name, $vmid, $vtype) = @+{qw(name vmid vtype)};
>> > +        $vtype = $vtype ? "$vtype-vol" : 'images';
>> >          my $format = $volname =~ m/\.qcow2$/ ? 'qcow2' : 'raw';
>> > -        return ('images', $name, $vmid, undef, undef, undef, $format);
>> > +        return ($vtype, $name, $vmid, undef, undef, undef, $format);
>> >      }
>> >  
>> >      die "unable to parse lvm volume name '$volname'\n";
>> > diff --git a/src/PVE/Storage/LvmThinPlugin.pm b/src/PVE/Storage/LvmThinPlugin.pm
>> > index cdf0fd0..751bd7b 100644
>> > --- a/src/PVE/Storage/LvmThinPlugin.pm
>> > +++ b/src/PVE/Storage/LvmThinPlugin.pm
>> > @@ -77,6 +77,19 @@ sub parse_volname {
>> >  
>> >      PVE::Storage::Plugin::parse_lvm_name($volname);
>> >  
>> > +    # New naming convention:
>> > +    if (
>> > +        $volname =~ m/^(?<name>
>> > +        (?<isbase>base-)?
>> > +        (?:vol-(?<vtype> vm|ct))-
>> > +        (?<vmid>\d+)-
>> > +        \S+)$/xn
>> > +    ) {
>> > +        my ($name, $vmid, $vtype, $isbase) = @+{qw(name vmid vtype isbase)};
>> > +        return ($vtype . '-vol', $name, $vmid, undef, undef, !!$isbase, 'raw');
>> > +    }
>> > +
>> > +    # Old naming convention:
>> >      if ($volname =~ m/^((vm|base)-(\d+)-\S+)$/) {
>> >          return ('images', $1, $3, undef, undef, $2 eq 'base', 'raw');
>> >      }
>> > diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
>> > index ae9d673..77bcfc7 100644
>> > --- a/src/PVE/Storage/Plugin.pm
>> > +++ b/src/PVE/Storage/Plugin.pm
>> > @@ -711,19 +711,46 @@ sub cluster_lock_storage {
>> >  my sub parse_snap_name {
>> >      my ($name) = @_;
>> >  
>> > +    if ($name =~ m/^snap-(.*)-vol-(?:vm|ct)(.*)$/) {
>> > +        return $1;
>> > +    }
>> > +
>> >      if ($name =~ m/^snap-(.*)-vm(.*)$/) {
>> >          return $1;
>> >      }
>> > +
>> > +    return;
>> >  }
>> >  
>> > +our $DISK_NAME_REGEX = qr!
>> > +    (?<name>
>> > +        (
>> > +            # New convention: 'base-' prefix, 'vol-vm' and 'subvol-ct'.
>> > +              (?<israw> (?<isbase>base-)? vol-    (?<vtype> vm|ct))
>> > +            | (?<issub> (?<isbase>base-)? subvol- (?<vtype> ct))
>> > +
>> > +            # Old convention: "vm" vs "base" and "subvol" vs "basevol"
>> > +            | (?<israw> vm)
>> 
>> `israw` is a weird identifier here. it's also not used anywhere, so
>> maybe we could drop it for now and think of a better name if we ever
>> need it?
> 
> (Yeah... I just left it in for better alignment/documentation :P)

but it's confusing - the opposite of subvol is not raw, the opposite of
subvol is .. file? image? ..?

>> > +            | (?<issub> subvol)
>> > +            | (?<israw> (?<isbase>base))
>> > +            | (?<issub> (?<isbase>basevol))
>> 
>> the old convention was actually a lot more loose, see below..
> 
> IMO the more loose part should be in an `else` in `parse_name_dir`.
> This regex is used in other plugins and I don't want this regex to allow
> zvols named `💩`.

fair enough.

> 
>> 
>> we probably should add a warning to pve8to9 for volumes on builtin
>> storage types that don't match this strict scheme at least?
>> 
>> > +        )
>> > +        -
>> > +        (?<vmid>\d+)
>> > +        -
>> > +        (?<label>[^/\s]+)
>> > +    )
>> > +!xn;
>> > +
>> >  sub parse_name_dir {
>> > -    my $name = shift;
>> > +    my ($name) = @_;
>> >  
>> > -    if ($name =~ m!^((vm-|base-|subvol-)(\d+)-[^/\s]+\.(raw|qcow2|vmdk|subvol))$!) {
>> > -        my $isbase = $2 eq 'base-' ? $2 : undef;
>> > -        return ($1, $4, $isbase); # (name, format, isBase)
>> > +    if ($name =~ m!^$DISK_NAME_REGEX\.(?<format>raw|qcow2|vmdk|subvol)$!xn) {
>> > +        my ($name, $israw, $issub, $isbase, $vmid, $vtype, $label, $format) =
>> > +            @+{qw(name israw issub isbase vmid vtype label format)};
>> > +        return ($name, $format, !!$isbase, $vtype ? "$vtype-vol" : "images");
>> >      } elsif ($name =~ m!^((base-)?[^/\s]+\.(raw|qcow2|vmdk|subvol))$!) {
>> > -        warn "this volume name `$name` is not supported anymore\n" if !parse_snap_name($name);
>> > +        die "this volume name `$name` is not supported anymore\n";
>> 
>> as discussed off list, this regressed recently and needs to be fixed -
>> we only want to skip snapshot volumes, and allow other things for the
>> legacy types..
>> 
>> >      }
>> >  
>> >      die "unable to parse volume filename '$name'\n";
>> > @@ -733,15 +760,14 @@ sub parse_volname {
>> >      my ($class, $volname) = @_;
>> >  
>> >      if ($volname =~ m!^(\d+)/(\S+)/(\d+)/(\S+)$!) {
>> > -        my ($basedvmid, $basename) = ($1, $2);
>> > +        my ($basedvmid, $basename, $vmid, $name) = ($1, $2, $3, $4);
>> >          parse_name_dir($basename);
>> > -        my ($vmid, $name) = ($3, $4);
>> > -        my (undef, $format, $isBase) = parse_name_dir($name);
>> > -        return ('images', $name, $vmid, $basename, $basedvmid, $isBase, $format);
>> > +        my (undef, $format, $isBase, $vtype) = parse_name_dir($name);
>> > +        return ($vtype, $name, $vmid, $basename, $basedvmid, $isBase, $format);
>> >      } elsif ($volname =~ m!^(\d+)/(\S+)$!) {
>> >          my ($vmid, $name) = ($1, $2);
>> > -        my (undef, $format, $isBase) = parse_name_dir($name);
>> > -        return ('images', $name, $vmid, undef, undef, $isBase, $format);
>> > +        my (undef, $format, $isBase, $vtype) = parse_name_dir($name);
>> > +        return ($vtype, $name, $vmid, undef, undef, $isBase, $format);
>> >      } elsif ($volname =~ m!^iso/([^/]+$PVE::Storage::ISO_EXT_RE_0)$!) {
>> >          return ('iso', $1, undef, undef, undef, undef, 'raw');
>> >      } elsif ($volname =~ m!^vztmpl/([^/]+$PVE::Storage::VZTMPL_EXT_RE_1)$!) {
>> 
>> [..]
> 


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