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 2021/01/18 07:47:45 UTC
[GitHub] [airflow] Swalloow opened a new pull request #13735: Add airflowExtraContainers configuration
Swalloow opened a new pull request #13735:
URL: https://github.com/apache/airflow/pull/13735
<!--
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/
-->
related: #13211
Add extra containers for the web/scheduler/worker.
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/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/master/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Swalloow edited a comment on pull request #13735: Support extraContainers configuration in Helm Chart
Posted by GitBox <gi...@apache.org>.
Swalloow edited a comment on pull request #13735:
URL: https://github.com/apache/airflow/pull/13735#issuecomment-763395353
@XD-DENG I added a short doc in `chart/README.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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Swalloow edited a comment on pull request #13735: Support extraContainers configuration in Helm Chart
Posted by GitBox <gi...@apache.org>.
Swalloow edited a comment on pull request #13735:
URL: https://github.com/apache/airflow/pull/13735#issuecomment-762229938
@potiuk I agree. I use a sidecar that syncs DAGs from object storage.
In version 2.0 with DAG serialization enabled, it only needs to be deployed to the scheduler.
So, define different containers for webserver/scheduler/workers would be great.
----------------------------------------------------------------
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] Swalloow commented on pull request #13735: Support extraContainers configuration in Helm Chart
Posted by GitBox <gi...@apache.org>.
Swalloow commented on pull request #13735:
URL: https://github.com/apache/airflow/pull/13735#issuecomment-773346755
@XD-DENG any updates?
----------------------------------------------------------------
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] mik-laj commented on a change in pull request #13735: Support extraContainers configuration in Helm Chart
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #13735:
URL: https://github.com/apache/airflow/pull/13735#discussion_r594663956
##########
File path: docs/helm-chart/using-additional-containers.rst
##########
@@ -0,0 +1,35 @@
+ .. Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ .. http://www.apache.org/licenses/LICENSE-2.0
+
+ .. Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+
+Using additional containers
+----------------------------
+
+If you are using your own sidecar container, you can add it through the ``extraContainers`` value.
+You can define different containers for scheduler, webserver and worker pods.
+
+For example, a sidecar that syncs DAGs from object storage.
Review comment:
Can you add little more docs tto manage-dags-files.rst also? I think it will be very helpful to understand how to use this feature.
----------------------------------------------------------------
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] gardnerdev commented on a change in pull request #13735: Support extraContainers configuration in Helm Chart
Posted by GitBox <gi...@apache.org>.
gardnerdev commented on a change in pull request #13735:
URL: https://github.com/apache/airflow/pull/13735#discussion_r560731235
##########
File path: chart/README.md
##########
@@ -366,6 +366,17 @@ helm install airflow . \
--set data.brokerUrl=redis://redis-user:password@redis-host:6379/0
```
+## Using additional containers
+
+If you are using your own sidecar container, you can add it through the `extraContainers` value. You can define different containers for scheduler, webserver and worker pods. For example, a sidecar that syncs DAGs from object storage.
+
+```bash
Review comment:
@Swalloow How to distinguish for example which container is for the sheduler and which one is for the webserver?
----------------------------------------------------------------
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] Swalloow commented on a change in pull request #13735: Support extraContainers configuration in Helm Chart
Posted by GitBox <gi...@apache.org>.
Swalloow commented on a change in pull request #13735:
URL: https://github.com/apache/airflow/pull/13735#discussion_r560734481
##########
File path: chart/README.md
##########
@@ -366,6 +366,17 @@ helm install airflow . \
--set data.brokerUrl=redis://redis-user:password@redis-host:6379/0
```
+## Using additional containers
+
+If you are using your own sidecar container, you can add it through the `extraContainers` value. You can define different containers for scheduler, webserver and worker pods. For example, a sidecar that syncs DAGs from object storage.
+
+```bash
Review comment:
@gardnerdev `extraContainers` are defined as properties of scheduler, webserver, and worker.
```
scheduler:
...
extraContainers: []
webserver:
...
extraContainers: []
workers:
...
extraContainers: []
```
----------------------------------------------------------------
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] Swalloow commented on a change in pull request #13735: Support extraContainers configuration in Helm Chart
Posted by GitBox <gi...@apache.org>.
Swalloow commented on a change in pull request #13735:
URL: https://github.com/apache/airflow/pull/13735#discussion_r580926640
##########
File path: chart/templates/workers/worker-deployment.yaml
##########
@@ -214,6 +214,9 @@ spec:
{{- include "custom_airflow_environment" . | indent 10 }}
{{- include "standard_airflow_environment" . | indent 10 }}
{{- end }}
+{{- if .Values.workers.extraContainers }}
+{{- toYaml .Values.workers.extraContainers | nindent 8 }}
Review comment:
I agree. I added the README that this option only supports CeleryExecutor.
And worker-deployment is not created in KubernetesExecutor.
----------------------------------------------------------------
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 #13735: Support extraContainers configuration in Helm Chart
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #13735:
URL: https://github.com/apache/airflow/pull/13735#discussion_r580918918
##########
File path: chart/templates/workers/worker-deployment.yaml
##########
@@ -214,6 +214,9 @@ spec:
{{- include "custom_airflow_environment" . | indent 10 }}
{{- include "standard_airflow_environment" . | indent 10 }}
{{- end }}
+{{- if .Values.workers.extraContainers }}
+{{- toYaml .Values.workers.extraContainers | nindent 8 }}
Review comment:
⚠️ Doing this with Kube executor would break things. If you add an extra container that has a daemon process running, then even if the "Airflow" worker container has finished, the pod is still active as the daemon in the other container is still up, and the pod never leaves the running state.
We (Astro) have had to hack around this to support running Istio sidecars, but it was not a pretty solution.
----------------------------------------------------------------
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] Swalloow commented on a change in pull request #13735: Support extraContainers configuration in Helm Chart
Posted by GitBox <gi...@apache.org>.
Swalloow commented on a change in pull request #13735:
URL: https://github.com/apache/airflow/pull/13735#discussion_r561942754
##########
File path: chart/values.yaml
##########
@@ -391,6 +392,9 @@ scheduler:
# Annotations to add to scheduler kubernetes service account.
serviceAccountAnnotations: {}
+ # Launch additional containers into scheduler.
+ extraContainers: []
+
# Mount additional volumes into scheduler.
extraVolumes: []
Review comment:
@gardnerdev You can follow `Using additional containers` document in `README.md`.
An example S3 sync container is as follows: https://github.com/Swalloow/s3-sync
If you use EKS, IAM roles for service account settings are required.
When DAG serialization is disabled, you need to add it to webserver/scheduler/workers.
```
extraContainers:
- name: s3-sync
image: myrepository/s3-sync:latest
imagePullPolicy: Always
volumeMounts:
- name: dags
mountPath: /opt/airflow/dags
env:
- name: AWS_BUCKET
value: airflow-src
- name: KEY_PATH
value: dags
- name: DEST_PATH
value: /opt/airflow/dags
- name: INTERVAL
value: "10"
```
----------------------------------------------------------------
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] XD-DENG commented on pull request #13735: Support extraContainers configuration in Helm Chart
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on pull request #13735:
URL: https://github.com/apache/airflow/pull/13735#issuecomment-763398019
@Swalloow Thanks for the doc update.
From what I can see so far, I'm not sure yet if this implementation itself is flexible enough or sufficient for most side car use cases. I would like to have our K8S guru @dimberman to take a look before we proceed.
----------------------------------------------------------------
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 #13735: Support extraContainers configuration in Helm Chart
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #13735:
URL: https://github.com/apache/airflow/pull/13735#discussion_r594716150
##########
File path: docs/helm-chart/using-additional-containers.rst
##########
@@ -0,0 +1,35 @@
+ .. Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ .. http://www.apache.org/licenses/LICENSE-2.0
+
+ .. Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+
+Using additional containers
+----------------------------
+
+If you are using your own sidecar container, you can add it through the ``extraContainers`` value.
+You can define different containers for scheduler, webserver and worker pods.
+
+For example, a sidecar that syncs DAGs from object storage.
Review comment:
I think that can be done in separate PR since this just adds abilities to add extraContainers and sync'ing dags is one of the example.
----------------------------------------------------------------
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] Swalloow commented on a change in pull request #13735: Support extraContainers configuration in Helm Chart
Posted by GitBox <gi...@apache.org>.
Swalloow commented on a change in pull request #13735:
URL: https://github.com/apache/airflow/pull/13735#discussion_r561942754
##########
File path: chart/values.yaml
##########
@@ -391,6 +392,9 @@ scheduler:
# Annotations to add to scheduler kubernetes service account.
serviceAccountAnnotations: {}
+ # Launch additional containers into scheduler.
+ extraContainers: []
+
# Mount additional volumes into scheduler.
extraVolumes: []
Review comment:
@gardnerdev You can follow `Using additional containers` document in `README.md`.
example S3 sync container : https://github.com/Swalloow/s3-sync
If you use EKS, IAM roles for service account settings are required.
When DAG serialization is disabled, you need to add it to webserver/scheduler/workers.
```
extraContainers:
- name: s3-sync
image: myrepository/s3-sync:latest
imagePullPolicy: Always
volumeMounts:
- name: dags
mountPath: /opt/airflow/dags
env:
- name: AWS_BUCKET
value: airflow-src
- name: KEY_PATH
value: dags
- name: DEST_PATH
value: /opt/airflow/dags
- name: INTERVAL
value: "10"
```
----------------------------------------------------------------
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] mik-laj commented on a change in pull request #13735: Support extraContainers configuration in Helm Chart
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #13735:
URL: https://github.com/apache/airflow/pull/13735#discussion_r562037474
##########
File path: chart/README.md
##########
@@ -366,6 +366,17 @@ helm install airflow . \
--set data.brokerUrl=redis://redis-user:password@redis-host:6379/0
```
+## Using additional containers
+
+If you are using your own sidecar container, you can add it through the `extraContainers` value. You can define different containers for scheduler, webserver and worker pods. For example, a sidecar that syncs DAGs from object storage.
+
+```bash
Review comment:
Concrete examples are very helpful, especially since file syncing is a very common case in Airflow. If we use specific examples, we should check if they really work.
----------------------------------------------------------------
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] mik-laj commented on a change in pull request #13735: Support extraContainers configuration in Helm Chart
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #13735:
URL: https://github.com/apache/airflow/pull/13735#discussion_r562040278
##########
File path: chart/templates/workers/worker-deployment.yaml
##########
@@ -214,6 +214,9 @@ spec:
{{- include "custom_airflow_environment" . | indent 10 }}
{{- include "standard_airflow_environment" . | indent 10 }}
{{- end }}
+{{- if .Values.workers.extraContainers }}
+{{- toYaml .Values.workers.extraContainers | nindent 8 }}
Review comment:
For now, we can only limit ourselves to Celery Executor, but I think it is worth adding an entry in the documentation for this limitation.
----------------------------------------------------------------
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 #13735: Support extraContainers configuration in Helm Chart
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #13735:
URL: https://github.com/apache/airflow/pull/13735#discussion_r580918918
##########
File path: chart/templates/workers/worker-deployment.yaml
##########
@@ -214,6 +214,9 @@ spec:
{{- include "custom_airflow_environment" . | indent 10 }}
{{- include "standard_airflow_environment" . | indent 10 }}
{{- end }}
+{{- if .Values.workers.extraContainers }}
+{{- toYaml .Values.workers.extraContainers | nindent 8 }}
Review comment:
⚠️ Doing this with Kube executor would break things - tasks would never complete.
If you add an extra container that has a daemon process running, then even if the "Airflow" worker container has finished, the pod is still active as the daemon in the other container is still up, and the pod never leaves the running state.
We (Astro) have had to hack around this to support running Istio sidecars, but it was not a pretty solution.
----------------------------------------------------------------
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] Nelill commented on pull request #13735: Support extraContainers configuration in Helm Chart
Posted by GitBox <gi...@apache.org>.
Nelill commented on pull request #13735:
URL: https://github.com/apache/airflow/pull/13735#issuecomment-777342127
Hello !
Any update on this PR ?
----------------------------------------------------------------
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 #13735: Support extraContainers configuration in Helm Chart
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #13735:
URL: https://github.com/apache/airflow/pull/13735#issuecomment-781744974
@Swalloow Can you add the doc around limitation: https://github.com/apache/airflow/pull/13735#discussion_r562040278
----------------------------------------------------------------
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] Swalloow commented on a change in pull request #13735: Support extraContainers configuration in Helm Chart
Posted by GitBox <gi...@apache.org>.
Swalloow commented on a change in pull request #13735:
URL: https://github.com/apache/airflow/pull/13735#discussion_r561955581
##########
File path: chart/README.md
##########
@@ -366,6 +366,17 @@ helm install airflow . \
--set data.brokerUrl=redis://redis-user:password@redis-host:6379/0
```
+## Using additional containers
+
+If you are using your own sidecar container, you can add it through the `extraContainers` value. You can define different containers for scheduler, webserver and worker pods. For example, a sidecar that syncs DAGs from object storage.
+
+```bash
Review comment:
sidecar container pattern can be implemented in many ways.
I'm not sure if it is correct to write a specific example 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #13735: Support extraContainers configuration in Helm Chart
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #13735:
URL: https://github.com/apache/airflow/pull/13735#issuecomment-797693324
I rebased and fixed the conflicts, lets wait for the CI results now
----------------------------------------------------------------
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] Swalloow commented on a change in pull request #13735: Support extraContainers configuration in Helm Chart
Posted by GitBox <gi...@apache.org>.
Swalloow commented on a change in pull request #13735:
URL: https://github.com/apache/airflow/pull/13735#discussion_r561942754
##########
File path: chart/values.yaml
##########
@@ -391,6 +392,9 @@ scheduler:
# Annotations to add to scheduler kubernetes service account.
serviceAccountAnnotations: {}
+ # Launch additional containers into scheduler.
+ extraContainers: []
+
# Mount additional volumes into scheduler.
extraVolumes: []
Review comment:
@gardnerdev You can follow `Using additional containers` document in `README.md`.
An example S3 sync container is as follows: https://github.com/Swalloow/s3-sync
If you use EKS, IAM roles for service account settings are required.
When DAG serialization is disabled, you need to add it to webserver/scheduler/workers.
```
extraContainers:
- name: s3-sync
image: myrepository/s3sync:latest
imagePullPolicy: Always
volumeMounts:
- name: dags
mountPath: /opt/airflow/dags
env:
- name: AWS_BUCKET
value: airflow-src
- name: KEY_PATH
value: dags
- name: DEST_PATH
value: /opt/airflow/dags
- name: INTERVAL
value: "10"
```
----------------------------------------------------------------
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] gardnerdev commented on a change in pull request #13735: Support extraContainers configuration in Helm Chart
Posted by GitBox <gi...@apache.org>.
gardnerdev commented on a change in pull request #13735:
URL: https://github.com/apache/airflow/pull/13735#discussion_r562110467
##########
File path: chart/values.yaml
##########
@@ -391,6 +392,9 @@ scheduler:
# Annotations to add to scheduler kubernetes service account.
serviceAccountAnnotations: {}
+ # Launch additional containers into scheduler.
+ extraContainers: []
+
# Mount additional volumes into scheduler.
extraVolumes: []
Review comment:
Thanks, I am using [ https://github.com/houqp/objinsync ]( https://github.com/houqp/objinsync ) but thanks to you I was able to configure it!
##########
File path: chart/values.yaml
##########
@@ -391,6 +392,9 @@ scheduler:
# Annotations to add to scheduler kubernetes service account.
serviceAccountAnnotations: {}
+ # Launch additional containers into scheduler.
+ extraContainers: []
+
# Mount additional volumes into scheduler.
extraVolumes: []
Review comment:
I am using [ https://github.com/houqp/objinsync ]( https://github.com/houqp/objinsync ) but thanks to you I was able to configure it!
----------------------------------------------------------------
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] pparthesh commented on pull request #13735: Support extraContainers configuration in Helm Chart
Posted by GitBox <gi...@apache.org>.
pparthesh commented on pull request #13735:
URL: https://github.com/apache/airflow/pull/13735#issuecomment-823282388
service config are missing based on extraContainers port.
--
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] gardnerdev commented on a change in pull request #13735: Support extraContainers configuration in Helm Chart
Posted by GitBox <gi...@apache.org>.
gardnerdev commented on a change in pull request #13735:
URL: https://github.com/apache/airflow/pull/13735#discussion_r561864186
##########
File path: chart/README.md
##########
@@ -366,6 +366,17 @@ helm install airflow . \
--set data.brokerUrl=redis://redis-user:password@redis-host:6379/0
```
+## Using additional containers
+
+If you are using your own sidecar container, you can add it through the `extraContainers` value. You can define different containers for scheduler, webserver and worker pods. For example, a sidecar that syncs DAGs from object storage.
+
+```bash
Review comment:
@Swalloow could you put in docs or here how you implemented sindecar container pattern? I mean mounting volumes example
----------------------------------------------------------------
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] gardnerdev commented on a change in pull request #13735: Support extraContainers configuration in Helm Chart
Posted by GitBox <gi...@apache.org>.
gardnerdev commented on a change in pull request #13735:
URL: https://github.com/apache/airflow/pull/13735#discussion_r561824853
##########
File path: chart/values.yaml
##########
@@ -391,6 +392,9 @@ scheduler:
# Annotations to add to scheduler kubernetes service account.
serviceAccountAnnotations: {}
+ # Launch additional containers into scheduler.
+ extraContainers: []
+
# Mount additional volumes into scheduler.
extraVolumes: []
Review comment:
How to mount scheduler dags folder to s3-sync? Any example?
----------------------------------------------------------------
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] mik-laj commented on a change in pull request #13735: Support extraContainers configuration in Helm Chart
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #13735:
URL: https://github.com/apache/airflow/pull/13735#discussion_r560844527
##########
File path: chart/templates/workers/worker-deployment.yaml
##########
@@ -214,6 +214,9 @@ spec:
{{- include "custom_airflow_environment" . | indent 10 }}
{{- include "standard_airflow_environment" . | indent 10 }}
{{- end }}
+{{- if .Values.workers.extraContainers }}
+{{- toYaml .Values.workers.extraContainers | nindent 8 }}
Review comment:
Should we also update `pod-template-file.kubernetes-helm-yaml` to support Kubernetes Workers?
https://github.com/apache/airflow/blob/master/chart/files/pod-template-file.kubernetes-helm-yaml
----------------------------------------------------------------
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] mik-laj commented on a change in pull request #13735: Support extraContainers configuration in Helm Chart
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #13735:
URL: https://github.com/apache/airflow/pull/13735#discussion_r562037474
##########
File path: chart/README.md
##########
@@ -366,6 +366,17 @@ helm install airflow . \
--set data.brokerUrl=redis://redis-user:password@redis-host:6379/0
```
+## Using additional containers
+
+If you are using your own sidecar container, you can add it through the `extraContainers` value. You can define different containers for scheduler, webserver and worker pods. For example, a sidecar that syncs DAGs from object storage.
+
+```bash
Review comment:
Specific examples are very helpful, especially since file syncing is a very common case in Airflow. If we use specific examples, we should check if they really work.
----------------------------------------------------------------
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] Swalloow commented on pull request #13735: Support extraContainers configuration in Helm Chart
Posted by GitBox <gi...@apache.org>.
Swalloow commented on pull request #13735:
URL: https://github.com/apache/airflow/pull/13735#issuecomment-762229938
@potiuk I agree. I use a sidecar that syncs DAGs from object storage.
But in version 2.0 with DAG serialization enabled, it only needs to be deployed to the scheduler.
So, define different containers for webserver/scheduler/workers would be great.
----------------------------------------------------------------
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 #13735: Support extraContainers configuration in Helm Chart
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #13735:
URL: https://github.com/apache/airflow/pull/13735#issuecomment-797680351
Thanks @Swalloow -- looks good now, can you resolve the conflicts and we can merge this.
----------------------------------------------------------------
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] Swalloow commented on a change in pull request #13735: Support extraContainers configuration in Helm Chart
Posted by GitBox <gi...@apache.org>.
Swalloow commented on a change in pull request #13735:
URL: https://github.com/apache/airflow/pull/13735#discussion_r561920861
##########
File path: chart/templates/workers/worker-deployment.yaml
##########
@@ -214,6 +214,9 @@ spec:
{{- include "custom_airflow_environment" . | indent 10 }}
{{- include "standard_airflow_environment" . | indent 10 }}
{{- end }}
+{{- if .Values.workers.extraContainers }}
+{{- toYaml .Values.workers.extraContainers | nindent 8 }}
Review comment:
Not tested with Kubernetes workers.
The `extraVolumes` and `extraVolumeMounts` options also seem to be excluded from the file.
Need to support it?
##########
File path: chart/values.yaml
##########
@@ -391,6 +392,9 @@ scheduler:
# Annotations to add to scheduler kubernetes service account.
serviceAccountAnnotations: {}
+ # Launch additional containers into scheduler.
+ extraContainers: []
+
# Mount additional volumes into scheduler.
extraVolumes: []
Review comment:
@gardnerdev You can follow `Using additional containers` document in `README.md`.
An example S3 sync container is as follows: https://github.com/Swalloow/s3-sync
If you use EKS, IAM roles for service account settings are required.
```
extraContainers:
- name: s3-sync
image: myrepository/s3sync:latest
imagePullPolicy: Always
volumeMounts:
- name: dags
mountPath: /opt/airflow/dags
env:
- name: AWS_BUCKET
value: airflow-src
- name: KEY_PATH
value: dags
- name: DEST_PATH
value: /opt/airflow/dags
- name: INTERVAL
value: "10"
```
##########
File path: chart/values.yaml
##########
@@ -391,6 +392,9 @@ scheduler:
# Annotations to add to scheduler kubernetes service account.
serviceAccountAnnotations: {}
+ # Launch additional containers into scheduler.
+ extraContainers: []
+
# Mount additional volumes into scheduler.
extraVolumes: []
Review comment:
@gardnerdev You can follow `Using additional containers` document in `README.md`.
An example S3 sync container is as follows: https://github.com/Swalloow/s3-sync
If you use EKS, IAM roles for service account settings are required.
For version 1.10.14, you need to add it to webserver/scheduler/workers.
```
extraContainers:
- name: s3-sync
image: myrepository/s3sync:latest
imagePullPolicy: Always
volumeMounts:
- name: dags
mountPath: /opt/airflow/dags
env:
- name: AWS_BUCKET
value: airflow-src
- name: KEY_PATH
value: dags
- name: DEST_PATH
value: /opt/airflow/dags
- name: INTERVAL
value: "10"
```
##########
File path: chart/values.yaml
##########
@@ -391,6 +392,9 @@ scheduler:
# Annotations to add to scheduler kubernetes service account.
serviceAccountAnnotations: {}
+ # Launch additional containers into scheduler.
+ extraContainers: []
+
# Mount additional volumes into scheduler.
extraVolumes: []
Review comment:
@gardnerdev You can follow `Using additional containers` document in `README.md`.
An example S3 sync container is as follows: https://github.com/Swalloow/s3-sync
If you use EKS, IAM roles for service account settings are required.
When DAG serialization is disabled, you need to add it to webserver/scheduler/workers.
```
extraContainers:
- name: s3-sync
image: myrepository/s3sync:latest
imagePullPolicy: Always
volumeMounts:
- name: dags
mountPath: /opt/airflow/dags
env:
- name: AWS_BUCKET
value: airflow-src
- name: KEY_PATH
value: dags
- name: DEST_PATH
value: /opt/airflow/dags
- name: INTERVAL
value: "10"
```
##########
File path: chart/values.yaml
##########
@@ -391,6 +392,9 @@ scheduler:
# Annotations to add to scheduler kubernetes service account.
serviceAccountAnnotations: {}
+ # Launch additional containers into scheduler.
+ extraContainers: []
+
# Mount additional volumes into scheduler.
extraVolumes: []
Review comment:
@gardnerdev You can follow `Using additional containers` document in `README.md`.
An example S3 sync container is as follows: https://github.com/Swalloow/s3-sync
If you use EKS, IAM roles for service account settings are required.
When DAG serialization is disabled, you need to add it to webserver/scheduler/workers.
```
extraContainers:
- name: s3-sync
image: myrepository/s3-sync:latest
imagePullPolicy: Always
volumeMounts:
- name: dags
mountPath: /opt/airflow/dags
env:
- name: AWS_BUCKET
value: airflow-src
- name: KEY_PATH
value: dags
- name: DEST_PATH
value: /opt/airflow/dags
- name: INTERVAL
value: "10"
```
##########
File path: chart/README.md
##########
@@ -366,6 +366,17 @@ helm install airflow . \
--set data.brokerUrl=redis://redis-user:password@redis-host:6379/0
```
+## Using additional containers
+
+If you are using your own sidecar container, you can add it through the `extraContainers` value. You can define different containers for scheduler, webserver and worker pods. For example, a sidecar that syncs DAGs from object storage.
+
+```bash
Review comment:
sidecar container pattern can be implemented in many ways.
I'm not sure if it is correct to write a specific example here.
##########
File path: chart/values.yaml
##########
@@ -391,6 +392,9 @@ scheduler:
# Annotations to add to scheduler kubernetes service account.
serviceAccountAnnotations: {}
+ # Launch additional containers into scheduler.
+ extraContainers: []
+
# Mount additional volumes into scheduler.
extraVolumes: []
Review comment:
@gardnerdev You can follow `Using additional containers` document in `README.md`.
example S3 sync container : https://github.com/Swalloow/s3-sync
If you use EKS, IAM roles for service account settings are required.
When DAG serialization is disabled, you need to add it to webserver/scheduler/workers.
```
extraContainers:
- name: s3-sync
image: myrepository/s3-sync:latest
imagePullPolicy: Always
volumeMounts:
- name: dags
mountPath: /opt/airflow/dags
env:
- name: AWS_BUCKET
value: airflow-src
- name: KEY_PATH
value: dags
- name: DEST_PATH
value: /opt/airflow/dags
- name: INTERVAL
value: "10"
```
##########
File path: chart/values.yaml
##########
@@ -391,6 +392,9 @@ scheduler:
# Annotations to add to scheduler kubernetes service account.
serviceAccountAnnotations: {}
+ # Launch additional containers into scheduler.
+ extraContainers: []
+
# Mount additional volumes into scheduler.
extraVolumes: []
Review comment:
@gardnerdev Here is an example S3 sync container: https://github.com/Swalloow/s3-sync
If you use EKS, IAM roles for service account settings are required.
When DAG serialization is disabled, you need to add it to webserver/scheduler/workers.
```
extraContainers:
- name: s3-sync
image: myrepository/s3-sync:latest
imagePullPolicy: Always
volumeMounts:
- name: dags
mountPath: /opt/airflow/dags
env:
- name: AWS_BUCKET
value: airflow-src
- name: KEY_PATH
value: dags
- name: DEST_PATH
value: /opt/airflow/dags
- name: INTERVAL
value: "10"
```
----------------------------------------------------------------
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] Swalloow commented on pull request #13735: Support extraContainers configuration in Helm Chart
Posted by GitBox <gi...@apache.org>.
Swalloow commented on pull request #13735:
URL: https://github.com/apache/airflow/pull/13735#issuecomment-782607981
@kaxil 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] Nelill edited a comment on pull request #13735: Support extraContainers configuration in Helm Chart
Posted by GitBox <gi...@apache.org>.
Nelill edited a comment on pull request #13735:
URL: https://github.com/apache/airflow/pull/13735#issuecomment-777342127
Hello !
Any update on this PR ?
When it will be merged ?
Thanks ! :D
----------------------------------------------------------------
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] Swalloow commented on a change in pull request #13735: Support extraContainers configuration in Helm Chart
Posted by GitBox <gi...@apache.org>.
Swalloow commented on a change in pull request #13735:
URL: https://github.com/apache/airflow/pull/13735#discussion_r561920861
##########
File path: chart/templates/workers/worker-deployment.yaml
##########
@@ -214,6 +214,9 @@ spec:
{{- include "custom_airflow_environment" . | indent 10 }}
{{- include "standard_airflow_environment" . | indent 10 }}
{{- end }}
+{{- if .Values.workers.extraContainers }}
+{{- toYaml .Values.workers.extraContainers | nindent 8 }}
Review comment:
Not tested with Kubernetes workers.
The `extraVolumes` and `extraVolumeMounts` options also seem to be excluded from the file.
Need to support it?
----------------------------------------------------------------
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] potiuk commented on pull request #13735: Support extraContainers configuration in Helm Chart
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #13735:
URL: https://github.com/apache/airflow/pull/13735#issuecomment-763451530
> @Swalloow Thanks for the doc update.
>
> From what I can see so far, I'm not sure yet if this implementation itself is flexible enough or sufficient for most side car use cases. I would like to have our K8S guru @dimberman to take a look before we proceed.
I approved it from my part - it looks great witth the docs and unit tests, but @dimberman comments here are needed (at lest for now - I have a feeling that we should not get into the situation we have in CI now where I am mostly Single Point of Failure! I hope at today's call we can talk about our approach to make helm chart relasable.
----------------------------------------------------------------
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 #13735: Support extraContainers configuration in Helm Chart
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13735:
URL: https://github.com/apache/airflow/pull/13735#issuecomment-764613034
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest master or amend the last commit of the PR, and push it with --force-with-lease.
----------------------------------------------------------------
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] gardnerdev commented on a change in pull request #13735: Support extraContainers configuration in Helm Chart
Posted by GitBox <gi...@apache.org>.
gardnerdev commented on a change in pull request #13735:
URL: https://github.com/apache/airflow/pull/13735#discussion_r560759051
##########
File path: chart/README.md
##########
@@ -366,6 +366,17 @@ helm install airflow . \
--set data.brokerUrl=redis://redis-user:password@redis-host:6379/0
```
+## Using additional containers
+
+If you are using your own sidecar container, you can add it through the `extraContainers` value. You can define different containers for scheduler, webserver and worker pods. For example, a sidecar that syncs DAGs from object storage.
+
+```bash
Review comment:
Thanks, looks awesome
----------------------------------------------------------------
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] Swalloow commented on a change in pull request #13735: Support extraContainers configuration in Helm Chart
Posted by GitBox <gi...@apache.org>.
Swalloow commented on a change in pull request #13735:
URL: https://github.com/apache/airflow/pull/13735#discussion_r561942754
##########
File path: chart/values.yaml
##########
@@ -391,6 +392,9 @@ scheduler:
# Annotations to add to scheduler kubernetes service account.
serviceAccountAnnotations: {}
+ # Launch additional containers into scheduler.
+ extraContainers: []
+
# Mount additional volumes into scheduler.
extraVolumes: []
Review comment:
@gardnerdev You can follow `Using additional containers` document in `README.md`.
An example S3 sync container is as follows: https://github.com/Swalloow/s3-sync
If you use EKS, IAM roles for service account settings are required.
For version 1.10.14, you need to add it to webserver/scheduler/workers.
```
extraContainers:
- name: s3-sync
image: myrepository/s3sync:latest
imagePullPolicy: Always
volumeMounts:
- name: dags
mountPath: /opt/airflow/dags
env:
- name: AWS_BUCKET
value: airflow-src
- name: KEY_PATH
value: dags
- name: DEST_PATH
value: /opt/airflow/dags
- name: INTERVAL
value: "10"
```
----------------------------------------------------------------
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] Swalloow commented on a change in pull request #13735: Support extraContainers configuration in Helm Chart
Posted by GitBox <gi...@apache.org>.
Swalloow commented on a change in pull request #13735:
URL: https://github.com/apache/airflow/pull/13735#discussion_r561942754
##########
File path: chart/values.yaml
##########
@@ -391,6 +392,9 @@ scheduler:
# Annotations to add to scheduler kubernetes service account.
serviceAccountAnnotations: {}
+ # Launch additional containers into scheduler.
+ extraContainers: []
+
# Mount additional volumes into scheduler.
extraVolumes: []
Review comment:
@gardnerdev Here is an example S3 sync container: https://github.com/Swalloow/s3-sync
If you use EKS, IAM roles for service account settings are required.
When DAG serialization is disabled, you need to add it to webserver/scheduler/workers.
```
extraContainers:
- name: s3-sync
image: myrepository/s3-sync:latest
imagePullPolicy: Always
volumeMounts:
- name: dags
mountPath: /opt/airflow/dags
env:
- name: AWS_BUCKET
value: airflow-src
- name: KEY_PATH
value: dags
- name: DEST_PATH
value: /opt/airflow/dags
- name: INTERVAL
value: "10"
```
----------------------------------------------------------------
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] Swalloow commented on a change in pull request #13735: Support extraContainers configuration in Helm Chart
Posted by GitBox <gi...@apache.org>.
Swalloow commented on a change in pull request #13735:
URL: https://github.com/apache/airflow/pull/13735#discussion_r561942754
##########
File path: chart/values.yaml
##########
@@ -391,6 +392,9 @@ scheduler:
# Annotations to add to scheduler kubernetes service account.
serviceAccountAnnotations: {}
+ # Launch additional containers into scheduler.
+ extraContainers: []
+
# Mount additional volumes into scheduler.
extraVolumes: []
Review comment:
@gardnerdev You can follow `Using additional containers` document in `README.md`.
An example S3 sync container is as follows: https://github.com/Swalloow/s3-sync
If you use EKS, IAM roles for service account settings are required.
```
extraContainers:
- name: s3-sync
image: myrepository/s3sync:latest
imagePullPolicy: Always
volumeMounts:
- name: dags
mountPath: /opt/airflow/dags
env:
- name: AWS_BUCKET
value: airflow-src
- name: KEY_PATH
value: dags
- name: DEST_PATH
value: /opt/airflow/dags
- name: INTERVAL
value: "10"
```
----------------------------------------------------------------
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] gardnerdev commented on a change in pull request #13735: Support extraContainers configuration in Helm Chart
Posted by GitBox <gi...@apache.org>.
gardnerdev commented on a change in pull request #13735:
URL: https://github.com/apache/airflow/pull/13735#discussion_r561824853
##########
File path: chart/values.yaml
##########
@@ -391,6 +392,9 @@ scheduler:
# Annotations to add to scheduler kubernetes service account.
serviceAccountAnnotations: {}
+ # Launch additional containers into scheduler.
+ extraContainers: []
+
# Mount additional volumes into scheduler.
extraVolumes: []
Review comment:
How to mount scheduler dags folder to s3-sync? Any example?
##########
File path: chart/README.md
##########
@@ -366,6 +366,17 @@ helm install airflow . \
--set data.brokerUrl=redis://redis-user:password@redis-host:6379/0
```
+## Using additional containers
+
+If you are using your own sidecar container, you can add it through the `extraContainers` value. You can define different containers for scheduler, webserver and worker pods. For example, a sidecar that syncs DAGs from object storage.
+
+```bash
Review comment:
@Swalloow could you put in docs or here how you implemented sindecar container pattern? I mean mounting volumes example
##########
File path: chart/values.yaml
##########
@@ -391,6 +392,9 @@ scheduler:
# Annotations to add to scheduler kubernetes service account.
serviceAccountAnnotations: {}
+ # Launch additional containers into scheduler.
+ extraContainers: []
+
# Mount additional volumes into scheduler.
extraVolumes: []
Review comment:
Thanks, I am using [ https://github.com/houqp/objinsync ]( https://github.com/houqp/objinsync ) but thanks to you I was able to configure it!
##########
File path: chart/values.yaml
##########
@@ -391,6 +392,9 @@ scheduler:
# Annotations to add to scheduler kubernetes service account.
serviceAccountAnnotations: {}
+ # Launch additional containers into scheduler.
+ extraContainers: []
+
# Mount additional volumes into scheduler.
extraVolumes: []
Review comment:
I am using [ https://github.com/houqp/objinsync ]( https://github.com/houqp/objinsync ) but thanks to you I was able to configure it!
----------------------------------------------------------------
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] mik-laj commented on a change in pull request #13735: Support extraContainers configuration in Helm Chart
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #13735:
URL: https://github.com/apache/airflow/pull/13735#discussion_r562040278
##########
File path: chart/templates/workers/worker-deployment.yaml
##########
@@ -214,6 +214,9 @@ spec:
{{- include "custom_airflow_environment" . | indent 10 }}
{{- include "standard_airflow_environment" . | indent 10 }}
{{- end }}
+{{- if .Values.workers.extraContainers }}
+{{- toYaml .Values.workers.extraContainers | nindent 8 }}
Review comment:
For now, we can only support Celery Executor, but I think it is worth adding some docs about this limitation.
----------------------------------------------------------------
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] mik-laj commented on a change in pull request #13735: Support extraContainers configuration in Helm Chart
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #13735:
URL: https://github.com/apache/airflow/pull/13735#discussion_r594663956
##########
File path: docs/helm-chart/using-additional-containers.rst
##########
@@ -0,0 +1,35 @@
+ .. Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ .. http://www.apache.org/licenses/LICENSE-2.0
+
+ .. Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+
+Using additional containers
+----------------------------
+
+If you are using your own sidecar container, you can add it through the ``extraContainers`` value.
+You can define different containers for scheduler, webserver and worker pods.
+
+For example, a sidecar that syncs DAGs from object storage.
Review comment:
Can you add little more docs tto manage-dags-files.rst also? I think it will be very helpful to understand how to use this feature. Related: https://github.com/apache/airflow/issues/14728
----------------------------------------------------------------
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 #13735: Support extraContainers configuration in Helm Chart
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13735:
URL: https://github.com/apache/airflow/pull/13735#issuecomment-764613034
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest master or amend the last commit of the PR, and push it with --force-with-lease.
----------------------------------------------------------------
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] Swalloow commented on pull request #13735: Support extraContainers configuration in Helm Chart
Posted by GitBox <gi...@apache.org>.
Swalloow commented on pull request #13735:
URL: https://github.com/apache/airflow/pull/13735#issuecomment-773346755
@XD-DENG any updates?
----------------------------------------------------------------
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 #13735: Support extraContainers configuration in Helm Chart
Posted by GitBox <gi...@apache.org>.
kaxil merged pull request #13735:
URL: https://github.com/apache/airflow/pull/13735
----------------------------------------------------------------
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] gardnerdev commented on pull request #13735: Support extraContainers configuration in Helm Chart
Posted by GitBox <gi...@apache.org>.
gardnerdev commented on pull request #13735:
URL: https://github.com/apache/airflow/pull/13735#issuecomment-763353727
@Swalloow @potiuk could it be merged? I am waiting for that feature
----------------------------------------------------------------
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] Swalloow commented on pull request #13735: Support extraContainers configuration in Helm Chart
Posted by GitBox <gi...@apache.org>.
Swalloow commented on pull request #13735:
URL: https://github.com/apache/airflow/pull/13735#issuecomment-763395353
@XD-DENG I added a short doc in chart/README.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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #13735: Support extraContainers configuration in Helm Chart
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #13735:
URL: https://github.com/apache/airflow/pull/13735#discussion_r562037474
##########
File path: chart/README.md
##########
@@ -366,6 +366,17 @@ helm install airflow . \
--set data.brokerUrl=redis://redis-user:password@redis-host:6379/0
```
+## Using additional containers
+
+If you are using your own sidecar container, you can add it through the `extraContainers` value. You can define different containers for scheduler, webserver and worker pods. For example, a sidecar that syncs DAGs from object storage.
+
+```bash
Review comment:
Concrete examples are very helpful, especially since file syncing is a very common case in Airflow. If we use specific examples, we should check if they really work.
##########
File path: chart/README.md
##########
@@ -366,6 +366,17 @@ helm install airflow . \
--set data.brokerUrl=redis://redis-user:password@redis-host:6379/0
```
+## Using additional containers
+
+If you are using your own sidecar container, you can add it through the `extraContainers` value. You can define different containers for scheduler, webserver and worker pods. For example, a sidecar that syncs DAGs from object storage.
+
+```bash
Review comment:
Specific examples are very helpful, especially since file syncing is a very common case in Airflow. If we use specific examples, we should check if they really work.
##########
File path: chart/templates/workers/worker-deployment.yaml
##########
@@ -214,6 +214,9 @@ spec:
{{- include "custom_airflow_environment" . | indent 10 }}
{{- include "standard_airflow_environment" . | indent 10 }}
{{- end }}
+{{- if .Values.workers.extraContainers }}
+{{- toYaml .Values.workers.extraContainers | nindent 8 }}
Review comment:
For now, we can only limit ourselves to Celery Executor, but I think it is worth adding an entry in the documentation for this limitation.
----------------------------------------------------------------
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 #13735: Support extraContainers configuration in Helm Chart
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #13735:
URL: https://github.com/apache/airflow/pull/13735#discussion_r578856362
##########
File path: chart/templates/workers/worker-deployment.yaml
##########
@@ -214,6 +214,9 @@ spec:
{{- include "custom_airflow_environment" . | indent 10 }}
{{- include "standard_airflow_environment" . | indent 10 }}
{{- end }}
+{{- if .Values.workers.extraContainers }}
+{{- toYaml .Values.workers.extraContainers | nindent 8 }}
Review comment:
@Swalloow Can you add the limitation to the docs
----------------------------------------------------------------
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] Swalloow commented on pull request #13735: Support extraContainers configuration in Helm Chart
Posted by GitBox <gi...@apache.org>.
Swalloow commented on pull request #13735:
URL: https://github.com/apache/airflow/pull/13735#issuecomment-792246619
@mik-laj Are there any more comments? 🙏
----------------------------------------------------------------
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