[pve-devel] [PATCH manager 2/3] pveceph: add createmgr/destroymgr commands

[Thomas Lamprecht]

some comments inline

On 07/25/2017 05:14 PM, Dominik Csapak wrote:
> this patch adds the create-/destroymgr commands to the api and pveceph,
> so that advanced users can split monitor and manager daemons
> 
> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
> ---
>   PVE/API2/Ceph.pm   | 105 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>   PVE/CLI/pveceph.pm |   2 +
>   2 files changed, 107 insertions(+)
> 
> diff --git a/PVE/API2/Ceph.pm b/PVE/API2/Ceph.pm
> index 307409b4..85413487 100644
> --- a/PVE/API2/Ceph.pm
> +++ b/PVE/API2/Ceph.pm
> @@ -1126,12 +1126,117 @@ __PACKAGE__->register_method ({
>   	    delete $cfg->{$monsection};
>   	    PVE::CephTools::write_ceph_config($cfg);
>   	    File::Path::remove_tree($mondir);
> +
> +	    # remove manager
> +	    if (!$param->{monitoronly}) {
> +
> +		eval {
> +		    $destroy_mgr->($monid);
> +		};
> +
> +		warn $@ if $@;
> +	    }

This should be part of patch 1 instead.

>   	};
>   
>   	return $rpcenv->fork_worker('cephdestroymon', $monsection,  $authuser, $worker);
>       }});
>   
>   __PACKAGE__->register_method ({
> +    name => 'createmgr',
> +    path => 'mgr',
> +    method => 'POST',
> +    description => "Create Ceph Manager",
> +    proxyto => 'node',
> +    protected => 1,
> +    permissions => {
> +	check => ['perm', '/', [ 'Sys.Modify' ]],
> +    },
> +    parameters => {
> +	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]?)',

The question mark should be after the parenthesis else a '-' at the end
of the ID would be allowed.
AFAIS, this needs to change on all id->pattern property entries you
added.

> +		description => "The ID for the monitor, when omitted the same as the nodename",
> +	    },
> +	},
> +    },
> +    returns => { type => 'string' },
> +    code => sub {
> +	my ($param) = @_;
> +
> +	PVE::CephTools::check_ceph_inited();
> +
> +	my $rpcenv = PVE::RPCEnvironment::get();
> +
> +	my $authuser = $rpcenv->get_user();
> +
> +	my $mgrid;
> +
> +	if (defined($param->{id})) {
> +	    $mgrid = $param->{id};
> +	} else {
> +	    $mgrid = $param->{node};
> +	}
> +
> +	die "unable to find usable manager id\n" if !defined($mgrid);
> +

Same as in patch 1, we will never take this branch, so can be omitted
too.

> +	my $worker = sub  {
> +	    my $upid = shift;
> +
> +	    my $rados = PVE::RADOS->new(timeout => PVE::CephTools::get_config('long_rados_timeout'));
> +
> +	    $create_mgr->($rados, $mgrid);
> +	};
> +
> +	return $rpcenv->fork_worker('cephcreatemgr', "mgr.$mgrid", $authuser, $worker);
> +    }});
> +
> +__PACKAGE__->register_method ({
> +    name => 'destroymgr',
> +    path => 'mgr/{mgrid}',
> +    method => 'DELETE',
> +    description => "Destroy Ceph Manager.",
> +    proxyto => 'node',
> +    protected => 1,
> +    permissions => {
> +	check => ['perm', '/', [ 'Sys.Modify' ]],
> +    },
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    node => get_standard_option('pve-node'),
> +	    mgrid => {

here you call it 'mgrid', above just 'id'. I understand that the
rationale may have been that the 'id' above has a sane fallback, but
it's still the manager ID, maybe stream lining this would be better?


> +		description => 'Manager ID',
> +		type => 'string',
> +		pattern => '[a-zA-Z0-9]([a-zA-Z0-9\-]*[a-zA-Z0-9]?)',
> +	    },
> +	},
> +    },
> +    returns => { type => 'string' },
> +    code => sub {
> +	my ($param) = @_;
> +
> +	my $rpcenv = PVE::RPCEnvironment::get();
> +
> +	my $authuser = $rpcenv->get_user();
> +
> +	PVE::CephTools::check_ceph_inited();
> +
> +	my $mgrid = $param->{mgrid};
> +
> +	my $worker = sub {
> +	    my $upid = shift;
> +
> +	    $destroy_mgr->($mgrid);
> +	};
> +
> +	return $rpcenv->fork_worker('cephdestroymgr', "mgr.$mgrid",  $authuser, $worker);
> +    }});
> +
> +__PACKAGE__->register_method ({
>       name => 'stop',
>       path => 'stop',
>       method => 'POST',
> diff --git a/PVE/CLI/pveceph.pm b/PVE/CLI/pveceph.pm
> index ee34cfc5..4f4a7708 100755
> --- a/PVE/CLI/pveceph.pm
> +++ b/PVE/CLI/pveceph.pm
> @@ -170,6 +170,8 @@ our $cmddef = {
>       destroyosd => [ 'PVE::API2::CephOSD', 'destroyosd', ['osdid'], { node => $nodename }, $upid_exit],
>       createmon => [ 'PVE::API2::Ceph', 'createmon', [], { node => $nodename }, $upid_exit],
>       destroymon => [ 'PVE::API2::Ceph', 'destroymon', ['monid'], { node => $nodename }, $upid_exit],
> +    createmgr => [ 'PVE::API2::Ceph', 'createmgr', [], { node => $nodename }, $upid_exit],
> +    destroymgr => [ 'PVE::API2::Ceph', 'destroymgr', ['mgrid'], { node => $nodename }, $upid_exit],
>       start => [ 'PVE::API2::Ceph', 'start', ['service'], { node => $nodename }, $upid_exit],
>       stop => [ 'PVE::API2::Ceph', 'stop', ['service'], { node => $nodename }, $upid_exit],
>       install => [ __PACKAGE__, 'install', [] ],
>