Re: [pve-devel] [PATCH V3 qemu-server 4/6] add ignore-storage-errors for removing VM with missing storage

[Stefan Hrdlicka <s.hrdlicka@proxmox.com>]

On 11/15/22 13:17, Fiona Ebner wrote:
> Am 15.11.22 um 11:55 schrieb Stefan Hrdlicka:
>> @@ -2341,10 +2346,10 @@ sub destroy_vm {
>>   
>>   		my $volid = $drive->{file};
>>   		return if !$volid || $volid =~ m|^/|;
>> -
>> -		die "base volume '$volid' is still in use by linked cloned\n"
>> -		    if PVE::Storage::volume_is_base_and_used($storecfg, $volid);
>> -
>> +		my $result;
>> +		eval { $result = PVE::Storage::volume_is_base_and_used($storecfg, $volid) };
>> +		die "Couldn't remove one or more disks: $@\n" if $@ && !$ignore_storage_errors;
> 
> This error message is wrong. The check failed, not the removal. The
> check should be repeated in vdisk_free anyways and you should get the
> appropriate error then below :)

yes :).

> 
> AFAIU base volumes should still survive if they are still referenced by
> linked clones, even when ignore-storage-errors is used (IMHO good). Is
> that correct?

Yes this is correct, the volumes still exist.

> 
> Nothing new and not directly related:
> I noticed that for containers, we don't have this heads-up check. Maybe
> worth adding there too? Arguably minor issue is that I can have a
> container template with a disk on lvm-thin and a second disk on
> non-lvm-thin. Even if there is a linked clone, removing the template
> might remove the lvm-thin disk, and then fail, because the second disk
> is referenced.

maybe a good idea, I will look into that

> 
>> +		die "base volume '$volid' is still in use by linked cloned\n" if $result;
>>   	});
>>       }
>>   
>> @@ -2370,7 +2375,8 @@ sub destroy_vm {
>>   	include_unused => 1,
>>   	extra_keys => ['vmstate'],
>>       };
>> -    PVE::QemuConfig->foreach_volume_full($conf, $include_opts, $remove_owned_drive);
>> +    eval { PVE::QemuConfig->foreach_volume_full($conf, $include_opts, $remove_owned_drive); };
>> +    die "Couldn't remove one or more disks: $@\n" if $@ && !$ignore_storage_errors;
> 
> So, $removed_owned_drive already ignores all storage errors beside if
> PVE::Storage::path() fails right? Can't we just add an eval around that
> and be done? No need for a new ignore-storage-errors parameter. Most
> storage errors are already ignored even without that parameter right
> now! I don't think it's a big issue to start ignoring the few missing
> ones as well?

Well I wasn't sure. Safe option was to tell the user that there is a 
problem and then the user decides if something should be forced deleted. 
But if you think this is fine without user input lets pretend this never 
existed :).

> 
>>   
>>       for my $snap (values %{$conf->{snapshots}}) {
>>   	next if !defined($snap->{vmstate});