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

[Fabian Grünbichler]

On December 9, 2019 10:51 am, Stefan Reiter wrote:
> 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.

but those shenanigans are new anyway - previously it only printed

"update disk 'scsi0' information."

so, we could either
A keep the output simple (no sizes)
B include unformatted sizes in output
C include formatted sizes in output

B & C still don't need to use a log_fn as argument ;) for C, you could 
either return the formatted sizes, or do the formatting at the one new 
call site and keep the existing users (which are just 'qm rescan', and 
'qmrestore') like they are.

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

better than the current version, but less flexible/future-proof than 
returning the sizes ;)