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

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

[Fiona Ebner]

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.

qemu-server:

Fiona Ebner (4):
  block job: monitor: document that job-complete is not used if source
    differs from target
  fix #8209: clone disk: do not ignore failure to query size of target
  clone disk: abort early when failing to query size of source disk
  clone disk: bump timeout for querying target volume size to 1 minute

 src/PVE/API2/Qemu.pm           | 21 ++++++++++++++-------
 src/PVE/QemuServer.pm          | 18 ++++++++++++++++--
 src/PVE/QemuServer/BlockJob.pm |  1 +
 3 files changed, 31 insertions(+), 9 deletions(-)


Summary over all repositories:
  3 files changed, 31 insertions(+), 9 deletions(-)

-- 
Generated by git-murpp 0.5.0

[PATCH qemu-server 1/4] block job: monitor: document that job-complete is not used if source differs from target

[Fiona Ebner]

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/QemuServer/BlockJob.pm | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/PVE/QemuServer/BlockJob.pm b/src/PVE/QemuServer/BlockJob.pm
index 493dda33..5757c24f 100644
--- a/src/PVE/QemuServer/BlockJob.pm
+++ b/src/PVE/QemuServer/BlockJob.pm
@@ -82,6 +82,7 @@ sub qemu_blockjobs_cancel {
 
 # $completion can be either
 # 'complete': wait until all jobs are ready, job-complete them (default)
+#     If the source peer is not the same as the destination VM, uses block-job-cancel instead.
 # 'cancel': wait until all jobs are ready, block-job-cancel them
 # 'skip': wait until all jobs are ready, return with block jobs in ready state
 # 'auto': wait until all jobs disappear, only use for jobs which complete automatically
-- 
2.47.3

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

[Fiona Ebner]

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

[PATCH qemu-server 3/4] clone disk: abort early when failing to query size of source disk

[Fiona Ebner]

Not all plugin implementations of the volume_size_info() method fail
if there is an error. In particular, the native file-based plugins
use file_size_info() which returns undef upon failure.

While clone is likely to fail later (e.g. vdisk_alloc() is called with
an undef size), avoid any surprises and fail early with a clean error.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/QemuServer.pm | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index 8f062466..4b7351b3 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -7895,6 +7895,7 @@ sub clone_disk {
             );
 
             $size = PVE::Storage::volume_size_info($storecfg, $drive->{file}, 10);
+            die "unable to query size of volume '$drive->{file}'\n" if !$size;
         }
         $source->{size} = $size;
         $newvolid = PVE::Storage::vdisk_alloc(
-- 
2.47.3

[PATCH qemu-server 4/4] clone disk: bump timeout for querying target volume size to 1 minute

[Fiona Ebner]

Since commit "fix #8209: clone disk: do not ignore failure to query
size of target", failure to query the size of the target volume after
clone is no longer quietly ignored. Bump the timeout to reduce the
likelyhood of harmless timeouts causing a warning. Print a message to
communicate that the query is happening to the user.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/QemuServer.pm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index 4b7351b3..0f8aef6f 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -7931,7 +7931,8 @@ sub clone_disk {
     }
 
     my $result = {};
-    my $size = eval { PVE::Storage::volume_size_info($storecfg, $newvolid, 10) };
+    print "querying size of target volume '$newvolid' after copy..\n";
+    my $size = eval { PVE::Storage::volume_size_info($storecfg, $newvolid, 60) };
     my $err = $@;
     $err = "no size in result" if !$err && !$size;
     if ($err) {
-- 
2.47.3

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

[Elias Huhsovitz]

Comments inline

(This is not a full review yet)

On Thu Oct 22, 2026 at 11:38 AM CEST, Fiona Ebner wrote:
> 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;

Since we now set $clone_result->{'after-copy-error'}, if e.g., the
target NFS share returns an error, we set $do_delete = 0 to prevent the
source disk from being deleted.

> +                }
> +
> +                if (!$do_delete) {
>                      my $unused_key = PVE::QemuConfig->add_unused_volume($conf, $old_volid);

So now in the case that $clone_result->{'after-copy-error'} is set, we
always call add_unused_volume(). But add_unused_volume() can fail with

"Too many unused volumes - please delete them first.\n"

according to AbstractConfig::add_unused_volume()

Couldn't this kill the disk during a live migration, if we are unable to
add the unused volume?

In Drive.pm the maximum number for unused disks is set to:

our $MAX_UNUSED_DISKS = 256;

IMO being unable to add a new disk in a production system, might be a
real possibility.

>                      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 {

Re: [PATCH qemu-server 1/4] block job: monitor: document that job-complete is not used if source differs from target

[Elias Huhsovitz]

Comments inline.

On Thu Oct 22, 2026 at 11:38 AM CEST, Fiona Ebner wrote:
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>  src/PVE/QemuServer/BlockJob.pm | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/src/PVE/QemuServer/BlockJob.pm b/src/PVE/QemuServer/BlockJob.pm
> index 493dda33..5757c24f 100644
> --- a/src/PVE/QemuServer/BlockJob.pm
> +++ b/src/PVE/QemuServer/BlockJob.pm
> @@ -82,6 +82,7 @@ sub qemu_blockjobs_cancel {
>  
>  # $completion can be either
>  # 'complete': wait until all jobs are ready, job-complete them (default)
> +#     If the source peer is not the same as the destination VM, uses block-job-cancel instead.

The comments reads like, this a special behaviour of the 'complete',
case. But in this case, isn't the 'complete' & 'cancel' behaviour the
same?

IMO this is just a property of the function. What about something like:

	If the source peer is not the same as the destination VM, no disk
	is switched: 'complete' and 'cancel' both block-job-cancel instead.
    ('skip' and 'auto' are unaffected.)

>  # 'cancel': wait until all jobs are ready, block-job-cancel them
>  # 'skip': wait until all jobs are ready, return with block jobs in ready state
>  # 'auto': wait until all jobs disappear, only use for jobs which complete automatically

Re: [PATCH qemu-server 3/4] clone disk: abort early when failing to query size of source disk

[Elias Huhsovitz]

Seems sensible!

Reviewed-by: Elias Huhsovitz <e.huhsovitz@proxmox.com>

On Thu Oct 22, 2026 at 11:38 AM CEST, Fiona Ebner wrote:
> Not all plugin implementations of the volume_size_info() method fail
> if there is an error. In particular, the native file-based plugins
> use file_size_info() which returns undef upon failure.
>
> While clone is likely to fail later (e.g. vdisk_alloc() is called with
> an undef size), avoid any surprises and fail early with a clean error.
>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>  src/PVE/QemuServer.pm | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
> index 8f062466..4b7351b3 100644
> --- a/src/PVE/QemuServer.pm
> +++ b/src/PVE/QemuServer.pm
> @@ -7895,6 +7895,7 @@ sub clone_disk {
>              );
>  
>              $size = PVE::Storage::volume_size_info($storecfg, $drive->{file}, 10);
> +            die "unable to query size of volume '$drive->{file}'\n" if !$size;
>          }
>          $source->{size} = $size;
>          $newvolid = PVE::Storage::vdisk_alloc(

Re: [PATCH qemu-server 4/4] clone disk: bump timeout for querying target volume size to 1 minute

[Elias Huhsovitz]

On Thu Oct 22, 2026 at 11:38 AM CEST, Fiona Ebner wrote:
> Since commit "fix #8209: clone disk: do not ignore failure to query
> size of target", failure to query the size of the target volume after
> clone is no longer quietly ignored. Bump the timeout to reduce the
> likelyhood of harmless timeouts causing a warning. Print a message to
> communicate that the query is happening to the user.
>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>  src/PVE/QemuServer.pm | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
> index 4b7351b3..0f8aef6f 100644
> --- a/src/PVE/QemuServer.pm
> +++ b/src/PVE/QemuServer.pm
> @@ -7931,7 +7931,8 @@ sub clone_disk {
>      }
>  
>      my $result = {};
> -    my $size = eval { PVE::Storage::volume_size_info($storecfg, $newvolid, 10) };
> +    print "querying size of target volume '$newvolid' after copy..\n";

nit: IMO the message should say: 'after clone...', since we are calling 
PVE::Storage::vdisk_clone.

super nit: i think it should be three dots (...), instead of 2 (..) ;)

> +    my $size = eval { PVE::Storage::volume_size_info($storecfg, $newvolid, 60) };

nit: The timeout is inconsistent with the volume_size_info() call on
line 7897. There it is still 10 seconds. I am not sure if we can
gurantee that querying the source volume is always faster than querying
the target volume.

general: Tbh I am not sure that a generic timeout is the optimal
long-term solution here. I feel like the timeout should be storage
specific. E.g., an incrased timeout of 60 seconds for a NFS, but keep
the 10 seconds for a local ZFS.

But for now, the incrased timeout is fine.

>      my $err = $@;
>      $err = "no size in result" if !$err && !$size;
>      if ($err) {

Re: [PATCH qemu-server 1/4] block job: monitor: document that job-complete is not used if source differs from target

[Fiona Ebner]

Am 23.10.26 um 10:44 AM schrieb Elias Huhsovitz:
> Comments inline.
> 
> On Thu Oct 22, 2026 at 11:38 AM CEST, Fiona Ebner wrote:
>> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
>> ---
>>  src/PVE/QemuServer/BlockJob.pm | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/src/PVE/QemuServer/BlockJob.pm b/src/PVE/QemuServer/BlockJob.pm
>> index 493dda33..5757c24f 100644
>> --- a/src/PVE/QemuServer/BlockJob.pm
>> +++ b/src/PVE/QemuServer/BlockJob.pm
>> @@ -82,6 +82,7 @@ sub qemu_blockjobs_cancel {
>>  
>>  # $completion can be either
>>  # 'complete': wait until all jobs are ready, job-complete them (default)
>> +#     If the source peer is not the same as the destination VM, uses block-job-cancel instead.
> 
> The comments reads like, this a special behaviour of the 'complete',
> case. But in this case, isn't the 'complete' & 'cancel' behaviour the
> same?

Yes, it reads like that, because it is. For cancel, this is not a
special case; cancel always does block-job-cancel. I would find it
somewhat confusing to talk about it for cancel if it is the same result.

> 
> IMO this is just a property of the function. What about something like:
> 
> 	If the source peer is not the same as the destination VM, no disk
> 	is switched: 'complete' and 'cancel' both block-job-cancel instead.
>     ('skip' and 'auto' are unaffected.)
> 
>>  # 'cancel': wait until all jobs are ready, block-job-cancel them
>>  # 'skip': wait until all jobs are ready, return with block jobs in ready state
>>  # 'auto': wait until all jobs disappear, only use for jobs which complete automatically
> 

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

[Fiona Ebner]

Am 22.10.26 um 3:25 PM schrieb Elias Huhsovitz:
> On Thu Oct 22, 2026 at 11:38 AM CEST, Fiona Ebner wrote:
>> @@ -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;
> 
> Since we now set $clone_result->{'after-copy-error'}, if e.g., the
> target NFS share returns an error, we set $do_delete = 0 to prevent the
> source disk from being deleted.
> 
>> +                }
>> +
>> +                if (!$do_delete) {
>>                      my $unused_key = PVE::QemuConfig->add_unused_volume($conf, $old_volid);
> 
> So now in the case that $clone_result->{'after-copy-error'} is set, we
> always call add_unused_volume(). But add_unused_volume() can fail with
> 
> "Too many unused volumes - please delete them first.\n"
> 
> according to AbstractConfig::add_unused_volume()
> 
> Couldn't this kill the disk during a live migration, if we are unable to
> add the unused volume?
> 
> In Drive.pm the maximum number for unused disks is set to:
> 
> our $MAX_UNUSED_DISKS = 256;
> 
> IMO being unable to add a new disk in a production system, might be a
> real possibility.

It's not a new issue (my patch only changes that 'delete' is not used in
a rare error scenario, delete=0 was already possible before). I'm not
aware of any reports about people running into this. It should be quite
rare to reach that limit in practice. Still, it's an issue and I'll add
a patch to fix it in v2.

And the issue is also present in case the template_create() call fails,
I'll guard that too.

> 
>>                      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]);

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

[Fiona Ebner]

Am 28.10.26 um 10:05 AM schrieb Fiona Ebner:
> Am 22.10.26 um 3:25 PM schrieb Elias Huhsovitz:
>> On Thu Oct 22, 2026 at 11:38 AM CEST, Fiona Ebner wrote:
>>> @@ -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;
>>
>> Since we now set $clone_result->{'after-copy-error'}, if e.g., the
>> target NFS share returns an error, we set $do_delete = 0 to prevent the
>> source disk from being deleted.
>>
>>> +                }
>>> +
>>> +                if (!$do_delete) {
>>>                      my $unused_key = PVE::QemuConfig->add_unused_volume($conf, $old_volid);
>>
>> So now in the case that $clone_result->{'after-copy-error'} is set, we
>> always call add_unused_volume(). But add_unused_volume() can fail with
>>
>> "Too many unused volumes - please delete them first.\n"
>>
>> according to AbstractConfig::add_unused_volume()
>>
>> Couldn't this kill the disk during a live migration, if we are unable to
>> add the unused volume?
>>
>> In Drive.pm the maximum number for unused disks is set to:
>>
>> our $MAX_UNUSED_DISKS = 256;
>>
>> IMO being unable to add a new disk in a production system, might be a
>> real possibility.
> 
> It's not a new issue (my patch only changes that 'delete' is not used in
> a rare error scenario, delete=0 was already possible before). I'm not
> aware of any reports about people running into this. It should be quite
> rare to reach that limit in practice. Still, it's an issue and I'll add
> a patch to fix it in v2.
> 
> And the issue is also present in case the template_create() call fails,
> I'll guard that too.

Ah no, since the VM can't be running if it's a template ;)

