Re: [pve-devel] [PATCH v2 manager] ceph install: improve ceph install checks during installation

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

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;
>      }
>