[pve-devel] [PATCH manager 1/3] pveceph: create mgr with mon, use nodename for id

[Thomas Lamprecht]

some comments inline

On 07/25/2017 05:14 PM, Dominik Csapak wrote:
> we now want to add a ceph-mgr daemon to every node where a ceph-mon
> daemon runs, as per ceph documentation recommendation, because in
> luminous the mgr daemons will not be automatically created/started
> with a monitor anymore
> 
> we also give the createmon an optional id parameter, so that one
> can set a custom id, and make the creation/removal of the manager
> optional but the default
> 
> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
> ---
>   PVE/API2/Ceph.pm | 88 +++++++++++++++++++++++++++++++++++++++++++++++++-------
>   PVE/CephTools.pm |  2 +-
>   2 files changed, 79 insertions(+), 11 deletions(-)
> 
> diff --git a/PVE/API2/Ceph.pm b/PVE/API2/Ceph.pm
> index 105ee37a..307409b4 100644
> --- a/PVE/API2/Ceph.pm
> +++ b/PVE/API2/Ceph.pm
> @@ -843,11 +843,53 @@ my $find_node_ip = sub {
>       die "unable to find local address within network '$cidr'\n";
>   };
>   
> +my $create_mgr = sub {
> +    my ($rados, $id) = @_;
> +
> +    my $clustername = PVE::CephTools::get_config('ccname');
> +    my $mgrdir = "/var/lib/ceph/mgr/$clustername-$id";
> +    my $mgrkeyring = "$mgrdir/keyring";
> +    my $mgrname = "mgr.$id";
> +
> +    die "ceph manager directory '$mgrdir' already exists\n"
> +	if -d $mgrdir;
> +
> +    mkdir $mgrdir;
> +    my $output = $rados->mon_command({ prefix => 'auth get-or-create',
> +				       entity => $mgrname,
> +				       caps => [
> +					   mon => 'allow profile mgr',
> +					   osd => 'allow *',
> +					   mds => 'allow *',
> +				       ],
> +				       format => 'plain'});
> +    PVE::Tools::file_set_contents($mgrkeyring, $output);
> +
> +    run_command(["chown", 'ceph:ceph', '-R', $mgrdir]);
> +
> +    PVE::CephTools::ceph_service_cmd('start', $mgrname);
> +    PVE::CephTools::ceph_service_cmd('enable', $mgrname);
> +};
> +
> +my $destroy_mgr = sub {
> +    my ($mgrid) = @_;
> +
> +    my $clustername = PVE::CephTools::get_config('ccname');
> +    my $mgrname = "mgr.$mgrid";
> +    my $mgrdir = "/var/lib/ceph/mgr/$clustername-$mgrid";
> +
> +    die "ceph manager '$mgrname' not found \n"
> +	if ! -d $mgrdir;


I'd make a similar error messages as above which fits the check, i.e.
that the manager directory does not exist.
Else a failed remove could lead to a non-existing directory but still
running service – where this error message may then be confusing.

> +
> +    PVE::CephTools::ceph_service_cmd('stop', $mgrname);
> +    File::Path::remove_tree($mgrdir);
> +};
> +
>   __PACKAGE__->register_method ({
>       name => 'createmon',
>       path => 'mon',
>       method => 'POST',
> -    description => "Create Ceph Monitor",
> +    description => "Create Ceph Monitor and Manager",
>       proxyto => 'node',
>       protected => 1,
>       permissions => {
> @@ -857,6 +899,18 @@ __PACKAGE__->register_method ({
>       	additionalProperties => 0,
>   	properties => {
>   	    node => get_standard_option('pve-node'),
> +	    id => {
> +		type => 'string',
> +		optional => 1,
> +		pattern => '[a-zA-Z0-9]([a-zA-Z0-9\-]*[a-zA-Z0-9]?)',
> +		description => "The ID for the monitor, when omitted the same as the nodename",
> +	    },
> +	    monitoronly => {
> +		type => 'boolean',
> +		optional => 1,
> +		default => 0,
> +		description => "When set, only a monitor will be created.",
> +	    },
>   	},
>       },
>       returns => { type => 'string' },
> @@ -891,12 +945,13 @@ __PACKAGE__->register_method ({
>   	}
>   
>   	my $monid;
> -	for (my $i = 0; $i < 7; $i++) {
> -	    if (!$cfg->{"mon.$i"}) {
> -		$monid = $i;
> -		last;
> -	    }
> +
> +	if (defined($param->{id})) {
> +	    $monid = $param->{id};
> +	} else {
> +	    $monid = $param->{node};
>   	}
> +
>   	die "unable to find usable monitor id\n" if !defined($monid);

This can be removed to, monid is guranteed to be defined here.
Either it's the optional `id` param or else the required `node` param.

>   
>   	my $monsection = "mon.$monid";
> @@ -944,14 +999,15 @@ __PACKAGE__->register_method ({
>   	    -d $mondir && die "monitor filesystem '$mondir' already exist\n";
>    
>   	    my $monmap = "/tmp/monmap";
> -	
> +
> +	    my $rados = PVE::RADOS->new(timeout => PVE::CephTools::get_config('long_rados_timeout'));
> +
>   	    eval {
>   		mkdir $mondir;
>   
>   		run_command("chown ceph:ceph $mondir") if $systemd_managed;
>   
>   		if ($moncount > 0) {
> -		    my $rados = PVE::RADOS->new(timeout => PVE::CephTools::get_config('long_rados_timeout'));
>   		    my $mapdata = $rados->mon_command({ prefix => 'mon getmap', format => 'plain' });
>   		    PVE::Tools::file_set_contents($monmap, $mapdata);
>   		} else {
> @@ -990,6 +1046,11 @@ __PACKAGE__->register_method ({
>   		}
>   		waitpid($create_keys_pid, 0);
>   	    }
> +
> +	    # create manager
> +	    if (!$param->{monitoronly}) {
> +		$create_mgr->($rados, $monid);

IMO, as we have a check here if we should do or do not something with
the manager, this option should rather have a 'manager'-related name,
e.g 'exclude-manager', or a default to true 'include-manager'.

> +	    }
>   	};
>   
>   	return $rpcenv->fork_worker('cephcreatemon', $monsection, $authuser, $worker);
> @@ -999,7 +1060,7 @@ __PACKAGE__->register_method ({
>       name => 'destroymon',
>       path => 'mon/{monid}',
>       method => 'DELETE',
> -    description => "Destroy Ceph monitor.",
> +    description => "Destroy Ceph Monitor and Manager.",
>       proxyto => 'node',
>       protected => 1,
>       permissions => {
> @@ -1011,8 +1072,15 @@ __PACKAGE__->register_method ({
>   	    node => get_standard_option('pve-node'),
>   	    monid => {
>   		description => 'Monitor ID',
> -		type => 'integer',
> +		type => 'string',
> +		pattern => '[a-zA-Z0-9]([a-zA-Z0-9\-]*[a-zA-Z0-9]?)',
>   	    },
> +	    monitoronly => {

same as above

> +		type => 'boolean',
> +		default => 0,
> +		optional => 1,
> +		description => "When set, removes only the monitor, not the manager"
> +	    }
>   	},
>       },
>       returns => { type => 'string' },
> diff --git a/PVE/CephTools.pm b/PVE/CephTools.pm
> index 0d26ea3e..0c0d7c18 100644
> --- a/PVE/CephTools.pm
> +++ b/PVE/CephTools.pm
> @@ -178,7 +178,7 @@ sub ceph_service_cmd {
>   
>       if (systemd_managed()) {
>   
> -	if ($service && $service =~ m/^(mon|osd|mds|radosgw)(\.([A-Za-z0-9]{1,32}))?$/) {
> +	if ($service && $service =~ m/^(mon|osd|mds|mgr|radosgw)(\.([A-Za-z0-9\-]{1,32}))?$/) {
>   	    $service = defined($3) ? "ceph-$1\@$3" : "ceph-$1.target";
>   	} else {
>   	    $service = "ceph.target";
> 

The remove hunk which slipped in patch 2 look good, I'd make the eval
one line with the warn directly below, though. We do that most of the
times if there is only one command to execute, but this is just me
nitpicking.