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 686B28A5B3 for ; Wed, 17 Aug 2022 13:53:39 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 5CE1126D73 for ; Wed, 17 Aug 2022 13:53:39 +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, 17 Aug 2022 13:53:37 +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 AF2F442E56 for ; Wed, 17 Aug 2022 13:53:36 +0200 (CEST) Date: Wed, 17 Aug 2022 13:53:24 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox VE development discussion References: <20220715115808.1385388-1-a.lauterer@proxmox.com> <20220715115808.1385388-4-a.lauterer@proxmox.com> In-Reply-To: <20220715115808.1385388-4-a.lauterer@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.15.0 (https://github.com/astroidmail/astroid) Message-Id: <1660736720.z5f0ftzprw.astroid@nora.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 0.162 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 - URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [proxmox.com, zfs.pm, lvmthin.pm, lvm.pm, directory.pm, config.pm, storage.pm] Subject: Re: [pve-devel] [PATCH storage v2 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, 17 Aug 2022 11:53:39 -0000 On July 15, 2022 1:58 pm, 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. >=20 > On any following node, this option needed to be disabled and the new > node manually added to the list of nodes for that storage. >=20 > 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. >=20 > Signed-off-by: Aaron Lauterer > --- > changes since v1: > - restructured the logic a bit. checks still need to be done the same, > but the create or update logic is in its own > API2::Storage::Config->create_or_update function which then either > calls the create or update API directly. This reduces a lot of code > repition. > - in Storage::assert_sid_usable_on_node additionally checks if the > parameter is configured in the storage and fails if not. We would have > gotten warnings about using and undef in a string concat due to how the > other error was presented. >=20 > PVE/API2/Disks/Directory.pm | 8 +++++--- > PVE/API2/Disks/LVM.pm | 8 +++++--- > PVE/API2/Disks/LVMThin.pm | 8 +++++--- > PVE/API2/Disks/ZFS.pm | 9 ++++++--- > PVE/API2/Storage/Config.pm | 22 ++++++++++++++++++++++ > PVE/Storage.pm | 27 +++++++++++++++++++++++++++ > 6 files changed, 70 insertions(+), 12 deletions(-) >=20 > diff --git a/PVE/API2/Disks/Directory.pm b/PVE/API2/Disks/Directory.pm > index 07190de..581e970 100644 > --- a/PVE/API2/Disks/Directory.pm > +++ b/PVE/API2/Disks/Directory.pm > @@ -209,7 +209,10 @@ __PACKAGE__->register_method ({ > =20 > $dev =3D 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 =3D { path =3D> $path }; > + PVE::Storage::assert_sid_usable_on_node($name, $node, 'dir', $verify_pa= rams) > + if $param->{add_storage}; > =20 > my $mounted =3D PVE::Diskmanage::mounted_paths(); > die "a mountpoint for '${name}' already exists: ${path} ($1)\n" if $mou= nted->{$path}; > @@ -293,8 +296,7 @@ __PACKAGE__->register_method ({ > path =3D> $path, > nodes =3D> $node, > }; > - > - PVE::API2::Storage::Config->create($storage_params); > + PVE::API2::Storage::Config->create_or_update($name, $node, $storag= e_params); > } > }); > }; > diff --git a/PVE/API2/Disks/LVM.pm b/PVE/API2/Disks/LVM.pm > index a27afe2..e9b3f25 100644 > --- a/PVE/API2/Disks/LVM.pm > +++ b/PVE/API2/Disks/LVM.pm > @@ -150,7 +150,10 @@ __PACKAGE__->register_method ({ > =20 > $dev =3D 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 =3D { vgname =3D> $name }; > + PVE::Storage::assert_sid_usable_on_node($name, $node, 'lvm', $verify_pa= rams) > + if $param->{add_storage}; > =20 > die "volume group with name '${name}' already exists\n" > if PVE::Storage::LVMPlugin::lvm_vgs()->{$name}; > @@ -177,8 +180,7 @@ __PACKAGE__->register_method ({ > shared =3D> 0, > nodes =3D> $node, > }; > - > - PVE::API2::Storage::Config->create($storage_params); > + PVE::API2::Storage::Config->create_or_update($name, $node, $storag= e_params); > } > }); > }; > diff --git a/PVE/API2/Disks/LVMThin.pm b/PVE/API2/Disks/LVMThin.pm > index 690c183..fb61489 100644 > --- a/PVE/API2/Disks/LVMThin.pm > +++ b/PVE/API2/Disks/LVMThin.pm > @@ -108,7 +108,10 @@ __PACKAGE__->register_method ({ > =20 > $dev =3D 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 =3D { vgname =3D> $name, thinpool =3D> $name }; > + PVE::Storage::assert_sid_usable_on_node($name, $node, 'lvmthin', $verif= y_params) > + if $param->{add_storage}; > =20 > die "volume group with name '${name}' already exists\n" > if PVE::Storage::LVMPlugin::lvm_vgs()->{$name}; > @@ -155,8 +158,7 @@ __PACKAGE__->register_method ({ > content =3D> 'rootdir,images', > nodes =3D> $node, > }; > - > - PVE::API2::Storage::Config->create($storage_params); > + PVE::API2::Storage::Config->create_or_update($name, $node, $storag= e_params); > } > }); > }; > diff --git a/PVE/API2/Disks/ZFS.pm b/PVE/API2/Disks/ZFS.pm > index 51380c4..e1c4b59 100644 > --- a/PVE/API2/Disks/ZFS.pm > +++ b/PVE/API2/Disks/ZFS.pm > @@ -342,6 +342,7 @@ __PACKAGE__->register_method ({ > my $name =3D $param->{name}; > my $node =3D $param->{node}; > my $devs =3D [PVE::Tools::split_list($param->{devices})]; > + my $node =3D $param->{node}; > my $raidlevel =3D $param->{raidlevel}; > my $compression =3D $param->{compression} // 'on'; > =20 > @@ -349,7 +350,10 @@ __PACKAGE__->register_method ({ > $dev =3D 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 =3D { pool =3D> $name }; > + PVE::Storage::assert_sid_usable_on_node($name, $node, 'zfspool', $verif= y_params) > + if $param->{add_storage}; > =20 > my $pools =3D get_pool_data(); > die "pool '${name}' already exists on node '$node'\n" > @@ -439,8 +443,7 @@ __PACKAGE__->register_method ({ > content =3D> 'rootdir,images', > nodes =3D> $param->{node}, > }; > - > - PVE::API2::Storage::Config->create($storage_params); > + PVE::API2::Storage::Config->create_or_update($name, $node, $storage_pa= rams); > } > }; > =20 > diff --git a/PVE/API2/Storage/Config.pm b/PVE/API2/Storage/Config.pm > index 6bd770e..2ea91b7 100755 > --- a/PVE/API2/Storage/Config.pm > +++ b/PVE/API2/Storage/Config.pm > @@ -65,6 +65,28 @@ sub cleanup_storages_for_node { > } > } > =20 > +# Decides if a storage needs to be created or updated. An update is need= ed, if > +# the storage has a node list configured, then the current node will be = added. > +# Check before if this is a sensible action, e.g. storage type and other= params > +# that need to match > +sub create_or_update { > + my ($self, $sid, $node, $storage_params) =3D @_; > + > + my $cfg =3D PVE::Storage::config(); > + if (my $scfg =3D PVE::Storage::storage_config($cfg, $sid, 1)) { > + if ($scfg->{nodes}) { > + $scfg->{nodes}->{$node} =3D 1; > + $self->update({ > + nodes =3D> join(',', sort keys $scfg->{nodes}->%*), > + storage =3D> $sid, > + }); > + print "Added '${node}' to nodes for storage '${sid}'\n"; > + } > + } else { > + $self->create($storage_params); > + } > +} > + > __PACKAGE__->register_method ({ > name =3D> 'index', > path =3D> '', > diff --git a/PVE/Storage.pm b/PVE/Storage.pm > index b9c53a1..21d575c 100755 > --- a/PVE/Storage.pm > +++ b/PVE/Storage.pm > @@ -2127,6 +2127,33 @@ sub assert_sid_unused { > return undef; > } > =20 > +# Checks if storage id can be used on the node, intended for local stora= ge types > +# Compares the type and verify_params hash against a potentially configu= red > +# storage. Use it for storage parameters that need to be the same throug= hout > +# the cluster. For example, pool for ZFS, vg_name for LVM, ... > +sub assert_sid_usable_on_node { > + my ($sid, $node, $type, $verify_params) =3D @_; couldn't this be folded into create_or_update, e.g. by adding a=20 check/dry_run flag that skips the actual update/creation? then we could=20 call it once at the beginning (with dry_run set) and once for actual=20 adding/updating, and we don't risk forgetting about some parameter later=20 on when changing the code. $node is always the local node (and passed to create_or_update via=20 $storage_params->{nodes}), $type is passed to create_or_update (via=20 $storage_params), and $verify_params is just a subset of $storage_params=20 (where we actually want to check that everything matches anyway, except=20 for nodes which already is special-cased for the update part), so we=20 might as well pass the full thing ;) or am I missing something here? > + > + my $cfg =3D config(); > + if (my $scfg =3D storage_config($cfg, $sid, 1)) { > + $node =3D 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} =3D $type; > + for my $key (keys %$verify_params) { > + if (!defined($scfg->{$key})) { > + die "Option '${key}' is not configured for storage '$sid', " > + ."expected it to be '$verify_params->{$key}'"; > + } > + if ($verify_params->{$key} ne $scfg->{$key}) { > + die "Option '${key}' ($verify_params->{$key}) does not match " > + ."existing storage configuration '$scfg->{$key}'\n"; > + } > + } > + } > +} > + > # removes leading/trailing spaces and (back)slashes completely > # substitutes every non-ASCII-alphanumerical char with '_', except '_.-' > sub normalize_content_filename { > --=20 > 2.30.2 >=20 >=20 >=20 > _______________________________________________ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel >=20 >=20 >=20