Re: [pve-devel] [PATCH manager 4/6] api: mon: mds: osd: add safety check endpoints

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

On 18.02.22 12:38, Aaron Lauterer wrote:
> for mon, mds and osd: ok-to-stop
> mon: ok-to-rm
> osd: safe-to-destroy
> 
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
> 
> I added the OSD safe-to-destroy endpoint even though I have no immediate
> use for it as of now. But plan to include it in the OSD panel once I
> have an idea how to do so in a sensible manner.
> 
> The main problem with safe-to-destroy is, that it only works if the OSD
> is still running and by the time we enable the destroy button, the OSD
> needs to be stopped.


above isn't really meta info for review but rather important for the commit
message itself.


In general I see lots of repetition, and in this case I'd rather have a single
enpoint that accepts one (or maybe better a list of) service-type(s), and an
action (stop/destroy) let's encode in the name (or at least description) that
it's a heuristical check, besides things that we possible miss to observe we
could never make it 100% safe as we cannot lock the whole ceph cluster between
checking and doing an operation, so this will always be a TOCTOU race that 
expects the admins to have some change management so that they do not interfere
with each others maintenance work.

So either `/nodes/<nodename>/ceph/cmd-safety-heuristic` or drop the heuristic
from the path and just refer to that detail in the description (which shows up
in the api viewer, so should be good enough) `/nodes/<nodename>/ceph/cmd-safety`

params could be: node, type, id and command

> 
>  PVE/API2/Ceph/MDS.pm |  50 ++++++++++++++++++++++
>  PVE/API2/Ceph/MON.pm | 100 +++++++++++++++++++++++++++++++++++++++++++
>  PVE/API2/Ceph/OSD.pm | 100 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 250 insertions(+)
> 
> diff --git a/PVE/API2/Ceph/MDS.pm b/PVE/API2/Ceph/MDS.pm
> index 1cb0b74f..2ed3cf5a 100644
> --- a/PVE/API2/Ceph/MDS.pm
> +++ b/PVE/API2/Ceph/MDS.pm
> @@ -230,4 +230,54 @@ __PACKAGE__->register_method ({
>      }
>  });
>  
> +__PACKAGE__->register_method ({
> +    name => 'oktostop',

FYI: the name is usable to call this directly via code API2::Ceph::MDS->oktostop(),
so it would be good to use the perl method naming convention.

> +    path => '{name}/ok-to-stop',
> +    method => 'GET',
> +    description => "Check if it is ok to stop the MON",

"heuristic check if..."

> +    proxyto => 'node',
> +    protected => 1,
> +    permissions => {
> +	check => ['perm', '/', [ 'Sys.Modify' ]],
> +    },

hmm, why Sys.Modify, I mean this doesn't do anything so Sys.Audit could be argued
too, or did you have something other in mind here (e.g., consistency with some other,
similarly categorized api endpoint)? 

> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    node => get_standard_option('pve-node'),
> +	    name => {
> +		type => 'string',
> +		pattern => PVE::Ceph::Services::SERVICE_REGEX,
> +		description => 'The name (ID) of the mds',
> +	    },
> +	},
> +    },
> +    returns => {
> +	type => "object",

please actually describe the return format for such simple api calls

> +    },
> +    code => sub {
> +	my ($param) = @_;
> +
> +	PVE::Ceph::Tools::check_ceph_inited();
> +
> +	my $name = $param->{name};
> +	my $rados = PVE::RADOS->new();
> +
> +	my $result = {
> +	    ok_to_stop => 0,
> +	    status => '',
> +	};
> +
> +	my $code;
> +	my $status;
> +	eval {
> +	    ($code, $status) = $rados->mon_command({ prefix => 'mds ok-to-stop', format => 'plain', ids => [ $name ] }, 1);
> +	};


eval's return their last expression (which can but doesn has to use a trailing semicolon)
so you could do:

my ($code, $status) = eval {
    $rados->mon_command(...)
};

> +	die $@ if $@;

ok, why bother with eval'ing if you die 1:1 in any error case anyway??

> +
> +	$result->{ok_to_stop} = $code == 0 ? 1 : 0;
> +	$result->{status} = $status;

for such simple logic we can avoid the intermediate $result and return directly,
that shaves off about 6 lines from this api call and please also use the kebab-case
for new properties, both in input parameters or output result.

return {
    'ok-to-stop' => $code == 0 ? 1 : 0,
    status => $status,
};

> +
> +	return $result;
> +    }});
> +
>  1;