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 AA3028AA11 for ; Fri, 19 Aug 2022 10:20:08 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 9FDD6182BF for ; Fri, 19 Aug 2022 10:20:08 +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 ; Fri, 19 Aug 2022 10:20:07 +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 9D73C433DE for ; Fri, 19 Aug 2022 10:20:07 +0200 (CEST) Date: Fri, 19 Aug 2022 10:20:00 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Aaron Lauterer , Proxmox VE development discussion References: <20220715115808.1385388-1-a.lauterer@proxmox.com> <20220715115808.1385388-3-a.lauterer@proxmox.com> <1660736115.7grkvr18dc.astroid@nora.none> In-Reply-To: MIME-Version: 1.0 User-Agent: astroid/0.15.0 (https://github.com/astroidmail/astroid) Message-Id: <1660896759.rnpqx2mr05.astroid@nora.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 0.161 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. [lvm.pm] Subject: Re: [pve-devel] [PATCH storage v2 2/3] disks: die if storage name is already in use 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: Fri, 19 Aug 2022 08:20:08 -0000 On August 18, 2022 5:22 pm, Aaron Lauterer wrote: >=20 >=20 > On 8/17/22 13:42, Fabian Gr=C3=BCnbichler wrote: >> On July 15, 2022 1:58 pm, Aaron Lauterer wrote: > [...] >>> - my $worker =3D sub { >>> - my $path =3D "/mnt/pve/$name"; >>> - my $mountunitname =3D PVE::Systemd::escape_unit($path, 1) . ".mou= nt"; >>> - my $mountunitpath =3D "/etc/systemd/system/$mountunitname"; >>> + my $mounted =3D PVE::Diskmanage::mounted_paths(); >>> + die "a mountpoint for '${name}' already exists: ${path} ($1)\n" if $m= ounted->{$path}; >>=20 >> nit: the mountpoint existing is not the issue (something already being >> mounted there is!). also, where does the $1 come from? >=20 > good point and thanks for catching the $1 >>=20 >>> + die "a systemd mount unit already exists: ${mountunitpath}\n" if -e $= mountunitpath; >>=20 >> could check if it's identical to the one we'd generate (in the spirit of >> patch #3 ;)) >=20 > I looked into it, depending on how hard we want to match the mount unit, = this=20 > could be a bit hard. It contains the /dev/disk/by-uuid/... path which wil= l not=20 > be the same as it changes with each FS creation (IIUC). >=20 makes sense, and no, we definitely don't want any fancy heuristics for=20 "close enough to ignore it already exists" ;) so this can stay as it is. >>=20 >>> =20 >>> + my $worker =3D sub { >>> PVE::Diskmanage::locked_disk_action(sub { >>> PVE::Diskmanage::assert_disk_unused($dev); >>> =20 >>> diff --git a/PVE/API2/Disks/LVM.pm b/PVE/API2/Disks/LVM.pm >>> index 6e4331a..a27afe2 100644 >>> --- a/PVE/API2/Disks/LVM.pm >>> +++ b/PVE/API2/Disks/LVM.pm >>> @@ -152,6 +152,9 @@ __PACKAGE__->register_method ({ >>> PVE::Diskmanage::assert_disk_unused($dev); >>> PVE::Storage::assert_sid_unused($name) if $param->{add_storage}; >>> =20 >>> + die "volume group with name '${name}' already exists\n" >>> + if PVE::Storage::LVMPlugin::lvm_vgs()->{$name}; >>=20 >> probably better off inside the worker, since `vgs` might take a while >> (although we also use it in the index API call in this module..) >=20 > From a GUI perspective: putting it in a worker would result in the user = to hit=20 > okay and then will see the failed task right? Keeping it as is will resul= t in an=20 > error popping up when clicking ok/create and the user can edit the name i= nstead=20 > of starting all over again. Though, if `vgs` really needs a bit, that err= or=20 > popping up could take a moment or two. yes, putting it in the worker means no early-failure. but not putting it=20 in the worker potentially means this API endpoint cannot be called on=20 systems with busy LVM at all (30s timeout for API requests!). so=20 early-failure checks should almost always only do "logical" things that=20 are cheap (like reading a conf file and checking invariants), and=20 nothing that can reasonably block for some time (like storage=20 operations, starting guests, ..). I know we are not 100% consistent=20 there (the worst offender is probably the storage content API endpoint=20 ;)), but we should try to not introduce more problems of that fashion=20 but rather work on removing the existing ones. >=20 > [...] >=20 >>> sub preparetree { >>> @@ -336,6 +340,7 @@ __PACKAGE__->register_method ({ >>> my $user =3D $rpcenv->get_user(); >>> =20 >>> my $name =3D $param->{name}; >>> + my $node =3D $param->{node}; >>=20 >> nit: please also change the usage further below in this API handler if >> you do this >=20 > what exactly do you mean? defining local variables for $param->{..} ones? the usage of $param->{node} below in this handler should (also) switch=20 to $node - we don't want two places with the same information, that can=20 cause subtle breakage in the future when only one of them gets=20 modified/.. >=20 >>=20 >>> my $devs =3D [PVE::Tools::split_list($param->{devices})]; >>> my $raidlevel =3D $param->{raidlevel}; >>> my $compression =3D $param->{compression} // 'on'; >>> @@ -346,6 +351,10 @@ __PACKAGE__->register_method ({ >>> } >>> PVE::Storage::assert_sid_unused($name) if $param->{add_storage}; >>> =20 >>> + my $pools =3D get_pool_data(); >>=20 >> and also here >>=20 >>> + die "pool '${name}' already exists on node '$node'\n" >>> + if grep { $_->{name} eq $name } @{$pools}; >>> + >>> my $numdisks =3D scalar(@$devs); >>> my $mindisks =3D { >>> single =3D> 1, >>> --=20 >>> 2.30.2