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 <a.lauterer@proxmox.com>
Reviewed-by: Dominik Csapak <d.csapak@proxmox.com>
Tested-by: Dominik Csapak <d.csapak@proxmox.com>

diff --git a/PVE/API2/Disks/Directory.pm b/PVE/API2/Disks/Directory.pm
index df63ba9d1f32e7318f295e0a577ed3a64cd8bd68..e2272fbb301837b09e393fec558e4f42c2b6f134 100644 (file)
--- 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 6e4331ad9708804babf25a77635468bb1b910bad..ef341d1ce67ebd54bb4d9ee9c341f35a8fda385c 100644 (file)
--- 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 58ecb37b0e06acc08420512e9fcfcfa34dadbd16..9a25e7ccd77aafc53b6d33380dd1746b348d7daa 100644 (file)
--- 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 eeb9f48d8c22c8d306e6bb336eb44adbb25fe9ae..27873cc7941624eafad00016a82012e07d8c6b70 100644 (file)
--- 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);