[pve-devel] [PATCH v2 qemu-server 2/3] Avoid collisions of unused disks when doing online migration with --targetstorage

[Fabian Grünbichler]

On November 25, 2019 12:40 pm, Fabian Ebner wrote:
> On 11/21/19 4:46 PM, Fabian Grünbichler wrote:
>> On November 4, 2019 11:23 am, Fabian Ebner wrote:
>>> On 10/31/19 10:19 AM, Thomas Lamprecht wrote:
>>>> On 10/30/19 10:54 AM, Fabian Ebner wrote:
>>>>> Doing an online migration with --targetstorage and two unused disks with the
>>>>> same name on different storages failed, because they would collide on the
>>>>> target storage. This patch makes sure that we don't use the same name twice.
>>>>>
>>>>> Signed-off-by: Fabian Ebner <f.ebner at proxmox.com>
>>>>> ---
>>>>>    PVE/QemuMigrate.pm | 8 ++++++--
>>>>>    1 file changed, 6 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
>>>>> index 045f3b0..94c5764 100644
>>>>> --- a/PVE/QemuMigrate.pm
>>>>> +++ b/PVE/QemuMigrate.pm
>>>>> @@ -10,6 +10,7 @@ use PVE::INotify;
>>>>>    use PVE::Tools;
>>>>>    use PVE::Cluster;
>>>>>    use PVE::Storage;
>>>>> +use PVE::Storage::Plugin;
>>>>>    use PVE::QemuServer;
>>>>>    use Time::HiRes qw( usleep );
>>>>>    use PVE::RPCEnvironment;
>>>>> @@ -466,9 +467,12 @@ sub sync_disks {
>>>>>    		next if $rep_volumes->{$volid};
>>>>>    		push @{$self->{volumes}}, $volid;
>>>>>    
>>>>> +		my $targetvolname = undef;
>>>>>    		if (defined($override_targetsid) && $self->{running} && $ref eq 'storage') {
>>>>> -		    my (undef, $targetvolname) = PVE::Storage::parse_volume_id($volid);
>>>>> +		    my $scfg = PVE::Storage::storage_config($self->{storecfg}, $targetsid);
>>>>> +		    $targetvolname = PVE::Storage::Plugin::get_next_vm_diskname($self->{online_unused_volumes}, $targetsid, $vmid, undef, $scfg, 0);
>>>>>    		    push @{$self->{online_unused_volumes}}, "${targetsid}:${targetvolname}";
>>>>> +		    $self->log('info', "$volid will become ${targetsid}:${targetvolname} on the target node");
>>>>
>>>> but isn't above racy? I only can assume that the new volid is
>>>> OK once I alloced the image on the target storage, or?
>>>> Or is it OK as we're protected by the migrate lock anyway?
>>>>
>>>
>>> Yes, we are protected by the migrate lock.
>>> I assume that the target storage does not currently contain any disks
>>> with our VM ID. Using get_next_vm_diskname doesn't look at which disks
>>> are present on the storage, but uses the list we pass along to determine
>>> the next free disk name. If there is a problem with storage_migrate we
>>> die anyways, so we can be sure that our online_unused_volumes list is
>>> consistent with the disks that land on the storage.
>> 
>> that's a pretty big assumption though? e.g., there might be orphaned
>> disks, or replication going on on that target storage?
>> 
> 
> The code as it is now makes that assumption as well. If a disk with the 
> same name is already present on the target storage, then storage_migrate 
> dies.

yes, partially. renaming makes the potential for collisions higher. I 
think we all agree that the status quo is far from ideal, and if we can 
fix that in one go even better :)

>> IMHO, the right approach would be to teach 'storage_migrate' to allocate
>> the disk (on storage X, with size Y, with prefered format Z), and return the
>> allocated volid so that we can put it into the (new) config as unused
>> volume. maybe we need a new helper/wrapper around storage_migrate for
>> that?
>> 
>> Aaron (CC-ed) is trying to implement similar functionality for
>> pve-container atm, but there we have the additional complication that we
>> also have to differentiate between block/image based volumes, and
>> subvols.. it would probably be a good idea to discuss the needed
>> interfaces/changes ;)
>> 
> 
> Sure, sounds good to me.
> 
>>>>>    		}
>>>>>    
>>>>>    		my $opts = $self->{opts};
>>>>> @@ -480,7 +484,7 @@ sub sync_disks {
>>>>>    		$bwlimit = $bwlimit * 1024 if defined($bwlimit);
>>>>>    
>>>>>    		PVE::Storage::storage_migrate($self->{storecfg}, $volid, $self->{ssh_info}, $targetsid,
>>>>> -					      undef, undef, undef, $bwlimit, $insecure, $with_snapshots);
>>>>> +					      $targetvolname, undef, undef, $bwlimit, $insecure, $with_snapshots);
>>>>>    	    }
>>>>>    	}
>>>>>        };
>>>>>
>>>
>>> _______________________________________________
>>> 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
>> 
>