[pve-devel] [PATCH v5 qemu-server 14/19] Update volume IDs in one go

[Fabian Ebner]

On 09.04.20 09:53, Fabian Grünbichler wrote:
> small nit inline
> 
> On April 8, 2020 11:25 am, Fabian Ebner wrote:
>> Use 'update_volume_ids' for the live-migrated disks as well.
>>
>> Signed-off-by: Fabian Ebner <f.ebner at proxmox.com>
>> ---
>>   PVE/QemuMigrate.pm | 26 ++++++++++----------------
>>   1 file changed, 10 insertions(+), 16 deletions(-)
>>
>> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
>> index 14f3fcb..eb78537 100644
>> --- a/PVE/QemuMigrate.pm
>> +++ b/PVE/QemuMigrate.pm
>> @@ -838,14 +838,20 @@ sub phase2 {
>>   	    my $source_drive = PVE::QemuServer::parse_drive($drive, $conf->{$drive});
>>   	    my $target_drive = PVE::QemuServer::parse_drive($drive, $target->{drivestr});
>>   
>> -	    my $source_sid = PVE::Storage::Plugin::parse_volume_id($source_drive->{file});
>> -	    my $target_sid = PVE::Storage::Plugin::parse_volume_id($target_drive->{file});
>> +	    my $source_volid = $source_drive->{file};
>> +	    my $target_volid = $target_drive->{file};
>> +
>> +	    my $source_sid = PVE::Storage::Plugin::parse_volume_id($source_volid);
>> +	    my $target_sid = PVE::Storage::Plugin::parse_volume_id($target_volid);
>>   
>>   	    my $bwlimit = PVE::Storage::get_bandwidth_limit('migration', [$source_sid, $target_sid], $opt_bwlimit);
>>   	    my $bitmap = $target->{bitmap};
>>   
>>   	    $self->log('info', "$drive: start migration to $nbd_uri");
>>   	    PVE::QemuServer::qemu_drive_mirror($vmid, $drive, $nbd_uri, $vmid, undef, $self->{storage_migration_jobs}, 'skip', undef, $bwlimit, $bitmap);
>> +
>> +	    $self->{volume_map}->{$source_volid} = $target_volid;
>> +	    $self->log('info', "volume '$source_volid' is '$target_volid' on the target\n");
> 
> potential followup: if we do that before attempting to start the mirror which might
> die, it would make the cleanup handling easier.

You're right. The reason I decided to do it here, was to avoid code 
duplication, because of the distinction between of unix socket/no unix 
socket above. And the drivestrings need to be parsed just for this 
otherwise.

I guess instead of saving the drivestring in the target_drive hash, we 
only need to save the volume ID. The other information that is in 
drivestring is not used anymore, because the old settings from the 
config are copied over, just the volume ID is updated.

This makes me realize a potential bug with only updating the volume_ids. 
If the newly allocated volume would have a different format than the 
original, but 'format' was explicitly set for the volume in the old 
config, the config will be wrong after migration. AFAICT this currently 
does not happen, but might change in the future. Also the size might be 
slightly off (e.g. image was a raw file, but newly allocated one is 
bigger because it's an LVM and uses extents).

For offline migration we (currently) don't have a way to send back the 
drivestring and in general it's desireable to preserve options for a 
drive. Does it make sense to add a method to be executed after 
migration, which would check/fix such errors, or is doing a rescan enough?

> even better if we would
> allocate each disk individually before calling start - then we'd also
> avoid the problem of 'third of five disks failed to allocate, but we
> haven't told you about the first two yet' ;) but for now, just moving
> these two lines two lines up would simplify the last patch of this series.
> 
>>   	}
>>       }
>>   
>> @@ -1114,13 +1120,6 @@ sub phase3_cleanup {
>>   
>>       my $tunnel = $self->{tunnel};
>>   
>> -    # Needs to happen before printing the drives that where migrated via qemu,
>> -    # since a new volume on the target might have the same ID as an old volume
>> -    # on the source and update_volume_ids relies on matching IDs in the config.
>> -    if ($self->{volume_map}) {
>> -	PVE::QemuConfig->update_volume_ids($conf, $self->{volume_map});
>> -    }
>> -
>>       if ($self->{storage_migration}) {
>>   	# finish block-job with block-job-cancel, to disconnect source VM from NBD
>>   	# to avoid it trying to re-establish it. We are in blockjob ready state,
>> @@ -1131,16 +1130,11 @@ sub phase3_cleanup {
>>   	    eval { PVE::QemuServer::qemu_blockjobs_cancel($vmid, $self->{storage_migration_jobs}) };
>>   	    eval { PVE::QemuMigrate::cleanup_remotedisks($self) };
>>   	    die "Failed to complete storage migration: $err\n";
>> -	} else {
>> -	    foreach my $target_drive (keys %{$self->{target_drive}}) {
>> -		my $drive = PVE::QemuServer::parse_drive($target_drive, $self->{target_drive}->{$target_drive}->{drivestr});
>> -		$conf->{$target_drive} = PVE::QemuServer::print_drive($drive);
>> -	    }
>>   	}
>>       }
>>   
>> -    # only write after successfully completing the migration
>> -    if ($self->{volume_map} || $self->{storage_migration}) {
>> +    if ($self->{volume_map}) {
>> +	PVE::QemuConfig->update_volume_ids($conf, $self->{volume_map});
>>   	PVE::QemuConfig->write_config($vmid, $conf);
>>       }
>>   
>> -- 
>> 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
>