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/01/10 21:33:19 UTC
[GitHub] [airflow] subkanthi opened a new pull request #20794: Add priority class
subkanthi opened a new pull request #20794:
URL: https://github.com/apache/airflow/pull/20794
closes: #20632
Added support for priorityClassName in flower, webserver, pgbouncer, scheduler, statsd and triggerer and workers.
<!--
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] jedcunningham commented on a change in pull request #20794: Helm Chart - support for priorityClassName
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #20794:
URL: https://github.com/apache/airflow/pull/20794#discussion_r785063195
##########
File path: chart/templates/flower/flower-deployment.yaml
##########
@@ -67,6 +67,7 @@ spec:
tolerations:
{{ toYaml $tolerations | indent 8 }}
serviceAccountName: {{ include "flower.serviceAccountName" . }}
+ priorityClassName: {{ .Values.flower.priorityClassName }}
Review comment:
I think we should conditionally add these:
```suggestion
{{- if .Values.flower.priorityClassName }}
priorityClassName: {{ .Values.flower.priorityClassName }}
{{- end }}
```
##########
File path: chart/values.yaml
##########
@@ -514,6 +514,7 @@ workers:
# Select certain nodes for airflow worker pods.
nodeSelector: {}
+ priorityClassName: ""
Review comment:
Yeah 👍
--
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 #20794: Helm Chart - support for priorityClassName
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #20794:
URL: https://github.com/apache/airflow/pull/20794#discussion_r785013493
##########
File path: chart/values.yaml
##########
@@ -514,6 +514,7 @@ workers:
# Select certain nodes for airflow worker pods.
nodeSelector: {}
+ priorityClassName: ""
Review comment:
```suggestion
priorityClassName: ~
```
to be consistent, preference @jedcunningham ?
--
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] subkanthi commented on a change in pull request #20794: Helm Chart - support for priorityClassName
Posted by GitBox <gi...@apache.org>.
subkanthi commented on a change in pull request #20794:
URL: https://github.com/apache/airflow/pull/20794#discussion_r793236928
##########
File path: chart/templates/flower/flower-deployment.yaml
##########
@@ -67,6 +67,7 @@ spec:
tolerations:
{{ toYaml $tolerations | indent 8 }}
serviceAccountName: {{ include "flower.serviceAccountName" . }}
+ priorityClassName: {{ .Values.flower.priorityClassName }}
Review comment:
Changed.
--
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 #20794: Helm Chart - support for priorityClassName
Posted by GitBox <gi...@apache.org>.
jedcunningham merged pull request #20794:
URL: https://github.com/apache/airflow/pull/20794
--
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] subkanthi commented on pull request #20794: Helm Chart - support for priorityClassName
Posted by GitBox <gi...@apache.org>.
subkanthi commented on pull request #20794:
URL: https://github.com/apache/airflow/pull/20794#issuecomment-1019387546
> @subkanthi ?
Thanks Jarek, will get to it in a day or two.
--
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 #20794: Helm Chart - support for priorityClassName
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #20794:
URL: https://github.com/apache/airflow/pull/20794#issuecomment-1019378636
@subkanthi ?
--
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] subkanthi commented on a change in pull request #20794: Helm Chart - support for priorityClassName
Posted by GitBox <gi...@apache.org>.
subkanthi commented on a change in pull request #20794:
URL: https://github.com/apache/airflow/pull/20794#discussion_r794950836
##########
File path: chart/tests/test_airflow_common.py
##########
@@ -268,3 +268,46 @@ def test_have_all_config_mounts_on_init_containers(self):
}
for doc in docs:
assert expected_mount in jmespath.search("spec.template.spec.initContainers[0].volumeMounts", doc)
+
+ def test_priority_class_name(self):
+ docs = render_chart(
+ values={
+ "flower": {"priorityClassName": "low-priority-flower"},
+ "pgbouncer": {"priorityClassName": "low-priority-pgbouncer"},
+ "scheduler": {"priorityClassName": "low-priority-scheduler"},
+ "statsd": {"priorityClassName": "low-priority-statsd"},
+ "triggerer": {"priorityClassName": "low-priority-triggerer"},
+ "webserver": {"priorityClassName": "low-priority-webserver"},
+ "workers": {"priorityClassName": "low-priority-worker"},
+ },
+ show_only=[
+ "templates/flower/flower-deployment.yaml",
+ "templates/pgbouncer/pgbouncer-deployment.yaml",
+ "templates/scheduler/scheduler-deployment.yaml",
+ "templates/statsd/statsd-deployment.yaml",
+ "templates/triggerer/triggerer-deployment.yaml",
+ "templates/webserver/webserver-deployment.yaml",
+ "templates/workers/worker-deployment.yaml",
+ ],
+ )
+
+ for doc in docs:
+ component = doc['metadata']['labels']['component']
+ priority = doc['spec']['template']['spec']['priorityClassName']
+
+ if component == "flower":
+ assert priority == "low-priority-flower"
+ elif component == "pgbouncer":
+ assert priority == "low-priority-pgbouncer"
+ elif component == "scheduler":
+ assert priority == "low-priority-scheduler"
+ elif component == "statsd":
+ assert priority == "low-priority-statsd"
+ elif component == "triggerer":
+ assert priority == "low-priority-triggerer"
+ elif component == "webserver":
+ assert priority == "low-priority-webserver"
+ elif component == "worker":
+ assert priority == "low-priority-worker"
+ else:
+ assert False
Review comment:
Thanks, thought about this, but didnt realize it also covers the 'assert False condition.
--
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 change in pull request #20794: Helm Chart - support for priorityClassName
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #20794:
URL: https://github.com/apache/airflow/pull/20794#discussion_r793733937
##########
File path: chart/templates/pgbouncer/pgbouncer-deployment.yaml
##########
@@ -59,6 +59,7 @@ spec:
checksum/pgbouncer-config-secret: {{ include (print $.Template.BasePath "/secrets/pgbouncer-config-secret.yaml") . | sha256sum }}
checksum/pgbouncer-certificates-secret: {{ include (print $.Template.BasePath "/secrets/pgbouncer-certificates-secret.yaml") . | sha256sum }}
spec:
+ priorityClassName: {{ .Values.pgbouncer.priorityClassName }}
Review comment:
I meant all instances of it should be conditional, not just flower :)
##########
File path: chart/values.yaml
##########
@@ -514,6 +514,7 @@ workers:
# Select certain nodes for airflow worker pods.
nodeSelector: {}
+ priorityClassName: ""
Review comment:
You just need to change the type in the schema, e.g: https://github.com/apache/airflow/blob/0ac3b8c3dd749c59e60cf0169580b9e7c5049d9e/chart/values.schema.json#L431-L434
--
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] subkanthi commented on a change in pull request #20794: Helm Chart - support for priorityClassName
Posted by GitBox <gi...@apache.org>.
subkanthi commented on a change in pull request #20794:
URL: https://github.com/apache/airflow/pull/20794#discussion_r793759521
##########
File path: chart/templates/pgbouncer/pgbouncer-deployment.yaml
##########
@@ -59,6 +59,7 @@ spec:
checksum/pgbouncer-config-secret: {{ include (print $.Template.BasePath "/secrets/pgbouncer-config-secret.yaml") . | sha256sum }}
checksum/pgbouncer-certificates-secret: {{ include (print $.Template.BasePath "/secrets/pgbouncer-certificates-secret.yaml") . | sha256sum }}
spec:
+ priorityClassName: {{ .Values.pgbouncer.priorityClassName }}
Review comment:
sure, will make those changes.
--
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] subkanthi commented on a change in pull request #20794: Helm Chart - support for priorityClassName
Posted by GitBox <gi...@apache.org>.
subkanthi commented on a change in pull request #20794:
URL: https://github.com/apache/airflow/pull/20794#discussion_r793236222
##########
File path: chart/values.yaml
##########
@@ -514,6 +514,7 @@ workers:
# Select certain nodes for airflow worker pods.
nodeSelector: {}
+ priorityClassName: ""
Review comment:
@jedcunningham , if I have it as tilde(~), then the validation fails, thoughts
```
Processing file: chart/values.yaml
None is not of type 'string'
Failed validating 'type' in schema['properties']['workers']['properties']['priorityClassName']:
{'default': '',
'description': 'Set priority value from the priority class name',
'type': 'string'}
On instance['workers']['priorityClassName']:
None
None is not equal to the default of
Failed validating 'default' in schema['properties']['workers']['properties']['priorityClassName']:
{'default': '',
'description': 'Set priority value from the priority class name',
'type': 'string'}
On instance['workers']['priorityClassName']:
None
```
--
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] subkanthi commented on a change in pull request #20794: Helm Chart - support for priorityClassName
Posted by GitBox <gi...@apache.org>.
subkanthi commented on a change in pull request #20794:
URL: https://github.com/apache/airflow/pull/20794#discussion_r793759521
##########
File path: chart/templates/pgbouncer/pgbouncer-deployment.yaml
##########
@@ -59,6 +59,7 @@ spec:
checksum/pgbouncer-config-secret: {{ include (print $.Template.BasePath "/secrets/pgbouncer-config-secret.yaml") . | sha256sum }}
checksum/pgbouncer-certificates-secret: {{ include (print $.Template.BasePath "/secrets/pgbouncer-certificates-secret.yaml") . | sha256sum }}
spec:
+ priorityClassName: {{ .Values.pgbouncer.priorityClassName }}
Review comment:
sure, not done yet, will make those changes.
--
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] subkanthi commented on a change in pull request #20794: Helm Chart - support for priorityClassName
Posted by GitBox <gi...@apache.org>.
subkanthi commented on a change in pull request #20794:
URL: https://github.com/apache/airflow/pull/20794#discussion_r799580496
##########
File path: chart/values.yaml
##########
@@ -638,6 +639,10 @@ scheduler:
# weight: 100
tolerations: []
+ # Set the priority class name
+ # https://kubernetes.io/docs/concepts/scheduling-eviction/pod-priority-preemption/#pod-priority
Review comment:
Removed.
##########
File path: chart/values.schema.json
##########
@@ -1351,6 +1351,14 @@
"type": "string"
}
},
+ "priorityClassName": {
+ "description": "Set priority value from the priority class name",
Review comment:
Updated all of them.
--
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 change in pull request #20794: Helm Chart - support for priorityClassName
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #20794:
URL: https://github.com/apache/airflow/pull/20794#discussion_r794930640
##########
File path: chart/values.schema.json
##########
@@ -1351,6 +1351,14 @@
"type": "string"
}
},
+ "priorityClassName": {
+ "description": "Set priority value from the priority class name",
Review comment:
```suggestion
"description": "Specify priority for worker pods.",
```
This reads better to me?
##########
File path: chart/tests/test_airflow_common.py
##########
@@ -268,3 +268,46 @@ def test_have_all_config_mounts_on_init_containers(self):
}
for doc in docs:
assert expected_mount in jmespath.search("spec.template.spec.initContainers[0].volumeMounts", doc)
+
+ def test_priority_class_name(self):
+ docs = render_chart(
+ values={
+ "flower": {"priorityClassName": "low-priority-flower"},
+ "pgbouncer": {"priorityClassName": "low-priority-pgbouncer"},
+ "scheduler": {"priorityClassName": "low-priority-scheduler"},
+ "statsd": {"priorityClassName": "low-priority-statsd"},
+ "triggerer": {"priorityClassName": "low-priority-triggerer"},
+ "webserver": {"priorityClassName": "low-priority-webserver"},
+ "workers": {"priorityClassName": "low-priority-worker"},
+ },
+ show_only=[
+ "templates/flower/flower-deployment.yaml",
+ "templates/pgbouncer/pgbouncer-deployment.yaml",
+ "templates/scheduler/scheduler-deployment.yaml",
+ "templates/statsd/statsd-deployment.yaml",
+ "templates/triggerer/triggerer-deployment.yaml",
+ "templates/webserver/webserver-deployment.yaml",
+ "templates/workers/worker-deployment.yaml",
+ ],
+ )
+
+ for doc in docs:
+ component = doc['metadata']['labels']['component']
+ priority = doc['spec']['template']['spec']['priorityClassName']
+
+ if component == "flower":
+ assert priority == "low-priority-flower"
+ elif component == "pgbouncer":
+ assert priority == "low-priority-pgbouncer"
+ elif component == "scheduler":
+ assert priority == "low-priority-scheduler"
+ elif component == "statsd":
+ assert priority == "low-priority-statsd"
+ elif component == "triggerer":
+ assert priority == "low-priority-triggerer"
+ elif component == "webserver":
+ assert priority == "low-priority-webserver"
+ elif component == "worker":
+ assert priority == "low-priority-worker"
+ else:
+ assert False
Review comment:
Maybe this instead?
```suggestion
assert priority == f"low-priority-{component}"
```
##########
File path: chart/values.yaml
##########
@@ -638,6 +639,10 @@ scheduler:
# weight: 100
tolerations: []
+ # Set the priority class name
+ # https://kubernetes.io/docs/concepts/scheduling-eviction/pod-priority-preemption/#pod-priority
Review comment:
```suggestion
```
I don't think we need these.
--
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 change in pull request #20794: Helm Chart - support for priorityClassName
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #20794:
URL: https://github.com/apache/airflow/pull/20794#discussion_r794957680
##########
File path: chart/values.schema.json
##########
@@ -1351,6 +1351,14 @@
"type": "string"
}
},
+ "priorityClassName": {
+ "description": "Set priority value from the priority class name",
Review comment:
I just suggested it once, but this applies to all of them too.
##########
File path: chart/values.yaml
##########
@@ -638,6 +639,10 @@ scheduler:
# weight: 100
tolerations: []
+ # Set the priority class name
+ # https://kubernetes.io/docs/concepts/scheduling-eviction/pod-priority-preemption/#pod-priority
Review comment:
Same here.
--
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