zhuxiangyi commented on issue #18311: URL: https://github.com/apache/dolphinscheduler/issues/18311#issuecomment-4744297059
@ruanwenjun @SbloodyS @qiuyanjun888 Following up on the "make shell-kill reliable" discussion — I researched how Airflow and Azkaban handle killing a task's process tree, and I'm sharing the findings below to inform the design, plus clarifying the scope of #18312. ## Research: how Airflow and Azkaban handle task process killing I looked into how two other schedulers solve "reliably kill a task's whole process tree", to inform the design discussion here. ### Airflow — process group, built in-process `airflow-core/src/airflow/utils/process_utils.py`: - **Create group at launch:** the task process runs `set_new_process_group()` as its first action, which calls `os.setpgid(0, 0)` — making itself the group leader (`pid == pgid`). Note it uses `setpgid`, **not** the `setsid` binary, so there is **no extra fork**. - **Kill by group:** `reap_process_group()` → `os.killpg(pgid, SIGTERM)`, wait, then `os.killpg(pgid, SIGKILL)`. - **Hardening worth noting:** on `EPERM` (run_as_user) it falls back to `sudo -n kill -<sig> <pids>`; on `ESRCH` (process hasn't switched group yet) it falls back to `os.kill(pid)`; and it guards `if pgid == os.getpgid(0): raise "I refuse to kill myself"`. The catch: this is cheap **only because the task is a process Airflow `fork()`s itself**, so it can `os.setpgid` in-process between fork and exec. ### Azkaban — does NOT use process groups at all - **Run-as-user:** a setuid C helper `az-exec-util/src/main/c/execute-as-user.c` that only does `setgid()`/`setuid()` + `execvp()`. It is **purely for privilege drop — no `setsid`/`setpgid`.** - **Kill:** `AzkabanProcess.softKill()/hardKill()` (driven by `ProcessJob.cancel()`) just run `kill <pid>` then `kill -9 <pid>` against the **single top-level PID** (obtained via reflection on `Process.pid`), plus JVM `process.destroy()`. - So Azkaban only kills the **root** process and accepts that descendant processes may be orphaned. It does not walk the tree. ### Comparison | | Create process group | Kill scope | Native binary | |---|---|---|---| | **Airflow** | Yes — `os.setpgid` in-process (relies on forked task process) | Whole group `killpg` + `sudo -n` fallback | None (pure Python) | | **Azkaban** | No | **Top-level PID only** + JVM destroy | Yes, but only for `setuid` | | **DolphinScheduler** | No | `pstree` → kill each PID | None | ### Conclusions 1. **Reliable process-group kill is genuinely hard in a JVM scheduler.** Java's `ProcessBuilder` (Java 8) has no hook to `setpgid` the child between fork and exec, the way Python's `start_new_session`/`preexec_fn` does. Azkaban simply punts on it and kills only the root PID. 2. **DolphinScheduler's current `pstree`-and-kill is already more thorough than Azkaban's**, so we are not behind the state of the art here. 3. **Airflow's approach does not port directly to DS**, because the cheap `os.setpgid(0,0)` depends on the task being a process Airflow forked itself. With DS launching an external `sudo -u <tenant>` shell, the only in-JVM way to get an isolated group is `setsid` (the option already raised, with its fork/orphan concerns). 4. **If we do pursue a robust mechanism**, the realistic path for DS is a small native helper (à la Azkaban's `execute-as-user`) that does `setpgid(0, 0)` then `execvp()` — no extra fork — after which the worker records the PID (== PGID) and kills via `kill -- -<pgid>`, optionally with an `EPERM → sudo -n kill` fallback like Airflow. Azkaban shows that shipping a per-platform native helper in a JVM scheduler is acceptable in practice; it just stops one line short (`setpgid`) of also solving the kill-tree problem. The trade-offs (native packaging, group-escape if a child re-groups, permission for root-owned descendants) are why this deserves its own design thread rather than being bundled into the immediate fix. --- ## On #18312 and next steps To be clear on scope: **#18312 is intentionally narrow — it fixes an existing, concrete bug, and is independent of the redesign discussion above.** **What #18312 fixes:** the worker currently misreports a successful kill while child processes are still alive. The root cause is purely a false-negative in liveness detection — `AbstractShell` throws `ExitCodeException` whenever stderr is non-empty **even when the exit code is 0** (e.g. the tenant login shell printing `HISTSIZE: readonly variable` warnings). Because `isProcessAlive` treated any exception as "not alive", the still-alive children were classified as terminated, so **the kill signal was never actually sent** — hence `All processes already terminated` in the logs while `ps` still showed the tree running, and the workflow stuck in `READY_STOP`. **The fix is one branch:** when `kill -0` raises `ExitCodeException` with **exit code 0**, the process is alive. This: - does **not** change the kill return semantics, - does **not** add any user-visible kill state, - keeps the previous behavior for every other case, - and in the #18311 scenario it's sufficient: once the tenant-owned children are actually killed, the root-owned `sudo` parent exits on its own. I've **dropped** the earlier `NO_PERMISSION` classification (the part that touched kill-result semantics) from the PR, per the review — that can be revisited separately or folded into the redesign. **Proposal:** consider #18312 as a small, low-risk fix for the existing bug — now narrowed per the review (happy to adjust it further if needed) — and let's keep the robust "task process boundary / process-group" mechanism as a separate effort, with the Airflow/Azkaban findings above as input. I'd be glad to take on designing and prototyping that new kill mechanism — specifically the native-helper approach (a small `setpgid(0, 0)` + `execvp()` binary, with group-based `kill -- -<pgid>`) outlined above — once we align on the direction here. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
