[PATCH qemu-server 2/4] fix #8209: clone disk: do not ignore failure to query size of target

[Fiona Ebner <f.ebner@proxmox.com>]

Since commit 3bae384f ("clone disk: avoid errors after disk was moved
by QEMU"), failure to query the target volume's size after clone is
ignored. It's sensible not to fail hard at this stage, because in case
of live mirror for the same VM with 'complete' completion mode, the
drive was already switched. Also, the error might be a harmless
timeout. Still, as reported in bug #8209, there could be an actual
issue, so log a warning and communicate a failure to the caller, so it
can avoid destructive action.

Some storage plugin implementations of the volume_size_info() method
do not fail with a hard error. In particular, the native file-based
plugins use file_size_info() which returns undef upon failure. Thus,
explicitly check the size.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/API2/Qemu.pm  | 21 ++++++++++++++-------
 src/PVE/QemuServer.pm | 14 +++++++++++++-
 2 files changed, 27 insertions(+), 8 deletions(-)

diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm
index f4dbd2a1..8e7ece52 100644
--- a/src/PVE/API2/Qemu.pm
+++ b/src/PVE/API2/Qemu.pm
@@ -432,7 +432,7 @@ my $import_from_volid = sub {
         $cloned = $clonefn->();
     }
 
-    return $cloned->@{qw(file size)};
+    return $cloned->{drive}->@{qw(file size)};
 };
 
 my sub prohibit_tpm_version_change {
@@ -4703,7 +4703,7 @@ __PACKAGE__->register_method({
                     $dest_info->{efisize} = PVE::QemuServer::get_efivars_size($oldconf)
                         if $opt eq 'efidisk0';
 
-                    my $newdrive = PVE::QemuServer::clone_disk(
+                    my $clone_result = PVE::QemuServer::clone_disk(
                         $storecfg,
                         $source_info,
                         $dest_info,
@@ -4715,7 +4715,7 @@ __PACKAGE__->register_method({
                         $clonelimit,
                     );
 
-                    $newconf->{$opt} = PVE::QemuServer::print_drive($newdrive);
+                    $newconf->{$opt} = PVE::QemuServer::print_drive($clone_result->{drive});
 
                     PVE::QemuConfig->write_config($newid, $newconf);
                     $i++;
@@ -4900,6 +4900,7 @@ __PACKAGE__->register_method({
         my $target_disk = extract_param($param, 'target-disk') // $disk;
         my $storeid = extract_param($param, 'storage');
         my $format = extract_param($param, 'format');
+        my $do_delete = extract_param($param, 'delete');
 
         my $storecfg = PVE::Storage::config();
 
@@ -4932,7 +4933,7 @@ __PACKAGE__->register_method({
                 $storecfg, $conf, $disk, $old_volid,
             );
             die "you can't move a disk with snapshots and delete the source\n"
-                if $snapshotted && $param->{delete};
+                if $snapshotted && $do_delete;
 
             return ($conf, $drive, $oldstoreid, $snapshotted);
         };
@@ -4986,7 +4987,7 @@ __PACKAGE__->register_method({
                 $dest_info->{efisize} = PVE::QemuServer::get_efivars_size($conf)
                     if $disk eq 'efidisk0';
 
-                my $newdrive = PVE::QemuServer::clone_disk(
+                my $clone_result = PVE::QemuServer::clone_disk(
                     $storecfg,
                     $source_info,
                     $dest_info,
@@ -4997,9 +4998,15 @@ __PACKAGE__->register_method({
                     $conf->{agent},
                     $movelimit,
                 );
+                my $newdrive = $clone_result->{drive};
                 $conf->{$disk} = PVE::QemuServer::print_drive($newdrive);
 
-                if (!$param->{delete}) {
+                if ($do_delete && $clone_result->{'after-copy-error'}) {
+                    warn "skipping delete of source because of error after copy\n";
+                    $do_delete = 0;
+                }
+
+                if (!$do_delete) {
                     my $unused_key = PVE::QemuConfig->add_unused_volume($conf, $old_volid);
                     print("adding source volume '$old_volid' as '$unused_key' to config\n");
                 }
@@ -5030,7 +5037,7 @@ __PACKAGE__->register_method({
                 die "storage migration failed: $err";
             }
 
-            if ($param->{delete}) {
+            if ($do_delete) {
                 print("removing source volume '$old_volid' after successful copy\n");
                 eval {
                     PVE::Storage::deactivate_volumes($storecfg, [$old_volid]);
diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index 1efeeb66..8f062466 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -7929,14 +7929,26 @@ sub clone_disk {
         }
     }
 
+    my $result = {};
     my $size = eval { PVE::Storage::volume_size_info($storecfg, $newvolid, 10) };
+    my $err = $@;
+    $err = "no size in result" if !$err && !$size;
+    if ($err) {
+        # The failure might be a harmless timeout. In case of live mirror for the same VM with
+        # 'complete' completion mode, the drive was already switched. Thus, avoid a hard error at
+        # this stage. Still, there could be an actual issue, so the caller can avoid destructive
+        # action based on the presence of 'after-copy-error'.
+        log_warn("unable to query size for '$newvolid' after copy - $err");
+        $result->{'after-copy-error'} = 1;
+    }
 
     my $disk = dclone($drive);
     delete $disk->{format};
     $disk->{file} = $newvolid;
     $disk->{size} = $size if defined($size) && !$unused;
 
-    return $disk;
+    $result->{drive} = $disk;
+    return $result;
 }
 
 sub qemu_use_old_bios_files {
-- 
2.47.3