kaxil commented on code in PR #72164:
URL: https://github.com/apache/airflow/pull/72164#discussion_r3963303760
##########
task-sdk/src/airflow/sdk/execution_time/supervisor.py:
##########
@@ -513,8 +513,18 @@ def exit(n: int) -> NoReturn:
def _should_use_exec() -> bool:
- """Whether forked children should ``exec`` a fresh interpreter on this
platform."""
- return sys.platform in _FORK_EXEC_PLATFORMS
+ """
+ Whether forked children should ``exec`` a fresh interpreter.
Review Comment:
`_should_use_exec()` is also what `DagFileProcessorProcess.start()` and
`TriggerRunnerSupervisor.start()` consult, so on Linux this flag now also execs
a fresh interpreter per Dag file per parse loop (skipping
`_pre_import_airflow_modules`) and once for the triggerer runner, while the
title, this docstring and the config text only describe the task process. Is
the Dag processor part intended? The same boot you measured at ~9s per task
lands in the parse loop, counts against `dag_file_processor_timeout`, and the
Helm chart's `config:` renders into the one `airflow.cfg` every component
mounts, so scoping it to workers means a per-component `env:` override instead.
If tasks are the intent, moving the conf read to `ActivitySubprocess.start()`
(next to the `target is _subprocess_main` check) keeps this as the platform
gate and matches what a v3-2/v3-3 backport can carry anyway, since those
branches have no `_should_use_exec()` and check darwin inline there. If all
three are intended, t
he config description needs to say so. (slice-soupam's latest comment on
#71707 makes the fork-rate case for splitting these.)
##########
task-sdk/tests/task_sdk/execution_time/test_supervisor.py:
##########
@@ -4479,6 +4479,31 @@ def
test_api_client_clears_dag_bag_override_when_dag_is_none():
in_process_api_server.cache_clear()
+class TestShouldUseExec:
+ """The config opt-in for fork+exec on platforms where it is not forced
on."""
+
+ @pytest.mark.parametrize(
+ ("platform", "config_value", "expected"),
+ [
+ ("darwin", None, True),
+ ("darwin", "False", True),
+ ("linux", None, False),
+ ("linux", "False", False),
+ ("linux", "True", True),
+ ],
+ )
+ def test_should_use_exec(self, monkeypatch, platform, config_value,
expected):
+ monkeypatch.setattr(supervisor.sys, "platform", platform)
+ # The supervisor reads the task-sdk conf; the env var reaches it
regardless
Review Comment:
`conf_vars` already patches the SDK conf too (it picks up
`airflow.sdk.configuration.conf` whenever that module is in `sys.modules`,
which it is once `supervisor` is imported), so the PR-body rationale for going
through the env var doesn't hold, and `with conf_vars({("core",
"execute_tasks_new_python_interpreter"): config_value}):` would match the rest
of this file. The env var route works as well, since env is first in the lookup
order, so this is only about consistency.
##########
airflow-core/src/airflow/config_templates/config.yml:
##########
@@ -223,6 +223,10 @@ 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 workers this also makes the task process ``exec`` a fresh
interpreter right after the
Review Comment:
Two things this paragraph should cover. The option has been a no-op for
tasks on Airflow 3 until now (Celery's reader is behind `if not
AIRFLOW_V_3_0_PLUS`, `settings.py` only has a removed-attribute shim, and the
Edge worker is the one component that reads it), so anyone who kept it `True`
from Airflow 2 gets the exec path and its per-task cost on upgrade with no
action; a `72164.significant.rst` newsfragment would flag that. And the Edge
worker already `Popen`s a fresh interpreter to run the supervisor when this is
`True` (`providers/edge3/.../cli/worker.py`, `_launch_job`), so those
deployments will now boot two interpreters per task. Both seem acceptable, but
they should be written down here rather than discovered in task duration.
##########
task-sdk/src/airflow/sdk/execution_time/supervisor.py:
##########
@@ -513,8 +513,18 @@ def exit(n: int) -> NoReturn:
def _should_use_exec() -> bool:
- """Whether forked children should ``exec`` a fresh interpreter on this
platform."""
- return sys.platform in _FORK_EXEC_PLATFORMS
+ """
+ Whether forked children should ``exec`` a fresh interpreter.
+
+ Always on for platforms where bare fork is unsafe (macOS). Elsewhere it
can be
+ opted into 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 deadlocks the task at its
first
+ TLS call — see #71707).
+ """
+ if sys.platform in _FORK_EXEC_PLATFORMS:
+ return True
+ return conf.getboolean("core", "execute_tasks_new_python_interpreter",
fallback=False)
Review Comment:
Turning this on for Linux drops the memory protection #62523 added.
`supervise_task()` calls `_make_process_nondumpable()` (`prctl(PR_SET_DUMPABLE,
0)`) in the supervisor and relies on `fork()` inheriting the flag, but `execve`
gives the child a fresh mm with dumpable reset to 1, and `_child_exec_main()`
never re-applies it. Reproduced in a `python:3.12-slim` container as uid 65534:
supervisor 0, bare-fork child 0, exec'd child 1, and a same-UID sibling could
read the exec'd child's `/proc/<pid>/environ` and `maps` while the bare-fork
child's raised `PermissionError`. On Celery, LocalExecutor and Edge workers,
where several tasks share one UID, that means task A's user code can read task
B's environment (the supervisor's env at exec time) and, where `ptrace_scope`
allows, its memory. On macOS this never showed because the prctl is a no-op
there. Calling `_make_process_nondumpable()` first thing in
`_child_exec_main()` closes it for both paths; `security/workload.rst` and
`security
_model.rst` ("this flag is inherited by the forked child") need a matching
edit, and a Linux-only test beside `test_nondumpable_blocks_sibling_proc_read`
that fork+execs and asserts `PR_GET_DUMPABLE == 0` would pin it.
--
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]
