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