-
Notifications
You must be signed in to change notification settings - Fork 16.4k
Fix connection resolution in CLI by setting server process context in decorators #61036
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
base: main
Are you sure you want to change the base?
Conversation
|
Could you please verify if this approach aligns with the project's long-term architectural goals regarding component isolation and database access? I believe this change is consistent with our future direction for the following reasons:
Your feedback on whether this centralization is the preferred way to handle CLI context would be greatly appreciated. |
7a9160e to
f9184ba
Compare
|
I think we are going to get rid of Some of the cli methods are shared between Maybe we can merge it as a temporary measure, but I don't see it as even mid-term sustainable approach. |
|
Thanks, @potiuk! You summarised it pretty clearly. We will have deprecations on the CLI and will start this pretty soon. In CLI, there will be isolation work on remote commands defined in AIP-81. Deprecation and migration are on the way. Airflow CLI will mainly be used for always server-side triggers and actions rather than anything we can safely provide from the API. The voting will start later today or tomorrow. There are incoming dev call, and I wanted to align the voting so we can maybe go over it all together with everyone in the call to see if there are any concerns which most of the part already agreed on in I would be on the safe side and won't put much effort into something we won't maintain eventually and not needed. I really like any improvement, but some can create more maintenance effort than we put into solving the actual problem that won't be there after 3-5 minor release according to how fast we can migrate, but I believe we have a good foundation to work on over CI/CD and more 🙌 For the TaskSDK side, @amoghrajesh can have a clearer answer |
|
Thanks @potiuk! Yeah we want to get rid of using that env variable entirely in long term but for dag processor and such, it will be dependent also on AIP92 (entirely decoupling dag processor and triggerer from core and having API first communication for those too). So this would be a good temporary solution until then |
|
The logic looks right to me, but I don’t like how a decorator magically modifies the global state (an environment variable) that leaks out after it exits. If we’re to do this, the environment varialbe needs to be restored when the context ends. |
ae74302 to
d3455f9
Compare
|
@uranusjr Thanks for the feedback. I've updated the logic to ensure the environment variable is restored when the context ends, avoiding any global state leakage. I appreciate the suggestion! |
…ly block for CLI decorators
d3455f9 to
fbbbc2a
Compare






Related Issues
Issue: #61033
Summary
This PR ensures that Airflow CLI commands are executed with the
serverprocess context by default.This is achieved by setting the
_AIRFLOW_PROCESS_CONTEXTenvironment variable to"server"within the core CLI decorators (action_cliandsuppress_logs_and_warning).Rationale
Previously, commands like
airflow dag-processor -B <bundle_name>failed with anAirflowNotFoundExceptionwhen initializing bundles that require database connections (e.g.,GitDagBundle).The failure occurred because:
-Bflag triggers bundle validation (validate_dag_bundle_argin_create_dag_processor_job_runner) during the command initialization phase.airflow/airflow-core/src/airflow/cli/commands/dag_processor_command.py
Lines 48 to 54 in 0cf89fa
airflow/airflow-core/src/airflow/cli/commands/dag_processor_command.py
Lines 35 to 45 in 0cf89fa
airflow/airflow-core/src/airflow/utils/cli.py
Lines 466 to 472 in 0cf89fa
servercontext was not yet set, causing the Task SDK to skipMetastoreBackendand fail to resolve connections from the metadata database.By moving the context setting to the decorator level, we ensure that all CLI management commands have the necessary privileges to access database-backed secrets before any validation or execution logic runs.
Key Changes
airflow/utils/cli.py:_AIRFLOW_PROCESS_CONTEXT = "server"inaction_clidecorator whencheck_dbis True._AIRFLOW_PROCESS_CONTEXT = "server"insuppress_logs_and_warningdecorator.airflow/cli/commands/connection_command.py: Removed redundant manual environment settings inconnections_getandconnections_test.Test plan
airflow dag-processor -B <bundle_name>.AirflowNotFoundException.airflow connections get) to ensure no regressions.Was generative AI tooling used to co-author this PR?
{pr_number}.significant.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.