kaxil commented on code in PR #72164:
URL: https://github.com/apache/airflow/pull/72164#discussion_r3972714813
##########
airflow-core/src/airflow/config_templates/config.yml:
##########
@@ -223,6 +223,13 @@ core:
* ``False``: Execute via forking of the parent process
* ``True``: Spawning a new python process, slower than fork, but means
plugin changes picked
up by tasks straight away
+
+ On Airflow 3 this applies to the task process only. When ``True`` the
supervisor ``exec``s
Review Comment:
The closing backticks in ``exec``s are followed by a letter, so rst does not
treat them as an end-string and the literal runs on to the ``True`` on line
232: the Configuration Reference renders everything from `exec` to `True` as
one monospace block, and docutils raises no warning (the `--docs-only` job
passed on b653700 with this text). The newsfragment already has the fix, ```
``exec``\ s ```. Small wording thing while here: 'will now start a second one'
is release-note tense for a permanent reference page; 'starts a second one'
reads right there.
##########
task-sdk/tests/task_sdk/execution_time/test_supervisor.py:
##########
@@ -4363,6 +4363,32 @@ def _drop_root_if_needed():
os.setuid(_NOBODY_UID)
[email protected](sys.platform != "linux", reason="PR_SET_DUMPABLE is
Linux-only")
+def test_exec_child_reapplies_nondumpable():
+ """execve resets PR_SET_DUMPABLE to 1; the exec'd child must set it to 0
again."""
+ probe = (
+ "import ctypes\n"
+ "from airflow.sdk.execution_time.supervisor import _PR_GET_DUMPABLE,
_make_process_nondumpable\n"
+ "libc = ctypes.CDLL(None, use_errno=True)\n"
+ "after_exec = libc.prctl(_PR_GET_DUMPABLE, 0, 0, 0, 0)\n"
+ "_make_process_nondumpable()\n"
Review Comment:
This pins execve resetting the flag and the helper restoring it, but the
probe calls `_make_process_nondumpable()` itself, so nothing checks that
`_child_exec_main` does: deleting supervisor.py:561 leaves the suite green.
Exec'ing the real bootstrap string (factored into a module constant) or a
`monkeypatch` spy on `_make_process_nondumpable` in `TestChildExecMain`,
asserted to run before `_fork_main`, would pin the wiring. The forked branch
also wants a `try/finally: os._exit(...)` like
`test_nondumpable_blocks_child_memory_read`, or a failing `execv` runs the rest
of pytest in the child while the parent blocks on the pipe.
##########
task-sdk/src/airflow/sdk/execution_time/supervisor.py:
##########
@@ -533,13 +549,16 @@ def _resolve_child_target(dotted: str) -> Callable[[],
None]:
def _child_exec_main():
"""
- Entry point for the child process when using fork+exec (macOS).
+ Entry point for the child process when using fork+exec.
After exec, FDs 0/1/2/3 are the requests/stdout/stderr/log sockets the
parent
placed there via dup2. The target to run is named in
``_AIRFLOW_CHILD_TARGET``
(``module:qualname``); it is rehydrated and handed to :func:`_fork_main`,
which
sets up the structured log channel from FD 3 exactly as the bare-fork path
does.
"""
+ # execve resets PR_SET_DUMPABLE to 1, so re-apply what supervise_task()
set before the
+ # fork; otherwise a same-UID sibling could read this child's
/proc/<pid>/environ.
+ _make_process_nondumpable()
Review Comment:
This restores the flag for the task's lifetime, but the child is dumpable
from `execve` until this line runs, which is interpreter start plus `import
airflow.sdk.execution_time.supervisor` (about a second on a dev checkout here,
more with plugins), and a `/proc/<pid>/mem` fd or `PTRACE_ATTACH` taken in that
window survives the prctl because the kernel checks access once, at open or
attach. It needs same-UID co-tenancy and `kernel.yama.ptrace_scope=0`, so not
blocking, but running the prctl in the `-c` bootstrap before the Airflow import
shrinks the window to interpreter start at no cost, with this call kept as the
logged fallback. The two security docs would then describe it that way rather
than 'as its first step', naming `ptrace_scope>=1` as what covers the remainder.
##########
task-sdk/tests/task_sdk/execution_time/test_supervisor.py:
##########
@@ -4479,6 +4505,25 @@ def
test_api_client_clears_dag_bag_override_when_dag_is_none():
in_process_api_server.cache_clear()
+class TestTaskProcessUsesExec:
+ """The config opt-in for fork+exec of the task process where the platform
does not force it."""
+
+ @pytest.mark.parametrize(
+ ("platform", "config_value", "expected"),
+ [
+ ("darwin", None, True),
+ ("darwin", "False", True),
+ ("linux", None, False),
+ ("linux", "False", False),
+ ("linux", "True", True),
+ ],
+ )
+ def test_task_process_uses_exec(self, monkeypatch, platform, config_value,
expected):
+ monkeypatch.setattr(supervisor.sys, "platform", platform)
+ with conf_vars({("core", "execute_tasks_new_python_interpreter"):
config_value}):
+ assert supervisor._task_process_uses_exec() is expected
Review Comment:
This binds the helper, but nothing asserts `ActivitySubprocess.start` passes
its result as `use_exec`, so reverting line 1437 to `_should_use_exec()` also
stays green. The Dag processor and triggerer each have a
`test_start_opts_into_fork_exec` (`test_processor.py:493`,
`test_triggerer_job.py:288`) that patches the gate, mocks
`WatchedSubprocess.start` and asserts `call_args.kwargs["use_exec"]`; a mirror
here, including the stub-target case expecting `False`, would close it.
##########
airflow-core/newsfragments/72164.significant.rst:
##########
@@ -0,0 +1,21 @@
+``[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 and
triggerer are not
+affected.
Review Comment:
Callbacks are the other thing these workers fork: `ExecuteCallback` goes
`run_workload` to `supervise_callback` to `CallbackSubprocess.start`, whose
closure target can never take the exec path (`WatchedSubprocess.start` rejects
closures under `use_exec`), so an `on_failure_callback` that does TLS keeps the
inherited-lock exposure after an operator flips this option to cure hangs.
Tasks-only is the agreed scope; I'm not sure whether this line (and the config
text) should say callbacks stay on bare fork, or whether a follow-up for an
importable callback entry point is the better home.
##########
task-sdk/src/airflow/sdk/execution_time/supervisor.py:
##########
@@ -517,6 +517,22 @@ def _should_use_exec() -> bool:
return sys.platform in _FORK_EXEC_PLATFORMS
+def _task_process_uses_exec() -> bool:
+ """
+ Whether the task process should ``exec`` a fresh interpreter after the
fork.
+
+ Forced where bare fork is unsafe (macOS); elsewhere a deployment opts in
with
+ ``[core] execute_tasks_new_python_interpreter``. exec replaces the child's
address
+ space, so it cannot inherit a lock a supervisor thread held at fork time
(e.g.
+ OpenSSL's, which otherwise hangs the task at its first TLS call; #71707).
Only the
+ task process reads the option: the Dag processor and triggerer fork far
more often
+ and keep the platform gate alone.
Review Comment:
`TriggerRunnerSupervisor.start` runs once per triggerer job
(`triggerer_job_runner.py:271`), so 'fork far more often' only holds for the
Dag processor; the reason that covers both is that the option has always
described task execution. The base `start()` docstring for `use_exec` (line
712) and the child comment at 764 also still say macOS only, though they now
describe the Linux opt-in path too.
--
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]
