gang-zh commented on code in PR #72164:
URL: https://github.com/apache/airflow/pull/72164#discussion_r3973082952
##########
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:
Fixed in f6f4740 — reworded so no literal is followed by a letter ("starts
the task in a fresh interpreter with `exec`"), and present tense for the
reference page.
##########
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:
Done in faa7ce7: the prelude is now a module constant
(`_CHILD_EXEC_PRELUDE`, part of `_CHILD_EXEC_BOOTSTRAP`) and the Linux test
execs the real string; a `TestChildExecMain` spy asserts
`_make_process_nondumpable()` runs before `_fork_main`; the forked branch exits
through `finally: os._exit(1)`.
##########
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:
Added in faa7ce7: `test_activity_start_opts_into_fork_exec`, same shape as
the processor/triggerer tests (patched gate, mocked `WatchedSubprocess.start`,
asserts `use_exec`; stub target expects `False`).
##########
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:
Good point — faa7ce7 moves the prctl into the `-c` bootstrap via ctypes,
before the Airflow import, so the window is interpreter start only; this call
stays as the logged fallback. Both security docs now describe it that way and
name `ptrace_scope >= 1` for the remainder (f6f4740).
##########
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:
Went with saying it: f6f4740 notes in the newsfragment and the config text
that task callbacks (closure target, cannot be named for an exec'd child) keep
bare fork. An importable callback entry point feels like its own follow-up;
happy to open an issue for it.
##########
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:
Right — reworded in faa7ce7 to lean on the option having always described
task execution (with the per-parse fork rate noted for the Dag processor only),
and the `use_exec` docstring and child comment no longer say macOS-only.
--
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]
