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

[Fabian Ebner]

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.

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