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