[pve-devel] [PATCH 07/19] phase2_cleanup : destroy remote external vm on error

[Thomas Lamprecht]

On 02/22/2017 02:33 PM, Alexandre Derumier wrote:
> Signed-off-by: Alexandre Derumier <aderumier at odiso.com>
> ---
>   PVE/QemuMigrate.pm | 28 ++++++++++++++++++++++++++--
>   1 file changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
> index 616632c..b711564 100644
> --- a/PVE/QemuMigrate.pm
> +++ b/PVE/QemuMigrate.pm
> @@ -483,6 +483,7 @@ sub phase2 {
>   
>               if ($line =~ m/^vm (\d+) created$/) {
>                   $vmid = $1;
> +		$self->{opts}->{targetvmid} = $1;
>               }
>   	});
>   
> @@ -820,6 +821,7 @@ sub phase2 {
>   sub phase2_cleanup {
>       my ($self, $vmid, $err) = @_;
>   
> +
>       return if !$self->{errors};
>       $self->{phase2errors} = 1;
>   
> @@ -852,15 +854,37 @@ sub phase2_cleanup {
>   	}
>       }
>   
> -    my $nodename = PVE::INotify::nodename();
> +    $vmid = $self->{opts}->{targetvmid} if $self->{opts}->{targetvmid};

You introduce the targetvmid parameter only later on, so this patch 
order is also wrong?

It's helps others really to get the order of the patches right, 
especially for review and testing.
Or in case something needs to be bisected by git,
here we would land in an not working state, no patch should break 
anything temporarily.

As this is RFC is totally OK, I'm a bit nit picking here, but for a next 
version it would be
really great to re order the series so that stuff gets only used if its 
already introduced.


>    
> -    my $cmd = [@{$self->{rem_ssh}}, 'qm', 'stop', $vmid, '--skiplock', '--migratedfrom', $nodename];
> +    my $cmd = [@{$self->{rem_ssh}}, 'qm', 'stop', $vmid, '--skiplock'];
> +    if (!$self->{opts}->{externalcluster}) {
> +	my $nodename = PVE::INotify::nodename();
> +        push @$cmd , '--migratedfrom', $nodename;
> +    }
> +
>       eval{ PVE::Tools::run_command($cmd, outfunc => sub {}, errfunc => sub {}) };
>       if (my $err = $@) {
>           $self->log('err', $err);
>           $self->{errors} = 1;
>       }
>   
> +    if ($self->{opts}->{externalcluster} && $self->{opts}->{targetvmid}) {
> +
> +        my $cmdunlock = [ @{$self->{rem_ssh}}, 'qm', 'unlock', $vmid ];
> +        eval { PVE::Tools::run_command($cmdunlock, outfunc => sub {}, errfunc => sub {}) };
> +        if (my $err = $@) {
> +            $self->log('err', $err);
> +            $self->{errors} = 1;
> +        }
> +
> +        my $cmddestroy = [@{$self->{rem_ssh}}, 'rm', "/etc/pve/qemu-server/$vmid.conf" ];

qm destroy (or it's API counter part) would be nicer here, disk are 
already synced over at this point, or not?

> +        eval{ PVE::Tools::run_command($cmddestroy, outfunc => sub {}, errfunc => sub {}) };
> +        if (my $err = $@) {
> +            $self->log('err', $err);
> +            $self->{errors} = 1;
> +        }
> +    }
> +
>       if ($self->{tunnel}) {
>   	eval { finish_tunnel($self, $self->{tunnel});  };
>   	if (my $err = $@) {