-
Notifications
You must be signed in to change notification settings - Fork 16.3k
Remove gunicorn daemonize for api-server #52860
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove gunicorn daemonize for api-server #52860
Conversation
1dfe755 to
7a44f9a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
access_log=access_logfile, # type: ignore[arg-type] this line is a strange part for me.
I add
# type: ignore[arg-type]to ingore the mypy warning for now.
|
Hi @potiuk, may I might need hints for replacing config. I thought replacing them in Thanks in advance! Update: I found the root cause. My step is correct but I set |
89b529e to
52151fe
Compare
jason810496
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @potiuk and @pierrejeambrun for the review!
Just resolved all comments and fixed the CI as well.
potiuk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. One NIT about updating newsfragment.
- the logging before daemonization is not logging the correct PID so remove it - should_setup_logging is default as False
52151fe to
ef4c730
Compare
* Remove gunicorn daemonize for api-server * Rename _CommonCLIUvicornTestClass * Add test for api-server with daemon option * Fix setproctitle on MacOS * Replace api/access_logfile with api/log_config * Fix [api/log_config] cli_config * Remove logging and unsed option in api_server_command - the logging before daemonization is not logging the correct PID so remove it - should_setup_logging is default as False * Fix test_run_command_daemon * Add significant rst
* Remove gunicorn daemonize for api-server * Rename _CommonCLIUvicornTestClass * Add test for api-server with daemon option * Fix setproctitle on MacOS * Replace api/access_logfile with api/log_config * Fix [api/log_config] cli_config * Remove logging and unsed option in api_server_command - the logging before daemonization is not logging the correct PID so remove it - should_setup_logging is default as False * Fix test_run_command_daemon * Add significant rst
* Remove gunicorn daemonize for api-server * Rename _CommonCLIUvicornTestClass * Add test for api-server with daemon option * Fix setproctitle on MacOS * Replace api/access_logfile with api/log_config * Fix [api/log_config] cli_config * Remove logging and unsed option in api_server_command - the logging before daemonization is not logging the correct PID so remove it - should_setup_logging is default as False * Fix test_run_command_daemon * Add significant rst (cherry picked from commit ab1ee72)
* Remove gunicorn daemonize for api-server * Rename _CommonCLIUvicornTestClass * Add test for api-server with daemon option * Fix setproctitle on MacOS * Replace api/access_logfile with api/log_config * Fix [api/log_config] cli_config * Remove logging and unsed option in api_server_command - the logging before daemonization is not logging the correct PID so remove it - should_setup_logging is default as False * Fix test_run_command_daemon * Add significant rst (cherry picked from commit ab1ee72)
* [v3-0-test] Remove gunicorn daemonize for api-server (#52860) * Remove gunicorn daemonize for api-server * Rename _CommonCLIUvicornTestClass * Add test for api-server with daemon option * Fix setproctitle on MacOS * Replace api/access_logfile with api/log_config * Fix [api/log_config] cli_config * Remove logging and unsed option in api_server_command - the logging before daemonization is not logging the correct PID so remove it - should_setup_logging is default as False * Fix test_run_command_daemon * Add significant rst (cherry picked from commit ab1ee72) * Fix spelling wordlist * Fix configuration
| ("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"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not safe to do - they are different config options. As mentioned in the comment above, this will use the old access_logfile value for log_config if it's set, which isn't what we want.
| 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way we can continue to support a simple access logfile config? I just worry a bit about backcompat, right now this is a breaking change. Maybe we leave access_logfile, build our own log_config file dynamically, and make log_config and access_logfile mutually exclusive? And we should emit a deprecation warning when access_logfile is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, good and bad news. Good news is no backcompat issues to worry about! I believe it got broken in #45103. Bad news is I don't think access_log ever worked in AF3.
…52860) (apache#53372)" This reverts commit 7212c88.
related: #52581
Why
When I attempt to remove
gunicorndependency in #52581, I found that we are still usinggunicornfor daemonization.What
gunicorn.daemonizewithrun_command_with_daemon_option, this also align the behavior of--daemonoption for scheduler, triggerer ... and api-server--daemonas well_CommonCLIGunicornTestClassto_CommonCLIUvicornTestClass, since we are usinguvicorninstead ofgunicornfor api-server now.[api/access_logfile]config with[api/log_config], detail in Remove gunicorn daemonize for api-server #52860 (comment)