From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <d.csapak@proxmox.com>
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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; Thu, 14 Jul 2022 13:13:40 +0200 (CEST)
Message-ID: <a60cbbbd-51a4-cd5a-85ea-94ed50757d14@proxmox.com>
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 <pve-devel@lists.proxmox.com>,
 Aaron Lauterer <a.lauterer@proxmox.com>
References: <20220713104758.651614-1-a.lauterer@proxmox.com>
 <20220713104758.651614-3-a.lauterer@proxmox.com>
From: Dominik Csapak <d.csapak@proxmox.com>
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 <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>, 
 <mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>, 
 <mailto:pve-devel-request@lists.proxmox.com?subject=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.lauterer@proxmox.com>
> ---
> 
> 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,