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/12/06 11:37:40 UTC
[GitHub] [airflow] ephraimbuddy opened a new pull request #20069: Add migration-wait-timeout to the helm chart
ephraimbuddy opened a new pull request #20069:
URL: https://github.com/apache/airflow/pull/20069
The `airflow db check-migrations` command is missing a migration wait timeout
value in the helm chart resulting in unexpected failures at times.
This PR adds a default timeout to the command
Closes:
---
**^ 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] ephraimbuddy merged pull request #20069: Add migration-wait-timeout to the helm chart
Posted by GitBox <gi...@apache.org>.
ephraimbuddy merged pull request #20069:
URL: https://github.com/apache/airflow/pull/20069
--
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] ephraimbuddy commented on pull request #20069: Add migration-wait-timeout to the helm chart
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on pull request #20069:
URL: https://github.com/apache/airflow/pull/20069#issuecomment-986767778
> Ah I see. But I hope #18439 could be also merged, that would help A LOT in a meaningful message to the user if the timeout actually happens. Or is timeout equivalent with "error" of the wait-for migration init container andr results with "error" ?
This, #18439 sadly doesn't get hit due to the TimeoutError as I observed. I'm taking a look at fixing 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] github-actions[bot] commented on pull request #20069: Add migration-wait-timeout to the helm chart
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #20069:
URL: https://github.com/apache/airflow/pull/20069#issuecomment-986825665
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main 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] potiuk edited a comment on pull request #20069: Add migration-wait-timeout to the helm chart
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #20069:
URL: https://github.com/apache/airflow/pull/20069#issuecomment-986744395
Question - what woudl be the cases where timing out on migration is "wrong" ? Wouls not that leave the DB in a state that is not expected ? However I think that one is fine to merge as long as we merge https://github.com/apache/airflow/pull/18439 - because then instead of "hanging" migration you'd get hard failure (which is much better).
Is that the idea behind 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 a change in pull request #20069: Add migration-wait-timeout to the helm chart
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #20069:
URL: https://github.com/apache/airflow/pull/20069#discussion_r763204846
##########
File path: chart/values.yaml
##########
@@ -56,6 +56,8 @@ images:
# will use the images from 'defaultAirflowRepository:defaultAirflowTag' values
# to run and wait for DB migrations .
useDefaultImageForMigration: false
+ # timeout for airflow-migrations to complete
Review comment:
```suggestion
# timeout (in seconds) for airflow-migrations to complete
```
##########
File path: chart/values.schema.json
##########
@@ -372,6 +372,12 @@
"x-docsSection": "Images",
"default": false
},
+ "migrationsWaitTimeout": {
+ "description": "The time to wait for the DB migrations to complete.",
Review comment:
```suggestion
"description": "The time (in seconds) to wait for the DB migrations to complete.",
```
--
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 #20069: Add migration-wait-timeout to the helm chart
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #20069:
URL: https://github.com/apache/airflow/pull/20069#issuecomment-986756000
Ah I see. But I hope #18439 could be also merged, that would help A LOT in a meaningful message to the user if the timeout actually happens. Or is timeout equivalent with "error" of the wait-for migration init container andr results with "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] ephraimbuddy commented on a change in pull request #20069: Add migration-wait-timeout to the helm chart
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #20069:
URL: https://github.com/apache/airflow/pull/20069#discussion_r762930605
##########
File path: chart/tests/conftest.py
##########
@@ -22,7 +22,7 @@
from filelock import FileLock
-@pytest.fixture(autouse=True, scope="session")
+@pytest.fixture(autouse=True, scope="module")
Review comment:
```suggestion
@pytest.fixture(autouse=True, scope="session")
```
--
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] ephraimbuddy edited a comment on pull request #20069: Add migration-wait-timeout to the helm chart
Posted by GitBox <gi...@apache.org>.
ephraimbuddy edited a comment on pull request #20069:
URL: https://github.com/apache/airflow/pull/20069#issuecomment-986752938
> Question - what woudl be the cases where timing out on migration is "wrong" ? Wouls not that leave the DB in a state that is not expected ? However I think that one is fine to merge as long as we merge #18439 - because then instead of "hanging" migration you'd get hard failure (which is much better).
>
> Is that the idea behind it ?
Actually, there was a timeout in the helm chart until this change: https://github.com/apache/airflow/commit/bced2d750fad420a1f9e6dd1cf95641e9fce79e5 (which is not released yet). So now, it doesn't wait for migration like before.
I added default=60 in the cli_parser but I can remove it if we want. I feel there would be no harm since it's just for checking migrations and would exit once the check is alright
--
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] ephraimbuddy commented on a change in pull request #20069: Add migration-wait-timeout to the helm chart
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #20069:
URL: https://github.com/apache/airflow/pull/20069#discussion_r762930605
##########
File path: chart/tests/conftest.py
##########
@@ -22,7 +22,7 @@
from filelock import FileLock
-@pytest.fixture(autouse=True, scope="session")
+@pytest.fixture(autouse=True, scope="module")
Review comment:
```suggestion
@pytest.fixture(autouse=True, scope="session")
```
mistakenly changed this
--
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 #20069: Add migration-wait-timeout to the helm chart
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #20069:
URL: https://github.com/apache/airflow/pull/20069#issuecomment-986744395
Question - what woudl be the cases where timing out on migration is "wrong" ? Woudl not that leave the DB in a state that is not expected ? However I think that one is fine to merge as long as we merge https://github.com/apache/airflow/pull/18439 - because then instead of "hanging" migration you'd get hard failure (which is much better).
Is that the idea behind 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] ephraimbuddy commented on pull request #20069: Add migration-wait-timeout to the helm chart
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on pull request #20069:
URL: https://github.com/apache/airflow/pull/20069#issuecomment-986752938
> Question - what woudl be the cases where timing out on migration is "wrong" ? Wouls not that leave the DB in a state that is not expected ? However I think that one is fine to merge as long as we merge #18439 - because then instead of "hanging" migration you'd get hard failure (which is much better).
>
> Is that the idea behind it ?
Actually, there was a timeout in the helm chart until this change: https://github.com/apache/airflow/commit/bced2d750fad420a1f9e6dd1cf95641e9fce79e5(which is not released yet). So now, it doesn't wait for migration like before.
I added default=60 in the cli_parser but I can remove it if we want. I feel there would be no harm since it's just for checking migrations and would exit once the check is alright
--
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 #20069: Add migration-wait-timeout to the helm chart
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #20069:
URL: https://github.com/apache/airflow/pull/20069#issuecomment-986756000
Ah I see. But I hopw #18439 could be also merged, that would help A LOT in a meaningful message to the user if the timeout actually happens. Or is timeout equivalent with "error" of the wait-for migration init container andr results with "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