[pve-devel] [PATCH qemu-server 2/2] update disk size before local disk migration

[Stefan Reiter]

On 12/6/19 9:46 AM, Fabian Grünbichler wrote:
> On December 5, 2019 4:11 pm, Stefan Reiter wrote:
>> Split out 'update_disksize' from the renamed 'update_disk_config' to
>> allow code reuse in QemuMigrate.
>>
>> Remove dots after messages to keep style consistent for migration log.
>>
>> After updating in sync_disks (phase1) of migration, write out updated
>> config. This means that even if migration fails or is aborted in later
>> stages, we keep the fixed config - this is not an issue, as it would
>> have been fixed on the next attempt anyway, and it can't hurt to have
>> the correct size instead of a wrong one either way.
>>
>> Signed-off-by: Stefan Reiter <s.reiter at proxmox.com>
>> ---
>>
>> @Thomas: Is this what you had in mind with the local disk migrate bug?
>>
>> Also, I'm not 100% sure about that last paragraph, if it really doesn't
>> matter...
>>
>>   PVE/QemuMigrate.pm | 13 +++++++++++++
>>   PVE/QemuServer.pm  | 37 +++++++++++++++++++++++++++++--------
>>   2 files changed, 42 insertions(+), 8 deletions(-)
>>
>> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
>> index 27c5b7a..37624b3 100644
>> --- a/PVE/QemuMigrate.pm
>> +++ b/PVE/QemuMigrate.pm
>> @@ -447,6 +447,16 @@ sub sync_disks {
>>   	       'PVE::QemuConfig', $jobcfg, $start_time, $start_time, $logfunc);
>>   	}
>>   
>> +	# sizes in config have to be accurate for remote node to correctly
>> +	# allocate disks, rescan to be sure
>> +	my $volid_hash = PVE::QemuServer::scan_volids($self->{storecfg}, $vmid);
>> +	PVE::QemuServer::foreach_drive($conf, sub {
>> +	    my ($key, $drive) = @_;
>> +	    my $updated = PVE::QemuServer::update_disksize($drive, $volid_hash,
>> +		sub { $self->log('info', $_[0]) });
>> +	    $conf->{$key} = PVE::QemuServer::print_drive($updated) if defined($updated);
>> +	});
>> +
>>   	$self->log('info', "copying local disk images") if scalar(%$local_volumes);
>>   
>>   	foreach my $volid (keys %$local_volumes) {
>> @@ -510,6 +520,9 @@ sub phase1 {
>>   
>>       sync_disks($self, $vmid);
>>   
>> +    # sync_disks fixes disk sizes to match their actual size, write changes so
>> +    # target allocates correct volumes
>> +    PVE::QemuConfig->write_config($vmid, $conf);
>>   };
>>   
>>   sub phase1_cleanup {
>> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
>> index c568d37..49419a4 100644
>> --- a/PVE/QemuServer.pm
>> +++ b/PVE/QemuServer.pm
>> @@ -6059,6 +6059,27 @@ sub is_volume_in_use {
>>   }
>>   
>>   sub update_disksize {
>> +    my ($drive, $volid_hash, $log_fn) = @_;
> 
> this is not a very nice interface.. maybe the logging/printing could be
> moved to the call site(s), by returning
> 
> wantarray ? ($drive, $old, $new) : $drive;
> 
> ?
> 

I did that to avoid writing the whole format_size shenanigans at each 
call site.

Maybe

my $log_msg = "size of disk updated [...]";
wantarray ? ($drive, $log_msg) : $drive;

would be ok too? Removes the $log_fn but keeps the call sites to a 
single print.

>> +
>> +    my $volid = $drive->{file};
>> +    return undef if !defined($volid);
>> +
>> +    my $oldsize = $drive->{size};
>> +    my $newsize = $volid_hash->{$volid}->{size};
>> +
>> +    if (defined($newsize) && defined($oldsize) && $newsize != $oldsize) {
>> +	my $old_fmt = PVE::JSONSchema::format_size($oldsize);
>> +	my $new_fmt = PVE::JSONSchema::format_size($newsize);
>> +	$log_fn->("size of disk '$volid' updated from $old_fmt to $new_fmt\n")
>> +	    if $log_fn;
>> +	$drive->{size} = $newsize;
>> +	return $drive;
>> +    }
>> +
>> +    return undef;
>> +}
>> +
>> +sub update_disk_config {
>>       my ($vmid, $conf, $volid_hash) = @_;
>>   
>>       my $changes;
>> @@ -6080,6 +6101,7 @@ sub update_disksize {
>>   	    my $volid = $drive->{file};
>>   	    next if !$volid;
>>   
>> +	    # mark volid as "in-use" for next step
>>   	    $referenced->{$volid} = 1;
>>   	    if ($volid_hash->{$volid} &&
>>   		(my $path = $volid_hash->{$volid}->{path})) {
>> @@ -6089,12 +6111,11 @@ sub update_disksize {
>>   	    next if drive_is_cdrom($drive);
>>   	    next if !$volid_hash->{$volid};
>>   
>> -	    $drive->{size} = $volid_hash->{$volid}->{size};
>> -	    my $new = print_drive($drive);
>> -	    if ($new ne $conf->{$opt}) {
>> +	    my $updated = update_disksize($drive, $volid_hash,
>> +		sub { print "$prefix $_[0]" });
>> +	    if (defined($updated)) {
>>   		$changes = 1;
>> -		$conf->{$opt} = $new;
>> -		print "$prefix update disk '$opt' information.\n";
>> +		$conf->{$opt} = print_drive($updated);
>>   	    }
>>   	}
>>       }
>> @@ -6105,7 +6126,7 @@ sub update_disksize {
>>   	my $volid = $conf->{$opt};
>>   	my $path = $volid_hash->{$volid}->{path} if $volid_hash->{$volid};
>>   	if ($referenced->{$volid} || ($path && $referencedpath->{$path})) {
>> -	    print "$prefix remove entry '$opt', its volume '$volid' is in use.\n";
>> +	    print "$prefix remove entry '$opt', its volume '$volid' is in use\n";
>>   	    $changes = 1;
>>   	    delete $conf->{$opt};
>>   	}
>> @@ -6122,7 +6143,7 @@ sub update_disksize {
>>   	next if $referencedpath->{$path};
>>   	$changes = 1;
>>   	my $key = PVE::QemuConfig->add_unused_volume($conf, $volid);
>> -	print "$prefix add unreferenced volume '$volid' as '$key' to config.\n";
>> +	print "$prefix add unreferenced volume '$volid' as '$key' to config\n";
>>   	$referencedpath->{$path} = 1; # avoid to add more than once (aliases)
>>       }
>>   
>> @@ -6156,7 +6177,7 @@ sub rescan {
>>   	    $vm_volids->{$volid} = $info if $info->{vmid} && $info->{vmid} == $vmid;
>>   	}
>>   
>> -	my $changes = update_disksize($vmid, $conf, $vm_volids);
>> +	my $changes = update_disk_config($vmid, $conf, $vm_volids);
>>   
>>   	PVE::QemuConfig->write_config($vmid, $conf) if $changes && !$dryrun;
>>       };
>> -- 
>> 2.20.1
>>
>>
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel at pve.proxmox.com
>> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>
>>
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>