diff --git a/airflow-core/newsfragments/52860.significant.rst b/airflow-core/newsfragments/52860.significant.rst new file mode 100644 index 0000000000000..5962897ec206d --- /dev/null +++ b/airflow-core/newsfragments/52860.significant.rst @@ -0,0 +1,17 @@ +Replace API server ``access_logfile`` configuration with ``log_config`` + +The API server configuration option ``[api] access_logfile`` has been replaced with ``[api] log_config`` to align with uvicorn's logging configuration instead of the legacy gunicorn approach. +The new ``log_config`` option accepts a path to a logging configuration file compatible with ``logging.config.fileConfig``, providing more flexible logging configuration for the API server. + +This change also removes the dependency on gunicorn for daemonization, making the API server ``--daemon`` option consistent with other Airflow components like scheduler and triggerer. + +* Types of change + + * [ ] Dag changes + * [x] Config changes + * [ ] API changes + * [ ] CLI changes + * [ ] Behaviour changes + * [ ] Plugin changes + * [ ] Dependency changes + * [ ] Code interface changes diff --git a/airflow-core/src/airflow/cli/cli_config.py b/airflow-core/src/airflow/cli/cli_config.py index 31022e17219c4..363839a85808c 100644 --- a/airflow-core/src/airflow/cli/cli_config.py +++ b/airflow-core/src/airflow/cli/cli_config.py @@ -633,10 +633,10 @@ def string_lower_type(val): default=conf.get("api", "host"), help="Set the host on which to run the API server", ) -ARG_API_SERVER_ACCESS_LOGFILE = Arg( - ("-A", "--access-logfile"), - default=conf.get("api", "access_logfile"), - help="The logfile to store the access log. Use '-' to print to stdout", +ARG_API_SERVER_LOG_CONFIG = Arg( + ("--log-config",), + default=conf.get("api", "log_config", fallback=None), + help="(Optional) Path to the logging configuration file for the uvicorn server. If not set, the default uvicorn logging configuration will be used.", ) ARG_API_SERVER_APPS = Arg( ("--apps",), @@ -1864,7 +1864,7 @@ class GroupCommand(NamedTuple): ARG_DAEMON, ARG_STDOUT, ARG_STDERR, - ARG_API_SERVER_ACCESS_LOGFILE, + ARG_API_SERVER_LOG_CONFIG, ARG_API_SERVER_APPS, ARG_LOG_FILE, ARG_SSL_CERT, diff --git a/airflow-core/src/airflow/cli/commands/api_server_command.py b/airflow-core/src/airflow/cli/commands/api_server_command.py index 343890aa8e4e8..5a8e1fb86411c 100644 --- a/airflow-core/src/airflow/cli/commands/api_server_command.py +++ b/airflow-core/src/airflow/cli/commands/api_server_command.py @@ -21,13 +21,13 @@ import logging import os import subprocess +import sys import textwrap import uvicorn -from gunicorn.util import daemonize -from setproctitle import setproctitle from airflow import settings +from airflow.cli.commands.daemon_utils import run_command_with_daemon_option from airflow.exceptions import AirflowConfigException from airflow.utils import cli as cli_utils from airflow.utils.providers_configuration_loader import providers_configuration_loaded @@ -40,6 +40,55 @@ # more info here: https://github.com/benoitc/gunicorn/issues/1877#issuecomment-1911136399 +def _run_api_server(args, apps: str, num_workers: int, worker_timeout: int, proxy_headers: bool): + """Run the API server.""" + log.info( + textwrap.dedent( + f"""\ + Running the uvicorn with: + Apps: {apps} + Workers: {num_workers} + Host: {args.host}:{args.port} + Timeout: {worker_timeout} + Logfiles: {args.log_file or "-"} + =================================================================""" + ) + ) + # get ssl cert and key filepaths here instead of passing them as arguments to reduce the number of arguments + ssl_cert, ssl_key = _get_ssl_cert_and_key_filepaths(args) + + # setproctitle causes issue on Mac OS: https://github.com/benoitc/gunicorn/issues/3021 + os_type = sys.platform + if os_type == "darwin": + log.debug("Mac OS detected, skipping setproctitle") + else: + from setproctitle import setproctitle + + setproctitle(f"airflow api_server -- host:{args.host} port:{args.port}") + + uvicorn_kwargs = { + "host": args.host, + "port": args.port, + "workers": num_workers, + "timeout_keep_alive": worker_timeout, + "timeout_graceful_shutdown": worker_timeout, + "ssl_keyfile": ssl_key, + "ssl_certfile": ssl_cert, + "access_log": True, + "proxy_headers": proxy_headers, + } + # Only set the log_config if it is provided, otherwise use the default uvicorn logging configuration. + if args.log_config and args.log_config != "-": + # The [api/log_config] is migrated from [api/access_logfile] and [api/access_logfile] defaults to "-" for stdout for Gunicorn. + # So we need to check if the log_config is set to "-" or not; if it is set to "-", we regard it as not set. + uvicorn_kwargs["log_config"] = args.log_config + + uvicorn.run( + "airflow.api_fastapi.main:app", + **uvicorn_kwargs, + ) + + @cli_utils.action_cli @providers_configuration_loaded def api_server(args): @@ -47,7 +96,6 @@ def api_server(args): print(settings.HEADER) apps = args.apps - access_logfile = args.access_logfile or "-" num_workers = args.workers worker_timeout = args.worker_timeout proxy_headers = args.proxy_headers @@ -74,6 +122,9 @@ def api_server(args): if args.proxy_headers: run_args.append("--proxy-headers") + if args.log_config and args.log_config != "-": + run_args.extend(["--log-config", args.log_config]) + # There is no way to pass the apps to airflow/api_fastapi/main.py in the development mode # because fastapi dev command does not accept any additional arguments # so environment variable is being used to pass it @@ -85,35 +136,16 @@ def api_server(args): process.wait() os.environ.pop("AIRFLOW_API_APPS") else: - if args.daemon: - daemonize() - log.info("Daemonized the API server process PID: %s", os.getpid()) - - log.info( - textwrap.dedent( - f"""\ - Running the uvicorn with: - Apps: {apps} - Workers: {num_workers} - Host: {args.host}:{args.port} - Timeout: {worker_timeout} - Logfiles: {access_logfile} - =================================================================""" - ) - ) - ssl_cert, ssl_key = _get_ssl_cert_and_key_filepaths(args) - setproctitle(f"airflow api_server -- host:{args.host} port:{args.port}") - uvicorn.run( - "airflow.api_fastapi.main:app", - host=args.host, - port=args.port, - workers=num_workers, - timeout_keep_alive=worker_timeout, - timeout_graceful_shutdown=worker_timeout, - ssl_keyfile=ssl_key, - ssl_certfile=ssl_cert, - access_log=access_logfile, - proxy_headers=proxy_headers, + run_command_with_daemon_option( + args=args, + process_name="api_server", + callback=lambda: _run_api_server( + args=args, + apps=apps, + num_workers=num_workers, + worker_timeout=worker_timeout, + proxy_headers=proxy_headers, + ), ) diff --git a/airflow-core/src/airflow/config_templates/config.yml b/airflow-core/src/airflow/config_templates/config.yml index e0354cc6fd04b..fa433c427c7ee 100644 --- a/airflow-core/src/airflow/config_templates/config.yml +++ b/airflow-core/src/airflow/config_templates/config.yml @@ -1371,13 +1371,14 @@ api: type: integer example: ~ default: "120" - access_logfile: + log_config: description: | - Log files for the api server. '-' means log to stderr. + Path to the logging configuration file for the uvicorn server. + If not set, the default uvicorn logging configuration will be used. version_added: ~ type: string - example: ~ - default: "-" + example: path/to/logging_config.yaml + default: ~ ssl_cert: description: | Paths to the SSL certificate and key for the api server. When both are diff --git a/airflow-core/src/airflow/configuration.py b/airflow-core/src/airflow/configuration.py index d1bcb8015a0f9..2fbaa798fd7df 100644 --- a/airflow-core/src/airflow/configuration.py +++ b/airflow-core/src/airflow/configuration.py @@ -373,6 +373,7 @@ def sensitive_config_values(self) -> set[tuple[str, str]]: ("api", "require_confirmation_dag_change"): ("webserver", "require_confirmation_dag_change", "3.1.0"), ("api", "instance_name"): ("webserver", "instance_name", "3.1.0"), ("dag_processor", "parsing_pre_import_modules"): ("scheduler", "parsing_pre_import_modules", "3.1.0"), + ("api", "log_config"): ("api", "access_logfile", "3.1.0"), } # A mapping of new section -> (old section, since_version). diff --git a/airflow-core/tests/unit/cli/commands/_common_cli_classes.py b/airflow-core/tests/unit/cli/commands/_common_cli_classes.py index 37bbca0784a28..3adfb78e69966 100644 --- a/airflow-core/tests/unit/cli/commands/_common_cli_classes.py +++ b/airflow-core/tests/unit/cli/commands/_common_cli_classes.py @@ -33,7 +33,7 @@ console = Console(width=400, color_system="standard") -class _CommonCLIGunicornTestClass: +class _CommonCLIUvicornTestClass: main_process_regexp: str = "process_to_look_for" @pytest.fixture(autouse=True) @@ -49,12 +49,12 @@ def _check_processes(self, ignore_running: bool): # Confirm that nmain procss hasn't been launched. # pgrep returns exit status 1 if no process matched. # Use more specific regexps (^) to avoid matching pytest run when running specific method. - # For instance, we want to be able to do: pytest -k 'gunicorn' + # For instance, we want to be able to do: pytest -k 'uvicorn' airflow_internal_api_pids = self._find_all_processes(self.main_process_regexp) - gunicorn_pids = self._find_all_processes(r"gunicorn: ") - if airflow_internal_api_pids or gunicorn_pids: + uvicorn_pids = self._find_all_processes(r"uvicorn: ") + if airflow_internal_api_pids or uvicorn_pids: console.print("[blue]Some processes are still running") - for pid in gunicorn_pids + airflow_internal_api_pids: + for pid in uvicorn_pids + airflow_internal_api_pids: with suppress(NoSuchProcess): console.print(psutil.Process(pid).as_dict(attrs=["pid", "name", "cmdline"])) console.print("[blue]Here list of processes ends") @@ -63,9 +63,9 @@ def _check_processes(self, ignore_running: bool): for pid in airflow_internal_api_pids: with suppress(NoSuchProcess): psutil.Process(pid).kill() - if gunicorn_pids: - console.print("[yellow]Forcefully killing all gunicorn processes") - for pid in gunicorn_pids: + if uvicorn_pids: + console.print("[yellow]Forcefully killing all uvicorn processes") + for pid in uvicorn_pids: with suppress(NoSuchProcess): psutil.Process(pid).kill() if not ignore_running: diff --git a/airflow-core/tests/unit/cli/commands/test_api_server_command.py b/airflow-core/tests/unit/cli/commands/test_api_server_command.py index 837ee0f61bd1e..89689d3d9ba85 100644 --- a/airflow-core/tests/unit/cli/commands/test_api_server_command.py +++ b/airflow-core/tests/unit/cli/commands/test_api_server_command.py @@ -24,19 +24,19 @@ from airflow.cli.commands import api_server_command from airflow.exceptions import AirflowConfigException -from unit.cli.commands._common_cli_classes import _CommonCLIGunicornTestClass +from unit.cli.commands._common_cli_classes import _CommonCLIUvicornTestClass console = Console(width=400, color_system="standard") @pytest.mark.db_test -class TestCliApiServer(_CommonCLIGunicornTestClass): +class TestCliApiServer(_CommonCLIUvicornTestClass): main_process_regexp = r"airflow api-server" @pytest.mark.parametrize( "args, expected_command", [ - ( + pytest.param( ["api-server", "--port", "9092", "--host", "somehost", "--dev"], [ "fastapi", @@ -47,8 +47,9 @@ class TestCliApiServer(_CommonCLIGunicornTestClass): "--host", "somehost", ], + id="dev mode with port and host", ), - ( + pytest.param( ["api-server", "--port", "9092", "--host", "somehost", "--dev", "--proxy-headers"], [ "fastapi", @@ -60,6 +61,31 @@ class TestCliApiServer(_CommonCLIGunicornTestClass): "somehost", "--proxy-headers", ], + id="dev mode with port, host and proxy headers", + ), + pytest.param( + [ + "api-server", + "--port", + "9092", + "--host", + "somehost", + "--dev", + "--log-config", + "my_log_config.yaml", + ], + [ + "fastapi", + "dev", + "airflow-core/src/airflow/api_fastapi/main.py", + "--port", + "9092", + "--host", + "somehost", + "--log-config", + "my_log_config.yaml", + ], + id="dev mode with port, host and log config", ), ], ) @@ -119,40 +145,158 @@ def test_apps_env_var_set_unset(self): # Assert that AIRFLOW_API_APPS was unset after subprocess mock_environ.pop.assert_called_with("AIRFLOW_API_APPS") - def test_args_to_uvicorn(self, ssl_cert_and_key): - cert_path, key_path = ssl_cert_and_key - - with ( - mock.patch("uvicorn.run") as mock_run, - ): - args = self.parser.parse_args( + @pytest.mark.parametrize( + "cli_args, expected_additional_kwargs", + [ + pytest.param( [ "api-server", "--pid", "/tmp/x.pid", "--ssl-cert", - str(cert_path), + "ssl_cert_path_placeholder", "--ssl-key", - str(key_path), + "ssl_key_path_placeholder", "--apps", "core", - ] - ) + ], + { + "ssl_keyfile": "ssl_key_path_placeholder", + "ssl_certfile": "ssl_cert_path_placeholder", + }, + id="api-server with SSL cert and key", + ), + pytest.param( + [ + "api-server", + "--log-config", + "my_log_config.yaml", + ], + { + "ssl_keyfile": None, + "ssl_certfile": None, + "log_config": "my_log_config.yaml", + }, + id="api-server with log config", + ), + ], + ) + def test_args_to_uvicorn(self, ssl_cert_and_key, cli_args, expected_additional_kwargs): + cert_path, key_path = ssl_cert_and_key + if "ssl_cert_path_placeholder" in cli_args: + cli_args[cli_args.index("ssl_cert_path_placeholder")] = str(cert_path) + expected_additional_kwargs["ssl_certfile"] = str(cert_path) + if "ssl_key_path_placeholder" in cli_args: + cli_args[cli_args.index("ssl_key_path_placeholder")] = str(key_path) + expected_additional_kwargs["ssl_keyfile"] = str(key_path) + + with ( + mock.patch("uvicorn.run") as mock_run, + ): + args = self.parser.parse_args(cli_args) api_server_command.api_server(args) mock_run.assert_called_with( "airflow.api_fastapi.main:app", - host="0.0.0.0", - port=8080, - workers=4, - timeout_keep_alive=120, - timeout_graceful_shutdown=120, - ssl_keyfile=str(key_path), - ssl_certfile=str(cert_path), - access_log="-", - proxy_headers=False, + **{ + "host": args.host, + "port": args.port, + "workers": args.workers, + "timeout_keep_alive": args.worker_timeout, + "timeout_graceful_shutdown": args.worker_timeout, + "access_log": True, + "proxy_headers": args.proxy_headers, + **expected_additional_kwargs, + }, ) + @pytest.mark.parametrize( + "demonize", + [True, False], + ) + @mock.patch("airflow.cli.commands.daemon_utils.TimeoutPIDLockFile") + @mock.patch("airflow.cli.commands.daemon_utils.setup_locations") + @mock.patch("airflow.cli.commands.daemon_utils.daemon") + @mock.patch("airflow.cli.commands.daemon_utils.check_if_pidfile_process_is_running") + @mock.patch("airflow.cli.commands.api_server_command.uvicorn") + def test_run_command_daemon( + self, mock_uvicorn, _, mock_daemon, mock_setup_locations, mock_pid_file, demonize + ): + mock_setup_locations.return_value = ( + mock.MagicMock(name="pidfile"), + mock.MagicMock(name="stdout"), + mock.MagicMock(name="stderr"), + mock.MagicMock(name="INVALID"), + ) + args = self.parser.parse_args( + [ + "api-server", + "--host", + "my-hostname", + "--port", + "9090", + "--workers", + "2", + "--worker-timeout", + "60", + ] + + (["--daemon"] if demonize else []) + ) + mock_open = mock.mock_open() + with mock.patch("airflow.cli.commands.daemon_utils.open", mock_open): + api_server_command.api_server(args) + + mock_uvicorn.run.assert_called_once_with( + "airflow.api_fastapi.main:app", + host="my-hostname", + port=9090, + workers=2, + timeout_keep_alive=60, + timeout_graceful_shutdown=60, + ssl_keyfile=None, + ssl_certfile=None, + access_log=True, + proxy_headers=False, + ) + + if demonize: + assert mock_daemon.mock_calls[:3] == [ + mock.call.DaemonContext( + pidfile=mock_pid_file.return_value, + files_preserve=None, + stdout=mock_open.return_value, + stderr=mock_open.return_value, + umask=0o077, + ), + mock.call.DaemonContext().__enter__(), + mock.call.DaemonContext().__exit__(None, None, None), + ] + assert mock_setup_locations.mock_calls == [ + mock.call( + process="api_server", + pid=None, + stdout=None, + stderr=None, + log=None, + ) + ] + mock_pid_file.assert_has_calls([mock.call(mock_setup_locations.return_value[0], -1)]) + assert mock_open.mock_calls == [ + mock.call(mock_setup_locations.return_value[1], "a"), + mock.call().__enter__(), + mock.call(mock_setup_locations.return_value[2], "a"), + mock.call().__enter__(), + mock.call().truncate(0), + mock.call().truncate(0), + mock.call().__exit__(None, None, None), + mock.call().__exit__(None, None, None), + ] + else: + assert mock_daemon.mock_calls == [] + mock_setup_locations.mock_calls == [] + mock_pid_file.assert_not_called() + mock_open.assert_not_called() + @pytest.mark.parametrize( "ssl_arguments, error_pattern", [