Skip to content

Allow Variable.get_variable_from_secrets to reuse session - #72121

Closed
saitejabandaru-in wants to merge 1 commit into
apache:mainfrom
saitejabandaru-in:fix-71801-variable-session
Closed

Allow Variable.get_variable_from_secrets to reuse session#72121
saitejabandaru-in wants to merge 1 commit into
apache:mainfrom
saitejabandaru-in:fix-71801-variable-session

Conversation

@saitejabandaru-in

Copy link
Copy Markdown

Closes#71801.

Motivation

When resolving VariableInterval deadlines (such as in _process_dagrun_deadline_alerts), the scheduler operates under prohibit_commit. However, querying MetastoreBackend.get_variable through Variable.get_variable_from_secrets does not pass down the session, which triggers @provide_session to create a scoped session that automatically commits upon exiting, thus tripping UNEXPECTED COMMIT guards.

Because of this, callers had to duplicate the secrets backend iteration loop directly in dag.py (see #68917) instead of utilizing get_variable_from_secrets.

Changes

  • Add session=None keyword-only argument to Variable.get_variable_from_secrets.
  • Explicitly check if the secrets_backend is MetastoreBackend, and if session is provided, forward it in the call_secrets_backend_method arguments.
  • This allows the caller's session to be reused across the metadata DB lookup without triggering unexpected commits, clearing the way for deduplicating the dag.py loop.

This adds an optional keyword-only `session=None` argument to `Variable.get_variable_from_secrets` and forwards it to `MetastoreBackend`. This allows callers running under `prohibit_commit` (like the scheduler) to resolve variables from the secrets chain without tripping unexpected commits via `@provide_session`, enabling deduplication of secrets backend iteration loops in `_process_dagrun_deadline_alerts`.\n\nCloses: apache#71801
@vincbeck

Copy link
Copy Markdown
Contributor

This is already being worked on here: #72121

@ferruzzi

Copy link
Copy Markdown
Contributor

I believe Vincent meant to link to #71968, but yes, the fix for this is already in review.

For what it's worth, if I were to review this as it sits now:

  • we would need unit tests for these changes,
  • this wouldn't have passed the formatting static checks or mypy from what I can tell,
  • you are also not actually passing session in to get_variable_from_secrets anywhere, so this wouldn't actually fix the bug.

@saitejabandaru-in

Copy link
Copy Markdown
Author

Thanks for the feedback! Since a fix is already in review in #71968, I will close this PR to avoid duplication. Appreciate your time!

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow Variable.get_variable_from_secrets to reuse a caller's session

4 participants

@saitejabandaru-in@vincbeck@ferruzzi@saitejabandaru