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});
_______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
