kaxil commented on code in PR #72164:
URL: https://github.com/apache/airflow/pull/72164#discussion_r3979588416
##########
task-sdk/src/airflow/sdk/execution_time/supervisor.py:
##########
@@ -531,15 +547,37 @@ def _resolve_child_target(dotted: str) -> Callable[[],
None]:
return pkgutil.resolve_name(dotted)
+# Runs in the exec'd child before anything else. execve reset PR_SET_DUMPABLE
(4 in
+# <linux/prctl.h>); restore it before the Airflow import so the window in
which a same-UID
+# sibling can open /proc/<pid>/mem or ptrace-attach is interpreter start only
(a descriptor
+# or attach taken in that window survives a later prctl -- the kernel checks
once, at open).
+# _child_exec_main() repeats the call as the logged fallback.
+_CHILD_EXEC_PRELUDE = """\
+import sys
+if sys.platform == "linux":
+ import ctypes
Review Comment:
`import ctypes` sits outside the `try` here, unlike
`_make_process_nondumpable()` which guards the import as well, so on an
interpreter built without `_ctypes` the bootstrap dies with
`ModuleNotFoundError` before `_child_exec_main` runs and the logged fallback
never gets its turn: every opted-in task exits 1 where bare fork degrades to a
warning today. Moving the import inside the `try` matches the helper; a probe
with `sys.modules['_ctypes'] = None` reproduces the failure at HEAD and passes
with the import moved.
##########
task-sdk/src/airflow/sdk/execution_time/supervisor.py:
##########
@@ -531,15 +547,37 @@ def _resolve_child_target(dotted: str) -> Callable[[],
None]:
return pkgutil.resolve_name(dotted)
+# Runs in the exec'd child before anything else. execve reset PR_SET_DUMPABLE
(4 in
+# <linux/prctl.h>); restore it before the Airflow import so the window in
which a same-UID
+# sibling can open /proc/<pid>/mem or ptrace-attach is interpreter start only
(a descriptor
+# or attach taken in that window survives a later prctl -- the kernel checks
once, at open).
+# _child_exec_main() repeats the call as the logged fallback.
+_CHILD_EXEC_PRELUDE = """\
+import sys
+if sys.platform == "linux":
+ import ctypes
+ try:
+ ctypes.CDLL(None, use_errno=True).prctl(4, 0, 0, 0, 0)
+ except Exception:
+ pass
+"""
+_CHILD_EXEC_BOOTSTRAP = _CHILD_EXEC_PRELUDE + (
Review Comment:
Nothing runs this string end to end: the Linux test execs
`_CHILD_EXEC_PRELUDE`, the one `use_exec=True` test raises at the closure guard
before the fork, and CI has no macOS runner, so dropping `_child_exec_main()`
from here, dropping the prelude, or losing `sys.executable` as argv[0] in the
`execv` call all stay green while failing every opted-in task. A parametrized
row of `test_run_simple_dag` with `_task_process_uses_exec` patched to `True`
would cover the assembled path on Linux CI: it goes through `supervise_task`
with the default target and so reaches the real `os.execv`, and the asserted
`task.stdout` line is re-logged by the supervisor, so the frozen timestamp
still holds. `assert _CHILD_EXEC_BOOTSTRAP.startswith(_CHILD_EXEC_PRELUDE)` is
a cheap floor alongside it.
##########
airflow-core/newsfragments/72164.significant.rst:
##########
@@ -0,0 +1,22 @@
+``[core] execute_tasks_new_python_interpreter`` now applies to Airflow 3 task
processes
+
+On Airflow 3 the option had no effect on task execution (only the Edge worker
read it). When set to
+``True``, the task supervisor now ``exec``\ s a fresh interpreter right after
forking the task process,
+which prevents the fork from inheriting a lock held by a supervisor thread (a
permanent hang at the
+task's first TLS call). Deployments that kept the option ``True`` from Airflow
2 get this behaviour,
+and its per-task interpreter start-up cost, on upgrade without further action;
set it to ``False`` to
+keep bare fork. Edge workers with the option ``True`` already start a fresh
interpreter for the
+supervisor and will now start a second one for the task. The Dag processor,
the triggerer and task
Review Comment:
One narrow inaccuracy here: the Edge worker reads this option through
`ExecutorConf(team_name)`, and a team-scoped read consults only the
`<team>___core` section (and the `AIRFLOW__<TEAM>___CORE__...` env var) with no
fallback to the global value, while the supervisor's new read is global. In a
multi-team Edge deployment a team-scoped `True` therefore gives a fresh
supervisor interpreter and a bare-forked task, and a global `True` the reverse;
a clause that the task process reads the global value would keep this sentence
and the config text accurate, and forwarding the resolved value into the
supervisor's env in `_launch_job_subprocess` looks like an edge3 follow-up.
##########
airflow-core/docs/security/workload.rst:
##########
@@ -67,8 +67,11 @@ Worker process memory protection (Linux)
''''''''''''''''''''''''''''''''''''''''
On Linux, the supervisor process calls ``prctl(PR_SET_DUMPABLE, 0)`` at the
start of
-``supervise_task()`` before forking the task process. This flag is inherited
by the forked
-child. Marking processes as non-dumpable prevents same-UID sibling processes
from reading
+``supervise_task()`` before forking the task process. A bare-forked child
inherits the flag;
+a child started through ``exec`` (macOS, or ``[core]
execute_tasks_new_python_interpreter``)
+restores it in its bootstrap, before importing Airflow, because ``execve``
resets it; the remaining
+window is interpreter start, which ``kernel.yama.ptrace_scope >= 1`` covers.
Marking processes as
Review Comment:
Following on from
https://github.com/apache/airflow/pull/72164#discussion_r3972714843, one
precision point on this sentence, since the paragraph goes on to list
`/proc/<pid>/environ` and `maps` among what is blocked: Yama only gates
`PTRACE_MODE_ATTACH` (`yama_ptrace_access_check` tests `mode &
PTRACE_MODE_ATTACH`), so `ptrace_scope >= 1` does close `/proc/<pid>/mem` and
`ptrace` attach for the interpreter-start window, but `environ_open` and
`proc_maps_open` use `PTRACE_MODE_READ` and stay openable in it, and
`environ_read` never re-checks. What that shows is the supervisor's
environment, which every task under the same worker already inherits, so
nothing new leaks; the sentence would be exact as "``kernel.yama.ptrace_scope
>= 1`` covers ``/proc/<pid>/mem`` and ``ptrace`` attach for that window", and
`security_model.rst` line 583 has the same wording.
--
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]
