this commit does more harm then good.
we experience following:

- some child processes still running after timeout and mtt killed the job.

before this commit - it worked fine.
please revert and test more.



On Sat, Jun 21, 2014 at 3:30 PM, MPI Team <mpit...@crest.iu.edu> wrote:

> The branch, master has been updated
>        via  016088f2a0831b32ab5fd6f60f4cabe67e92e594 (commit)
>        via  7fb4c6a4c9d71be127ea53bd463178510577f71f (commit)
>        via  381ba177d835a54c3197d846f5a4edfc314efe27 (commit)
>        via  cfdd29de2012eeb7592706f00dd07a52dd48cf6b (commit)
>        via  940030ca20eb1eaf256e898b83866c1cb83aca5c (commit)
>       from  c99ed7c7b159a2cab58a251bd7c0dad8972ff901 (commit)
>
> Those revisions listed above that are new to this repository have
> not appeared on any other notification email; so we list those
> revisions in full, below.
>
> - Log -----------------------------------------------------------------
>
> https://github.com/open-mpi/mtt/commit/016088f2a0831b32ab5fd6f60f4cabe67e92e594
>
> commit 016088f2a0831b32ab5fd6f60f4cabe67e92e594
> Author: Jeff Squyres <jsquy...@cisco.com>
> Date:   Sat Jun 21 04:58:45 2014 -0700
>
>     DoCommand: several fixes to kill_proc logic
>
>     1. Fix the kill(0, $pid) test to see if the process was still alive.
>
>     2. Rename _kill_proc() to _kill_proc_tree() to indicate that it's
>     really killing not only the PID in question, but also all of its
>     descendants.
>
>     3. In _kill_proc_tree(), change the order to kill the main PID first,
>     and ''then'' kill all the descendants.
>
>     The main use case is when killing mpirun: if we kill mpirun's
>     descendants ''first'', mpirun will detect its childrens' deaths and
>     then cleanup and exit.  Later, when MTT finally gets around to killing
>     mpirun, MTT will detect that mpirun is already dead and therefore emit
>     a confusing "mpirun died right at end of timeout" message.  This is
>     misleading at best; it doesn't indicate what actually happened.
>
>     However, if we kill mpirun first, it will take care of killing all of
>     its descendants.  MTT will therefore emit the right messages about
>     killing mpirun.  MTT will then redundantly try to kill a bunch of
>     now-nonexistent descendant processes of mpirun, but that's ok/safe.
>     We actually ''want'' this try-to-kill-mpirun's-descendants behavior to
>     handle the case when mpirun is misbehaving / not cleaning up its
>     descendants.
>
>     4. DoCommand() is used for more than launching mpirun, so pass down
>     $argv0 so that we can print the actual command name that is being
>     killed in various Verbose/Debug messages, not the hard-coded "mpirun"
>     string (which, in practice, was probably almost always correct, but
>     still...).
> ---
>  lib/MTT/DoCommand.pm | 78
> ++++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 55 insertions(+), 23 deletions(-)
>
> diff --git a/lib/MTT/DoCommand.pm b/lib/MTT/DoCommand.pm
> index 02cdb94..646ca31 100644
> --- a/lib/MTT/DoCommand.pm
> +++ b/lib/MTT/DoCommand.pm
> @@ -2,7 +2,7 @@
>  #
>  # Copyright (c) 2005-2006 The Trustees of Indiana University.
>  #                         All rights reserved.
> -# Copyright (c) 2006-2013 Cisco Systems, Inc.  All rights reserved.
> +# Copyright (c) 2006-2014 Cisco Systems, Inc.  All rights reserved.
>  # Copyright (c) 2007-2008 Sun Microsystems, Inc.  All rights reserved.
>  # Copyright (c) 2007-2012 High Performance Computing Center Stuttgart,
>  #                         University of Stuttgart.  All rights reserved.
> @@ -57,23 +57,27 @@ sub DoCommand {
>      ($time_arg, $no_execute) = @_;
>  }
>
> +# This function is called for killing both mpirun and each of its
> +# descendants.  We really only want to see verbose output for when we
> +# kill mpirun itself, so only show output when the caller provides us
> +# with a $argv0 value.
>  sub _kill_proc_one {
> -    my ($pid) = @_;
> +    my ($pid, $argv0) = @_;
>
>      # How long to wait after each kill()
>      my $wait_time = 5;
>
>      # See if the proc is alive first
> -    my $kid;
> -    kill(0, $pid);
> -    $kid = waitpid($pid, WNOHANG);
> -    return "mpirun died right at end of timeout (MTT did not have to kill
> it)"
> -        if (-1 == $kid);
> +    my $num_alive = kill(0, $pid);
> +    return "$argv0 died right at end of timeout (MTT did not have to kill
> it)"
> +        if (0 == $num_alive);
>
>      # Try an easy kill
>      kill("TERM", $pid);
> -    Verbose("*** Killing mpirun with SIGTERM\n");
> +    Verbose("*** Killing $argv0 with SIGTERM\n")
> +        if (defined($argv0));
>      # Give mpirun some time to cleanup before we try to reap it.
> +    my $kid;
>      my $i = $wait_time;
>      while ($i > 0) {
>          sleep(1);
> @@ -85,45 +89,74 @@ sub _kill_proc_one {
>          # process no longer exists (i.e., we get -1 back from
>          # waitpid), or we successfully killed it (i.e., we got the PID
>          # back from waitpid).
> -        return "MTT killed mpirun via SIGTERM" if (0 != $kid);
> +        return "MTT killed $argv0 via SIGTERM" if (0 != $kid);
>
>          --$i;
>      }
> -    Verbose("** Kill TERM (after $wait_time seconds) didn't work!\n");
> +    Verbose("** Kill TERM (after $wait_time seconds) didn't work!\n")
> +        if (defined($argv0));
>
>      # That didn't work either.  Try SIGINT;
> -    Verbose("*** Killing mpirun with SIGINT\n");
> +    Verbose("*** Killing $argv0 with SIGINT\n")
> +        if (defined($argv0));
>      kill("INT", $pid);
>      my $i = $wait_time;
>      while ($i > 0) {
>          sleep(1);
>          $kid = waitpid($pid, WNOHANG);
> -        return "MTT killed mpirun via SIGINT" if (0 != $kid);
> +        return "MTT killed $argv0 via SIGINT" if (0 != $kid);
>          --$i;
>      }
> -    Verbose("** Kill INT (after $wait_time seconds) didn't work!\n");
> +    Verbose("** Kill INT (after $wait_time seconds) didn't work!\n")
> +        if (defined($argv0));
>
>      # Ok, now we're mad.  Be violent.
> -    Verbose("*** Now I'm mad.  Killing mpirun with SIGKILL\n");
> +    Verbose("*** Now I'm mad.  Killing $argv0 with SIGKILL\n")
> +        if (defined($argv0));
>      my $count = 0;
>      while (1) {
>          kill("KILL", $pid);
>          ++$count;
>          $kid = waitpid($pid, WNOHANG);
> -        return "MTT killed mpirun via $count SIGKILLs" if (0 != $kid);
> -        Verbose("** Kill KILL didn't work!  Sleeping and trying
> again...\n");
> +        return "MTT killed $argv0 via $count SIGKILLs" if (0 != $kid);
> +        Verbose("** Kill KILL didn't work!  Sleeping and trying
> again...\n")
> +            if (defined($argv0));
>          sleep(1);
>      }
>  }
>
>
> -sub _kill_proc {
> -    my ($pid) = @_;
> -    # kill the group, take the names later
> -    foreach my $process (descendant_processes($pid)) {
> +sub _kill_proc_tree {
> +    my ($pid, $argv0) = @_;
> +
> +    # Find all descendent processes of the main PID
> +    my @children = descendant_processes($pid);
> +
> +    # Kill the main PID.  Note that _kill_proc_one() will give the
> +    # process time to react/cleanup, so there's no need for an
> +    # additional delay after it returns.
> +    my $ret = _kill_proc_one($pid, $argv0);
> +
> +    # Now kill all the processes that descended from the base PID.
> +    #
> +    # Note: when mpirun is working properly (which is one of the
> +    # biggest use cases for DoCommand), this is redundant -- all the
> +    # children will already be dead (because killing mpirun will
> +    # ensure that all descendant processes are also killed).
> +    #
> +    # That being said, a) DoCommand() is used to launch more than just
> +    # mpirun, b) mpirun breaks sometimes and doesn't clean up its
> +    # descendants, and c) it's safe to call _kill_proc_one() on a PID
> +    # that is already dead.
> +    foreach my $process (@children) {
> +        Debug("=== killing child proc: $process->{pid},
> $process->{argv0}\n");
> +
> +        # Ignore the return.  For example, we don't care if the child
> +        # is already dead
>          _kill_proc_one($process->{pid});
>      }
> -    return _kill_proc_one($pid);
> +
> +    return $ret;
>  }
>
>
>  #--------------------------------------------------------------------------
> @@ -338,7 +371,6 @@ sub Cmd {
>
>      my $tokens = _quote_escape($cmd);
>
> -
>      my $pid;
>      if (! $no_execute) {
>
> @@ -568,7 +600,7 @@ sub Cmd {
>
>                      $done = 0;
>                  }
> -                $timeout_message = _kill_proc($pid);
> +                $timeout_message = _kill_proc_tree($pid, ${$tokens}[0]);
>
>                  # We don't care about the exit status if we timed out
>                  # -- fill it with a bogus value.
>
>
> https://github.com/open-mpi/mtt/commit/7fb4c6a4c9d71be127ea53bd463178510577f71f
>
> commit 7fb4c6a4c9d71be127ea53bd463178510577f71f
> Author: Jeff Squyres <jsquy...@cisco.com>
> Date:   Sat Jun 21 04:42:19 2014 -0700
>
>     DoCommand: replace obscure perl code with easier-to-grok version
>
>     The descendant_processes() and flatten() codes were written in a
>     nicely efficient yet very difficult to understand manner.  This commit
>     replaces these two subs with admittedly less efficient but much easier
>     to understand code.
>
>     The new code also saves the first token of the command for each PID so
>     that it can be used in Debug/Verbose statements (e.g., identify the
>     command that is being killed, etc.).
> ---
>  lib/MTT/DoCommand.pm | 63
> ++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 49 insertions(+), 14 deletions(-)
>
> diff --git a/lib/MTT/DoCommand.pm b/lib/MTT/DoCommand.pm
> index c3be8a8..02cdb94 100644
> --- a/lib/MTT/DoCommand.pm
> +++ b/lib/MTT/DoCommand.pm
> @@ -120,8 +120,8 @@ sub _kill_proc_one {
>  sub _kill_proc {
>      my ($pid) = @_;
>      # kill the group, take the names later
> -    foreach my $p (descendant_processes($pid)) {
> -        _kill_proc_one($p);
> +    foreach my $process (descendant_processes($pid)) {
> +        _kill_proc_one($process->{pid});
>      }
>      return _kill_proc_one($pid);
>  }
> @@ -527,7 +527,8 @@ sub Cmd {
>                  if (defined($timeout_backtrace_program) and
> !$got_backtrace) {
>                      $backtrace = "";
>                      if ( $timeout_before_backtrace_program ) {
> -                        foreach my $p (descendant_processes($pid)) {
> +                        foreach my $process (descendant_processes($pid)) {
> +                            my $p = $process->{pid};
>                              my $c = $timeout_before_backtrace_program;
>                              $c =~ s/%PID%/$p/g;
>                              Debug("_pre_backtrace cmd: $c\n");
> @@ -538,7 +539,8 @@ sub Cmd {
>                      $backtrace .=
> _get_backtrace($timeout_backtrace_program, $pid, $pre_pernode);
>
>                      if ( $timeout_after_backtrace_program ) {
> -                        foreach my $p (descendant_processes($pid)) {
> +                        foreach my $process (descendant_processes($pid)) {
> +                            my $p = $process->{pid};
>                              my $c = $timeout_after_backtrace_program;
>                              $c =~ s/%PID%/$p/g;
>                              Debug("_post_backtrace cmd: $c\n");
> @@ -738,19 +740,50 @@ sub _do_email_timeout_notification {
>      close(TIMEOUT_SENTINEL_FILE);
>  }
>
> -sub flatten {
> -    map{ (ref($_) eq "ARRAY") ? map{flatten($_)}@$_ : $_ } @_;
> +sub find_children {
> +    my ($pids, $start) = @_;
> +
> +    my @ret;
> +    # Scan the PID list and see if any other PIDs list $start as their
> +    # PPID.  If so, save that PID and then recursively look for *that*
> +    # PID's children.
> +    foreach my $pid (sort(keys(%{$pids}))) {
> +        if ($pids->{$pid}->{ppid} == $start) {
> +            push(@ret, {
> +                pid => $pid,
> +                argv0 => $pids->{$pid}->{argv0},
> +                 });
> +
> +            # Add this PID's descendants to the return array
> +            push(@ret, find_children($pids, $pid));
> +        }
> +    }
> +
> +    return @ret;
>  }
>
>  sub descendant_processes {
>      my ($base) = (@_, $$);
> -    my %parentage = (`ps -eo pid,ppid` =~ /\d+/g);
> -    my %reverse = map { ($_, [$_]) } %parentage;
> -    while (my ($pid,$ppid) = each %parentage){
> -        push @{$reverse{$ppid}}, $reverse{$pid};
> +    open(PS, "ps -eo pid,ppid,cmd|") || die "Can't open ps";
> +
> +    # Skip the title line
> +    <PS>;
> +
> +    # Read all the data lines
> +    my $pids;
> +    while (<PS>) {
> +        # Grab the PID, PPID, and first token of the command
> +        $_ =~ m/(\d+)\s+(\d+)\s+(\S+)/;
> +        $pids->{$1} = {
> +            pid => $1,
> +            ppid => $2,
> +            argv0 => $3,
> +        };
>      }
> -    shift @{$reverse{$base}};
> -    flatten($reverse{$base});
> +    close(PS);
> +
> +    # Find all the descendants of the $base PID
> +    return find_children($pids, $base);
>  }
>
>  sub _get_backtrace {
> @@ -774,7 +807,8 @@ sub _get_backtrace {
>
>              # Use ps -Af output to fetch the child pids,
>              # and grab a stack trace from each one
> -            foreach my $p (descendant_processes($pid)) {
> +            foreach my $process (descendant_processes($pid)) {
> +                my $p = $process->{pid};
>                  $gdb_cmd = "gdb - $p --command=$gdb_command_filename
> --batch";
>                  $ret .= "\n $gdb_cmd";
>                  $ret .= "\n" . `$gdb_cmd`;
> @@ -814,8 +848,9 @@ sub _get_backtrace {
>                  }
>              }
>              Debug("Stacktrace: base name $return_basename\n");
> -            #foreach my $p (descendant_processes($pid))
> +            #foreach my $process (descendant_processes($pid))
>              #{
> +            #    my $p = $process->{pid};
>              #    my $gstack_cmd = "gstack $p";
>              #    $ret .= "\n $gstack_cmd";
>              #    $ret .= "\n" . `$gstack_cmd`;
>
>
> https://github.com/open-mpi/mtt/commit/381ba177d835a54c3197d846f5a4edfc314efe27
>
> commit 381ba177d835a54c3197d846f5a4edfc314efe27
> Author: Jeff Squyres <jsquy...@cisco.com>
> Date:   Sat Jun 21 03:48:55 2014 -0700
>
>     DoCommand: ensure to only kill the process once
>
>     When the DoCommand timeout fires, MTT will kill the entire process
>     tree of the launched command.  However, the stdout/stderr sockets may
>     still be draining, and therefore the main DoCommand loop may still
>     execute more iterations.  In cases like these, ensure that we don't
>     try to backtrace/kill the process tree again.
>
>     Also add a few more Debug statements for detailed output.
> ---
>  lib/MTT/DoCommand.pm | 23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/lib/MTT/DoCommand.pm b/lib/MTT/DoCommand.pm
> index 0e253f0..c3be8a8 100644
> --- a/lib/MTT/DoCommand.pm
> +++ b/lib/MTT/DoCommand.pm
> @@ -471,6 +471,7 @@ sub Cmd {
>              my $len = sysread(OUTread, $data, 99999);
>              if (0 == $len) {
>                  vec($rin, fileno(OUTread), 1) = 0;
> +                Debug("*** Child process stdout closed\n");
>                  --$done;
>              } else {
>                  _append($data, $print_timestamp ? localtime() : "",
> @@ -485,6 +486,7 @@ sub Cmd {
>              my $len = sysread(ERRread, $data, 99999);
>              if (0 == $len) {
>                  vec($rin, fileno(ERRread), 1) = 0;
> +                Debug("*** Child process stderr closed\n");
>                  --$done;
>              } else {
>                  _append($data, $print_timestamp ? localtime() : "",
> @@ -493,11 +495,19 @@ sub Cmd {
>              }
>          }
>
> -        # If we're running with a timeout, bail if we're past the end
> -        # time
> -        if (defined($end_time) && time() > $end_time) {
> +        # If we're running with a timeout, check to see if a) the
> +        # process is still running (i.e., stdout/stderr is still
> +        # open), and b) we're past the end time.
> +        if ($done > 0 && defined($end_time) && time() > $end_time) {
>              my $over = time() - $end_time;
> -            if ($over > $last_over) {
> +
> +            # Note that we only want to backtrace/kill the process
> +            # *once*.  Consider: it is possible that we kill the
> +            # process tree, but then stdout/stderr are still draining,
> +            # and therefore we loop around here again *even though the
> +            # process tree is already dead*.  So put a little
> +            # do-this-only-once protection in here.
> +            if (!defined($timeout_message) && $over > $last_over) {
>                  Verbose("*** Past timeout of $timeout seconds by $over
> seconds\n");
>
>                  # Handle timeout file
> @@ -555,10 +565,8 @@ sub Cmd {
>                      );
>
>                      $done = 0;
> -                    $timeout_message = _kill_proc($pid);
> -                } else {
> -                    $timeout_message = _kill_proc($pid);
>                  }
> +                $timeout_message = _kill_proc($pid);
>
>                  # We don't care about the exit status if we timed out
>                  # -- fill it with a bogus value.
> @@ -576,6 +584,7 @@ sub Cmd {
>              }
>          }
>      }
> +    Debug("*** Child process now dead\n");
>      close OUTerr;
>      close OUTread
>          if (!$merge_output);
>
>
> https://github.com/open-mpi/mtt/commit/cfdd29de2012eeb7592706f00dd07a52dd48cf6b
>
> commit cfdd29de2012eeb7592706f00dd07a52dd48cf6b
> Author: Jeff Squyres <jsquy...@cisco.com>
> Date:   Sat Jun 21 03:36:08 2014 -0700
>
>     DoCommand: remove unused variables
> ---
>  lib/MTT/DoCommand.pm | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/lib/MTT/DoCommand.pm b/lib/MTT/DoCommand.pm
> index 0740b28..0e253f0 100644
> --- a/lib/MTT/DoCommand.pm
> +++ b/lib/MTT/DoCommand.pm
> @@ -445,7 +445,6 @@ sub Cmd {
>          $end_time = time() + $timeout;
>          Debug("Timeout: $timeout - $end_time (vs. now: " . time() .
> ")\n");
>      }
> -    my $killed_status = undef;
>      my $last_over = 0;
>      my $partial_out;
>      my $partial_err;
> @@ -900,7 +899,6 @@ sub Win_Cmd {
>          $end_time = time() + $timeout;
>          Debug("Timeout: $timeout - $end_time (vs. now: " . time() .
> ")\n");
>      }
> -    my $killed_status = undef;
>      my $last_over = 0;
>
>      while(<OUTread>) {
>
>
> https://github.com/open-mpi/mtt/commit/940030ca20eb1eaf256e898b83866c1cb83aca5c
>
> commit 940030ca20eb1eaf256e898b83866c1cb83aca5c
> Author: Jeff Squyres <jsquy...@cisco.com>
> Date:   Fri Jun 20 17:13:08 2014 -0700
>
>     Diskfree.pm: fix df handle leak
>
>     This left a zombied "df" process in the process table.  The Perl
>     runtime eventually cleaned it up, but it was weird to see a zombied df
>     in ps listings.
>
>     There's no point in reporting this bug upstream; according to the
>     Changes file, v0.06 was published in 1998.  There's been no activity
>     on http://search.cpan.org/dist/Filesys-DiskFree/DiskFree.pm since
>     v0.06.
> ---
>  lib/Filesys/DiskFree.pm | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/lib/Filesys/DiskFree.pm b/lib/Filesys/DiskFree.pm
> index 1fa655d..41a27d7 100644
> --- a/lib/Filesys/DiskFree.pm
> +++ b/lib/Filesys/DiskFree.pm
> @@ -124,8 +124,9 @@ sub df(){
>      $cmd=$self->command() or
>         croak "No df command known for format ".$self->{'FORMAT'};
>      open(HANDLE,"$cmd|") or croak("Cannot fork \"$cmd\": $!");
> -    return $self->load(\*HANDLE);
> +    my $ret = $self->load(\*HANDLE);
>      close(HANDLE) or croak("Cannot df $!");
> +    return $ret;
>  }
>
>  sub load()  {
>
> -----------------------------------------------------------------------
>
> Summary of changes:
>  lib/Filesys/DiskFree.pm |   3 +-
>  lib/MTT/DoCommand.pm    | 162
> +++++++++++++++++++++++++++++++++++-------------
>  2 files changed, 120 insertions(+), 45 deletions(-)
>
>
> hooks/post-receive
> --
> MTT
> _______________________________________________
> mtt-svn mailing list
> mtt-...@open-mpi.org
> http://www.open-mpi.org/mailman/listinfo.cgi/mtt-svn
>

Reply via email to