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

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

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