[pve-devel] [v2 manager 13/27] ceph: implement removestorage API path

[Thomas Lamprecht]

On 08/29/2017 01:04 PM, Fabian Grünbichler wrote:
> removes all storages configured for a pool, or a single
> specified one.
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler at proxmox.com>
> ---
> changes since v1:
> - storageS in API path
> - die on error(s)
> - die if any of the storages were not successfully removed
> 
> note: should we name this 'removestorageS' instead?
> 
>   PVE/API2/Ceph.pm | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 52 insertions(+)
> 
> diff --git a/PVE/API2/Ceph.pm b/PVE/API2/Ceph.pm
> index 606b8f6b..2492fff8 100644
> --- a/PVE/API2/Ceph.pm
> +++ b/PVE/API2/Ceph.pm
> @@ -1652,6 +1652,58 @@ __PACKAGE__->register_method ({
>       }});
>   
>   __PACKAGE__->register_method ({
> +    name => 'removestorage',
> +    path => 'pools/{name}/storages/{storage}',
> +    method => 'DELETE',
> +    description => "Remove storage configuration(s) of a pool",
> +    proxyto => 'node',
> +    protected => 1,
> +    permissions => {
> +	check => [ 'perm', '/', [ 'Sys.Modify', 'Datastore.Allocate' ] ],
> +    },
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    node => get_standard_option('pve-node'),
> +	    name => {
> +		description => "The name of the pool.",
> +		type => 'string',
> +	    },
> +	    storage => get_standard_option('pve-storage-id', { optional => 1 }),
> +	},
> +    },
> +    returns => { type => 'null' },
> +    code => sub {
> +	my ($param) = @_;
> +
> +	PVE::CephTools::check_ceph_inited();
> +
> +	my $pve_ckeyring_path = PVE::CephTools::get_config('pve_ckeyring_path');
> +
> +	die "not fully configured - missing '$pve_ckeyring_path'\n"
> +	    if ! -f $pve_ckeyring_path;
> +
> +	my $target_storage = ;
> +	my $pool_storages = $get_storages->($param->{name});
> +	die "no storage named '$target_storage' for pool $param->{name}"
> +	    if $target_storage && !$pool_storages->{$target_storage};
> +

I'd split this either in a full distinction between the case where the storage
param is given or not.

if(my $target_storage = $param->{storage}) {
  ...
} else {
  ...
}

While this may make it a little longer it improves readability quite a bit, IMO.

As an alternative we could omit the storage param here completely?
It would be nicer to omit this param here and remove the keyring in the default:
DELETE  /storage/{storage}
API path, as now, if I added a pool to a storage here and then delete it in
Datacenter -> Storage the keyring remains and I cannot add this pool as a Storage
with the same name again here - but I can use the name for an arbitrary other
storage through Datacenter->Storages - including RBD.

This may be argumented as OK if we just look at the API, but as with this series,
we can add pools as storages directly through the GUI but then never remove them
sanely (with keyring cleanup) again via GUI, it seems a bit strange, tbh.

Having a plugin specific "cleanup on storage removal" which would be called in:
PVE::API2::Storage::Config->delete()
could solve that.
In this case it could check for pveceph and delete the storage ceph keyring from
/etc/pve/priv if the former is set.
Or is that to hacky?

> +	my $err;
> +	foreach my $storeid (keys %$pool_storages) {
> +	    next if $target_storage && $storeid ne $target_storage;
> +	    eval { $remove_storage->($storeid); };
> +	    if ($@) {
> +		warn "failed to remove storage '$storeid': $@\n";
> +		$err = 1;
> +	    }
> +	}
> +	die "failed to remove (some) storages - check log and remove manually!\n"
> +	    if $err;
> +
> +	return undef;
> +    }});
> +
> +__PACKAGE__->register_method ({
>       name => 'createpool',
>       path => 'pools',
>       method => 'POST',
>