You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "LipuFei (via GitHub)" <gi...@apache.org> on 2024/08/16 08:57:16 UTC
[PR] Chart: Allow hybrid executors [airflow]
LipuFei opened a new pull request, #41524:
URL: https://github.com/apache/airflow/pull/41524
Airflow 2.10.0 has the new feature to use multiple executors. The value schema check in the current Helm chart doesn't allow this because it has a strict list of allowed values. I removed this enum check list to enable custom and combinations of executors.
Another change is the `executor` label in the scheduler Deployment. Because Kubernetes allows max 63 characters for label values, and hybrid executor values can easily exceed this limit, therefore I added a `trunc -63` to prevent this. This value has no functional use AFAIK, so doing a truncate here will not cause any functional problems.
--
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
Re: [PR] Chart: Allow hybrid executors [airflow]
Posted by "LipuFei (via GitHub)" <gi...@apache.org>.
LipuFei commented on code in PR #41524:
URL: https://github.com/apache/airflow/pull/41524#discussion_r1722812751
##########
chart/values.schema.json:
##########
@@ -516,16 +516,7 @@
"description": "Airflow executor.",
"type": "string",
"x-docsSection": "Common",
- "default": "CeleryExecutor",
- "enum": [
- "LocalExecutor",
- "LocalKubernetesExecutor",
- "CeleryExecutor",
- "KubernetesExecutor",
- "CeleryKubernetesExecutor",
- "airflow.providers.amazon.aws.executors.batch.AwsBatchExecutor",
- "airflow.providers.amazon.aws.executors.ecs.AwsEcsExecutor"
- ]
+ "default": "CeleryExecutor"
Review Comment:
What if we keep this as a list of enums? Then, in the value, we concatenate them with `,` into a comma separated list? Downside is, this is still a fixed list of executors. You cannot specify custom executors or use short names such as `airflow.providers.amazon.aws.executors.ecs.AwsEcsExecutor:AwsEcsExecutor`.
--
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
Re: [PR] Chart: Allow hybrid executors [airflow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #41524: Chart: Allow hybrid executors
URL: https://github.com/apache/airflow/pull/41524
--
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
Re: [PR] Chart: Allow hybrid executors [airflow]
Posted by "LipuFei (via GitHub)" <gi...@apache.org>.
LipuFei commented on PR #41524:
URL: https://github.com/apache/airflow/pull/41524#issuecomment-2306476685
> We have a ton of conditionals like [if CeleryExecutor](https://github.com/apache/airflow/blob/09ce3a5100c266369350c85e9f9a0f72ecca9e98/chart/templates/workers/worker-deployment.yaml#L26) so we can provision the right stuff for the release, but you've not accounted for any of that with this change. Try it with `CeleryExecutor,KubernetesExecutor`, for example, to see the problems this brings.
>
> I think @hussein-awala idea of an array of executors makes sense, but I agree the shortname/enum thing is definitely a problem. We will need to come up with something here.
I agree. After some digging, I indeed see a lot of references to `.Values.executor` everywhere, not to mention the `executor` references to `.Values.config.core.executor` and they need to be the same...
I will work on this slowly, excluding the short name support. It's a lot more complex than I initially expected.
--
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
Re: [PR] Chart: Allow hybrid executors [airflow]
Posted by "LipuFei (via GitHub)" <gi...@apache.org>.
LipuFei commented on code in PR #41524:
URL: https://github.com/apache/airflow/pull/41524#discussion_r1722816043
##########
chart/templates/scheduler/scheduler-deployment.yaml:
##########
@@ -53,7 +53,6 @@ metadata:
release: {{ .Release.Name }}
chart: "{{ .Chart.Name }}-{{ .Chart.Version }}"
heritage: {{ .Release.Service }}
- executor: {{ .Values.executor }}
Review Comment:
In my humble opinion, sanitizing this value is a little too much work (complexity) for something that has no functional use. I'd prefer to remove it then making it too complex...
--
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
Re: [PR] Chart: Allow hybrid executors [airflow]
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #41524:
URL: https://github.com/apache/airflow/pull/41524#discussion_r1719804506
##########
chart/values.schema.json:
##########
@@ -516,16 +516,7 @@
"description": "Airflow executor.",
"type": "string",
"x-docsSection": "Common",
- "default": "CeleryExecutor",
- "enum": [
- "LocalExecutor",
- "LocalKubernetesExecutor",
- "CeleryExecutor",
- "KubernetesExecutor",
- "CeleryKubernetesExecutor",
- "airflow.providers.amazon.aws.executors.batch.AwsBatchExecutor",
- "airflow.providers.amazon.aws.executors.ecs.AwsEcsExecutor"
- ]
+ "default": "CeleryExecutor"
Review Comment:
What about adding two new values (executors which is an array of executors, and defaultExecutor) and deprecate this one 🤔
##########
chart/templates/scheduler/scheduler-deployment.yaml:
##########
@@ -53,7 +53,6 @@ metadata:
release: {{ .Release.Name }}
chart: "{{ .Chart.Name }}-{{ .Chart.Version }}"
heritage: {{ .Release.Service }}
- executor: {{ .Values.executor }}
Review Comment:
I think we can sanitize the value or add it when it contains only acceptable characters and when it is shorter than 64 characters.
--
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
Re: [PR] Chart: Allow hybrid executors [airflow]
Posted by "LipuFei (via GitHub)" <gi...@apache.org>.
LipuFei commented on code in PR #41524:
URL: https://github.com/apache/airflow/pull/41524#discussion_r1722812751
##########
chart/values.schema.json:
##########
@@ -516,16 +516,7 @@
"description": "Airflow executor.",
"type": "string",
"x-docsSection": "Common",
- "default": "CeleryExecutor",
- "enum": [
- "LocalExecutor",
- "LocalKubernetesExecutor",
- "CeleryExecutor",
- "KubernetesExecutor",
- "CeleryKubernetesExecutor",
- "airflow.providers.amazon.aws.executors.batch.AwsBatchExecutor",
- "airflow.providers.amazon.aws.executors.ecs.AwsEcsExecutor"
- ]
+ "default": "CeleryExecutor"
Review Comment:
Just realise we are talking about the same thing... 😄
~What if we keep this as a list of enums? Then, in the value, we concatenate them with `,` into a comma separated list? Downside is, this is still a fixed list of executors. You cannot specify custom executors or use short names such as `airflow.providers.amazon.aws.executors.ecs.AwsEcsExecutor:AwsEcsExecutor`.~
--
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
Re: [PR] Chart: Allow hybrid executors [airflow]
Posted by "LipuFei (via GitHub)" <gi...@apache.org>.
LipuFei commented on code in PR #41524:
URL: https://github.com/apache/airflow/pull/41524#discussion_r1722816043
##########
chart/templates/scheduler/scheduler-deployment.yaml:
##########
@@ -53,7 +53,6 @@ metadata:
release: {{ .Release.Name }}
chart: "{{ .Chart.Name }}-{{ .Chart.Version }}"
heritage: {{ .Release.Service }}
- executor: {{ .Values.executor }}
Review Comment:
In my humble opinion, sanitizing this value is a little too much work (complexity) for this field that has no functional use. I'd prefer to remove it then making it too complex...
--
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