From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 604F67B5C4 for ; Wed, 12 May 2021 13:21:07 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 5441DC858 for ; Wed, 12 May 2021 13:20:37 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 4C13DC849 for ; Wed, 12 May 2021 13:20:36 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 22CD046557 for ; Wed, 12 May 2021 13:20:36 +0200 (CEST) To: pve-devel@lists.proxmox.com, l.stechauner@proxmox.com References: <20210511110213.64953-1-l.stechauner@proxmox.com> From: Fabian Ebner Message-ID: Date: Wed, 12 May 2021 13:20:35 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.0 MIME-Version: 1.0 In-Reply-To: <20210511110213.64953-1-l.stechauner@proxmox.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.003 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.001 Looks like a legit reply (A) 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 container] fix #3421: allow custom storage plugins to support rootfs 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: , X-List-Received-Date: Wed, 12 May 2021 11:21:07 -0000 Had a look at this patch. The idea is fine, but there are a few minor problems that could be avoided. Am 11.05.21 um 13:02 schrieb Lorenz Stechauner: > Signed-off-by: Lorenz Stechauner > --- > src/PVE/LXC.pm | 28 +++++++--------------------- > 1 file changed, 7 insertions(+), 21 deletions(-) > > diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm > index 7e6f378..9c1227c 100644 > --- a/src/PVE/LXC.pm > +++ b/src/PVE/LXC.pm > @@ -1685,8 +1685,7 @@ sub __mountpoint_mount { > if ($scfg->{path}) { > $mounted_dev = run_with_loopdev($domount, $path, $readonly); > $use_loopdev = 1; > - } elsif ($scfg->{type} eq 'drbd' || $scfg->{type} eq 'lvm' || > - $scfg->{type} eq 'rbd' || $scfg->{type} eq 'lvmthin') { > + } elsif ($scfg->{content}->{rootdir}) { > $mounted_dev = $path; > &$domount($path); > } else { Nit: The error message in the else branch does not match up with what is checked for anymore. This also breaks starting containers where the content type for one of the mount point volumes is incorrectly set, which did work previously. Maybe just always take the other branch for non-filesystem-based storages (i.e. those without $scfg->{path})? > @@ -1871,30 +1870,17 @@ sub alloc_disk { > > eval { > my $do_format = 0; > - if ($scfg->{type} eq 'dir' || $scfg->{type} eq 'nfs' || $scfg->{type} eq 'cifs' ) { > + if ($scfg->{type} eq 'zfspool') { > + $volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, 'subvol', undef, $size_kb); Style nit: please avoid line length > 100. > + $needs_chown = 1; > + } elsif ($scfg->{content}->{rootdir}) { > if ($size_kb > 0) { > - $volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, 'raw', > - undef, $size_kb); > + $volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, 'raw', undef, $size_kb); Style nit: please avoid line length > 100. > $do_format = 1; > } else { > - $volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, 'subvol', > - undef, 0); > + $volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, 'subvol', undef, 0); > $needs_chown = 1; > } > - } elsif ($scfg->{type} eq 'zfspool') { > - > - $volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, 'subvol', > - undef, $size_kb); > - $needs_chown = 1; > - } elsif ($scfg->{type} eq 'drbd' || $scfg->{type} eq 'lvm' || $scfg->{type} eq 'lvmthin') { > - > - $volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, 'raw', undef, $size_kb); > - $do_format = 1; > - > - } elsif ($scfg->{type} eq 'rbd') { > - > - $volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, 'raw', undef, $size_kb); > - $do_format = 1; This changes the requested format and $do_format when size is 0 for rbd, drbd, lvm and lvm-thin plugins. Might be worth mentioning in the commit message. It's not really a problem if "previously fails" iff "now fails", but for rbd it's not the case. After applying the patch it's possible to create 0-sized rbd disks, which are not formatted. Previously, formatting would fail. Would not be an issue if the rbd storage plugin would actually complain about getting the wrong format in the first place, like the other ones do ;) Still, I feel like it would be better to only handle the size 0 special case for file-system based storages, using $scfg->{path} like above. > } else { > die "unable to create containers on storage type '$scfg->{type}'\n"; > } > Same situation as for the else branch above, although this one probably cannot be triggered, as there's an earlier check for content type support.