Thank you. You have identified a bug. Attached are two patches -
can you give them a spin please?
Ways forward:
a) Keep heuristic
Attached are two patches, the first is the actual fix. It will only
consider a session interactive if salloc has been invoked from a
login shell. But it is not a real fix, since there are users who
operate from a non-login shell (they could use salloc other-shell
from the login shell, though).
The second patch fixes the condition "setsid salloc [args...]".
b) Use explicit interactive mode (instead of heuristic)
This solution is used in bash: use -i to indicate interactive mode
(alternatively, use -c to indicate scripted mode). However, shells
use `-i' as default, `-c' apparently is not likely to find plaisir.
c) Use redirected stdin to indicate non-interactive mode
Requires to update some scripts (as does (b)).
d) Revert everything
Rely on other means of terminating hung salloc sessions (kill(all),
scancel, exit terminal window).
Danny, Moe are you ok with (d) -- we would use a private fork until we have
solved our use case where job control improved (but not completely solved)
the situation.
On Thu, 17 Feb 2011 18:58:56 -0700 [email protected] wrote:
> There seems to be another anomaly with the changes to salloc in SLURM
> version 2.2.0 and 2.2.1. One of our testers submitted a bug where a
> script with multiple salloc commands will only execute the first one, the
> second one gets the "salloc: error: Waiting for program to be placed in
> the foreground" message and is left in the background, even though the
> salloc command lines do not contain an "&".
salloc: fix bug in the definition of 'is_interactive'
This restricts the heuristic for considering a salloc session 'interactive'.
That mode will only be enabled if, in addition to the previous conditions,
salloc has been invoked from a login shell.
This allows scripted execution but will not work in cases where people
run interactively from a non-login shell.
---
src/salloc/salloc.c | 42 ++++++++++++++++++++++--------------------
1 file changed, 22 insertions(+), 20 deletions(-)
--- a/src/salloc/salloc.c
+++ b/src/salloc/salloc.c
@@ -206,37 +206,39 @@ int main(int argc, char *argv[])
_set_rlimits(env);
}
- if ((!opt.no_shell) && isatty(STDIN_FILENO)) {
- bool sent_msg = false;
+ /*
+ * Job control for interactive salloc sessions.
+ *
+ * This uses the following heuristic:
+ * a) salloc is not run in allocation-only (--no-shell) mode,
+ * b) stdin is from a terminal (tcgetattr does not return ENOTTY),
+ * c) salloc has been invoked from a login shell (not a nested one),
+ * d) salloc has been configured at compile-time to support background
+ * execution and is not currently in the background process group.
+ */
+ if ((!opt.no_shell) &&
+ (tcgetattr(STDIN_FILENO, &saved_tty_attributes) != -1) &&
+ (getppid() == getsid(0))) {
- /*
- * Job control: interactive sub-processes run in the foreground
- * process group of the controlling terminal. In order to grant
- * this (tcsetpgrp), salloc needs to be in the foreground first.
- */
pid = getpgrp();
-#ifdef SALLOC_RUN_BACKGROUND
if (tcgetpgrp(STDIN_FILENO) == pid)
is_interactive = true;
-#else
+#if !SALLOC_RUN_BACKGROUND
while (tcgetpgrp(STDIN_FILENO) != pid) {
- if (!sent_msg) {
+ if (!is_interactive) {
error("Waiting for program to be placed in "
"the foreground");
- sent_msg = true;
+ is_interactive = true;
}
killpg(pid, SIGTTIN);
}
- is_interactive = true;
#endif
- }
- /*
- * Save tty attributes and reset at exit, in case a child
- * process died before properly resetting terminal.
- */
- if (is_interactive) {
- tcgetattr (STDIN_FILENO, &saved_tty_attributes);
- atexit (_reset_input_mode);
+ /*
+ * Reset saved tty attributes at exit, in case a child
+ * process died before properly resetting terminal.
+ */
+ if (is_interactive)
+ atexit (_reset_input_mode);
}
/*
salloc: avoid running in new session without --no-shell
When running salloc in a new session, such as e.g.
> setsid salloc [args ...]
there is no controlling terminal, hence this patch terminates the program
unless --no-shell has been chosen.
---
src/salloc/salloc.c | 3 +++
1 file changed, 3 insertions(+)
--- a/src/salloc/salloc.c
+++ b/src/salloc/salloc.c
@@ -239,6 +239,9 @@ int main(int argc, char *argv[])
*/
if (is_interactive)
atexit (_reset_input_mode);
+ } else if ((!opt.no_shell) && (getpid() == getsid(0))) {
+ error("can not run salloc in a new session without --no-shell");
+ exit(error_exit);
}
/*