Re: [PATCH qemu-server 4/4] clone disk: bump timeout for querying target volume size to 1 minute

[Fiona Ebner]

Am 23.10.26 um 11:31 AM schrieb Elias Huhsovitz:
> On Thu Oct 22, 2026 at 11:38 AM CEST, Fiona Ebner wrote:
>> Since commit "fix #8209: clone disk: do not ignore failure to query
>> size of target", failure to query the size of the target volume after
>> clone is no longer quietly ignored. Bump the timeout to reduce the
>> likelyhood of harmless timeouts causing a warning. Print a message to
>> communicate that the query is happening to the user.
>>
>> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
>> ---
>>  src/PVE/QemuServer.pm | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
>> index 4b7351b3..0f8aef6f 100644
>> --- a/src/PVE/QemuServer.pm
>> +++ b/src/PVE/QemuServer.pm
>> @@ -7931,7 +7931,8 @@ sub clone_disk {
>>      }
>>  
>>      my $result = {};
>> -    my $size = eval { PVE::Storage::volume_size_info($storecfg, $newvolid, 10) };
>> +    print "querying size of target volume '$newvolid' after copy..\n";
> 
> nit: IMO the message should say: 'after clone...', since we are calling 
> PVE::Storage::vdisk_clone.

The message is user-facing, so it does not matter what the function in
Perl is called. But we do log "create full|linked clone of drive ..." in
the beginning of the function, so I'll change it to be consistent with
that :)

> 
> super nit: i think it should be three dots (...), instead of 2 (..) ;)

We do have quite some instances with .. but I'll change it.

> 
>> +    my $size = eval { PVE::Storage::volume_size_info($storecfg, $newvolid, 60) };
> 
> nit: The timeout is inconsistent with the volume_size_info() call on
> line 7897. There it is still 10 seconds. I am not sure if we can
> gurantee that querying the source volume is always faster than querying
> the target volume.

That's fair, especially since there can be multiple disks involved in a
full guest clone. I'll bump it there too.

> general: Tbh I am not sure that a generic timeout is the optimal
> long-term solution here. I feel like the timeout should be storage
> specific. E.g., an incrased timeout of 60 seconds for a NFS, but keep
> the 10 seconds for a local ZFS.

I don't think we should try to predict how fast a storage "should" be
based on the plugin type. That depends much more on the
hardware/workloads. Also, clone is already a long-running operation and
failure can be a lot of wasted work, so I don't think trying to
"optimize" the tolerated timeout would be useful. If a storage is fast,
it will just not run into the timeout; if it is slow, the larger timeout
is beneficial; if it is usually fast, but not right now, it is still
beneficial to have the larger timeout.

> 
> But for now, the incrased timeout is fine.
> 
>>      my $err = $@;
>>      $err = "no size in result" if !$err && !$size;
>>      if ($err) {
>