On 5/31/24 14:56, Fiona Ebner wrote:
Am 19.04.24 um 14:45 schrieb Dominik Csapak:
we currently only call deactivate_volumes, but we actually want to call
the whole vm_stop_cleanup, since that is not invoked by the vm_stop
above (we cannot parse the config anymore) and might do other cleanups
we also want to do (like mdev cleanup).
For this to work properly we have to clone the original config at the
beginning, since we might modify the volids.
Signed-off-by: Dominik Csapak <d.csa...@proxmox.com>
---
PVE/QemuMigrate.pm | 12 ++++++------
test/MigrationTest/Shared.pm | 3 +++
2 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
index 8d9b35ae..381022f5 100644
--- a/PVE/QemuMigrate.pm
+++ b/PVE/QemuMigrate.pm
@@ -5,6 +5,7 @@ use warnings;
use IO::File;
use IPC::Open2;
+use Storable qw(dclone);
use Time::HiRes qw( usleep );
use PVE::Cluster;
Needs a rebase (because of added include for PVE::AccessControl)
@@ -1455,7 +1456,8 @@ sub phase3_cleanup {
my $tunnel = $self->{tunnel};
- my $sourcevollist = PVE::QemuServer::get_vm_volumes($conf);
+ # we'll need an unmodified copy of the config later for the cleanup
+ my $oldconf = dclone($conf);
if ($self->{volume_map} && !$self->{opts}->{remote}) {
my $target_drives = $self->{target_drive};
@@ -1586,12 +1588,10 @@ sub phase3_cleanup {
$self->{errors} = 1;
}
- # always deactivate volumes - avoid lvm LVs to be active on several nodes
- eval {
- PVE::Storage::deactivate_volumes($self->{storecfg}, $sourcevollist);
- };
+ # stop with nocheck does not do a cleanup, so do it here with the original
config
+ eval { PVE::QemuServer::vm_stop_cleanup($self->{storecfg}, $vmid,
$oldconf) };
if (my $err = $@) {
- $self->log('err', $err);
+ $self->log('err', "cleanup for vm failed - $err");
Nit: "Cleanup after stopping VM failed"
Is it better to only execute this in case vm_stop() did not return an
error? Although I guess attempting cleanup in that case also doesn't hurt.
not sure honestly, we cannot really know at this point when the vm stop failed
and
if we should do a cleanup.. my guess is that when the vm is still running
the cleanup will fail anyway at some step
but IMHO doing it and potentially generating more warning/error output
vs. not doing it and missing some cleanup, i'd prefer the former
$self->{errors} = 1;
}
diff --git a/test/MigrationTest/Shared.pm b/test/MigrationTest/Shared.pm
index aa7203d1..2347e60a 100644
--- a/test/MigrationTest/Shared.pm
+++ b/test/MigrationTest/Shared.pm
@@ -130,6 +130,9 @@ $qemu_server_module->mock(
clear_reboot_request => sub {
return 1;
},
+ vm_stop_cleanup => sub {
+ return 1;
Nit: I'd just have it be return; without a value.
+ },
get_efivars_size => sub {
return 128 * 1024;
},
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
