[pve-devel] applied: [PATCH v2 qemu-server] Improve handling and description of migrate with --online

[Fabian Ebner]

On 9/23/19 2:19 PM, Thomas Lamprecht wrote:
> On 9/23/19 9:56 AM, Fabian Ebner wrote:
>> Thanks to Stefan and Thomas for the suggestions.
>>
>> Changes from v1:
>> * update parameter description
>> * warn instead of die
> This information is better to go a bit below, namely...
>
>> Signed-off-by: Fabian Ebner <f.ebner at proxmox.com>
>> ---
> .. here, after the "---" and before the diff-stat below.
>
> This way it's visible in for all mail/patch readers but not included
> in the commit once applied to git, which is OK as it's "patch-historical
> meta" information.
>

Ok, I'll do that next time.


>>   PVE/API2/Qemu.pm | 11 +++++++----
>>   1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
>> index 8be0b7b..b355051 100644
>> --- a/PVE/API2/Qemu.pm
>> +++ b/PVE/API2/Qemu.pm
>> @@ -3257,7 +3257,7 @@ __PACKAGE__->register_method({
>>               }),
>>   	    online => {
>>   		type => 'boolean',
>> -		description => "Use online/live migration.",
>> +		description => "Use online/live migration if VM is running. Ignored if VM is stopped.",
>>   		optional => 1,
>>   	    },
>>   	    force => {
>> @@ -3318,9 +3318,6 @@ __PACKAGE__->register_method({
>>   
>>   	my $vmid = extract_param($param, 'vmid');
>>   
>> -	raise_param_exc({ targetstorage => "Live storage migration can only be done online." })
>> -	    if !$param->{online} && $param->{targetstorage};
>> -
>>   	raise_param_exc({ force => "Only root may use this option." })
>>   	    if $param->{force} && $authuser ne 'root at pam';
>>   
>> @@ -3341,8 +3338,14 @@ __PACKAGE__->register_method({
>>   	if (PVE::QemuServer::check_running($vmid)) {
>>   	    die "cant migrate running VM without --online\n"
>>   		if !$param->{online};
>> +	} else {
>> +	    warn "VM isn't running. Doing offline migration instead." if $param->{online};
> With perl "warn" and "die" have a bit of an special behavior depending of the
> string ending. If it ends with a new line the warning or "exception" gets
> outputted as is. If it does not end with a \n the perl module and line gets added
> by perl, compare:
>
> # perl -we 'warn "foo\n"'
> # perl -we 'warn "foo"'
>
> Normally, we only omit the newline on purpose on internal "safe-guard" errors,
> those which normally should never get seen by an user. (I fixed this up for you
> in a follow-up patch).

Got it.


>> +	    $param->{online} = 0;
>>   	}
>>   
>> +	raise_param_exc({ targetstorage => "Live storage migration can only be done online." })
>> +	    if !$param->{online} && $param->{targetstorage};
> could it make sense to add a modus (or change this into one) where we start the VM up
> paused and do our normal storage migrations (over QEMU) then? or rather improve storage
> migration code to be able to migrate those storages to other ones?

Yes, I'll try to work on a solution where for disks we migrate with 
storage_migrate() the VM config is updated with the new location and 
where new disk names are used in case of collisions. Currently that only 
happens with disks migrated via QEMU. This should make it possible to 
specify --targetstorage for offline migration as well and is also needed 
for online migration with unused disks.


>> +
>>   	my $storecfg = PVE::Storage::config();
>>   
>>   	if( $param->{targetstorage}) {
>>