[pve-devel] [PATCH manager] implement checks for ceph version & binaries

[Thomas Lamprecht]

some comments inline

On 08/23/2017 07:02 PM, Alwin Antreich wrote:
> add version check to ceph init to require luminous or higher and
> fix #1481: check existence of ceph binaries before use
> 
> Signed-off-by: Alwin Antreich <a.antreich at proxmox.com>
> ---
>   PVE/API2/Ceph.pm | 17 ++++++++++++++++-
>   PVE/CephTools.pm | 44 +++++++++++++++++++++++++++++++++++++++-----
>   2 files changed, 55 insertions(+), 6 deletions(-)
> 
> diff --git a/PVE/API2/Ceph.pm b/PVE/API2/Ceph.pm
> index c4d6ffcb..2a87cb67 100644
> --- a/PVE/API2/Ceph.pm
> +++ b/PVE/API2/Ceph.pm
> @@ -233,6 +233,8 @@ __PACKAGE__->register_method ({
>   
>   	PVE::CephTools::setup_pve_symlinks();
>   
> +	PVE::CephTools::check_ceph_installed(undef, "ceph_osd");
> +
>   	my $bluestore = $param->{bluestore} // 0;
>   
>   	my $journal_dev;
> @@ -827,7 +829,13 @@ __PACKAGE__->register_method ({
>       code => sub {
>   	my ($param) = @_;
>   
> -	PVE::CephTools::check_ceph_installed();
> +	my $version = PVE::CephTools::get_local_version(1);
> +
> +	if (!$version || $version < 12) {
> +	    die "Ceph version luminous or higher is required\n";
> +	} else {
> +	    PVE::CephTools::check_ceph_installed();
> +	}
>   
>   	# simply load old config if it already exists
>   	my $cfg = PVE::CephTools::parse_ceph_config();
> @@ -982,6 +990,11 @@ __PACKAGE__->register_method ({
>       code => sub {
>   	my ($param) = @_;
>   
> +	PVE::CephTools::check_ceph_installed(undef, "ceph_mon");
> +
> +	PVE::CephTools::check_ceph_installed(undef, "ceph_mgr")
> +	    if (!$param->{'exclude-manager'});
> +
>   	PVE::CephTools::check_ceph_inited();
>   
>   	PVE::CephTools::setup_pve_symlinks();
> @@ -1220,6 +1233,8 @@ __PACKAGE__->register_method ({
>       code => sub {
>   	my ($param) = @_;
>   
> +	PVE::CephTools::check_ceph_installed(undef, "ceph_mgr");
> +>   	PVE::CephTools::check_ceph_inited();
>   
>   	my $rpcenv = PVE::RPCEnvironment::get();
> diff --git a/PVE/CephTools.pm b/PVE/CephTools.pm
> index 0c0d7c18..f13d2796 100644
> --- a/PVE/CephTools.pm
> +++ b/PVE/CephTools.pm
> @@ -20,7 +20,12 @@ my $pve_ckeyring_path = "/etc/pve/priv/$ccname.client.admin.keyring";
>   my $ceph_bootstrap_osd_keyring = "/var/lib/ceph/bootstrap-osd/$ccname.keyring";
>   my $ceph_bootstrap_mds_keyring = "/var/lib/ceph/bootstrap-mds/$ccname.keyring";
>   
> -my $ceph_bin = "/usr/bin/ceph";
> +my $ceph_service = {
> +    ceph_bin => "/usr/bin/ceph",
> +    ceph_mon => "/usr/bin/ceph-mon",
> +    ceph_mgr => "/usr/bin/ceph-mgr",
> +    ceph_osd => "/usr/bin/ceph-osd"
> +};
>   
>   my $config_hash = {
>       ccname => $ccname,
> @@ -32,6 +37,23 @@ my $config_hash = {
>       long_rados_timeout => 60,
>   };
>   
> +sub get_local_version {
> +    my ($noerr) = @_;
> +
> +    if (PVE::CephTools::check_ceph_installed($noerr)) {
> +	my $ceph_version;
> +	run_command([$ceph_service->{ceph_bin}, '--version'],
> +	            noerr => $noerr,
> +	            outfunc => sub { $ceph_version = shift; });
> +	if ($ceph_version && $ceph_version =~ /^ceph.*\s((\d+)\.(\d+)\.(\d+))/) {
> +	    # return (version, major, minor, patch) : major;
> +	    return wantarray ? ($1, $2, $3, $4) : $2;
> +	}
> +    }
> +
> +    return undef;
> +}
> +
>   sub get_config {
>       my $key = shift;
>   
> @@ -60,11 +82,23 @@ sub purge_all_ceph_files {
>   }
>   
>   sub check_ceph_installed {
> -    my ($noerr) = @_;
> +    my ($noerr, $service) = @_;

Our convention is to have "noerr" variables at the end, so that they can be easily omitted.
I understand your approach, as service is new and can also be omitted so you put it after
"noerr" to not affect existing code. But as this is used on only two places in the same file
(CephTools) it could be worth to hold our convention up and swap them.
Honestly, I can live with both.

>   
> -    if (! -x $ceph_bin) {
> -	die "ceph binaries not installed\n" if !$noerr;
> -	return undef;
> +    if (!defined($service)) {
> +	    if (! -x $ceph_service->{ceph_bin}) {

One indentation level to much, there should be just a single tab here, omitting the four spaces.

> +		die "binary not installed: $ceph_service->{ceph_bin}\n" if !$noerr;

the last tab should be four spaces instead. I know our indentation level can be seen as a bit special
but a good editor can take care of it so that you do not need to worry :)

> +		return undef;
> +	    }
> +
> +    } elsif (defined($service)) {

This is the exact opposite check of the if above, so it will always be taken if the above
branch wasn't and vice versa. This should also be an else branch.


> +	foreach my $key (keys %$ceph_service) {
> +	    next if $service ne $key;
> +
> +	    if (! -x $ceph_service->{$key}){

Why not use $ceph_service->{$service} directly, making the foreach loop unnecessary?
Then both branches look suspiciously similar, you could merge now both branches.
Somewhat like:

...
my ($service, $noerr) = @_;

$service = 'ceph_bin' if !defined($service);

if (! -x $ceph_service->{$service}) {
...

or, if you like this alternative more:

...
my ($tool, $noerr) = @_;

my $service = defined($tool) ? $ceph_service->{$tool} : $ceph_service->{ceph_bin};

if (! -x $service) {
...

> +		die "binary not installed: $ceph_service->{$key}\n" if !$noerr;
> +		return undef;
> +	    }
> +	}
>       }
>   
>       return 1;
>