From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id BD3191FF183 for ; Wed, 30 Jul 2025 16:25:59 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id B1E2110E8A; Wed, 30 Jul 2025 16:27:24 +0200 (CEST) Date: Wed, 30 Jul 2025 16:26:48 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox VE development discussion References: <20250729111557.136012-1-w.bumiller@proxmox.com> <20250729111557.136012-18-w.bumiller@proxmox.com> In-Reply-To: MIME-Version: 1.0 User-Agent: astroid/0.16.0 (https://github.com/astroidmail/astroid) Message-Id: <1753885476.gayexuf55e.astroid@yuna.none> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1753885600817 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.046 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH storage 17/26] plugins: add vtype parameter to alloc_image X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" 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 >> > --- 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 = [ _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel