From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 202657BE03 for ; Wed, 13 Jul 2022 12:48:33 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id F15E51919 for ; Wed, 13 Jul 2022 12:48:02 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Wed, 13 Jul 2022 12:48:00 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id C3F2A42165 for ; Wed, 13 Jul 2022 12:48:00 +0200 (CEST) From: Aaron Lauterer To: pve-devel@lists.proxmox.com Date: Wed, 13 Jul 2022 12:47:58 +0200 Message-Id: <20220713104758.651614-4-a.lauterer@proxmox.com> X-Mailer: git-send-email 2.30.2 In-Reply-To: <20220713104758.651614-1-a.lauterer@proxmox.com> References: <20220713104758.651614-1-a.lauterer@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.017 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - Subject: [pve-devel] [PATCH storage 3/3] disks: allow add_storage for already configured local storage X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 13 Jul 2022 10:48:33 -0000 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 --- I decided to make the storage type a parameter of the 'assert_sid_usable_on_node' function and not part of the $verify_params, so that it cannot be forgotten when calling it. PVE/API2/Disks/Directory.pm | 32 ++++++++++++++++++----------- PVE/API2/Disks/LVM.pm | 30 +++++++++++++++++---------- PVE/API2/Disks/LVMThin.pm | 30 +++++++++++++++++---------- PVE/API2/Disks/ZFS.pm | 29 +++++++++++++++++--------- PVE/Storage.pm | 41 +++++++++++++++++++++++++++++++++++++ 5 files changed, 118 insertions(+), 44 deletions(-) diff --git a/PVE/API2/Disks/Directory.pm b/PVE/API2/Disks/Directory.pm index 8e03229..00b9a37 100644 --- a/PVE/API2/Disks/Directory.pm +++ b/PVE/API2/Disks/Directory.pm @@ -203,10 +203,14 @@ __PACKAGE__->register_method ({ my $dev = $param->{device}; my $node = $param->{node}; my $type = $param->{filesystem} // 'ext4'; + my $path = "/mnt/pve/$name"; $dev = PVE::Diskmanage::verify_blockdev_path($dev); PVE::Diskmanage::assert_disk_unused($dev); - PVE::Storage::assert_sid_unused($name) if $param->{add_storage}; + + my $verify_params = { path => $path }; + PVE::Storage::assert_sid_usable_on_node($name, $node, 'dir', $verify_params) + if $param->{add_storage}; my $mounted = PVE::Diskmanage::mounted_paths(); if ($mounted->{$path} =~ /^(\/dev\/.+)$/ ) { @@ -214,7 +218,6 @@ __PACKAGE__->register_method ({ } my $worker = sub { - my $path = "/mnt/pve/$name"; my $mountunitname = PVE::Systemd::escape_unit($path, 1) . ".mount"; my $mountunitpath = "/etc/systemd/system/$mountunitname"; @@ -287,16 +290,21 @@ __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); + my $status = PVE::Storage::create_or_update_storage($name, $node); + if ($status->{create}) { + 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); + } elsif ($status->{update_params}) { + print "Adding '${node}' to nodes for storage '${name}'\n"; + PVE::API2::Storage::Config->update($status->{update_params}); + } # no action needed if storage exists, but not limited to nodes } }); }; diff --git a/PVE/API2/Disks/LVM.pm b/PVE/API2/Disks/LVM.pm index a27afe2..88b8d0b 100644 --- a/PVE/API2/Disks/LVM.pm +++ b/PVE/API2/Disks/LVM.pm @@ -150,7 +150,10 @@ __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 $verify_params = { vgname => $name }; + PVE::Storage::assert_sid_usable_on_node($name, $node, 'lvm', $verify_params) + if $param->{add_storage}; die "volume group with name '${name}' already exists\n" if PVE::Storage::LVMPlugin::lvm_vgs()->{$name}; @@ -169,16 +172,21 @@ __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); + my $status = PVE::Storage::create_or_update_storage($name, $node); + if ($status->{create}) { + my $storage_params = { + type => 'lvm', + vgname => $name, + storage => $name, + content => 'rootdir,images', + shared => 0, + nodes => $node, + }; + PVE::API2::Storage::Config->create($storage_params); + } elsif ($status->{update_params}) { + print "Adding '${node}' to nodes for storage '${name}'\n"; + PVE::API2::Storage::Config->update($status->{update_params}); + } # no action needed if storage exists, but not limited to nodes } }); }; diff --git a/PVE/API2/Disks/LVMThin.pm b/PVE/API2/Disks/LVMThin.pm index 690c183..d880154 100644 --- a/PVE/API2/Disks/LVMThin.pm +++ b/PVE/API2/Disks/LVMThin.pm @@ -108,7 +108,10 @@ __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 $verify_params = { vgname => $name, thinpool => $name }; + PVE::Storage::assert_sid_usable_on_node($name, $node, 'lvmthin', $verify_params) + if $param->{add_storage}; die "volume group with name '${name}' already exists\n" if PVE::Storage::LVMPlugin::lvm_vgs()->{$name}; @@ -147,16 +150,21 @@ __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); + my $status = PVE::Storage::create_or_update_storage($name, $node); + if ($status->{create}) { + my $storage_params = { + type => 'lvmthin', + vgname => $name, + thinpool => $name, + storage => $name, + content => 'rootdir,images', + nodes => $node, + }; + PVE::API2::Storage::Config->create($storage_params); + } elsif ($status->{update_params}) { + print "Adding '${node}' to nodes for storage '${name}'\n"; + PVE::API2::Storage::Config->update($status->{update_params}); + } # no action needed if storage exists, but not limited to nodes } }); }; diff --git a/PVE/API2/Disks/ZFS.pm b/PVE/API2/Disks/ZFS.pm index ceb0212..793bc14 100644 --- a/PVE/API2/Disks/ZFS.pm +++ b/PVE/API2/Disks/ZFS.pm @@ -337,6 +337,7 @@ __PACKAGE__->register_method ({ my $name = $param->{name}; my $devs = [PVE::Tools::split_list($param->{devices})]; + my $node = $param->{node}; my $raidlevel = $param->{raidlevel}; my $compression = $param->{compression} // 'on'; @@ -344,7 +345,10 @@ __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 $verify_params = { pool => $name }; + PVE::Storage::assert_sid_usable_on_node($name, $node, 'zfspool', $verify_params) + if $param->{add_storage}; my $pools = PVE::API2::Disks::ZFS->index({ node => $param->{node} }); my $poollist = { map { $_->{name} => 1 } @{$pools} }; @@ -427,15 +431,20 @@ __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 => $param->{node}, - }; - - PVE::API2::Storage::Config->create($storage_params); + my $status = PVE::Storage::create_or_update_storage($name, $node); + if ($status->{create}) { + my $storage_params = { + type => 'zfspool', + pool => $name, + storage => $name, + content => 'rootdir,images', + nodes => $param->{node}, + }; + PVE::API2::Storage::Config->create($storage_params); + } elsif ($status->{update_params}) { + print "Adding '${node}' to nodes for storage '${name}'\n"; + PVE::API2::Storage::Config->update($status->{update_params}); + } # no action needed if storage exists, but not limited to nodes } }; diff --git a/PVE/Storage.pm b/PVE/Storage.pm index b9c53a1..6dfcc3c 100755 --- a/PVE/Storage.pm +++ b/PVE/Storage.pm @@ -2127,6 +2127,47 @@ sub assert_sid_unused { return undef; } +# Checks if storage id can be used on the node, intended for local storage types +# Compares the type and verify_params hash against a potentially configured +# storage. Use it for storage parameters that need to be the same throughout +# the cluster. For example, pool for ZFS, vg_name for LVM, ... +sub assert_sid_usable_on_node { + my ($sid, $node, $type, $verify_params) = @_; + + my $cfg = config(); + if (my $scfg = storage_config($cfg, $sid, 1)) { + $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}; + + $verify_params->{type} = $type; + for my $key (keys %$verify_params) { + die "option '${key}' ($verify_params->{$key}) does not match ". + "existing storage configuration: $scfg->{$key}\n" + if $verify_params->{$key} ne $scfg->{$key}; + } + } +} + +# returns if storage needs to be created or updated. In the update case it +# checks for a node list and returns the needed parameters to update the +# storage with the new node list +sub create_or_update_storage { + my ($sid, $node) = @_; + + my $cfg = config(); + my $status = { create => 1 }; + if (my $scfg = storage_config($cfg, $sid, 1)) { + $status->{create} = 0; + if ($scfg->{nodes}) { + $scfg->{nodes}->{$node} = 1; + $status->{update_params}->{nodes} = join(',', sort keys $scfg->{nodes}->%*), + $status->{update_params}->{storage} = $sid; + } + } + return $status; +} + # removes leading/trailing spaces and (back)slashes completely # substitutes every non-ASCII-alphanumerical char with '_', except '_.-' sub normalize_content_filename { -- 2.30.2