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

[Thomas Lamprecht]

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.

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

> +	    $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?

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