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 2022/08/11 21:16:56 UTC
[GitHub] [airflow] danielhoherd opened a new pull request, #25684: Add 'executor' label to airflow scheduler deployment
danielhoherd opened a new pull request, #25684:
URL: https://github.com/apache/airflow/pull/25684
Add 'executor' label to airflow scheduler deployment. This allows system operators to filter airflow scheduler deployments by their executor using something like `kubectl get deployments -A -l 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] jedcunningham commented on a diff in pull request #25684: Add 'executor' label to airflow scheduler deployment
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on code in PR #25684:
URL: https://github.com/apache/airflow/pull/25684#discussion_r943948597
##########
tests/charts/test_airflow_common.py:
##########
@@ -350,3 +350,24 @@ def test_priority_class_name(self):
priority = doc['spec']['template']['spec']['priorityClassName']
assert priority == f"low-priority-{component}"
+
+ @pytest.mark.parametrize(
+ "executor",
+ [
+ ("LocalExecutor"),
+ ("LocalKubernetesExecutor"),
+ ("CeleryExecutor"),
+ ("KubernetesExecutor"),
+ ("CeleryKubernetesExecutor"),
+ ],
+ )
+ def test_scheduler_deployment_has_executor_label(self, executor):
Review Comment:
Can you put this in `test_scheduler.py` instead?
##########
tests/charts/test_airflow_common.py:
##########
@@ -350,3 +350,24 @@ def test_priority_class_name(self):
priority = doc['spec']['template']['spec']['priorityClassName']
assert priority == f"low-priority-{component}"
+
+ @pytest.mark.parametrize(
+ "executor",
+ [
+ ("LocalExecutor"),
+ ("LocalKubernetesExecutor"),
+ ("CeleryExecutor"),
+ ("KubernetesExecutor"),
+ ("CeleryKubernetesExecutor"),
+ ],
+ )
+ def test_scheduler_deployment_has_executor_label(self, executor):
+ docs = render_chart(
+ values={"executor": executor},
+ show_only=[
+ "templates/scheduler/scheduler-deployment.yaml",
+ ],
Review Comment:
```suggestion
show_only=["templates/scheduler/scheduler-deployment.yaml"],
```
nit
--
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] danielhoherd commented on pull request #25684: Add 'executor' label to airflow scheduler deployment
Posted by GitBox <gi...@apache.org>.
danielhoherd commented on PR #25684:
URL: https://github.com/apache/airflow/pull/25684#issuecomment-1265449200
@pingzh @jedcunningham Any further thoughts on this?
When I think about this. the question seems to be: what is the executor a property of? If we think about the namespace as being The Airflow Deployment, then it makes sense there. If we think about the executor being a property of the scheduler that is scheduling the execution of processes, the label makes sense there. I don't know the answer and I don't have strong feelings one way or the other. I'm just trying to get this issue moving again.
--
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 commented on pull request #25684: Add 'executor' label to airflow scheduler deployment
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on PR #25684:
URL: https://github.com/apache/airflow/pull/25684#issuecomment-1265800068
Let's keep it as-is for now. @danielhoherd, can you fix the 2 test failures?
--
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 #25684: Add 'executor' label to airflow scheduler deployment
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #25684:
URL: https://github.com/apache/airflow/pull/25684#issuecomment-1264773532
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.
--
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 commented on pull request #25684: Add 'executor' label to airflow scheduler deployment
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on PR #25684:
URL: https://github.com/apache/airflow/pull/25684#issuecomment-1213373168
I don't have any objections to namespacing it, though I think that we should consider doing that in a more all encompassing change (e.g. what's set on KPO/KubernetesExecutor worker too).
--
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 commented on a diff in pull request #25684: Add 'executor' label to airflow scheduler deployment
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on code in PR #25684:
URL: https://github.com/apache/airflow/pull/25684#discussion_r986067370
##########
tests/charts/test_scheduler.py:
##########
@@ -674,6 +674,25 @@ def test_persistence_volume_annotations(self):
)
assert {"foo": "bar"} == jmespath.search("spec.volumeClaimTemplates[0].metadata.annotations", docs[0])
+ @parameterized.expand(
+ "executor",
+ [
+ ("LocalExecutor"),
+ ("LocalKubernetesExecutor"),
+ ("CeleryExecutor"),
+ ("KubernetesExecutor"),
+ ("CeleryKubernetesExecutor"),
+ ],
Review Comment:
```suggestion
[
"LocalExecutor",
"LocalKubernetesExecutor",
"CeleryExecutor",
"KubernetesExecutor",
"CeleryKubernetesExecutor",
]
```
This will fix one of them at least.
--
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 merged pull request #25684: Add 'executor' label to airflow scheduler deployment
Posted by GitBox <gi...@apache.org>.
jedcunningham merged PR #25684:
URL: https://github.com/apache/airflow/pull/25684
--
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