[pve-devel] [PATCH manager 10/14] ceph: mon create: lock monitor creation

[Thomas Lamprecht]

On 6/18/19 3:42 PM, Dominik Csapak wrote:
> otherwise it is possible that multiple users create monitors at the same
> time, resulting in a wrong ceph.conf and probably worse
> 
> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
> ---
> best viewed with -w
>  PVE/API2/Ceph/MON.pm | 97 +++++++++++++++++++++++++++++-----------------------
>  1 file changed, 54 insertions(+), 43 deletions(-)
> 
> diff --git a/PVE/API2/Ceph/MON.pm b/PVE/API2/Ceph/MON.pm
> index 6cba7f8e..832a275e 100644
> --- a/PVE/API2/Ceph/MON.pm
> +++ b/PVE/API2/Ceph/MON.pm
> @@ -196,62 +196,73 @@ __PACKAGE__->register_method ({
>  	my $worker = sub  {
>  	    my $upid = shift;
>  
> -	    my $client_keyring = PVE::Ceph::Tools::get_or_create_admin_keyring();
> -	    my $mon_keyring = PVE::Ceph::Tools::get_config('pve_mon_key_path');
> +	    PVE::Cluster::cfs_lock_file('ceph.conf', undef, sub {
> +		# update cfg content and reassert prereqs inside the lock
> +		$cfg = cfs_read_file('ceph.conf');
> +		# reopen with longer timeout
> +		if (!$firstmon) {

Even if we'd say it's OK to add the $firstmon variable to encode that
correlation into the variable name I still would like to do:
s/!$firstmon/$rados/
here.. IMO this is independent, if we have a $rados connection we re-open
it with bigger time out, we do not need to care why we have the $rados
connection, just makes things more brittle, IMO..

> +		    $rados = PVE::RADOS->new(timeout => PVE::Ceph::Tools::get_config('long_rados_timeout'));
> +		}
> +		$monhash = PVE::Ceph::Services::get_services_info('mon', $cfg, $rados);
> +		$assert_mon_prerequisites->($cfg, $monhash, $monid, $ip);
>  
> -	    if (! -f $mon_keyring) {
> -		run_command("ceph-authtool --create-keyring $mon_keyring ".
> -		    " --gen-key -n mon. --cap mon 'allow *'");
> -		run_command("ceph-authtool $mon_keyring --import-keyring $client_keyring");
> -	    }
> +		my $client_keyring = PVE::Ceph::Tools::get_or_create_admin_keyring();
> +		my $mon_keyring = PVE::Ceph::Tools::get_config('pve_mon_key_path');
>  
> -	    my $ccname = PVE::Ceph::Tools::get_config('ccname');
> -	    my $mondir =  "/var/lib/ceph/mon/$ccname-$monid";
> -	    -d $mondir && die "monitor filesystem '$mondir' already exist\n";
> +		if (! -f $mon_keyring) {
> +		    run_command("ceph-authtool --create-keyring $mon_keyring ".
> +			" --gen-key -n mon. --cap mon 'allow *'");
> +		    run_command("ceph-authtool $mon_keyring --import-keyring $client_keyring");
> +		}
>  
> -	    my $monmap = "/tmp/monmap";
> +		my $ccname = PVE::Ceph::Tools::get_config('ccname');
> +		my $mondir =  "/var/lib/ceph/mon/$ccname-$monid";
> +		-d $mondir && die "monitor filesystem '$mondir' already exist\n";
>  
> -	    eval {
> -		mkdir $mondir;
> +		my $monmap = "/tmp/monmap";
>  
> -		run_command("chown ceph:ceph $mondir");
> +		eval {
> +		    mkdir $mondir;
>  
> -		if (!$firstmon) {
> -		    my $rados = PVE::RADOS->new(timeout => PVE::Ceph::Tools::get_config('long_rados_timeout'));
> -		    my $mapdata = $rados->mon_command({ prefix => 'mon getmap', format => 'plain' });
> -		    file_set_contents($monmap, $mapdata);
> -		} else {
> -		    my $monaddr = $ip;
> -		    if (Net::IP::ip_is_ipv6($ip)) {
> -			$monaddr = "[$ip]";
> -			$cfg->{global}->{ms_bind_ipv6} = 'true';
> +		    run_command("chown ceph:ceph $mondir");
> +
> +		    if (!$firstmon) {
> +			my $mapdata = $rados->mon_command({ prefix => 'mon getmap', format => 'plain' });
> +			file_set_contents($monmap, $mapdata);
> +		    } else {
> +			my $monaddr = $ip;
> +			if (Net::IP::ip_is_ipv6($ip)) {
> +			    $monaddr = "[$ip]";
> +			    $cfg->{global}->{ms_bind_ipv6} = 'true';
> +			}
> +			run_command("monmaptool --create --clobber --addv $monid '[v2:$monaddr:3300,v1:$monaddr:6789]' --print $monmap");
>  		    }
> -		    run_command("monmaptool --create --clobber --addv $monid '[v2:$monaddr:3300,v1:$monaddr:6789]' --print $monmap");
> +
> +		    run_command("ceph-mon --mkfs -i $monid --monmap $monmap --keyring $mon_keyring --public-addr $ip");
> +		    run_command("chown ceph:ceph -R $mondir");
> +		};
> +		my $err = $@;
> +		unlink $monmap;
> +		if ($err) {
> +		    File::Path::remove_tree($mondir);
> +		    die $err;
>  		}
>  
> -		run_command("ceph-mon --mkfs -i $monid --monmap $monmap --keyring $mon_keyring --public-addr $ip");
> -		run_command("chown ceph:ceph -R $mondir");
> -	    };
> -	    my $err = $@;
> -	    unlink $monmap;
> -	    if ($err) {
> -		File::Path::remove_tree($mondir);
> -		die $err;
> -	    }
> +		# update ceph.conf
> +		my $monhost = $cfg->{global}->{mon_host} // "";
> +		$monhost .= " $ip";
> +		$cfg->{global}->{mon_host} = $monhost;
>  
> -	    # update ceph.conf
> -	    my $monhost = $cfg->{global}->{mon_host} // "";
> -	    $monhost .= " $ip";
> -	    $cfg->{global}->{mon_host} = $monhost;
> +		cfs_write_file('ceph.conf', $cfg);
>  
> -	    cfs_write_file('ceph.conf', $cfg);
> +		PVE::Ceph::Services::ceph_service_cmd('start', $monsection);
>  
> -	    PVE::Ceph::Services::ceph_service_cmd('start', $monsection);
> +		eval { PVE::Ceph::Services::ceph_service_cmd('enable', $monsection) };
> +		warn "Enable ceph-mon\@${monid}.service failed, do manually: $@\n" if $@;
>  
> -	    eval { PVE::Ceph::Services::ceph_service_cmd('enable', $monsection) };
> -	    warn "Enable ceph-mon\@${monid}.service failed, do manually: $@\n" if $@;
> -
> -	    PVE::Ceph::Services::broadcast_ceph_services();
> +		PVE::Ceph::Services::broadcast_ceph_services();
> +	    });
> +	    die $@ if $@;
>  	};
>  
>  	return $rpcenv->fork_worker('cephcreatemon', $monsection, $authuser, $worker);
>