[pve-devel] [PATCH v2 qemu-server] fix #1013 : migrate : sync_disk : --targetstorage with offline disk

[Thomas Lamprecht]

On 12/17/18 11:56 AM, Thomas Lamprecht wrote:
> On 12/10/18 5:28 PM, Alexandre Derumier wrote:
>> targetsid was not used, for disk unused (offline copy)
>> ---
>>  PVE/QemuMigrate.pm | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
>> index e9e9075..0bae076 100644
>> --- a/PVE/QemuMigrate.pm
>> +++ b/PVE/QemuMigrate.pm
>> @@ -446,6 +446,7 @@ sub sync_disks {
>>  
>>  	foreach my $volid (keys %$local_volumes) {
>>  	    my ($sid, $volname) = PVE::Storage::parse_volume_id($volid);
>> +	    my $targetsid = $self->{opts}->{targetstorage} ? $self->{opts}->{targetstorage} : $sid;
> 
> Hmm, I took a closer look at this and found out that it now breaks the case where
> no targetstorage is passed, previously we falled back to the same storage as the
> volume had on the source node, to mark this we do (a few lines above your change):
> 
> if ($self->{running} && !$sharedvm && !$self->{opts}->{targetstorage}) {
>     $self->{opts}->{targetstorage} = 1; #use same sid for remote local
> }
> 
> with your changes the '1' is used a literal target storage, which then naturally
> fails... E.e., this fails> 
> # qm migrate 101 pve2 --online --with-local-disks
> 
> but this works:
> # qm migrate 101 pve2 --online --with-local-disks --targetstorage local-lvm
> 

but IMO, your change is correct and the fix for above would be (on top of your patch):

----8<----
diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
index 0bae076..947adc7 100644
--- a/PVE/QemuMigrate.pm
+++ b/PVE/QemuMigrate.pm
@@ -404,10 +404,6 @@ sub sync_disks {
            $self->log('warn', "$err");
        }

-       if ($self->{running} && !$sharedvm && !$self->{opts}->{targetstorage}) {
-           $self->{opts}->{targetstorage} = 1; #use same sid for remote local
-       }
-
        if ($abort) {
            die "can't migrate VM - check log\n";
        }
@@ -447,7 +443,7 @@ sub sync_disks {
        foreach my $volid (keys %$local_volumes) {
            my ($sid, $volname) = PVE::Storage::parse_volume_id($volid);
            my $targetsid = $self->{opts}->{targetstorage} ? $self->{opts}->{targetstorage} : $sid;
-           if ($self->{running} && $self->{opts}->{targetstorage} && $local_volumes->{$volid}->{ref} eq 'config') {
+           if ($self->{running} && $local_volumes->{$volid}->{ref} eq 'config') {
                push @{$self->{online_local_volumes}}, $volid;
            } else {
                next if $rep_volumes->{$volid};
--

the check for targetstorage in above if is obsolete, AFAICT, as we already check
for running and if it's an local disk, and in such a case we always set target
storage to 1 (truthy) in the "use same sid for remote local" if above.
So removing it should led to the same if hits but fixes your $targetsid check.


> 
>>  	    if ($self->{running} && $self->{opts}->{targetstorage} && $local_volumes->{$volid}->{ref} eq 'config') {
>>  		push @{$self->{online_local_volumes}}, $volid;
>>  	    } else {
>> @@ -453,7 +454,7 @@ sub sync_disks {
>>  		push @{$self->{volumes}}, $volid;
>>  		my $insecure = $self->{opts}->{migration_type} eq 'insecure';
>>  		my $with_snapshots = $local_volumes->{$volid}->{snapshots};
>> -		PVE::Storage::storage_migrate($self->{storecfg}, $volid, $self->{ssh_info}, $sid,
>> +		PVE::Storage::storage_migrate($self->{storecfg}, $volid, $self->{ssh_info}, $targetsid,
>>  					      undef, undef, undef, undef, $insecure, $with_snapshots);
>>  	    }
>>  	}
>>