Skip to content

plm/pals: failed_launch is never armed, so a failed launch is never reported (needs validation on PALS hardware) #2547

Description

@rhc54

The defect

In plm_pals_module.c, failed_launch is a file-scope static:

static prte_proc_t *palsrun = NULL;
static bool failed_launch;

Every other plm component declares it as a local initialized to true at the
top of launch_daemons(), so that the cleanup: label at the bottom can tell
a failure from a success:

cleanup:
    ...
    /* check for failed launch - if so, force terminate */
    if (failed_launch) {
        PRTE_ACTIVATE_JOB_STATE(state->jdata, PRTE_JOB_STATE_FAILED_TO_START);
    }

In pals it is at file scope because pals_wait_cb() also consults it, to tell
"aprun never started the daemons" (FAILED_TO_START) from "a daemon died after
launch" (ABORTED). But nothing ever set it back to true: it is
zero-initialized and only ever assigned false, on the success path.

Consequences:

  1. launch_daemons() can never report a failure. Every error path jumps to
    cleanup: with failed_launch == false, so FAILED_TO_START is never
    activated and a launch that failed - aprun not found, the vpid string
    failing to build, plm_pals_start_proc failing to fork - simply hangs
    instead of aborting the job.
  2. pals_wait_cb() misclassifies. A non-zero aprun exit before any daemon
    came up is reported as ABORTED rather than FAILED_TO_START.

A second, smaller item in the same file: plm_pals_start_proc() creates
palsrun and registers its prte_wait_cb before the parent/child branch, so
the forked child also registers a waitpid callback on a proc whose pid is 0,
moments before execve replaces it. Slurm does this correctly (parent only).

Fix

Arming the flag on entry to launch_daemons(), and moving the palsrun
bookkeeping into the parent branch. I have both changes ready and will put them
up as part of a src/mca/plm review PR.

Why this issue exists

I cannot test any of it. PALS is built only where Cray PALS is detected
(PRTE_CHECK_PALS), so all I have is a --enable-testbuild-launchers compile.
The first change alters behavior on a path that only runs on Cray hardware: it
makes launch_daemons() start reporting failures it previously swallowed, so
any pre-existing condition on those systems that used to fail silently will
now abort the job with FAILED_TO_START. That seems clearly correct, but it
wants confirmation from someone with a PALS machine.

Asking for: a review from a Cray/HPE PALS user, and ideally a smoke test of
both a good launch and a deliberately broken one (e.g. --prtemca plm_pals_aprun /nonexistent) to confirm the failure is now reported rather than hung.

Found during a review of src/mca/plm.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions