[pve-devel] [PATCH manager 3/6] ceph: add PUT 'flags' api call

[Thomas Lamprecht]

On 7/22/19 4:08 PM, Dominik Csapak wrote:
> this api call can set multiple flags at once, but does this in a
> worker since this can take quiet some time
> 
> also we only set/unset flags that are not already set/unset (respectively)
> 
> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
> ---
>  PVE/API2/Ceph.pm | 65 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 65 insertions(+)
> 
> diff --git a/PVE/API2/Ceph.pm b/PVE/API2/Ceph.pm
> index 72cac7e5..9cc6234a 100644
> --- a/PVE/API2/Ceph.pm
> +++ b/PVE/API2/Ceph.pm
> @@ -918,6 +918,71 @@ __PACKAGE__->register_method ({
>  	return $stat->{flags} // '';
>      }});
>  
> +__PACKAGE__->register_method ({
> +    name => 'set_flags',
> +    path => 'flags',
> +    method => 'PUT',
> +    description => "Set/Unset multiple ceph flags at once.",
> +    proxyto => 'node',
> +    protected => 1,
> +    permissions => {
> +	check => ['perm', '/', [ 'Sys.Modify' ]],
> +    },
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    node => get_standard_option('pve-node'),
> +	    %$possible_flags,
> +	},
> +    },
> +    returns => { type => 'string' },
> +    code => sub {
> +	my ($param) = @_;
> +
> +	my $rpcenv = PVE::RPCEnvironment::get();
> +	my $user = $rpcenv->get_user();
> +	PVE::Ceph::Tools::check_ceph_inited();
> +
> +	my $pve_ckeyring_path = PVE::Ceph::Tools::get_config('pve_ckeyring_path');
> +
> +	die "not fully configured - missing '$pve_ckeyring_path'\n"
> +	    if ! -f $pve_ckeyring_path;

I added a new "check_ceph_configured" method which does both, a
ceph_check_inited and the above keyring file check - we had that at
quite a few places. May want to replace above with a call to that in
a v2

> +
> +	my $rados = PVE::RADOS->new();
> +
> +	my $worker = sub {
> +	    # reopen rados object
> +	    my $rados = PVE::RADOS->new();
> +	    my $stat = $rados->mon_command({ prefix => 'osd dump' });
> +	    my $setflags = {};
> +	    map { $setflags->{$_} = 1 } PVE::Tools::split_list($stat->{flags} // '');

can we do something like a 
my $setflags = $stat->{flags} // '';

further above, IMO a bit nicer than those inline, slightly hidden, fall backs.

Also you can then do the map like:
$setflags = { map { $_ => 1 } PVE::Tools::split_list($setflags) };

(no need to set the hash inline, it can create a hash itself)

(or if you do not like the variable reuse prefix the one from rados with e.g.
"_" to underline that they're the same, just another representation)



> +	    my $errors = 0;
> +	    foreach my $flag (sort keys %$possible_flags) {
> +		next if !defined($param->{$flag});

I mean, a possibility would also be to extract the "node" param and then
loop over the remaining param keys. But above is also totally OK, just
an idea.

> +		my $val = $param->{$flag};
> +		my $realflag = $flagmap->{$flag} // $flag;
> +		next if $val && $setflags->{$realflag}; # we do not set already set flags
> +		next if !$val && !$setflags->{$realflag}; # we do not unset not set flags

is a:
next if $val == $setflags->{$realflag}; 
not enough? Both should only be (perl) boolean-like?

> +
> +		my $prefix = $val ? 'set' : 'unset';
> +		eval {
> +		    warn "$prefix $flag\n";

s/warn/print/  ?
I know in the async fork worker both are the same. but in a CLI RPCEnv
they wouldn't be.


> +		    $rados->mon_command({ prefix => "osd $prefix", key => $flag, });
> +		};
> +		if (my $err = $@) {
> +		    warn $err;
> +		    $errors++;
> +		}
> +	    }
> +
> +	    if ($errors) {
> +		die "could not set/unset $errors flags\n";

maybe we could make $errors a hashref and but the actual failed flags in
there, so that a user has better immediate feedback?

> +	    }
> +	};
> +
> +	return $rpcenv->fork_worker('cephsetflags', undef,  $user, $worker);
> +    }});
> +
>  __PACKAGE__->register_method ({
>      name => 'set_flag',
>      path => 'flags/{flag}',
>