[pve-devel] [RFC storage] rbd: unprotect all snapshots on image removal

[Fabian Grünbichler]

On November 30, 2019 6:50 pm, Thomas Lamprecht wrote:
> On 11/29/19 12:00 PM, Fabian Grünbichler wrote:
>> we need to unprotect more snapshots than just the base one, since we
>> allow linked clones of regular VM snapshots. unprotection will only work
>> if no linked clones exist anymore.
>> 
>> Signed-off-by: Fabian Grünbichler <f.gruenbichler at proxmox.com>
>> ---
>> it's still rather ugly if such a linked clone exists, since unprotection
>> and thus deletion will fail, but the VM config is gone. at least with
>> this patch, "pvesm free storage:image" will work after
>> removing/flattening all the linked clones ;)
>> 
>> alternatively we could iterate over all snapshots in vm_destroy and
>> attempt to delete them (which will unprotect them in case of RBD),
>> leaving non-PVE-managed snapshots untouched.
>> 
> 
> that's sounds honestly slightly nicer - but I guess it's harder to do
> or else you would not had chosen this way?
> 

not really harder, but I guess it depends on the storage whether it's 
better or not ;)

for ZFS and qcow2 it's probably faster to just (recursively) delete the
volume itself including all snapshots, instead of each snapshot that we
know about individually up-front, and then the image and any remaining 
snapshots as second step.

for Ceph it's about the same work anyway, the difference is just whether 
we want to cleanup non-PVE-managed, protected snapshots or leave them 
alone and not remove the volume instead..

for LVM-thin I think it does not matter much?

>>  PVE/Storage/RBDPlugin.pm | 39 +++++++++++++++++++++++++++++++++++----
>>  1 file changed, 35 insertions(+), 4 deletions(-)
>> 
>> diff --git a/PVE/Storage/RBDPlugin.pm b/PVE/Storage/RBDPlugin.pm
>> index 10b54e5..2ead5de 100644
>> --- a/PVE/Storage/RBDPlugin.pm
>> +++ b/PVE/Storage/RBDPlugin.pm
>> @@ -227,6 +227,38 @@ sub rbd_ls {
>>      return $list;
>>  }
>>  
>> +sub rbd_ls_snap {
>> +    my ($scfg, $storeid, $name) = @_;
>> +
>> +    my $cmd = &$rbd_cmd($scfg, $storeid, 'snap', 'ls', $name, '--format', 'json');
>> +
>> +    my $raw = '';
>> +    run_rbd_command($cmd, errmsg => "rbd error", errfunc => sub {}, outfunc => sub { $raw .= shift; });
>> +
>> +    my $list;
>> +    if ($raw =~ m/^(\[.*\])$/s) { # untaint
>> +	$list = eval { JSON::decode_json($1) };
>> +	die "invalid JSON output from 'rbd snap ls $name': $@\n" if $@;
>> +    } else {
>> +	die "got unexpected data from 'rbd snap ls $name': '$raw'\n";
>> +    }
>> +
>> +    $list = [] if !defined($list);
>> +
>> +    my $res = {};
>> +    foreach my $el (@$list) {
>> +	my $snap = $el->{name};
>> +	my $protected = defined($el->{protected}) && $el->{protected} eq "true" ? 1 : undef;
>> +	$res->{$snap} = {
>> +	    name => $snap,
>> +	    id => $el->{id} // undef,
>> +	    size => $el->{size} // 0,
>> +	    protected => $protected,
>> +	};
>> +    }
>> +    return $res;
>> +}
>> +
>>  sub rbd_volume_info {
>>      my ($scfg, $storeid, $volname, $snap) = @_;
>>  
>> @@ -483,10 +515,9 @@ sub free_image {
>>      my ($vtype, $name, $vmid, undef, undef, undef) =
>>  	$class->parse_volname($volname);
>>  
>> -    if ($isBase) {
>> -	my $snap = '__base__';
>> -	my (undef, undef, undef, $protected) = rbd_volume_info($scfg, $storeid, $name, $snap);
>> -	if ($protected){
>> +    my $snaps = rbd_ls_snap($scfg, $storeid, $name);
>> +    foreach my $snap (keys %$snaps) {
>> +	if ($snaps->{$snap}->{protected}) {
>>  	    my $cmd = &$rbd_cmd($scfg, $storeid, 'snap', 'unprotect', $name, '--snap', $snap);
>>  	    run_rbd_command($cmd, errmsg => "rbd unprotect $name snap '$snap' error");
>>  	}
>> 
> 
> 
>