From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <t.lamprecht@proxmox.com>
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 2999171617
for <pve-devel@lists.proxmox.com>; Mon, 4 Oct 2021 08:16:58 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 1B41816E87
for <pve-devel@lists.proxmox.com>; Mon, 4 Oct 2021 08:16:58 +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) server-digest SHA256)
(No client certificate requested)
by firstgate.proxmox.com (Proxmox) with ESMTPS id 28F5816E79
for <pve-devel@lists.proxmox.com>; Mon, 4 Oct 2021 08:16:57 +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 F051344930
for <pve-devel@lists.proxmox.com>; Mon, 4 Oct 2021 08:16:56 +0200 (CEST)
Message-ID: <52e1a156-853a-51b4-42e0-a7b926eff2fd@proxmox.com>
Date: Mon, 4 Oct 2021 08:15:48 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:93.0) Gecko/20100101
Thunderbird/93.0
Content-Language: en-US
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>,
Aaron Lauterer <a.lauterer@proxmox.com>
References: <20211001135934.3429632-1-a.lauterer@proxmox.com>
<20211001140611.3467172-1-a.lauterer@proxmox.com>
From: Thomas Lamprecht <t.lamprecht@proxmox.com>
In-Reply-To: <20211001140611.3467172-1-a.lauterer@proxmox.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.229 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
Subject: Re: [pve-devel] [PATCH v2 manager] ceph install: improve ceph
install checks during installation
X-BeenThere: pve-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=subscribe>
X-List-Received-Date: Mon, 04 Oct 2021 06:16:58 -0000
On 01.10.21 16:06, Aaron Lauterer wrote:
> Adding a lock file during the Ceph installation helps to cover the time
> span in which the binary is already present but the installation not yet
> done.
>
> The most noticeable effect is that the 'Next' button in the GUI will
> only become active once the installation is actually finished and not
> earlier.
>
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
>
> checking if the file exists should be better done with -f than with -e
> to check for plain files only.
>
> PVE/CLI/pveceph.pm | 9 +++++++++
> PVE/Ceph/Tools.pm | 4 +++-
> 2 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/PVE/CLI/pveceph.pm b/PVE/CLI/pveceph.pm
> index 356af282..3bee3413 100755
> --- a/PVE/CLI/pveceph.pm
> +++ b/PVE/CLI/pveceph.pm
> @@ -177,7 +177,14 @@ __PACKAGE__->register_method ({
> );
>
> print "start installation\n";
> +
some comment for why we do that would be nice to have here too.
> + my $install_lock_file = '/var/lock/ceph_install.lck';
this is not a real lock but just a best-effort flag, so lets not pretend it has
lock characteristics.. I'd change the variable and the file name to:
my $install_flag_fn = '/run/pve-ceph-install-flag';
(and please no underscores in file names if we can avoid it ;P)
> + open(INSTALL_LOCK, ">${install_lock_file}") ||
> + die "could not lock Ceph installation\n";
3 issues:
1. two argument open is considered bad, perlcritic would have caught that:
https://pve.proxmox.com/wiki/Perl_Style_Guide#Basic_Linting_with_perlcritic
See Damian Conway's Perl Best Practice, pg. 207, "Opening Cleanly"
2. Please include the system error ($!) for such things, else the user has no idea
what actually went wrong
3. Bareword file handle opened, See pages 202,204 of PBP.
Rather use the following template `open(my $fh, '>', $file) or die "meh - $!;`
(fyi: I made a PDF of PBP available on our internal iso share)
in summary, do:
open(my $install_flag, '>', $install_flag) or die "couldn't open install flag - $!";
> + close INSTALL_LOCK;
> +
> if (system(@apt_install, @ceph_packages) != 0) {
> + unlink $install_lock_file;
always note the error on system functions, but unlink is a bit special as if the
file has gone on itself it may not be seen as error, here it would be fine though,
it's an internal flag file that the outside should not touch, so plain warning
would be good:
unlink $install_flag or warn "failed to unlink install-flag file - $!";
> die "apt failed during ceph installation ($?)\n";
> }
>
> @@ -188,6 +195,8 @@ __PACKAGE__->register_method ({
> 'systemctl', 'try-reload-or-restart', 'pvedaemon.service', 'pveproxy.service'
> ]);
>
> + unlink $install_lock_file;
same here irg. to error handling
> +
> return undef;
> }});
>
> diff --git a/PVE/Ceph/Tools.pm b/PVE/Ceph/Tools.pm
> index f54d837a..c09dd778 100644
> --- a/PVE/Ceph/Tools.pm
> +++ b/PVE/Ceph/Tools.pm
> @@ -155,7 +155,9 @@ sub check_ceph_installed {
>
> $service = 'ceph_bin' if !defined($service);
>
> - if (! -x $ceph_service->{$service}) {
> + # ceph_install.lck is checked to cover the time where the binary is already
# the flag file is checked as on new installation the binary gets extracted by
# dpkg before installation finished
> + # present, but the installation not yet done
> + if (! -x $ceph_service->{$service} || -f '/var/lock/ceph_install.lck') {
> die "binary not installed: $ceph_service->{$service}\n" if !$noerr;
> return undef;
> }
>