[pve-devel] [PATCH v4 1/3] migrate: collect migration tunnel child process

[Dietmar Maurer]

I can see the reason to use waitpid instead of check_process_running(),
but why do you change the rest of the code?

Can we have minimal patches, where each patch states the reason for the change
in the commit log?


> On June 2, 2016 at 2:44 PM Thomas Lamprecht <t.lamprecht at proxmox.com> wrote:
> 
> 
> As we open2 it we also need to collect it to avoid zombies
> 
> Wait for 15 seconds if the tunnel is still running then send a
> SIGTERM, after another 15 seconds a SIGKILL takes care if it totally
> hung up.
> 
> As a this stage the tunnel is not used anymore it can safely be
> killed, but we wait a little as a gracefull exit is always nicer.
> 
> Signed-off-by: Thomas Lamprecht <t.lamprecht at proxmox.com>
> ---
> 
> changes since v3:
> * just signal the tunnel once with sigterm and eventually sigkill
> * wait 15 seconds for sending a sigterm and another 15 for sending the sigkil
> 
> 
> 
>  PVE/QemuMigrate.pm | 36 ++++++++++++++++++------------------
>  1 file changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
> index a25efff..8afe099 100644
> --- a/PVE/QemuMigrate.pm
> +++ b/PVE/QemuMigrate.pm
> @@ -5,6 +5,7 @@ use warnings;
>  use PVE::AbstractMigrate;
>  use IO::File;
>  use IPC::Open2;
> +use POSIX qw( WNOHANG );
>  use PVE::INotify;
>  use PVE::Tools;
>  use PVE::Cluster;
> @@ -42,7 +43,10 @@ sub fork_command_pipe {
>  }
>  
>  sub finish_command_pipe {
> -    my ($self, $cmdpipe, $timeout) = @_;
> +    my ($self, $cmdpipe) = @_;
> +
> +    my $cpid = $cmdpipe->{pid};
> +    return undef if !$cpid;
>  
>      my $writer = $cmdpipe->{writer};
>      my $reader = $cmdpipe->{reader};
> @@ -50,27 +54,23 @@ sub finish_command_pipe {
>      $writer->close();
>      $reader->close();
>  
> -    my $cpid = $cmdpipe->{pid};
> +    # collect child process
> +    for (my $i = 1; $i <= 30; $i++) {
> +	my $waitpid = waitpid($cpid, WNOHANG);
> +	last if (defined($waitpid) && ($waitpid == $cpid));
>  
> -    if ($timeout) {
> -	for (my $i = 0; $i < $timeout; $i++) {
> -	    return if !PVE::ProcFSTools::check_process_running($cpid);
> -	    sleep(1);
> +	if ($i == 15) {
> +	    $self->log('info', "ssh tunnel still running - terminating now with
> SIGTERM");
> +	    kill(15, $cpid);
> +	} elsif ($i == 29) { # kill for sure it and do an other round to collect it
> +	    $self->log('info', "ssh tunnel still running - terminating now with
> SIGKILL");
> +	    kill(9, $cpid);
>  	}
> -    }
> -
> -    $self->log('info', "ssh tunnel still running - terminating now with
> SIGTERM\n");
> -    kill(15, $cpid);
>  
> -    # wait again
> -    for (my $i = 0; $i < 10; $i++) {
> -	return if !PVE::ProcFSTools::check_process_running($cpid);
> -	sleep(1);
> +	sleep (1);
>      }
>  
> -    $self->log('info', "ssh tunnel still running - terminating now with
> SIGKILL\n");
> -    kill 9, $cpid;
> -    sleep 1;
> +    delete $cmdpipe->{cpid};
>  }
>  
>  sub fork_tunnel {
> @@ -114,7 +114,7 @@ sub finish_tunnel {
>      };
>      my $err = $@;
>  
> -    $self->finish_command_pipe($tunnel, 30);
> +    $self->finish_command_pipe($tunnel);
>  
>      die $err if $err;
>  }
> -- 
> 2.1.4
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> http://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>