Skip to content
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

Fix 2.7.0 db migration job errors #33652

Merged
merged 2 commits into from
Aug 23, 2023
Merged

Conversation

LipuFei
Copy link
Contributor

@LipuFei LipuFei commented Aug 23, 2023

Related to #33651

Fixes 2 issues when running Airflow 2.7.0 python 3.11 on Kubernetes with CeleryKubernetesExecutor and Sentry.

  • A circular import from kubernetes_executor.py
  • Do not validate SQL when Sentry is initialising. It's still in the configuration phase.

I've tested this on my Kubernetes cluster and it works.

The only doubt I have is the validate=False change. I'm not sure if this is the intended use.

@ephraimbuddy ephraimbuddy added this to the Airflow 2.7.1 milestone Aug 23, 2023
@ephraimbuddy ephraimbuddy added the type:bug-fix Changelog: Bug Fixes label Aug 23, 2023
@potiuk
Copy link
Member

potiuk commented Aug 23, 2023

Nice catch !

@potiuk potiuk merged commit 4bdf908 into apache:main Aug 23, 2023
47 checks passed
@ephraimbuddy
Copy link
Contributor

How do we prevent this from happening again? I think there's a bug somewhere because it was made to work when imported directly. See

# Things to lazy import in form {local_name: ('target_module', 'target_name')}
__lazy_imports: dict[str, tuple[str, str]] = {
"DAG": (".models.dag", "DAG"),
"Dataset": (".datasets", "Dataset"),
"XComArg": (".models.xcom_arg", "XComArg"),
"AirflowException": (".exceptions", "AirflowException"),
"version": (".version", ""),
}

@potiuk
Copy link
Member

potiuk commented Aug 23, 2023

I think it's difficult to prevent - or detect upfront. This was fairly specific to having sentry integration enabled. The only way I can think of is to run our "test migration" job in CI to run will all possible options (like sentry) enabled. All options that can potentially follow different "flow" of imports.

Unfortunately the way how our configuration/imporrs are done does not make it easy to avoid/detect.

@LipuFei LipuFei deleted the bugfix/db-migration-job branch August 23, 2023 16:15
@gil-tober
Copy link

@LipuFei
Does this also closes #31442?

@LipuFei
Copy link
Contributor Author

LipuFei commented Aug 24, 2023

@LipuFei
Does this also closes #31442?

Yes, I will close #31442.

Sorry, thought it was my ticket... This doesn't close #31442.

@ephraimbuddy
Copy link
Contributor

I think it's difficult to prevent - or detect upfront. This was fairly specific to having sentry integration enabled. The only way I can think of is to run our "test migration" job in CI to run will all possible options (like sentry) enabled. All options that can potentially follow different "flow" of imports.

Unfortunately the way how our configuration/imporrs are done does not make it easy to avoid/detect.

I was thinking that the import should work given that we implemented a lazy import for AirflowException so that if it's import directly it works as seen here

# Things to lazy import in form {local_name: ('target_module', 'target_name')}
__lazy_imports: dict[str, tuple[str, str]] = {
"DAG": (".models.dag", "DAG"),
"Dataset": (".datasets", "Dataset"),
"XComArg": (".models.xcom_arg", "XComArg"),
"AirflowException": (".exceptions", "AirflowException"),
"version": (".version", ""),
}
, same way we import DAG directly.

ephraimbuddy pushed a commit that referenced this pull request Aug 28, 2023
* Fix circular import from kubernetes_executor

* Do not validate SQL when initialising Sentry

---------

Co-authored-by: Lipu Fei <[email protected]>
(cherry picked from commit 4bdf908)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers provider:cncf-kubernetes Kubernetes provider related issues type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants