Re: [pve-devel] [PATCH storage 2/3] disks: die if storage name is already in use

[Dominik Csapak <d.csapak@proxmox.com>]

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,