disks: allow add_storage for already configured local storage

One of the smaller annoyances, especially for less experienced users, is
the fact, that when creating a local storage (ZFS, LVM (thin), dir) in a
cluster, one can only leave the "Add Storage" option enabled the first
time.

On any following node, this option needed to be disabled and the new
node manually added to the list of nodes for that storage.

This patch changes the behavior. If a storage of the same name already
exists, it will verify that necessary parameters match the already
existing one.
Then, if the 'nodes' parameter is set, it adds the current node and
updates the storage config.
In case there is no nodes list, nothing else needs to be done, and the
GUI will stop showing the question mark for the configured, but until
then, not existing local storage.

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 e2272fbb301837b09e393fec558e4f42c2b6f134..4fdb06874d0754a55ac4e9beba80664db45960b9 100644 (file)
--- a/PVE/API2/Disks/Directory.pm
+++ b/PVE/API2/Disks/Directory.pm
@@ -209,7 +209,26 @@ __PACKAGE__->register_method ({
 
        $dev = PVE::Diskmanage::verify_blockdev_path($dev);
        PVE::Diskmanage::assert_disk_unused($dev);
-       PVE::Storage::assert_sid_unused($name) if $param->{add_storage};
+
+       my $storage_params = {
+           type => 'dir',
+           storage => $name,
+           content => 'rootdir,images,iso,backup,vztmpl,snippets',
+           is_mountpoint => 1,
+           path => $path,
+           nodes => $node,
+       };
+       my $verify_params = [qw(path)];
+
+       if ($param->{add_storage}) {
+           PVE::API2::Storage::Config->create_or_update(
+               $name,
+               $node,
+               $storage_params,
+               $verify_params,
+               1,
+           );
+       }
 
        my $mounted = PVE::Diskmanage::mounted_paths();
        die "the path for '${name}' is already mounted: ${path} ($mounted->{$path})\n"
@@ -286,16 +305,12 @@ __PACKAGE__->register_method ({
                run_command(['systemctl', 'start', $mountunitname]);
 
                if ($param->{add_storage}) {
-                   my $storage_params = {
-                       type => 'dir',
-                       storage => $name,
-                       content => 'rootdir,images,iso,backup,vztmpl,snippets',
-                       is_mountpoint => 1,
-                       path => $path,
-                       nodes => $node,
-                   };
-
-                   PVE::API2::Storage::Config->create($storage_params);
+                   PVE::API2::Storage::Config->create_or_update(
+                       $name,
+                       $node,
+                       $storage_params,
+                       $verify_params,
+                   );
                }
            });
        };

diff --git a/PVE/API2/Disks/LVM.pm b/PVE/API2/Disks/LVM.pm
index ef341d1ce67ebd54bb4d9ee9c341f35a8fda385c..fe875452bdaa170040b36a98f493f7876ff51f4b 100644 (file)
--- a/PVE/API2/Disks/LVM.pm
+++ b/PVE/API2/Disks/LVM.pm
@@ -150,7 +150,26 @@ __PACKAGE__->register_method ({
 
        $dev = PVE::Diskmanage::verify_blockdev_path($dev);
        PVE::Diskmanage::assert_disk_unused($dev);
-       PVE::Storage::assert_sid_unused($name) if $param->{add_storage};
+
+       my $storage_params = {
+           type => 'lvm',
+           vgname => $name,
+           storage => $name,
+           content => 'rootdir,images',
+           shared => 0,
+           nodes => $node,
+       };
+       my $verify_params = [qw(vgname)];
+
+       if ($param->{add_storage}) {
+           PVE::API2::Storage::Config->create_or_update(
+               $name,
+               $node,
+               $storage_params,
+               $verify_params,
+               1,
+           );
+       }
 
        my $worker = sub {
            PVE::Diskmanage::locked_disk_action(sub {
@@ -168,16 +187,12 @@ __PACKAGE__->register_method ({
                PVE::Diskmanage::udevadm_trigger($dev);
 
                if ($param->{add_storage}) {
-                   my $storage_params = {
-                       type => 'lvm',
-                       vgname => $name,
-                       storage => $name,
-                       content => 'rootdir,images',
-                       shared => 0,
-                       nodes => $node,
-                   };
-
-                   PVE::API2::Storage::Config->create($storage_params);
+                   PVE::API2::Storage::Config->create_or_update(
+                       $name,
+                       $node,
+                       $storage_params,
+                       $verify_params,
+                   );
                }
            });
        };

diff --git a/PVE/API2/Disks/LVMThin.pm b/PVE/API2/Disks/LVMThin.pm
index 9a25e7ccd77aafc53b6d33380dd1746b348d7daa..038310a4864c6e82234ee7dc75fbaa2a7ce6b1de 100644 (file)
--- a/PVE/API2/Disks/LVMThin.pm
+++ b/PVE/API2/Disks/LVMThin.pm
@@ -108,7 +108,26 @@ __PACKAGE__->register_method ({
 
        $dev = PVE::Diskmanage::verify_blockdev_path($dev);
        PVE::Diskmanage::assert_disk_unused($dev);
-       PVE::Storage::assert_sid_unused($name) if $param->{add_storage};
+
+       my $storage_params = {
+           type => 'lvmthin',
+           vgname => $name,
+           thinpool => $name,
+           storage => $name,
+           content => 'rootdir,images',
+           nodes => $node,
+       };
+       my $verify_params = [qw(vgname thinpool)];
+
+       if ($param->{add_storage}) {
+           PVE::API2::Storage::Config->create_or_update(
+               $name,
+               $node,
+               $storage_params,
+               $verify_params,
+               1,
+           );
+       }
 
        my $worker = sub {
            PVE::Diskmanage::locked_disk_action(sub {
@@ -147,16 +166,12 @@ __PACKAGE__->register_method ({
                PVE::Diskmanage::udevadm_trigger($dev);
 
                if ($param->{add_storage}) {
-                   my $storage_params = {
-                       type => 'lvmthin',
-                       vgname => $name,
-                       thinpool => $name,
-                       storage => $name,
-                       content => 'rootdir,images',
-                       nodes => $node,
-                   };
-
-                   PVE::API2::Storage::Config->create($storage_params);
+                   PVE::API2::Storage::Config->create_or_update(
+                       $name,
+                       $node,
+                       $storage_params,
+                       $verify_params,
+                   );
                }
            });
        };

diff --git a/PVE/API2/Disks/ZFS.pm b/PVE/API2/Disks/ZFS.pm
index 27873cc7941624eafad00016a82012e07d8c6b70..dd59375ebbe621c5b07ebecea83c77660a9a0152 100644 (file)
--- a/PVE/API2/Disks/ZFS.pm
+++ b/PVE/API2/Disks/ZFS.pm
@@ -342,14 +342,33 @@ __PACKAGE__->register_method ({
        my $name = $param->{name};
        my $node = $param->{node};
        my $devs = [PVE::Tools::split_list($param->{devices})];
+       my $node = $param->{node};
        my $raidlevel = $param->{raidlevel};
        my $compression = $param->{compression} // 'on';
 
        for my $dev (@$devs) {
            $dev = PVE::Diskmanage::verify_blockdev_path($dev);
            PVE::Diskmanage::assert_disk_unused($dev);
+
+       }
+       my $storage_params = {
+           type => 'zfspool',
+           pool => $name,
+           storage => $name,
+           content => 'rootdir,images',
+           nodes => $node,
+       };
+       my $verify_params = [qw(pool)];
+
+       if ($param->{add_storage}) {
+           PVE::API2::Storage::Config->create_or_update(
+               $name,
+               $node,
+               $storage_params,
+               $verify_params,
+               1,
+           );
        }
-       PVE::Storage::assert_sid_unused($name) if $param->{add_storage};
 
        my $pools = get_pool_data();
        die "pool '${name}' already exists on node '${node}'\n"
@@ -432,15 +451,12 @@ __PACKAGE__->register_method ({
            PVE::Diskmanage::udevadm_trigger($devs->@*);
 
            if ($param->{add_storage}) {
-               my $storage_params = {
-                   type => 'zfspool',
-                   pool => $name,
-                   storage => $name,
-                   content => 'rootdir,images',
-                   nodes => $node,
-               };
-
-               PVE::API2::Storage::Config->create($storage_params);
+               PVE::API2::Storage::Config->create_or_update(
+                   $name,
+                   $node,
+                   $storage_params,
+                   $verify_params,
+               );
            }
        };
 

diff --git a/PVE/API2/Storage/Config.pm b/PVE/API2/Storage/Config.pm
index 6bd770edcff39d6aa4c779467ad65f1188b8cf54..821db2148efdf95771ad9aebd0b4451cfe518b71 100755 (executable)
--- a/PVE/API2/Storage/Config.pm
+++ b/PVE/API2/Storage/Config.pm
@@ -65,6 +65,57 @@ sub cleanup_storages_for_node {
     }
 }
 
+# Decides if a storage needs to be created or updated. An update is needed, if
+# the storage has a node list configured, then the current node will be added.
+# The verify_params parameter is an array of parameter names that need to match
+# if there already is a storage config of the same name present.  This is
+# mainly intended for local storage types as certain parameters need to be the
+# same.  For exmaple 'pool' for ZFS, 'vg_name' for LVM, ...
+# Set the dryrun parameter, to only verify the parameters without updating or
+# creating the storage.
+sub create_or_update {
+    my ($self, $sid, $node, $storage_params, $verify_params, $dryrun) = @_;
+
+    my $cfg = PVE::Storage::config();
+    my $scfg = PVE::Storage::storage_config($cfg, $sid, 1);
+
+    if ($scfg) {
+       die "storage config for '${sid}' exists but no parameters to verify were provided\n"
+           if !$verify_params;
+
+       $node = PVE::INotify::nodename() if !$node || ($node eq 'localhost');
+       die "Storage ID '${sid}' already exists on node ${node}\n"
+           if !defined($scfg->{nodes}) || $scfg->{nodes}->{$node};
+
+       push @$verify_params, 'type';
+       for my $key (@$verify_params) {
+           if (!defined($scfg->{$key})) {
+               die "Option '${key}' is not configured for storage '$sid', "
+                   ."expected it to be '$storage_params->{$key}'";
+           }
+           if ($storage_params->{$key} ne $scfg->{$key}) {
+               die "Option '${key}' ($storage_params->{$key}) does not match "
+                   ."existing storage configuration '$scfg->{$key}'\n";
+           }
+       }
+    }
+
+    if (!$dryrun) {
+       if ($scfg) {
+           if ($scfg->{nodes}) {
+               $scfg->{nodes}->{$node} = 1;
+               $self->update({
+                   nodes => join(',', sort keys $scfg->{nodes}->%*),
+                   storage => $sid,
+               });
+               print "Added '${node}' to nodes for storage '${sid}'\n";
+           }
+       } else {
+           $self->create($storage_params);
+       }
+    }
+}
+
 __PACKAGE__->register_method ({
     name => 'index',
     path => '',