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 2020/12/22 09:28:15 UTC
[GitHub] [airflow] dstandish opened a new pull request #13247: fix celery kubernetes executor bug preventing scheduler from starting
dstandish opened a new pull request #13247:
URL: https://github.com/apache/airflow/pull/13247
Before SchedulerJob starts executor it sets `self.executor.job_id = self.id`, where self.id is the primary key in the jobs table.
See here: https://github.com/apache/airflow/blob/master/airflow/jobs/scheduler_job.py#L1265.
So with KubernetesExecutor, when `start` is called, the executor has a value under `job_id`.
But with CeleryKubernetesExecutor enabled, the actual executors are under attributes on the CKE object, so they don't have the job ID, so when K8s executor tries to start, it fails because it doesn't find a job id.
This PR fixes this issue by propagating the job_id to the "child" executors.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #13247: Fix critical CeleryKubernetesExecutor bug
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #13247:
URL: https://github.com/apache/airflow/pull/13247#discussion_r570366452
##########
File path: airflow/cli/cli_parser.py
##########
@@ -58,9 +58,15 @@ class DefaultHelpParser(argparse.ArgumentParser):
def _check_value(self, action, value):
"""Override _check_value and check conditionally added command"""
executor = conf.get('core', 'EXECUTOR')
- if value == 'celery' and executor not in (CELERY_EXECUTOR, CELERY_KUBERNETES_EXECUTOR):
- message = f'celery subcommand works only with CeleryExecutor, your current executor: {executor}'
- raise ArgumentError(action, message)
+ if value == 'celery':
+ if executor != CELERY_EXECUTOR:
Review comment:
> we should at least take care of it when running "airflow celery worker" (a less hacky --- so that users don't need to have a different configuration for the Celery Worker.
another way to think about it @kaxil is, why does the celery worker even need _any_ executor configured?
the celery worker does what the celery worker does. it shouldn't really need to instantiate an executor at all i would think. shouldn't only the scheduler need to do that?
so while presumably we can fix this fork issue by making changes to CKE like you were doing in your local branch, another possible approach might be to make it so that the celery worker command somehow doesn't create / use / invoke an executor.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #13247: Fix critical CeleryKubernetesExecutor bug
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #13247:
URL: https://github.com/apache/airflow/pull/13247#discussion_r567066090
##########
File path: airflow/cli/cli_parser.py
##########
@@ -58,9 +58,15 @@ class DefaultHelpParser(argparse.ArgumentParser):
def _check_value(self, action, value):
"""Override _check_value and check conditionally added command"""
executor = conf.get('core', 'EXECUTOR')
- if value == 'celery' and executor not in (CELERY_EXECUTOR, CELERY_KUBERNETES_EXECUTOR):
- message = f'celery subcommand works only with CeleryExecutor, your current executor: {executor}'
- raise ArgumentError(action, message)
+ if value == 'celery':
+ if executor != CELERY_EXECUTOR:
Review comment:
i.e. if we don't run it via the Helm Chart, it should still work so I don't think this change is needed, apart from improving the error message
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #13247: Fix critical CeleryKubernetesExecutor bug
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13247:
URL: https://github.com/apache/airflow/pull/13247#issuecomment-751529729
[The Workflow run](https://github.com/apache/airflow/actions/runs/447739261) is cancelling this PR. Building images for the PR has failed. Follow the the workflow link to check the reason.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on pull request #13247: Fix critical CeleryKubernetesExecutor bug
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #13247:
URL: https://github.com/apache/airflow/pull/13247#issuecomment-768543566
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #13247: [WIP] fix celery kubernetes executor bug preventing scheduler from starting
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13247:
URL: https://github.com/apache/airflow/pull/13247#issuecomment-749792568
[The Workflow run](https://github.com/apache/airflow/actions/runs/438990722) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #13247: Fix critical CeleryKubernetesExecutor bug
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #13247:
URL: https://github.com/apache/airflow/pull/13247#issuecomment-827194453
> (Moved into the 2.0.3 milestone as it was never cherry picked into 2.0.1)
🤦 I just cherry-picked it to v2-0-test
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #13247: Fix critical CeleryKubernetesExecutor bug
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #13247:
URL: https://github.com/apache/airflow/pull/13247#discussion_r570366452
##########
File path: airflow/cli/cli_parser.py
##########
@@ -58,9 +58,15 @@ class DefaultHelpParser(argparse.ArgumentParser):
def _check_value(self, action, value):
"""Override _check_value and check conditionally added command"""
executor = conf.get('core', 'EXECUTOR')
- if value == 'celery' and executor not in (CELERY_EXECUTOR, CELERY_KUBERNETES_EXECUTOR):
- message = f'celery subcommand works only with CeleryExecutor, your current executor: {executor}'
- raise ArgumentError(action, message)
+ if value == 'celery':
+ if executor != CELERY_EXECUTOR:
Review comment:
> we should at least take care of it when running "airflow celery worker" (a less hacky --- so that users don't need to have a different configuration for the Celery Worker.
another way to think about it @kaxil is, why does the celery worker even need _any_ executor configured?
the celery worker does what the celery worker does. it shouldn't really need to instantiate an executor at all i would think. shouldn't only the scheduler need to do that?
so while presumably we can fix this fork issue by making changes to CKE like you were doing in your local branch, another possible approach might be to make it so that the celery worker command somehow doesn't create / use / invoke an executor at all.
update: this comment assumed that the executor was about how tasks are queued up, since i was thinking executor is just a concept for the scheduler.... and the the worker part would be separate code... but looks like the executor is used both in the worker and scheduler so never 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #13247: Fix critical CeleryKubernetesExecutor bug
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #13247:
URL: https://github.com/apache/airflow/pull/13247#discussion_r567927107
##########
File path: airflow/cli/cli_parser.py
##########
@@ -58,9 +58,15 @@ class DefaultHelpParser(argparse.ArgumentParser):
def _check_value(self, action, value):
"""Override _check_value and check conditionally added command"""
executor = conf.get('core', 'EXECUTOR')
- if value == 'celery' and executor not in (CELERY_EXECUTOR, CELERY_KUBERNETES_EXECUTOR):
- message = f'celery subcommand works only with CeleryExecutor, your current executor: {executor}'
- raise ArgumentError(action, message)
+ if value == 'celery':
+ if executor != CELERY_EXECUTOR:
Review comment:
See issue #13263
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #13247: [WIP] fix celery kubernetes executor bug preventing scheduler from starting
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13247:
URL: https://github.com/apache/airflow/pull/13247#issuecomment-749821263
[The Workflow run](https://github.com/apache/airflow/actions/runs/439098766) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #13247: Fix critical CeleryKubernetesExecutor bug
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #13247:
URL: https://github.com/apache/airflow/pull/13247#discussion_r567264843
##########
File path: airflow/cli/cli_parser.py
##########
@@ -58,9 +58,15 @@ class DefaultHelpParser(argparse.ArgumentParser):
def _check_value(self, action, value):
"""Override _check_value and check conditionally added command"""
executor = conf.get('core', 'EXECUTOR')
- if value == 'celery' and executor not in (CELERY_EXECUTOR, CELERY_KUBERNETES_EXECUTOR):
- message = f'celery subcommand works only with CeleryExecutor, your current executor: {executor}'
- raise ArgumentError(action, message)
+ if value == 'celery':
+ if executor != CELERY_EXECUTOR:
Review comment:
@kaxil the problem is that when executor==CELERY_KUBERNETES_EXECUTOR _on the celery worker_, the celery worker does not run properly.
you get some fork pool error
running `airflow celery worker` with `CELERY_EXECUTOR` instead resolves the issue
celery workers don't need to know that _the scheduler_ is using CKE
i know this is a bit hacky. our code should handle having all components set to use the same executor. however, that simply doesn't work right now, and this change (forcing the C workers to think CeleryExecutor is used) makes CKE work again immediately.
perhaps it makes sense to push out this hacky fix and then later look into why we get fork pool issues. but that's a question for you guys
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #13247: Fix critical CeleryKubernetesExecutor bug
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #13247:
URL: https://github.com/apache/airflow/pull/13247#discussion_r567264843
##########
File path: airflow/cli/cli_parser.py
##########
@@ -58,9 +58,15 @@ class DefaultHelpParser(argparse.ArgumentParser):
def _check_value(self, action, value):
"""Override _check_value and check conditionally added command"""
executor = conf.get('core', 'EXECUTOR')
- if value == 'celery' and executor not in (CELERY_EXECUTOR, CELERY_KUBERNETES_EXECUTOR):
- message = f'celery subcommand works only with CeleryExecutor, your current executor: {executor}'
- raise ArgumentError(action, message)
+ if value == 'celery':
+ if executor != CELERY_EXECUTOR:
Review comment:
@kaxil the problem is that when executor==CELERY_KUBERNETES_EXECUTOR, `airflow celery worker` does not work
you get some fork pool error
running `airflow celery worker` with `CELERY_EXECUTOR` instead resolves the issue
celery workers don't need to know that _the scheduler_ is using CKE
i know this is a bit hacky. our code should handle having all components set to use the same executor. however, that simply doesn't work right now, and this change (forcing the C workers to think CeleryExecutor is used) makes CKE work again immediately.
perhaps it makes sense to push out this hacky fix and then later look into why we get fork pool issues. but that's a question for you guys
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #13247: Fix critical CeleryKubernetesExecutor bug
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #13247:
URL: https://github.com/apache/airflow/pull/13247#discussion_r567922385
##########
File path: airflow/cli/cli_parser.py
##########
@@ -58,9 +58,15 @@ class DefaultHelpParser(argparse.ArgumentParser):
def _check_value(self, action, value):
"""Override _check_value and check conditionally added command"""
executor = conf.get('core', 'EXECUTOR')
- if value == 'celery' and executor not in (CELERY_EXECUTOR, CELERY_KUBERNETES_EXECUTOR):
- message = f'celery subcommand works only with CeleryExecutor, your current executor: {executor}'
- raise ArgumentError(action, message)
+ if value == 'celery':
+ if executor != CELERY_EXECUTOR:
Review comment:
Yea that one feels too hacky, we should at least take care of it when running "airflow celery worker" (a less hacky --- so that users don't need to have a different configuration for the Celery Worker.
Can you paste the full stack trace of the error please too -- want to see if I have seen that before
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #13247: Fix critical CeleryKubernetesExecutor bug
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #13247:
URL: https://github.com/apache/airflow/pull/13247#discussion_r552081114
##########
File path: airflow/cli/cli_parser.py
##########
@@ -30,7 +30,6 @@
from airflow.cli.commands.legacy_commands import check_legacy_command
from airflow.configuration import conf
from airflow.exceptions import AirflowException
-from airflow.executors.executor_constants import CELERY_EXECUTOR, CELERY_KUBERNETES_EXECUTOR
Review comment:
for context, in [a previous change](https://github.com/apache/airflow/commit/d752d92a4d8ef0dee93519397e76c1ecb0eda548#diff-521fc27e2c47783cd582bbe615daeba4b6d10eee86f7f923b72e5adbc15f8eb1), when i thought that CeleryKubernetesExecutor should be consistently set across the cluster, i had added one of them.
in _this_ pr though i was essentially reverting that change.
in my memory the constants were not use prior to my change, but in actuality, the constant was used prior to my prev change -- in the if statement but not in the logging. so it is actually a removal. and i will restore use of constant.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #13247: Fix critical CeleryKubernetesExecutor bug
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #13247:
URL: https://github.com/apache/airflow/pull/13247#discussion_r567264843
##########
File path: airflow/cli/cli_parser.py
##########
@@ -58,9 +58,15 @@ class DefaultHelpParser(argparse.ArgumentParser):
def _check_value(self, action, value):
"""Override _check_value and check conditionally added command"""
executor = conf.get('core', 'EXECUTOR')
- if value == 'celery' and executor not in (CELERY_EXECUTOR, CELERY_KUBERNETES_EXECUTOR):
- message = f'celery subcommand works only with CeleryExecutor, your current executor: {executor}'
- raise ArgumentError(action, message)
+ if value == 'celery':
+ if executor != CELERY_EXECUTOR:
Review comment:
@kaxil the problem is that when executor==CELERY_KUBERNETES_EXECUTOR _on the celery worker_, `airflow celery worker` does not work
you get some fork pool error
running `airflow celery worker` with `CELERY_EXECUTOR` instead resolves the issue
celery workers don't need to know that _the scheduler_ is using CKE
i know this is a bit hacky. our code should handle having all components set to use the same executor. however, that simply doesn't work right now, and this change (forcing the C workers to think CeleryExecutor is used) makes CKE work again immediately.
perhaps it makes sense to push out this hacky fix and then later look into why we get fork pool issues. but that's a question for you guys
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil edited a comment on pull request #13247: Fix critical CeleryKubernetesExecutor bug
Posted by GitBox <gi...@apache.org>.
kaxil edited a comment on pull request #13247:
URL: https://github.com/apache/airflow/pull/13247#issuecomment-767112588
@dstandish Can you rebase on the latest master and fix the conflicts?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #13247: Fix critical CeleryKubernetesExecutor bug
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #13247:
URL: https://github.com/apache/airflow/pull/13247#discussion_r570366452
##########
File path: airflow/cli/cli_parser.py
##########
@@ -58,9 +58,15 @@ class DefaultHelpParser(argparse.ArgumentParser):
def _check_value(self, action, value):
"""Override _check_value and check conditionally added command"""
executor = conf.get('core', 'EXECUTOR')
- if value == 'celery' and executor not in (CELERY_EXECUTOR, CELERY_KUBERNETES_EXECUTOR):
- message = f'celery subcommand works only with CeleryExecutor, your current executor: {executor}'
- raise ArgumentError(action, message)
+ if value == 'celery':
+ if executor != CELERY_EXECUTOR:
Review comment:
> we should at least take care of it when running "airflow celery worker" (a less hacky --- so that users don't need to have a different configuration for the Celery Worker.
another way to think about it @kaxil is, why does the celery worker even need _any_ executor configured?
the celery worker does what the celery worker does. it shouldn't really need to instantiate an executor at all i would think. shouldn't only the scheduler need to do that?
so while presumably we can fix this fork issue by making changes to CKE like you were doing in your local branch, another possible approach might be to make it so that the celery worker command somehow doesn't create / use / invoke an executor at all.
update: this comment assumed that the executor was about how tasks are queued up, since i was thinking executor is just a concept for the scheduler.... and the the worker part would be separate code... but looks like the executor is used both in the worker and scheduler so never 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #13247: Fix critical CeleryKubernetesExecutor bug
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13247:
URL: https://github.com/apache/airflow/pull/13247#issuecomment-768679375
[The Workflow run](https://github.com/apache/airflow/actions/runs/516668392) is cancelling this PR. Building images for the PR has failed. Follow the the workflow link to check the reason.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #13247: Fix critical CeleryKubernetesExecutor bug
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #13247:
URL: https://github.com/apache/airflow/pull/13247#discussion_r552081114
##########
File path: airflow/cli/cli_parser.py
##########
@@ -30,7 +30,6 @@
from airflow.cli.commands.legacy_commands import check_legacy_command
from airflow.configuration import conf
from airflow.exceptions import AirflowException
-from airflow.executors.executor_constants import CELERY_EXECUTOR, CELERY_KUBERNETES_EXECUTOR
Review comment:
for context, in [a previous change](https://github.com/apache/airflow/commit/d752d92a4d8ef0dee93519397e76c1ecb0eda548#diff-521fc27e2c47783cd582bbe615daeba4b6d10eee86f7f923b72e5adbc15f8eb1), when i thought that CeleryKubernetesExecutor should be consistently set across the cluster, i had added one of them.
in _this_ pr though i was essentially reverting that change and in my mind the constants were not use prior to that change. but in actuality, the constant _was_ used prior to my prev change -- in the `if` statement but not in the logging. so this _is_ actually a removal and not just a revert of a recent change.
i will restore use of constant.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #13247: WIP fix celery kubernetes executor bug preventing scheduler from starting
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13247:
URL: https://github.com/apache/airflow/pull/13247#issuecomment-749477092
[The Workflow run](https://github.com/apache/airflow/actions/runs/437841312) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #13247: Fix critical CeleryKubernetesExecutor bug
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #13247:
URL: https://github.com/apache/airflow/pull/13247#discussion_r570366452
##########
File path: airflow/cli/cli_parser.py
##########
@@ -58,9 +58,15 @@ class DefaultHelpParser(argparse.ArgumentParser):
def _check_value(self, action, value):
"""Override _check_value and check conditionally added command"""
executor = conf.get('core', 'EXECUTOR')
- if value == 'celery' and executor not in (CELERY_EXECUTOR, CELERY_KUBERNETES_EXECUTOR):
- message = f'celery subcommand works only with CeleryExecutor, your current executor: {executor}'
- raise ArgumentError(action, message)
+ if value == 'celery':
+ if executor != CELERY_EXECUTOR:
Review comment:
> we should at least take care of it when running "airflow celery worker" (a less hacky --- so that users don't need to have a different configuration for the Celery Worker.
another way to think about it @kaxil is, why does the celery worker even need _any_ executor configured?
the celery worker does what the celery worker does. it shouldn't really need to instantiate an executor at all i would think. shouldn't only the scheduler need to do that?
so while presumably we can fix this fork issue by making changes to CKE like you were doing in your local branch, another possible approach might be to make it so that the celery worker command somehow doesn't create / use / invoke an executor.
##########
File path: airflow/cli/cli_parser.py
##########
@@ -58,9 +58,15 @@ class DefaultHelpParser(argparse.ArgumentParser):
def _check_value(self, action, value):
"""Override _check_value and check conditionally added command"""
executor = conf.get('core', 'EXECUTOR')
- if value == 'celery' and executor not in (CELERY_EXECUTOR, CELERY_KUBERNETES_EXECUTOR):
- message = f'celery subcommand works only with CeleryExecutor, your current executor: {executor}'
- raise ArgumentError(action, message)
+ if value == 'celery':
+ if executor != CELERY_EXECUTOR:
Review comment:
> we should at least take care of it when running "airflow celery worker" (a less hacky --- so that users don't need to have a different configuration for the Celery Worker.
another way to think about it @kaxil is, why does the celery worker even need _any_ executor configured?
the celery worker does what the celery worker does. it shouldn't really need to instantiate an executor at all i would think. shouldn't only the scheduler need to do that?
so while presumably we can fix this fork issue by making changes to CKE like you were doing in your local branch, another possible approach might be to make it so that the celery worker command somehow doesn't create / use / invoke an executor at all.
##########
File path: airflow/cli/cli_parser.py
##########
@@ -58,9 +58,15 @@ class DefaultHelpParser(argparse.ArgumentParser):
def _check_value(self, action, value):
"""Override _check_value and check conditionally added command"""
executor = conf.get('core', 'EXECUTOR')
- if value == 'celery' and executor not in (CELERY_EXECUTOR, CELERY_KUBERNETES_EXECUTOR):
- message = f'celery subcommand works only with CeleryExecutor, your current executor: {executor}'
- raise ArgumentError(action, message)
+ if value == 'celery':
+ if executor != CELERY_EXECUTOR:
Review comment:
> we should at least take care of it when running "airflow celery worker" (a less hacky --- so that users don't need to have a different configuration for the Celery Worker.
another way to think about it @kaxil is, why does the celery worker even need _any_ executor configured?
the celery worker does what the celery worker does. it shouldn't really need to instantiate an executor at all i would think. shouldn't only the scheduler need to do that?
so while presumably we can fix this fork issue by making changes to CKE like you were doing in your local branch, another possible approach might be to make it so that the celery worker command somehow doesn't create / use / invoke an executor at all.
update: this comment assumed that the executor was about how tasks are queued up, since i was thinking executor is just a concept for the scheduler.... and the the worker part would be separate code... but looks like the executor is used both in the worker and scheduler so never mind...
##########
File path: airflow/cli/cli_parser.py
##########
@@ -58,9 +58,15 @@ class DefaultHelpParser(argparse.ArgumentParser):
def _check_value(self, action, value):
"""Override _check_value and check conditionally added command"""
executor = conf.get('core', 'EXECUTOR')
- if value == 'celery' and executor not in (CELERY_EXECUTOR, CELERY_KUBERNETES_EXECUTOR):
- message = f'celery subcommand works only with CeleryExecutor, your current executor: {executor}'
- raise ArgumentError(action, message)
+ if value == 'celery':
+ if executor != CELERY_EXECUTOR:
Review comment:
> we should at least take care of it when running "airflow celery worker" (a less hacky --- so that users don't need to have a different configuration for the Celery Worker.
another way to think about it @kaxil is, why does the celery worker even need _any_ executor configured?
the celery worker does what the celery worker does. it shouldn't really need to instantiate an executor at all i would think. shouldn't only the scheduler need to do that?
so while presumably we can fix this fork issue by making changes to CKE like you were doing in your local branch, another possible approach might be to make it so that the celery worker command somehow doesn't create / use / invoke an executor at all.
update: this comment assumed that the executor was about how tasks are queued up, since i was thinking executor is just a concept for the scheduler.... and the the worker part would be separate code... but looks like the executor is used both in the worker and scheduler so never 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on pull request #13247: Fix critical CeleryKubernetesExecutor bug
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #13247:
URL: https://github.com/apache/airflow/pull/13247#issuecomment-768441765
Will asap.
Have new family member, bit hectic, but soon!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #13247: Fix critical CeleryKubernetesExecutor bug
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #13247:
URL: https://github.com/apache/airflow/pull/13247#discussion_r570366452
##########
File path: airflow/cli/cli_parser.py
##########
@@ -58,9 +58,15 @@ class DefaultHelpParser(argparse.ArgumentParser):
def _check_value(self, action, value):
"""Override _check_value and check conditionally added command"""
executor = conf.get('core', 'EXECUTOR')
- if value == 'celery' and executor not in (CELERY_EXECUTOR, CELERY_KUBERNETES_EXECUTOR):
- message = f'celery subcommand works only with CeleryExecutor, your current executor: {executor}'
- raise ArgumentError(action, message)
+ if value == 'celery':
+ if executor != CELERY_EXECUTOR:
Review comment:
> we should at least take care of it when running "airflow celery worker" (a less hacky --- so that users don't need to have a different configuration for the Celery Worker.
another way to think about it @kaxil is, why does the celery worker even need _any_ executor configured?
the celery worker does what the celery worker does. it shouldn't really need to instantiate an executor at all i would think. shouldn't only the scheduler need to do that?
so while presumably we can fix this fork issue by making changes to CKE like you were doing in your local branch, another possible approach might be to make it so that the celery worker command somehow doesn't create / use / invoke an executor at all.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil merged pull request #13247: Fix critical CeleryKubernetesExecutor bug
Posted by GitBox <gi...@apache.org>.
kaxil merged pull request #13247:
URL: https://github.com/apache/airflow/pull/13247
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #13247: Fix critical CeleryKubernetesExecutor bug
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #13247:
URL: https://github.com/apache/airflow/pull/13247#issuecomment-767112588
@dstandish Can you rebase on the latest master, please to fix the conflicts?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #13247: Fix critical CeleryKubernetesExecutor bug
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #13247:
URL: https://github.com/apache/airflow/pull/13247#discussion_r552073531
##########
File path: airflow/cli/cli_parser.py
##########
@@ -30,7 +30,6 @@
from airflow.cli.commands.legacy_commands import check_legacy_command
from airflow.configuration import conf
from airflow.exceptions import AirflowException
-from airflow.executors.executor_constants import CELERY_EXECUTOR, CELERY_KUBERNETES_EXECUTOR
Review comment:
not a particularly good reason, will add back
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #13247: Fix critical CeleryKubernetesExecutor bug
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #13247:
URL: https://github.com/apache/airflow/pull/13247#discussion_r552081114
##########
File path: airflow/cli/cli_parser.py
##########
@@ -30,7 +30,6 @@
from airflow.cli.commands.legacy_commands import check_legacy_command
from airflow.configuration import conf
from airflow.exceptions import AirflowException
-from airflow.executors.executor_constants import CELERY_EXECUTOR, CELERY_KUBERNETES_EXECUTOR
Review comment:
for context, in [a previous change](https://github.com/apache/airflow/commit/d752d92a4d8ef0dee93519397e76c1ecb0eda548#diff-521fc27e2c47783cd582bbe615daeba4b6d10eee86f7f923b72e5adbc15f8eb1), when i thought that CeleryKubernetesExecutor should be consistently set across the cluster, i had added one of them.
in _this_ pr though i was essentially reverting that change.
n my memory the constants were not use prior to my change (but in actuality, the constant was used prior to my prev change -- in the if statement but not in the logging).
`CeleryExecutor` will almost certainly never change. and even if it were to, i think you can make a case that the using the constant arguably impairs readability and _searchability_. there are so many hardcoded references to CeleryExecutor in docs and in code -- you won't be able to get away with just changing the constant.
in any case, i'll use the constant :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #13247: Fix critical CeleryKubernetesExecutor bug
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13247:
URL: https://github.com/apache/airflow/pull/13247#issuecomment-768601843
[The Workflow run](https://github.com/apache/airflow/actions/runs/516247430) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on pull request #13247: Fix critical CeleryKubernetesExecutor bug
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on pull request #13247:
URL: https://github.com/apache/airflow/pull/13247#issuecomment-827192687
(Moved into the 2.0.3 milestone as it was never cherry picked into 2.0.1)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #13247: Fix critical CeleryKubernetesExecutor bug
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13247:
URL: https://github.com/apache/airflow/pull/13247#issuecomment-768649215
[The Workflow run](https://github.com/apache/airflow/actions/runs/516470496) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #13247: Fix critical CeleryKubernetesExecutor bug
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #13247:
URL: https://github.com/apache/airflow/pull/13247#discussion_r551847915
##########
File path: airflow/cli/cli_parser.py
##########
@@ -30,7 +30,6 @@
from airflow.cli.commands.legacy_commands import check_legacy_command
from airflow.configuration import conf
from airflow.exceptions import AirflowException
-from airflow.executors.executor_constants import CELERY_EXECUTOR, CELERY_KUBERNETES_EXECUTOR
Review comment:
Why did you stop using these constants?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #13247: [WIP] fix celery kubernetes executor bug preventing scheduler from starting
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13247:
URL: https://github.com/apache/airflow/pull/13247#issuecomment-749792379
[The Workflow run](https://github.com/apache/airflow/actions/runs/438987142) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #13247: Fix critical CeleryKubernetesExecutor bug
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13247:
URL: https://github.com/apache/airflow/pull/13247#issuecomment-768649520
[The Workflow run](https://github.com/apache/airflow/actions/runs/516480672) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #13247: Fix critical CeleryKubernetesExecutor bug
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #13247:
URL: https://github.com/apache/airflow/pull/13247#issuecomment-767112588
@dstandish Can you rebase on the latest master, please to fix the conflicts?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #13247: Fix critical CeleryKubernetesExecutor bug
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #13247:
URL: https://github.com/apache/airflow/pull/13247#discussion_r567065401
##########
File path: airflow/cli/cli_parser.py
##########
@@ -58,9 +58,15 @@ class DefaultHelpParser(argparse.ArgumentParser):
def _check_value(self, action, value):
"""Override _check_value and check conditionally added command"""
executor = conf.get('core', 'EXECUTOR')
- if value == 'celery' and executor not in (CELERY_EXECUTOR, CELERY_KUBERNETES_EXECUTOR):
- message = f'celery subcommand works only with CeleryExecutor, your current executor: {executor}'
- raise ArgumentError(action, message)
+ if value == 'celery':
+ if executor != CELERY_EXECUTOR:
Review comment:
This does not cover `CELERY_KUBERNETES_EXECUTOR` though, 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #13247: Fix critical CeleryKubernetesExecutor bug
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13247:
URL: https://github.com/apache/airflow/pull/13247#issuecomment-751509459
[The Workflow run](https://github.com/apache/airflow/actions/runs/447584162) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #13247: Fix fatal celery kubernetes executor bug preventing scheduler from starting
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13247:
URL: https://github.com/apache/airflow/pull/13247#issuecomment-750949216
[The Workflow run](https://github.com/apache/airflow/actions/runs/442876055) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org