-
-
Notifications
You must be signed in to change notification settings - Fork 204
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.
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
Add Helm chart for Airflow (fix #7) #16
Conversation
b634d58
to
026e949
Compare
@stibbons a helm chart would be awesome. However, do you think it might be a better fit for https://github.com/kubernetes/charts? That would allow better discoverability, review, and long term maintenance. |
Yes, I plan to land it to kube-airflow for testing, and if it works, merge it to kubernetes/charts |
airflow/templates/configmaps.yaml
Outdated
{{- if .Values.airflow.fernet_key }} | ||
FERNET_KEY: "{{- .Values.airflow.fernet_key -}}" | ||
{{- end }} | ||
RABBITMQ_HOST: "{{- .Values.airflow.prefix -}}{{- .Values.db.rabbitmq.basename -}}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: hostname
instead of basename
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to emphasis it is not a full hostname. the final hostname will be prefix with airflow.prefix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case shouldn't the corresponding variable in values.yaml
be using basename
instead of hostname
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hum this part works pretty well, I don't see your point. values.yaml allows user to specify the basehostname, and here it prefixed with the generic prefix (I use it so it can be possible to start several airflow instance in the same namespace)
airflow/templates/configmaps.yaml
Outdated
{{- if .Values.db.rabbitmq.user }} | ||
RABBITMQ_CREDS: "{{- .Values.db.rabbitmq.user -}}:{{- .Values.db.rabbitmq.password -}}" | ||
{{- end }} | ||
POSTGRES_HOST: "{{- .Values.airflow.prefix -}}{{- .Values.db.postgres.basename -}}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: hostname
instead of basename
@stibbons I tried your helm chart with your docker image - few things didn't work for me.
|
You need to use PS: I have fixed an issue today can you try again by retrieving the latest version of this PR |
Also, I will probably propose a version that store the complete airflow.cfg in a configmap, it should be much simpler |
airflow/values.yaml
Outdated
use_embedded: true | ||
user: airflow | ||
password: airflow | ||
hostname: rabbitmq |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean shouldn't this key be basename
instead of hostname
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ho, yes indeed
@stibbons I used your image
|
@chhetripradeep should be fixed on latest rev. |
@stibbons sorry the above exception is my bad. i didnt pull the latest helm chart branch. |
I just fixed it now :) |
@stibbons Ack that the chart now works like a charm 🎉 🎉 |
I am also new to helm and k8s ecosystem but few feedbacks i would suggest -
Overall, chart looks great. |
There will be probably 2 helm charts:
I'll add quickly the ingress switch, i'll try pvc if i have the time, and i also wanted to switch to config map but with this image and the entrypoint.sh it will be tricky to do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a note. Be careful with git-sync and airflow. If the scheduler reloads a dag in the middle of a dagrun then the dagrun will run actually start using the new version of the dag in the middle of execution. This is a known issue with airflow and it means it's unsafe in general to use a git-sync like solution with airflow without:
- using explicit locking, ie never pull down a new dag if a dagrun is in progress
- make dags immutable, never modify your dag always make a new one
Indeed, good to know. It is just an option, and probably needs more documentation. By the way, do you know if there are official "recommendation" on airflow website, about Dag update/deployment? I don't see such information in the doc |
Added a dependency on main PostgreSQL chart
flower instead of path prefixes.
Use puckel/docker-airflow image
@gsemet Hey! I finally managed to catch all the things up made until now. Thanks for the all the your efforts and the fork, and the PR to official Kubernetes charts repository 👍 Everyone, see his awesome work at helm/charts#3959. I'll keep this repository for mostly for a reference for a while. |
Add Helm chart for Airflow (fix mumoshu#7)
Here is a big Pull Request adding an Helm Chart for Airflow (see #7). I propose the following:
docker pull stibbons31/kube-airflow:airflow-1.8.0.0-kube-1.6
)For information, I was greatly inspired by the zero-to-jupyterhub helm repository.
Usage:
Features
./airflow
airflow.cfg
override by configmap support to set default environment variables for web, scheduler and worker (but still requires set of some env vars)To test this pull request:
stibbons31/kube-airflow:helm_chart_dockerfile
instead ofmumoshu/kube-airflow:1.8.0.0-1.6.1
mumoshu/kube-airflow:1.8.0.0-1.6.1
needs to be updated with the newDockerfile.template
proposed in this PR (using more recent version of Debian and python 3, new configuration files, etc)...Please note this PR depends on a patch on airflow (#2723) for supporting
url_prefix
for the webservice and flower, so you cannot use a released version of Airflow so far. I maintain a fork with my patch until my Pull Request get integrated.