[pve-devel] [PATCH manager v3 10/20] ceph/destroypool: optionally remove storages

[Fabian Grünbichler]

On Tue, Sep 05, 2017 at 09:32:57AM +0200, Thomas Lamprecht wrote:
> On 08/31/2017 11:38 AM, Fabian Grünbichler wrote:
> > only storages which have the 'pveceph' flag set are removed
> > 
> > Signed-off-by: Fabian Grünbichler <f.gruenbichler at proxmox.com>
> > ---
> > changes since v2:
> > - adapted for $get_storages changes
> > - inlined $remove_storage
> > 
> > changes since v1:
> > - die if any of the storages could not be removed
> > - move storage retrieval into lower if (refactoring was dropped in v2)
> > 
> >   PVE/API2/Ceph.pm | 28 +++++++++++++++++++++++++++-
> >   1 file changed, 27 insertions(+), 1 deletion(-)
> > 
> > diff --git a/PVE/API2/Ceph.pm b/PVE/API2/Ceph.pm
> > index a8ea2b39..5620d6b3 100644
> > --- a/PVE/API2/Ceph.pm
> > +++ b/PVE/API2/Ceph.pm
> > @@ -1820,7 +1820,13 @@ __PACKAGE__->register_method ({
> >   		type => 'boolean',
> >   		optional => 1,
> >   		default => 0,
> > -	    }
> > +	    },
> > +	    remove_storages => {
> > +		description => "Remove all pveceph-managed storages configured for this pool",
> > +		type => 'boolean',
> > +		optional => 1,
> > +		default => 0,
> > +	    },
> >   	},
> >       },
> >       returns => { type => 'null' },
> > @@ -1829,7 +1835,13 @@ __PACKAGE__->register_method ({
> >   	PVE::CephTools::check_ceph_inited();
> > +	my $rpcenv = PVE::RPCEnvironment::get();
> > +	my $user = $rpcenv->get_user();
> > +	$rpcenv->check($user, '/storage', ['Datastore.Allocate'])
> > +	    if $param->{remove_storages};
> > +
> 
> possible nitpick: in the former patch in the create API call you use an
> if block over this whole check, i.e. only declaring $rpcenv and $user
> maybe use that here too? a) for consistency and b) the other one is easier
> to read, IMO :)

those declarations were then pulled out of the if a few patches later,
because I made createpool/destroypool async and need the user and env
for forking a worker ;) the only difference is that createpool has an
additional check regarding the pool/storage name, hence it gets a 'real'
if.

maybe adding a new line before the check improves readability?

> 
> >   	my $pool = $param->{name};
> > +	my $storages = $get_storages->($pool);
> >   	# if not forced, destroy ceph pool only when no
> >   	# vm disks are on it anymore
> > @@ -1857,6 +1869,20 @@ __PACKAGE__->register_method ({
> >   	    format => 'plain',
> >           });
> > +	if ($param->{remove_storages}) {
> > +	    my $err;
> > +	    foreach my $storeid (keys %$storages) {
> > +		next if !$storages->{$storeid}->{pveceph};
> > +		eval { PVE::API2::Storage::Config->delete({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;
> >       }});
> > 
> 
>