From 55553bd4322bef1966d6822aaec96e4321ee77bc Mon Sep 17 00:00:00 2001 From: Aaron Lauterer Date: Fri, 19 Aug 2022 17:01:20 +0200 Subject: [PATCH] disks: die if storage name is already in use 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 Reviewed-by: Dominik Csapak Tested-by: Dominik Csapak --- PVE/API2/Disks/Directory.pm | 12 ++++-- PVE/API2/Disks/LVM.pm | 2 + PVE/API2/Disks/LVMThin.pm | 3 ++ PVE/API2/Disks/ZFS.pm | 79 +++++++++++++++++++++---------------- 4 files changed, 57 insertions(+), 39 deletions(-) diff --git a/PVE/API2/Disks/Directory.pm b/PVE/API2/Disks/Directory.pm index df63ba9..e2272fb 100644 --- a/PVE/API2/Disks/Directory.pm +++ b/PVE/API2/Disks/Directory.pm @@ -203,16 +203,20 @@ __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 "the path for '${name}' is already mounted: ${path} ($mounted->{$path})\n" + if $mounted->{$path}; + die "a systemd mount unit already exists: ${mountunitpath}\n" if -e $mountunitpath; + 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..ef341d1 100644 --- a/PVE/API2/Disks/LVM.pm +++ b/PVE/API2/Disks/LVM.pm @@ -155,6 +155,8 @@ __PACKAGE__->register_method ({ my $worker = sub { PVE::Diskmanage::locked_disk_action(sub { PVE::Diskmanage::assert_disk_unused($dev); + die "volume group with name '${name}' already exists on node '${node}'\n" + if PVE::Storage::LVMPlugin::lvm_vgs()->{$name}; if (PVE::Diskmanage::is_partition($dev)) { eval { PVE::Diskmanage::change_parttype($dev, '8E00'); }; diff --git a/PVE/API2/Disks/LVMThin.pm b/PVE/API2/Disks/LVMThin.pm index 58ecb37..9a25e7c 100644 --- a/PVE/API2/Disks/LVMThin.pm +++ b/PVE/API2/Disks/LVMThin.pm @@ -114,6 +114,9 @@ __PACKAGE__->register_method ({ PVE::Diskmanage::locked_disk_action(sub { PVE::Diskmanage::assert_disk_unused($dev); + die "volume group with name '${name}' already exists on node '${node}'\n" + if PVE::Storage::LVMPlugin::lvm_vgs()->{$name}; + if (PVE::Diskmanage::is_partition($dev)) { eval { PVE::Diskmanage::change_parttype($dev, '8E00'); }; warn $@ if $@; diff --git a/PVE/API2/Disks/ZFS.pm b/PVE/API2/Disks/ZFS.pm index eeb9f48..27873cc 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}; 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(); + die "pool '${name}' already exists on node '${node}'\n" + if grep { $_->{name} eq $name } @{$pools}; + my $numdisks = scalar(@$devs); my $mindisks = { single => 1, @@ -428,7 +437,7 @@ __PACKAGE__->register_method ({ pool => $name, storage => $name, content => 'rootdir,images', - nodes => $param->{node}, + nodes => $node, }; PVE::API2::Storage::Config->create($storage_params);