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 174EA7C0D6 for ; Thu, 14 Jul 2022 13:13:48 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 129831BE33 for ; Thu, 14 Jul 2022 13:13:48 +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 for ; Thu, 14 Jul 2022 13:13:46 +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 B715D421EB for ; Thu, 14 Jul 2022 13:13:40 +0200 (CEST) Message-ID: Date: Thu, 14 Jul 2022 13:13:39 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Content-Language: en-US To: Proxmox VE development discussion , Aaron Lauterer References: <20220713104758.651614-1-a.lauterer@proxmox.com> <20220713104758.651614-3-a.lauterer@proxmox.com> From: Dominik Csapak In-Reply-To: <20220713104758.651614-3-a.lauterer@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.098 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% 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 T_SCC_BODY_TEXT_LINE -0.01 - URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [lvmthin.pm, zfs.pm, directory.pm, lvm.pm] Subject: Re: [pve-devel] [PATCH storage 2/3] disks: die if storage name is already in use 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: Thu, 14 Jul 2022 11:13:48 -0000 comments inline On 7/13/22 12:47, Aaron Lauterer wrote: > If a storage of that type and name already exists (LVM, zpool, ...) but > we do not have a Proxmox VE Storage config for it, it is likely that the > creation will fail mid way due to checks done by the underlying storage > layer itself. This in turn can lead to disks that are already > partitioned. Users would need to clean this up themselves. > > By adding checks early on, not only checking against the PVE storage > config, but against the actual storage type itself, we can die early > enough, before we touch any disk. > > Signed-off-by: Aaron Lauterer > --- > > A somewhat sensible way I found for Directory storages was to check if the > path is already in use / mounted. Maybe there are additional ways? > > For zpools we don't have anything in the ZFSPoolPlugin.pm, in contrast > to LVM where the storage plugins provides easily callable methods to get > a list of VGs. > I therefore chose to call the zpool index API to get the list of ZFS > pools. Not sure if I should refactor that logic into a separate function > right away or wait until we might need it at more and different places? > > PVE/API2/Disks/Directory.pm | 5 +++++ > PVE/API2/Disks/LVM.pm | 3 +++ > PVE/API2/Disks/LVMThin.pm | 3 +++ > PVE/API2/Disks/ZFS.pm | 4 ++++ > 4 files changed, 15 insertions(+) > > diff --git a/PVE/API2/Disks/Directory.pm b/PVE/API2/Disks/Directory.pm > index df63ba9..8e03229 100644 > --- a/PVE/API2/Disks/Directory.pm > +++ b/PVE/API2/Disks/Directory.pm > @@ -208,6 +208,11 @@ __PACKAGE__->register_method ({ > PVE::Diskmanage::assert_disk_unused($dev); > PVE::Storage::assert_sid_unused($name) if $param->{add_storage}; > > + my $mounted = PVE::Diskmanage::mounted_paths(); > + if ($mounted->{$path} =~ /^(\/dev\/.+)$/ ) { same reasoning as the last patch, we don't need to actually filter by '/dev/' here since we don't want *anything* mounted there. but even if we do want to filter, there's no need imo for doing both in 'mounted_paths' and here, one place should be enough aside from that, there are other checks that we could do here too, e.g. checking if the mount unit already exists because the dir does not have to be currently mounted (we even have that variable below, we'd just have to pull it out of the worker and do a '-e $mountunitpath') > + die "a mountpoint for '${name}' already exists: ${path} ($1)\n"; > + } > + > my $worker = sub { > my $path = "/mnt/pve/$name"; > my $mountunitname = PVE::Systemd::escape_unit($path, 1) . ".mount"; > diff --git a/PVE/API2/Disks/LVM.pm b/PVE/API2/Disks/LVM.pm > index 6e4331a..a27afe2 100644 > --- a/PVE/API2/Disks/LVM.pm > +++ b/PVE/API2/Disks/LVM.pm > @@ -152,6 +152,9 @@ __PACKAGE__->register_method ({ > PVE::Diskmanage::assert_disk_unused($dev); > PVE::Storage::assert_sid_unused($name) if $param->{add_storage}; > > + die "volume group with name '${name}' already exists\n" > + if PVE::Storage::LVMPlugin::lvm_vgs()->{$name}; > + > my $worker = sub { > PVE::Diskmanage::locked_disk_action(sub { > PVE::Diskmanage::assert_disk_unused($dev); > diff --git a/PVE/API2/Disks/LVMThin.pm b/PVE/API2/Disks/LVMThin.pm > index 58ecb37..690c183 100644 > --- a/PVE/API2/Disks/LVMThin.pm > +++ b/PVE/API2/Disks/LVMThin.pm > @@ -110,6 +110,9 @@ __PACKAGE__->register_method ({ > PVE::Diskmanage::assert_disk_unused($dev); > PVE::Storage::assert_sid_unused($name) if $param->{add_storage}; > > + die "volume group with name '${name}' already exists\n" > + if PVE::Storage::LVMPlugin::lvm_vgs()->{$name}; > + > my $worker = sub { > PVE::Diskmanage::locked_disk_action(sub { > PVE::Diskmanage::assert_disk_unused($dev); > diff --git a/PVE/API2/Disks/ZFS.pm b/PVE/API2/Disks/ZFS.pm > index eeb9f48..ceb0212 100644 > --- a/PVE/API2/Disks/ZFS.pm > +++ b/PVE/API2/Disks/ZFS.pm > @@ -346,6 +346,10 @@ __PACKAGE__->register_method ({ > } > PVE::Storage::assert_sid_unused($name) if $param->{add_storage}; > > + my $pools = PVE::API2::Disks::ZFS->index({ node => $param->{node} }); personally i'd refactor that out, especially since were in the same file, so there's not much headache regarding public functions/scope > + my $poollist = { map { $_->{name} => 1 } @{$pools} }; does that really make sense here? would it not be easier to just iterate? e.g. ---- for my $pool (@$pools) { die "..." if $pool->{name} eq $name; } ---- (i admit, it's 1 line longer, but a bit more readable?) > + die "pool '${name}' already exists on node '$node'\n" if $poollist->{$name}; > + > my $numdisks = scalar(@$devs); > my $mindisks = { > single => 1,