From 75854662699b261915289f36e8fbe953c5f77b7c Mon Sep 17 00:00:00 2001 From: Thomas Lamprecht Date: Mon, 25 Jan 2021 15:06:19 +0100 Subject: [PATCH] vm destroy: allow opt-out of purging unreferenced disks Since an old change released with a version bump on 2009-09-07, we search all enabled storages for VMID maching volumes on VM removal and purge those too. This has multiple pitfalls and may be quite unexpected for some users. It can make problems when: * on recovery a VM is created, before disks are reattached the admin notices some settings issues and chooses to just recreate the VM; but during destroying the dummy VM all related disks get destroyed unconditionally which may result in data loss. This actually happened and is the original reason for the decision to change this. * a storage is shared between PVE instance (between a set of clusters and/or single nodes), while this is against our rules it may still come as a surprise if destroying a VM on node A may destroy unrelated and unreferenced disks on the unrelated node B without asking or allowing to avoid that. As this the removal of matching but unreferenced disks can result in permanent data loss (up to the last backup) and may be to subtle and unforgiving, allow to opt-out of it. In the long run we want to make this opt-in, but that is an API change and so needs to wait for next major release. But, we can adapt the GUI already to make it opt-in there, catching most of the cases. side-note: CT do not have this behavior at all Signed-off-by: Thomas Lamprecht --- PVE/API2/Qemu.pm | 16 +++++++++++++++- PVE/QemuServer.pm | 17 +++++++++-------- 2 files changed, 24 insertions(+), 9 deletions(-) diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm index e2d2d67b..3710bf1e 100644 --- a/PVE/API2/Qemu.pm +++ b/PVE/API2/Qemu.pm @@ -1517,6 +1517,12 @@ __PACKAGE__->register_method({ description => "Remove VMID from configurations, like backup & replication jobs and HA.", optional => 1, }, + 'destroy-unreferenced-disks' => { + type => 'boolean', + description => "If set, destroy all disks with the VMID from all enabled storages.", + optional => 1, + default => 1, # FIXME: replace to false in PVE 7.0, this is dangerous! + } }, }, returns => { @@ -1566,7 +1572,15 @@ __PACKAGE__->register_method({ # repeat, config might have changed my $ha_managed = $early_checks->(); - PVE::QemuServer::destroy_vm($storecfg, $vmid, $skiplock, { lock => 'destroyed' }); + # FIXME: drop fallback to true with 7.0, to dangerous for default + my $purge_unreferenced = $param->{'destroy-unreferenced-disks'} // 1; + + PVE::QemuServer::destroy_vm( + $storecfg, + $vmid, + $skiplock, { lock => 'destroyed' }, + $purge_unreferenced, + ); PVE::AccessControl::remove_vm_access($vmid); PVE::Firewall::remove_vmfw_conf($vmid); diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm index ae9d6dd3..eb2270c5 100644 --- a/PVE/QemuServer.pm +++ b/PVE/QemuServer.pm @@ -2074,7 +2074,7 @@ sub check_type { } sub destroy_vm { - my ($storecfg, $vmid, $skiplock, $replacement_conf) = @_; + my ($storecfg, $vmid, $skiplock, $replacement_conf, $purge_unreferenced) = @_; my $conf = PVE::QemuConfig->load_config($vmid); @@ -2110,13 +2110,14 @@ sub destroy_vm { warn "Could not remove disk '$volid', check manually: $@" if $@; }); - # also remove unused disk - my $vmdisks = PVE::Storage::vdisk_list($storecfg, undef, $vmid); - PVE::Storage::foreach_volid($vmdisks, sub { - my ($volid, $sid, $volname, $d) = @_; - eval { PVE::Storage::vdisk_free($storecfg, $volid) }; - warn $@ if $@; - }); + if ($purge_unreferenced) { # also remove unreferenced disk + my $vmdisks = PVE::Storage::vdisk_list($storecfg, undef, $vmid); + PVE::Storage::foreach_volid($vmdisks, sub { + my ($volid, $sid, $volname, $d) = @_; + eval { PVE::Storage::vdisk_free($storecfg, $volid) }; + warn $@ if $@; + }); + } if (defined $replacement_conf) { PVE::QemuConfig->write_config($vmid, $replacement_conf);