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) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 129957C12D for ; Thu, 14 Jul 2022 13:23:54 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 2660F1C2EC for ; Thu, 14 Jul 2022 13:23:23 +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 ; Thu, 14 Jul 2022 13:23:21 +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 9642D421EB for ; Thu, 14 Jul 2022 13:23:21 +0200 (CEST) Message-ID: <09ed56f6-7d7f-2915-3e33-916cab38b9ad@proxmox.com> Date: Thu, 14 Jul 2022 13:23:19 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Content-Language: en-US To: Proxmox VE development discussion , Aaron Lauterer References: <20220713104758.651614-1-a.lauterer@proxmox.com> <20220713104758.651614-4-a.lauterer@proxmox.com> From: Dominik Csapak In-Reply-To: <20220713104758.651614-4-a.lauterer@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.098 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 NICE_REPLY_A -0.001 Looks like a legit reply (A) 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 - URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [lvmthin.pm, zfs.pm, directory.pm, storage.pm, lvm.pm] Subject: Re: [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: Thu, 14 Jul 2022 11:23:54 -0000 comments inline On 7/13/22 12:47, Aaron Lauterer wrote: > 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 i've read that chunk 4 times now (apart from the params assembly ofc), and imho i think this could belong into the 'create_or_update_storage' function itself, at least we wouldn't have the same if (create) { create() } else { update() } code multiple times if you're concerned with dependency of PVE::Storage and PVE::API2::Storage::Config, id just move it there, then there is no real problem > } > }; > > 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}; that'll log a warning if $scfg->{$key} is undef so a "// ''" could help here (or explicit definedness check) > + } > + } > +} > + > +# 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 {