Skip to content

Chart: Allow hybrid executors #41524

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
LipuFei wants to merge 1 commit into from
Closed

Chart: Allow hybrid executors #41524

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion chart/templates/scheduler/scheduler-deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@ metadata:
release: {{ .Release.Name }}
chart: "{{ .Chart.Name }}-{{ .Chart.Version }}"
heritage: {{ .Release.Service }}
executor: {{ .Values.executor }}
Copy link
Member

@hussein-awala hussein-awala Aug 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can sanitize the value or add it when it contains only acceptable characters and when it is shorter than 64 characters.

Copy link
Contributor Author

@LipuFei LipuFei Aug 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

{{- with .Values.labels }}
{{- toYaml . | nindent 4 }}
{{- end }}
Expand Down
11 changes: 1 addition & 10 deletions chart/values.schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Member

@hussein-awala hussein-awala Aug 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about adding two new values (executors which is an array of executors, and defaultExecutor) and deprecate this one 🤔

Copy link
Contributor Author

@LipuFei LipuFei Aug 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

},
"allowPodLaunching": {
"description": "Whether various Airflow components launch pods.",
Expand Down
20 changes: 2 additions & 18 deletions helm_tests/airflow_aux/test_basic_helm_chart.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -421,8 +421,6 @@ def test_labels_are_valid(self):
}
if component:
expected_labels["component"] = component
if k8s_object_name == f"{release_name}-scheduler":
expected_labels["executor"] = "CeleryExecutor"
actual_labels = kind_k8s_obj_labels_tuples.pop((k8s_object_name, kind))
assert actual_labels == expected_labels

Expand Down Expand Up @@ -538,6 +536,8 @@ def get_k8s_objs_with_image(obj: list[Any] | dict[str, Any]) -> list[dict[str, A
"CeleryKubernetesExecutor",
"airflow.providers.amazon.aws.executors.batch.AwsBatchExecutor",
"airflow.providers.amazon.aws.executors.ecs.AwsEcsExecutor",
# Hybrid executors
"CeleryExecutor,airflow.providers.amazon.aws.executors.ecs.AwsEcsExecutor:AwsEcsExecutor",
],
)
def test_supported_executor(self, executor):
Expand All @@ -548,22 +548,6 @@ def test_supported_executor(self, executor):
},
)

def test_unsupported_executor(self):
with pytest.raises(CalledProcessError) as ex_ctx:
render_chart(
"test-basic",
{
"executor": "SequentialExecutor",
},
)
assert (
'executor must be one of the following: "LocalExecutor", '
'"LocalKubernetesExecutor", "CeleryExecutor", '
'"KubernetesExecutor", "CeleryKubernetesExecutor", '
'"airflow.providers.amazon.aws.executors.batch.AwsBatchExecutor", '
'"airflow.providers.amazon.aws.executors.ecs.AwsEcsExecutor"' in ex_ctx.value.stderr.decode()
)

@pytest.mark.parametrize(
"image",
["airflow", "pod_template", "flower", "statsd", "redis", "pgbouncer", "pgbouncerExporter", "gitSync"],
Expand Down
19 changes: 0 additions & 19 deletions helm_tests/airflow_core/test_scheduler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -777,25 +777,6 @@ def test_persistence_volume_annotations(self):
)
assert {"foo": "bar"} == jmespath.search("spec.volumeClaimTemplates[0].metadata.annotations", docs[0])

@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"],
)

assert 1 == len(docs)
assert executor == docs[0]["metadata"]["labels"].get("executor")

def test_should_add_component_specific_annotations(self):
docs = render_chart(
values={
Expand Down