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

[Fabian Ebner]

On 14.04.20 12:41, Fabian Ebner wrote:
> 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).
> 

Just tested around a bit. For online migration this does happen, because 
the format can change when e.g. going from a directory storage with a 
qcow2 image to an LVM storage.

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

Now I think it might be enough to extend rescan to also check+fix the 
format (the needed information is already present) and call it after the 
migration.
Another way would be to just delete eventual 'format' entries from the 
volume hashes in update_volids or is there any case where the format 
isn't encoded in the volid (of course taking into account the storage 
type) and the 'format=...' part of the property string is actually needed?

>> 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
>>
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel