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

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On July 15, 2022 1:58 pm, 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 possible that
> the creation will fail midway 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.
> 
> For ZFS, the logic to gather pool data is moved into its own function to
> be called from the index API endpoint and the check in the create
> endpoint.
> 
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
> 
> changes since v1:
> - moved zfs pool gathering from index api endpoint into its own function
> to easily call it for the check as well
> - zfs pool check is now using perl grep
> - removed check if mounted path if a /dev one
> - added check if a mount unit exists
>     - moved definitions of $path, $mountunitname and $mountunitpath out
>     of the worker
> 
>  PVE/API2/Disks/Directory.pm | 11 ++++--
>  PVE/API2/Disks/LVM.pm       |  3 ++
>  PVE/API2/Disks/LVMThin.pm   |  3 ++
>  PVE/API2/Disks/ZFS.pm       | 77 +++++++++++++++++++++----------------
>  4 files changed, 56 insertions(+), 38 deletions(-)
> 
> diff --git a/PVE/API2/Disks/Directory.pm b/PVE/API2/Disks/Directory.pm
> index df63ba9..07190de 100644
> --- a/PVE/API2/Disks/Directory.pm
> +++ b/PVE/API2/Disks/Directory.pm
> @@ -203,16 +203,19 @@ __PACKAGE__->register_method ({
>  	my $dev = $param->{device};
>  	my $node = $param->{node};
>  	my $type = $param->{filesystem} // 'ext4';
> +	my $path = "/mnt/pve/$name";
> +	my $mountunitname = PVE::Systemd::escape_unit($path, 1) . ".mount";
> +	my $mountunitpath = "/etc/systemd/system/$mountunitname";
>  
>  	$dev = PVE::Diskmanage::verify_blockdev_path($dev);
>  	PVE::Diskmanage::assert_disk_unused($dev);
>  	PVE::Storage::assert_sid_unused($name) if $param->{add_storage};
>  
> -	my $worker = sub {
> -	    my $path = "/mnt/pve/$name";
> -	    my $mountunitname = PVE::Systemd::escape_unit($path, 1) . ".mount";
> -	    my $mountunitpath = "/etc/systemd/system/$mountunitname";
> +	my $mounted = PVE::Diskmanage::mounted_paths();
> +	die "a mountpoint for '${name}' already exists: ${path} ($1)\n" if $mounted->{$path};

nit: the mountpoint existing is not the issue (something already being 
mounted there is!). also, where does the $1 come from?

> +	die "a systemd mount unit already exists: ${mountunitpath}\n" if -e $mountunitpath;

could check if it's identical to the one we'd generate (in the spirit of 
patch #3 ;))

>  
> +	my $worker = sub {
>  	    PVE::Diskmanage::locked_disk_action(sub {
>  		PVE::Diskmanage::assert_disk_unused($dev);
>  
> 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};

probably better off inside the worker, since `vgs` might take a while 
(although we also use it in the index API call in this module..)

> +
>  	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};
> +

same here

>  	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..51380c4 100644
> --- a/PVE/API2/Disks/ZFS.pm
> +++ b/PVE/API2/Disks/ZFS.pm
> @@ -18,6 +18,43 @@ use base qw(PVE::RESTHandler);
>  my $ZPOOL = '/sbin/zpool';
>  my $ZFS = '/sbin/zfs';
>  
> +sub get_pool_data {
> +    if (!-f $ZPOOL) {
> +	die "zfsutils-linux not installed\n";
> +    }
> +
> +    my $propnames = [qw(name size alloc free frag dedup health)];
> +    my $numbers = {
> +	size => 1,
> +	alloc => 1,
> +	free => 1,
> +	frag => 1,
> +	dedup => 1,
> +    };
> +
> +    my $cmd = [$ZPOOL,'list', '-HpPLo', join(',', @$propnames)];
> +
> +    my $pools = [];
> +
> +    run_command($cmd, outfunc => sub {
> +	my ($line) = @_;
> +
> +	    my @props = split('\s+', trim($line));
> +	    my $pool = {};
> +	    for (my $i = 0; $i < scalar(@$propnames); $i++) {
> +		if ($numbers->{$propnames->[$i]}) {
> +		    $pool->{$propnames->[$i]} = $props[$i] + 0;
> +		} else {
> +		    $pool->{$propnames->[$i]} = $props[$i];
> +		}
> +	    }
> +
> +	    push @$pools, $pool;
> +    });
> +
> +    return $pools;
> +}
> +
>  __PACKAGE__->register_method ({
>      name => 'index',
>      path => '',
> @@ -74,40 +111,7 @@ __PACKAGE__->register_method ({
>      code => sub {
>  	my ($param) = @_;
>  
> -	if (!-f $ZPOOL) {
> -	    die "zfsutils-linux not installed\n";
> -	}
> -
> -	my $propnames = [qw(name size alloc free frag dedup health)];
> -	my $numbers = {
> -	    size => 1,
> -	    alloc => 1,
> -	    free => 1,
> -	    frag => 1,
> -	    dedup => 1,
> -	};
> -
> -	my $cmd = [$ZPOOL,'list', '-HpPLo', join(',', @$propnames)];
> -
> -	my $pools = [];
> -
> -	run_command($cmd, outfunc => sub {
> -	    my ($line) = @_;
> -
> -		my @props = split('\s+', trim($line));
> -		my $pool = {};
> -		for (my $i = 0; $i < scalar(@$propnames); $i++) {
> -		    if ($numbers->{$propnames->[$i]}) {
> -			$pool->{$propnames->[$i]} = $props[$i] + 0;
> -		    } else {
> -			$pool->{$propnames->[$i]} = $props[$i];
> -		    }
> -		}
> -
> -		push @$pools, $pool;
> -	});
> -
> -	return $pools;
> +	return get_pool_data();
>      }});
>  
>  sub preparetree {
> @@ -336,6 +340,7 @@ __PACKAGE__->register_method ({
>  	my $user = $rpcenv->get_user();
>  
>  	my $name = $param->{name};
> +	my $node = $param->{node};

nit: please also change the usage further below in this API handler if 
you do this

>  	my $devs = [PVE::Tools::split_list($param->{devices})];
>  	my $raidlevel = $param->{raidlevel};
>  	my $compression = $param->{compression} // 'on';
> @@ -346,6 +351,10 @@ __PACKAGE__->register_method ({
>  	}
>  	PVE::Storage::assert_sid_unused($name) if $param->{add_storage};
>  
> +	my $pools = get_pool_data();

and also here

> +	die "pool '${name}' already exists on node '$node'\n"
> +	    if grep { $_->{name} eq $name } @{$pools};
> +
>  	my $numdisks = scalar(@$devs);
>  	my $mindisks = {
>  	    single => 1,
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>