You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/11/15 17:12:22 UTC
[GitHub] [airflow] ashb opened a new pull request #19597: Fix `airflow db check-migrations`
ashb opened a new pull request #19597:
URL: https://github.com/apache/airflow/pull/19597
This command broke after [Define datetime and StringID column types centrally in migrations](#19408) was merged, but we didn't notice as Github styles timedout checks badly.
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-969366801
Shoudl we add some test for that one? It might be similarly broken in the future?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#discussion_r749946476
##########
File path: airflow/utils/db.py
##########
@@ -627,27 +627,32 @@ def check_migrations(timeout):
:param timeout: Timeout for the migration in seconds
:return: None
"""
- from alembic.runtime.migration import MigrationContext
+ from alembic.runtime.environment import EnvironmentContext
from alembic.script import ScriptDirectory
config = _get_alembic_config()
script_ = ScriptDirectory.from_config(config)
- with settings.engine.connect() as connection:
- context = MigrationContext.configure(connection)
- ticker = 0
- while True:
- source_heads = set(script_.get_heads())
- db_heads = set(context.get_current_heads())
- if source_heads == db_heads:
- break
- if ticker >= timeout:
- raise TimeoutError(
- f"There are still unapplied migrations after {ticker} seconds. "
- f"Migration Head(s) in DB: {db_heads} | Migration Head(s) in Source Code: {source_heads}"
- )
- ticker += 1
- time.sleep(1)
- log.info('Waiting for migrations... %s second(s)', ticker)
+ with EnvironmentContext(
+ config,
+ script_,
+ ) as env:
+ with settings.engine.connect() as connection:
Review comment:
```suggestion
with EnvironmentContext(
config,
script_,
) as env, settings.engine.connect() as connection:
```
Is this syntax available yet?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-972635255
> Helm Chart tests have been failing consistently for a while now.
Yeah. But this one was supposed to fix'em :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk edited a comment on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-972725321
Hmm. Looks like we are testing the right thing:
1) first we download the image ghcr.io/apache/airflow/main/prod/python3.6:ed09464667f7d83e25b4eb377839b873dbc5e925 - i pulled it and it contains latest changes from the PR:
~/.local/lib/python3.6/site-packages/airflow/utils/db.py:
```
def check_migrations(timeout):
"""
Function to wait for all airflow migrations to complete.
:param timeout: Timeout for the migration in seconds
:return: None
"""
from alembic.runtime.environment import EnvironmentContext
```
2) Then we use that image and create: `ghcr.io/apache/airflow/main/kubernetes/python3.6:latest` from it with embedded DAGs and kubernetes_executor_templates
3) then that image is loaded to kind:
```
kind load docker-image
--name airflow-python-3.6-v1.18.15 ghcr.io/apache/airflow/main/kubernetes/python3.6:latest
```
4) and it used by the helm installation:
```
helm install airflow . --timeout 10m0s --namespace airflow
--set defaultAirflowRepository=ghcr.io/apache/airflow/main/kubernetes/python3.6
--set images.airflow.repository=ghcr.io/apache/airflow/main/kubernetes/python3.6
--set images.airflow.tag=latest -v 1 --set defaultAirflowTag=latest -v 1
--set config.api.auth_backend=airflow.api.auth.backend.basic_auth
--set config.logging.logging_level=DEBUG --set executor=LocalExecutor
```
So everything there looks legit. The problem is that when webserver starts (for example) it raises this "migration" error.
I believe "check-migrations" was fixed by this PR (but it did not fix the problem). Previously we saw this error in output in "migration" container - but now we see similar error in webserw.
https://github.com/apache/airflow/actions/runs/1473732053 (see the artifacts - pick any of them and go to worker/containers/webserver logs).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-972776389
Should not be too difficult - we could check what was the command python was executed with :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-972767605
Whhhoooopsss yeah -- I think it was in Helm Chart as previous Airflow versions didn't have support for "airflow db wait-for-migrations" command
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb edited a comment on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
ashb edited a comment on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-972768671
Yeah, that's the problem. The helm chart isn't using that command.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-972743325
It's impossible - we do not even publish that image to ghcr.io - it's local-only name https://github.com/orgs/apache/packages?tab=packages&q=kubernetes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham closed pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
jedcunningham closed pull request #19597:
URL: https://github.com/apache/airflow/pull/19597
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-972561502
Helm Chart tests have been failing consistently for a while now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk edited a comment on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-972725321
Hmm. Looks like we are testing the right thing:
1) first we download the image ghcr.io/apache/airflow/main/prod/python3.6:ed09464667f7d83e25b4eb377839b873dbc5e925 - i pulled it and it contains latest changes from the PR:
~/.local/lib/python3.6/site-packages/airflow/utils/db.py:
```
def check_migrations(timeout):
"""
Function to wait for all airflow migrations to complete.
:param timeout: Timeout for the migration in seconds
:return: None
"""
from alembic.runtime.environment import EnvironmentContext
```
2) Then we use that image and create: `ghcr.io/apache/airflow/main/kubernetes/python3.6:latest` from it with embedded DAGs and kubernetes_executor_templates
3) then that image is loaded to kind:
```
kind load docker-image
--name airflow-python-3.6-v1.18.15 ghcr.io/apache/airflow/main/kubernetes/python3.6:latest
```
4) and it used by the helm installation:
```
helm install airflow . --timeout 10m0s --namespace airflow
--set defaultAirflowRepository=ghcr.io/apache/airflow/main/kubernetes/python3.6
--set images.airflow.repository=ghcr.io/apache/airflow/main/kubernetes/python3.6
--set images.airflow.tag=latest -v 1 --set defaultAirflowTag=latest -v 1
--set config.api.auth_backend=airflow.api.auth.backend.basic_auth
--set config.logging.logging_level=DEBUG --set executor=LocalExecutor
```
So everythin there looks legit. The problem is that when webserver starts (for example) it raises this "migration" error.
I believe "check-migrations" was fixed by this PR (but it did not fix the problem). Previously we saw this error in output in "migration" container - but now we see similar error in webserw.
https://github.com/apache/airflow/actions/runs/1473732053 (see the artifacts - pick any of them and go to worker/containers/webserver logs).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk edited a comment on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-972725321
Hmm. Looks like we are testing the right thing:
1) first we download the image ghcr.io/apache/airflow/main/prod/python3.6:ed09464667f7d83e25b4eb377839b873dbc5e925 - i pulled it and it contains latest changes from the PR:
~/.local/lib/python3.6/site-packages/airflow/utils/db.py:
```
def check_migrations(timeout):
"""
Function to wait for all airflow migrations to complete.
:param timeout: Timeout for the migration in seconds
:return: None
"""
from alembic.runtime.environment import EnvironmentContext
```
2) Then we use that image and create: `ghcr.io/apache/airflow/main/kubernetes/python3.6:latest` from it with embedded DAGs and kubernetes_executor_templates
3) then that image is loaded to kind:
```
kind load docker-image --name airflow-python-3.6-v1.18.15 ghcr.io/apache/airflow/main/kubernetes/python3.6:latest
```
4) and it used by the helm installation:
```
helm install airflow . --timeout 10m0s --namespace airflow --set defaultAirflowRepository=ghcr.io/apache/airflow/main/kubernetes/python3.6 --set images.airflow.repository=ghcr.io/apache/airflow/main/kubernetes/python3.6 --set images.airflow.tag=latest -v 1 --set defaultAirflowTag=latest -v 1 --set config.api.auth_backend=airflow.api.auth.backend.basic_auth --set config.logging.logging_level=DEBUG --set executor=LocalExecutor
```
So everythin there looks legit. The problem is that when webserver starts (for example) it raises this "migration" error.
I believe "check-migrations" was fixed by this PR (but it did not fix the problem). Previously we saw this error in output in "migration" container - but now we see similar error in webserw.
https://github.com/apache/airflow/actions/runs/1473732053 (see the artifacts - pick any of them and go to worker/containers/webserver logs).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-971869024
Looks like prod image building was broken by a failed runner or smth (and it was needed to test the Helm Chart). Closed/reopened to re-run it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-972773240
Not possible to make it backwards compatible somehow 😬 ? https://github.com/apache/airflow/pull/19408 is mainly just to centralize the IDs and types that are used in Migration
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-970130926
> Shoudl we add some test for that one? It might be similarly broken in the future?
Yes good call. Added a simple happy-path test for now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#discussion_r750086405
##########
File path: airflow/utils/db.py
##########
@@ -627,27 +627,32 @@ def check_migrations(timeout):
:param timeout: Timeout for the migration in seconds
:return: None
"""
- from alembic.runtime.migration import MigrationContext
+ from alembic.runtime.environment import EnvironmentContext
from alembic.script import ScriptDirectory
config = _get_alembic_config()
script_ = ScriptDirectory.from_config(config)
- with settings.engine.connect() as connection:
- context = MigrationContext.configure(connection)
- ticker = 0
- while True:
- source_heads = set(script_.get_heads())
- db_heads = set(context.get_current_heads())
- if source_heads == db_heads:
- break
- if ticker >= timeout:
- raise TimeoutError(
- f"There are still unapplied migrations after {ticker} seconds. "
- f"Migration Head(s) in DB: {db_heads} | Migration Head(s) in Source Code: {source_heads}"
- )
- ticker += 1
- time.sleep(1)
- log.info('Waiting for migrations... %s second(s)', ticker)
+ with EnvironmentContext(
+ config,
+ script_,
+ ) as env:
+ with settings.engine.connect() as connection:
Review comment:
Oh yes https://docs.python.org/3.6/reference/compound_stmts.html#the-with-statement
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-972708266
Let me take a close look
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-972739599
Ah yeah. You are right. My mistake - I looked at "containers" logs and missed the "wait-for-migration" suffix.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-972641054
I think one of the problem with CI we have that we are "accepting" such errors rathe than fix them. That leads to "broken window" policy - once tests are broken we ignore it and miss real problems.
For the future, whenever we see such failures logs of the kind/test containers are uploaded as artifacts of such failed build - they are visible at "summary" page when you scroll the page the bottom)
The problem seems to be still with the new alembic EnvironmentContext:
https://github.com/apache/airflow/actions/runs/1473732053 (see CeleryExecutor logs / webserver):
````
ation.py:[0m201} INFO[0m - Context impl PostgresqlImpl.[0m
2021-11-17T20:46:29.932473057Z stdout F [[34m2021-11-17 20:46:29,932[0m] {[34mmigration.py:[0m204} INFO[0m - Will assume transactional DDL.[0m
2021-11-17T20:46:29.945091208Z stderr F Traceback (most recent call last):
2021-11-17T20:46:29.945109248Z stderr F File "<string>", line 5, in get_bind
2021-11-17T20:46:29.94532407Z stderr F NameError: name '_proxy' is not defined
2021-11-17T20:46:29.94533169Z stderr F
2021-11-17T20:46:29.94533516Z stderr F The above exception was the direct cause of the following exception:
2021-11-17T20:46:29.94533839Z stderr F
2021-11-17T20:46:29.94534107Z stderr F Traceback (most recent call last):
2021-11-17T20:46:29.94534375Z stderr F File "<string>", line 25, in <module>
2021-11-17T20:46:29.945673083Z stderr F File "/home/airflow/.local/lib/python3.9/site-packages/alembic/script/base.py", line 391, in get_heads
2021-11-17T20:46:29.945833315Z stderr F return list(self.revision_map.heads)
2021-11-17T20:46:29.945841105Z stderr F File "/home/airflow/.local/lib/python3.9/site-packages/sqlalchemy/util/langhelpers.py", line 893, in __get__
2021-11-17T20:46:29.946260398Z stderr F obj.__dict__[self.__name__] = result = self.fget(obj)
2021-11-17T20:46:29.946266478Z stderr F File "/home/airflow/.local/lib/python3.9/site-packages/alembic/script/revision.py", line 135, in heads
2021-11-17T20:46:29.94640733Z stderr F self._revision_map
2021-11-17T20:46:29.94641463Z stderr F File "/home/airflow/.local/lib/python3.9/site-packages/sqlalchemy/util/langhelpers.py", line 893, in __get__
2021-11-17T20:46:29.946818553Z stderr F obj.__dict__[self.__name__] = result = self.fget(obj)
2021-11-17T20:46:29.946823733Z stderr F File "/home/airflow/.local/lib/python3.9/site-packages/alembic/script/revision.py", line 189, in _revision_map
2021-11-17T20:46:29.946974695Z stderr F for revision in self._generator():
2021-11-17T20:46:29.946981495Z stderr F File "/home/airflow/.local/lib/python3.9/site-packages/alembic/script/base.py", line 136, in _load_revisions
2021-11-17T20:46:29.947114956Z stderr F script = Script._from_filename(self, vers, file_)
2021-11-17T20:46:29.947121396Z stderr F File "/home/airflow/.local/lib/python3.9/site-packages/alembic/script/base.py", line 999, in _from_filename
2021-11-17T20:46:29.947665881Z stderr F module = util.load_python_file(dir_, filename)
2021-11-17T20:46:29.947672701Z stderr F File "/home/airflow/.local/lib/python3.9/site-packages/alembic/util/pyfiles.py", line 92, in load_python_file
2021-11-17T20:46:29.947803542Z stderr F module = load_module_py(module_id, path)
2021-11-17T20:46:29.947808422Z stderr F File "/home/airflow/.local/lib/python3.9/site-packages/alembic/util/pyfiles.py", line 108, in load_module_py
2021-11-17T20:46:29.947988114Z stderr F spec.loader.exec_module(module) # type: ignore
2021-11-17T20:46:29.947998674Z stderr F File "<frozen importlib._bootstrap_external>", line 850, in exec_module
2021-11-17T20:46:29.948138375Z stderr F File "<frozen importlib._bootstrap>", line 228, in _call_with_frames_removed
2021-11-17T20:46:29.948301086Z stderr F File "/home/airflow/.local/lib/python3.9/site-packages/airflow/migrations/versions/f2ca10b85618_add_dag_stats_table.py", line 29, in <module>
2021-11-17T20:46:29.948429508Z stderr F from airflow.migrations.db_types import StringID
2021-11-17T20:46:29.948436538Z stderr F File "/home/airflow/.local/lib/python3.9/site-packages/airflow/migrations/db_types.py", line 72, in __getattr__
2021-11-17T20:46:29.948610759Z stderr F dialect = context.get_bind().dialect.name
2021-11-17T20:46:29.948619249Z stderr F File "<string>", line 7, in get_bind
2021-11-17T20:46:29.94867927Z stderr F File "/home/airflow/.local/lib/python3.9/site-packages/alembic/util/langhelpers.py", line 96, in _name_error
2021-11-17T20:46:29.948850441Z stderr F raise NameError(
2021-11-17T20:46:29.948856321Z stderr F NameError: Can't invoke function 'get_bind', as the proxy object has not yet been established for the Alembic 'EnvironmentContext' class. Try placing this code inside a callable.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-972743663
```
helm install airflow . --timeout 10m0s --namespace airflow
--set defaultAirflowRepository=ghcr.io/apache/airflow/main/kubernetes/python3.6
--set images.airflow.repository=ghcr.io/apache/airflow/main/kubernetes/python3.6
--set images.airflow.tag=latest -v 1 --set defaultAirflowTag=latest -v 1
--set config.api.auth_backend=airflow.api.auth.backend.basic_auth
--set config.logging.logging_level=DEBUG --set executor=LocalExecutor
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-972740077
Ah, the "container" logs is output from all the containers in the webserver pod, but if you look under worker/pods/webserver you see the error is coming from the wait_for_migration container. (and that's the only container that is launched)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-972776947
And as of chart 1.4.0 we would do it "properly".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-972759799
Yeah. Reproduced it with latest PR merged too (same sequence as above):
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-972645360
Actually the same error seems to be printed in all containers - looks like the new StringID is being imported and failes because of no-proxy has been yet established in the Environment Context. It's quite a bit arcane sqlalchemy/alembic convolution.
It feels a bit strange tha alembic proxy is needed for runtime of airflow - is this really the right approach @ashb? Should we reconsider this change ? I have a feeling that you are the only one who knows what it does under the hood :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk edited a comment on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-972725321
Hmm. Looks like we are testing the right thing:
1) first we download the image ghcr.io/apache/airflow/main/prod/python3.6:ed09464667f7d83e25b4eb377839b873dbc5e925 - i pulled it and it contains latest changes from the PR:
~/.local/lib/python3.6/site-packages/airflow/utils/db.py:
```
def check_migrations(timeout):
"""
Function to wait for all airflow migrations to complete.
:param timeout: Timeout for the migration in seconds
:return: None
"""
from alembic.runtime.environment import EnvironmentContext
```
2) Then we use that image and create: `ghcr.io/apache/airflow/main/kubernetes/python3.6:latest` from it with embedded DAGs and kubernetes_executor_templates
3) then that image is loaded to kind:
```
kind load docker-image
--name airflow-python-3.6-v1.18.15 ghcr.io/apache/airflow/main/kubernetes/python3.6:latest
```
4) and it used by the helm installation:
```
helm install airflow . --timeout 10m0s --namespace airflow
--set defaultAirflowRepository=ghcr.io/apache/airflow/main/kubernetes/python3.6
--set images.airflow.repository=ghcr.io/apache/airflow/main/kubernetes/python3.6
--set images.airflow.tag=latest -v 1 --set defaultAirflowTag=latest -v 1
--set config.api.auth_backend=airflow.api.auth.backend.basic_auth
--set config.logging.logging_level=DEBUG --set executor=LocalExecutor
```
So everything there looks legit. The problem is that when webserver starts (for example) it raises this "migration" error.
I believe "check-migrations" was fixed by this PR (but it did not fix the problem). Previously we saw this error in output in "migration" container - but now we see similar error in webserver
https://github.com/apache/airflow/actions/runs/1473732053 (see the artifacts - pick any of them and go to worker/containers/webserver logs).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr merged pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
uranusjr merged pull request #19597:
URL: https://github.com/apache/airflow/pull/19597
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk edited a comment on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-972645360
Actually the same error seems to be printed in all containers - looks like the new StringID is being imported and fails because of the proxy has not yet been established in the EnvironmentContext. It's quite a bit arcane sqlalchemy/alembic convolution.
It feels a bit strange that alembic proxy is needed for runtime of airflow - is this really the right approach @ashb? Should we reconsider this change ? I have a feeling that you are the only one who knows what it does under the hood :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk edited a comment on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-972774579
Another option is smart workaround. For example what we could do:
in settings (`from airlfow import settings`) we could check if this import is run from "wait-for-db-migrations" container and if so - we could run the migration using the "check_migrations" and sys.exit(0).
Not the "cleanest" solution but should work.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-972775589
or even in "__init__.py" of "airflow" package - it is imported in the first line
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham closed pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
jedcunningham closed pull request #19597:
URL: https://github.com/apache/airflow/pull/19597
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-969257228
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#discussion_r750124443
##########
File path: airflow/utils/db.py
##########
@@ -627,27 +627,32 @@ def check_migrations(timeout):
:param timeout: Timeout for the migration in seconds
:return: None
"""
- from alembic.runtime.migration import MigrationContext
+ from alembic.runtime.environment import EnvironmentContext
from alembic.script import ScriptDirectory
config = _get_alembic_config()
script_ = ScriptDirectory.from_config(config)
- with settings.engine.connect() as connection:
- context = MigrationContext.configure(connection)
- ticker = 0
- while True:
- source_heads = set(script_.get_heads())
- db_heads = set(context.get_current_heads())
- if source_heads == db_heads:
- break
- if ticker >= timeout:
- raise TimeoutError(
- f"There are still unapplied migrations after {ticker} seconds. "
- f"Migration Head(s) in DB: {db_heads} | Migration Head(s) in Source Code: {source_heads}"
- )
- ticker += 1
- time.sleep(1)
- log.info('Waiting for migrations... %s second(s)', ticker)
+ with EnvironmentContext(
+ config,
+ script_,
+ ) as env:
+ with settings.engine.connect() as connection:
Review comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-972695519
@potiuk Do you know in which log file (inside the logs artifact) I need to look at to see exactly what image we have deployed in to the Kind clusters?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-972705373
I don't get what is going on here.
From https://github.com/apache/airflow/runs/4244820374?check_suite_focus=true#step:7:736
```
Step 1/3 : FROM ghcr.io/apache/airflow/main/prod/python3.6:ed09464667f7d83e25b4eb377839b873dbc5e925
---> e8c11daeadcc
Step 2/3 : COPY airflow/example_dags/ ${AIRFLOW_HOME}/dags/
---> e56206018bef
Step 3/3 : COPY airflow/kubernetes_executor_templates/ ${AIRFLOW_HOME}/pod_templates/
---> 7a7d6fd42db9
Successfully built 7a7d6fd42db9
Successfully tagged ghcr.io/apache/airflow/main/kubernetes/python3.6:latest
The ghcr.io/apache/airflow/main/kubernetes/python3.6:ed09464667f7d83e25b4eb377839b873dbc5e925 is prepared for test kubernetes deployment.
```
```
Status: Downloaded newer image for ghcr.io/apache/airflow/main/prod/python3.6:ed09464667f7d83e25b4eb377839b873dbc5e925
ghcr.io/apache/airflow/main/prod/python3.6:ed09464667f7d83e25b4eb377839b873dbc5e925
~/code/airflow/airflow/kind-logs/kind_logs_2021-11-17_1473732053_tests-kubernetes fix-db-check-migrations-command* 56s
❯ docker run --rm -ti --entrypoint bash ghcr.io/apache/airflow/main/prod/python3.6:ed09464667f7d83e25b4eb377839b873dbc5e925 [9:47] ✓ !10025
airflow@079a051c717c:/opt/airflow$ airflow db check-migrations
[2021-11-18 09:49:11,504] {migration.py:201} INFO - Context impl SQLiteImpl.
[2021-11-18 09:49:11,504] {migration.py:208} INFO - Will assume non-transactional DDL.
[2021-11-18 09:49:12,860] {providers_manager.py:280} WARNING - The package apache-airflow-providers-celery is not compatible with this version of Airflow. The package has version 2.1.0.dev0 but the minimum supported version of the package is 2.1.0
[2021-11-18 09:49:14,304] {providers_manager.py:148} WARNING - Exception when importing 'airflow.providers.google.leveldb.hooks.leveldb.LevelDBHook' from 'apache-airflow-providers-google' package: No module named 'plyvel'
[2021-11-18 09:49:15,167] {providers_manager.py:280} WARNING - The package apache-airflow-providers-celery is not compatible with this version of Airflow. The package has version 2.1.0.dev0 but the minimum supported version of the package is 2.1.0
[2021-11-18 09:49:15,169] {providers_manager.py:148} WARNING - Exception when importing 'airflow.providers.google.leveldb.hooks.leveldb.LevelDBHook' from 'apache-airflow-providers-google' package: No module named 'plyvel'
Traceback (most recent call last):
File "/home/airflow/.local/bin/airflow", line 8, in <module>
sys.exit(main())
File "/home/airflow/.local/lib/python3.6/site-packages/airflow/__main__.py", line 48, in main
args.func(args)
File "/home/airflow/.local/lib/python3.6/site-packages/airflow/cli/cli_parser.py", line 48, in command
return func(*args, **kwargs)
File "/home/airflow/.local/lib/python3.6/site-packages/airflow/cli/commands/db_command.py", line 54, in check_migrations
db.check_migrations(timeout=args.migration_wait_timeout)
File "/home/airflow/.local/lib/python3.6/site-packages/airflow/utils/db.py", line 649, in check_migrations
f"There are still unapplied migrations after {ticker} seconds. Migration"
TimeoutError: There are still unapplied migrations after 0 seconds. MigrationHead(s) in DB: set() | Migration Head(s) in Source Code: {'7b2661a43ba3'}
```
So using the base image that was built from, I don't see the error that _then_ appeared in the logs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-972772223
We could (somehow) say the Airflow 2.3 will need Helm Chart >= 1.4 I guess? Far from ideal.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-972768438
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-972685360
100% agreed about accepting broken tests -- it infuriates me!
The error should only show up when you try to import the migration files without activating an active MigrationContext, which is what this PR fixed. At least for me locally.
Let me pull these images and work out what isn't working right.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-972771653
Yeah. But the problem is for those who already use the chart
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-972769837
So eventually the Helm errors ARE real errors :). Happy we have the tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-972774579
Another option is smart workarounds (for example what we could do:
in settings (`from airlfow import settings`) we could check if this import is run from "wait-for-db-migrations" container and if so - we could run the migration using the "check_migrations" and sys.exit(0).
Not the "cleanest" solution but should work.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk edited a comment on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-972725321
Hmm. Looks like we are testing the right thing:
1) first we download the image ghcr.io/apache/airflow/main/prod/python3.6:ed09464667f7d83e25b4eb377839b873dbc5e925 - i pulled it and it contains latest changes from the PR:
~/.local/lib/python3.6/site-packages/airflow/utils/db.py:
```
def check_migrations(timeout):
"""
Function to wait for all airflow migrations to complete.
:param timeout: Timeout for the migration in seconds
:return: None
"""
from alembic.runtime.environment import EnvironmentContext
```
2) Then we use that image and create: `ghcr.io/apache/airflow/main/kubernetes/python3.6:latest` from it with embedded DAGs and kubernetes_executor_templates
3) then that image is loaded to kind:
```
kind load docker-image --name airflow-python-3.6-v1.18.15 ghcr.io/apache/airflow/main/kubernetes/python3.6:latest
```
4) and it used by the helm installation:
```
kind load docker-image --name airflow-python-3.6-v1.18.15 ghcr.io/apache/airflow/main/kubernetes/python3.6:latest
```
So everythin there looks legit. The problem is that when webserver starts (for example) it raises this "migration" error.
I believe "check-migrations" was fixed by this PR (but it did not fix the problem). Previously we saw this error in output in "migration" container - but now we see similar error in webserw.
https://github.com/apache/airflow/actions/runs/1473732053 (see the artifacts - pick any of them and go to worker/containers/webserver logs).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-972735606
```
airflow ❯ rg 'get_bind' -l [10:27] ✓ !10727
airflow-python-3.8-v1.20.2-worker/pods/airflow_airflow-triggerer-7f5d6957bb-hgm57_11da0220-bc2f-4d4b-b47e-2353d09bfeb6/wait-for-airflow-migrations/4.log
airflow-python-3.9-v1.19.7-worker/pods/airflow_airflow-webserver-79d88458f8-fn8n4_10d15550-4ca1-458b-986e-64e20de3c7ac/wait-for-airflow-migrations/4.log
airflow-python-3.9-v1.19.7-worker/pods/airflow_airflow-scheduler-0_f60074ff-e031-4fdb-81b3-989415953743/wait-for-airflow-migrations/4.log
airflow-python-3.9-v1.19.7-worker/pods/airflow_airflow-triggerer-57869c96c-qxfd7_8baf8168-b921-4183-8d06-fe637f49d8b3/wait-for-airflow-migrations/4.log
airflow-python-3.8-v1.20.2-worker/pods/airflow_airflow-scheduler-0_fdddc597-9a09-4c38-9031-d2229c173882/wait-for-airflow-migrations/4.log
airflow-python-3.8-v1.20.2-worker/pods/airflow_airflow-webserver-6d7c59c75-fj744_33821054-e45f-4bfa-aea6-a27c02293dac/wait-for-airflow-migrations/4.log
airflow-python-3.6-v1.18.15-worker/pods/airflow_airflow-webserver-969d886c-ptv97_f6a3e7c1-53ae-484d-90ff-37b36916d692/wait-for-airflow-migrations/4.log
airflow-python-3.6-v1.18.15-worker/pods/airflow_airflow-triggerer-76b78576f9-w5xkp_3cde75bf-e68a-41a9-be24-d700d8ce664f/wait-for-airflow-migrations/4.log
airflow-python-3.6-v1.18.15-worker/pods/airflow_airflow-scheduler-0_2cd00058-b5f2-4e58-959a-a242cf265b96/wait-for-airflow-migrations/4.log
```
I only see that error from the wait_for_migrations contianer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-972753907
My kind cluster is now working (https://kind.sigs.k8s.io/docs/user/known-issues/#failure-to-create-cluster-with-cgroups-v2 was my problem), also trying.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-972789172
And back-compat fix incoming.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-970373808
> Yes good call. Added a simple happy-path test for now.
That's exactly what I had in mind :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#discussion_r750125215
##########
File path: airflow/utils/db.py
##########
@@ -642,8 +646,8 @@ def check_migrations(timeout):
break
if ticker >= timeout:
raise TimeoutError(
- f"There are still unapplied migrations after {ticker} seconds. "
- f"Migration Head(s) in DB: {db_heads} | Migration Head(s) in Source Code: {source_heads}"
+ f"There are still unapplied migrations after {ticker} seconds. Migration"
+ f"Head(s) in DB: {db_heads} | Migration Head(s) in Source Code: {source_heads}"
Review comment:
```suggestion
f"There are still unapplied migrations after {ticker} seconds. "
f"Migration Head(s) in DB: {db_heads} | Migration Head(s) in Source Code: {source_heads}"
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-972747331
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-972947649
This is fixed in main now, and 2.3 won't need a particular version of the Helm chart either.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk edited a comment on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-972645360
Actually the same error seems to be printed in all containers - looks like the new StringID is being imported and failes because of no-proxy has been yet established in the Environment Context. It's quite a bit arcane sqlalchemy/alembic convolution.
It feels a bit strange that alembic proxy is needed for runtime of airflow - is this really the right approach @ashb? Should we reconsider this change ? I have a feeling that you are the only one who knows what it does under the hood :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-972725321
Hmm. Looks like we are testing the right thing:
1) first we download the image ghcr.io/apache/airflow/main/prod/python3.6:ed09464667f7d83e25b4eb377839b873dbc5e925 - i pulled it and it contains latest changes from the PR:
~/.local/lib/python3.6/site-packages/airflow/utils/db.py:
```
def check_migrations(timeout):
"""
Function to wait for all airflow migrations to complete.
:param timeout: Timeout for the migration in seconds
:return: None
"""
from alembic.runtime.environment import EnvironmentContext
```
2) Then we use that image and create: ghcr.io/apache/airflow/main/kubernetes/python3.6:latest from it with embedded DAGs and kubernetes_executor_templates
3) then that image is loaded to kind:
```
kind load docker-image --name airflow-python-3.6-v1.18.15 ghcr.io/apache/airflow/main/kubernetes/python3.6:latest
```
4) and it used by the helm installation:
```
kind load docker-image --name airflow-python-3.6-v1.18.15 ghcr.io/apache/airflow/main/kubernetes/python3.6:latest
```
So everythin there looks legit. The problem is that when webserver starts (for example) it raises this "migration" error.
I believe "check-migrations" was fixed by this PR (but it did not fix the problem). Previously we saw this error in output in "migration" container - but now we see similar error in webserw.
https://github.com/apache/airflow/actions/runs/1473732053 (see the artifacts - pick any of them and go to worker/containers/webserver logs).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-972771004
I think for now reverting the change is the only choice and it will be a breaking change for anyone using Helm < 1.4.0 even if we fix it there (and run airflow db wait-for-migrations for 2+)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb edited a comment on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
ashb edited a comment on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-972768671
Yeah, that's the problem. The helm chart isn't using that command.
5 mins too slow.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-972767122
Not the best idea to have it in the helm chart.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-972799462
https://github.com/apache/airflow/pull/19677
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham closed pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
jedcunningham closed pull request #19597:
URL: https://github.com/apache/airflow/pull/19597
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-972764692
AAAAAAHHHHH
```
{{ define "wait-for-migrations-command" }}
{{/* From Airflow 2.0.0 this can become [airflow, db, check-migrations] */}}
- python
- -c
- |
import airflow
import logging
import os
import time
from alembic.config import Config
from alembic.runtime.migration import MigrationContext
from alembic.script import ScriptDirectory
from airflow import settings
package_dir = os.path.abspath(os.path.dirname(airflow.__file__))
directory = os.path.join(package_dir, 'migrations')
config = Config(os.path.join(package_dir, 'alembic.ini'))
config.set_main_option('script_location', directory)
config.set_main_option('sqlalchemy.url', settings.SQL_ALCHEMY_CONN.replace('%', '%%'))
script_ = ScriptDirectory.from_config(config)
timeout=60
with settings.engine.connect() as connection:
context = MigrationContext.configure(connection)
ticker = 0
while True:
source_heads = set(script_.get_heads())
db_heads = set(context.get_current_heads())
if source_heads == db_heads:
break
if ticker >= timeout:
raise TimeoutError("There are still unapplied migrations after {} seconds.".format(ticker))
ticker += 1
time.sleep(1)
logging.info('Waiting for migrations... %s second(s)', ticker)
{{- end }}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-972746794
I am able to reproduce it easily locally with breeze:
1) `./breeze kind-cluster start`
2) `./breeze kind-cluster deploy`
it will fail eventually while waiting for webserver
3) `./breeze kind-cluster k9s`:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-972741401
Still, I can't quite work out how.
Is it possible that `kind load docker-image --name "${KIND_CLUSTER_NAME}" "${AIRFLOW_IMAGE_KUBERNETES}:latest"` is doing a remote load rather than loading from the local system? That's about the only way I could think that this would trigger the behaviour I see (but if that's the case I'd have thought we'd have seen bugs like this a long time ago)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk closed pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
potiuk closed pull request #19597:
URL: https://github.com/apache/airflow/pull/19597
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-972744137
Right -- I didn't think it was likely.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-972777831
Here is the "proper" fix anyway which should make tests go green https://github.com/apache/airflow/pull/19676
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-972761819
Confirmed I have the PR in the airflow installed in the image, so this is a "legit" error
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-972771171
Something like this will do the job:
```python
try:
from airflow.cli.commands.db_command import check_migrations
except ImportError:
# existing code
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk edited a comment on pull request #19597: Fix `airflow db check-migrations`
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #19597:
URL: https://github.com/apache/airflow/pull/19597#issuecomment-972641054
I think one of the problem with CI we have that we are "accepting" such errors rather than fixing them. That leads to "broken window" policy - once tests are broken we ignore it and miss real problems.
For the future, whenever we see such failures logs of the kind/test containers are uploaded as artifacts of such failed build - they are visible at "summary" page when you scroll the page the bottom)
The problem seems to be still with the new alembic EnvironmentContext:
https://github.com/apache/airflow/actions/runs/1473732053 (see CeleryExecutor logs / webserver):
````
ation.py:[0m201} INFO[0m - Context impl PostgresqlImpl.[0m
2021-11-17T20:46:29.932473057Z stdout F [[34m2021-11-17 20:46:29,932[0m] {[34mmigration.py:[0m204} INFO[0m - Will assume transactional DDL.[0m
2021-11-17T20:46:29.945091208Z stderr F Traceback (most recent call last):
2021-11-17T20:46:29.945109248Z stderr F File "<string>", line 5, in get_bind
2021-11-17T20:46:29.94532407Z stderr F NameError: name '_proxy' is not defined
2021-11-17T20:46:29.94533169Z stderr F
2021-11-17T20:46:29.94533516Z stderr F The above exception was the direct cause of the following exception:
2021-11-17T20:46:29.94533839Z stderr F
2021-11-17T20:46:29.94534107Z stderr F Traceback (most recent call last):
2021-11-17T20:46:29.94534375Z stderr F File "<string>", line 25, in <module>
2021-11-17T20:46:29.945673083Z stderr F File "/home/airflow/.local/lib/python3.9/site-packages/alembic/script/base.py", line 391, in get_heads
2021-11-17T20:46:29.945833315Z stderr F return list(self.revision_map.heads)
2021-11-17T20:46:29.945841105Z stderr F File "/home/airflow/.local/lib/python3.9/site-packages/sqlalchemy/util/langhelpers.py", line 893, in __get__
2021-11-17T20:46:29.946260398Z stderr F obj.__dict__[self.__name__] = result = self.fget(obj)
2021-11-17T20:46:29.946266478Z stderr F File "/home/airflow/.local/lib/python3.9/site-packages/alembic/script/revision.py", line 135, in heads
2021-11-17T20:46:29.94640733Z stderr F self._revision_map
2021-11-17T20:46:29.94641463Z stderr F File "/home/airflow/.local/lib/python3.9/site-packages/sqlalchemy/util/langhelpers.py", line 893, in __get__
2021-11-17T20:46:29.946818553Z stderr F obj.__dict__[self.__name__] = result = self.fget(obj)
2021-11-17T20:46:29.946823733Z stderr F File "/home/airflow/.local/lib/python3.9/site-packages/alembic/script/revision.py", line 189, in _revision_map
2021-11-17T20:46:29.946974695Z stderr F for revision in self._generator():
2021-11-17T20:46:29.946981495Z stderr F File "/home/airflow/.local/lib/python3.9/site-packages/alembic/script/base.py", line 136, in _load_revisions
2021-11-17T20:46:29.947114956Z stderr F script = Script._from_filename(self, vers, file_)
2021-11-17T20:46:29.947121396Z stderr F File "/home/airflow/.local/lib/python3.9/site-packages/alembic/script/base.py", line 999, in _from_filename
2021-11-17T20:46:29.947665881Z stderr F module = util.load_python_file(dir_, filename)
2021-11-17T20:46:29.947672701Z stderr F File "/home/airflow/.local/lib/python3.9/site-packages/alembic/util/pyfiles.py", line 92, in load_python_file
2021-11-17T20:46:29.947803542Z stderr F module = load_module_py(module_id, path)
2021-11-17T20:46:29.947808422Z stderr F File "/home/airflow/.local/lib/python3.9/site-packages/alembic/util/pyfiles.py", line 108, in load_module_py
2021-11-17T20:46:29.947988114Z stderr F spec.loader.exec_module(module) # type: ignore
2021-11-17T20:46:29.947998674Z stderr F File "<frozen importlib._bootstrap_external>", line 850, in exec_module
2021-11-17T20:46:29.948138375Z stderr F File "<frozen importlib._bootstrap>", line 228, in _call_with_frames_removed
2021-11-17T20:46:29.948301086Z stderr F File "/home/airflow/.local/lib/python3.9/site-packages/airflow/migrations/versions/f2ca10b85618_add_dag_stats_table.py", line 29, in <module>
2021-11-17T20:46:29.948429508Z stderr F from airflow.migrations.db_types import StringID
2021-11-17T20:46:29.948436538Z stderr F File "/home/airflow/.local/lib/python3.9/site-packages/airflow/migrations/db_types.py", line 72, in __getattr__
2021-11-17T20:46:29.948610759Z stderr F dialect = context.get_bind().dialect.name
2021-11-17T20:46:29.948619249Z stderr F File "<string>", line 7, in get_bind
2021-11-17T20:46:29.94867927Z stderr F File "/home/airflow/.local/lib/python3.9/site-packages/alembic/util/langhelpers.py", line 96, in _name_error
2021-11-17T20:46:29.948850441Z stderr F raise NameError(
2021-11-17T20:46:29.948856321Z stderr F NameError: Can't invoke function 'get_bind', as the proxy object has not yet been established for the Alembic 'EnvironmentContext' class. Try placing this code inside a callable.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org