-
Notifications
You must be signed in to change notification settings - Fork 10.4k
[bitnami/airflow] Allow customizing the empty-dir volume parameters #36377
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
base: main
Are you sure you want to change the base?
Conversation
jotamartos
left a comment
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.
Thanks for your contribution. Can you take a look at my suggestions?
Signed-off-by: Kirill Buev <[email protected]>
b9bab6a to
3e073b8
Compare
|
Force-pushed a version with all the suggested changes. |
Signed-off-by: Bitnami Bot <[email protected]>
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.
Sorry for not asking this before, there are 2 changes that are not related to the emptyDir. Could you please clarify why did you add them? I suggest you create a separate PR for them as it's a separate issue.
| - name: empty-dir | ||
| mountPath: /opt/bitnami/airflow/config/airflow_local_settings.py | ||
| subPath: app-conf-dir/airflow_local_settings.py |
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.
One final question, why did you include this change? This is not related to the emptyDir volume.
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.
This mount is necessary to pass the Airflow local settings file to all components that might need it.
For example, it can be used to change the SQLAlchemy connection parameters which are expected to be passed as a dictionary. Here is an example of doing this via the bitnami/airflow chart values:
localSettings: |-
my_args = {
"keepalives": 1,
"keepalives_idle": 240,
"keepalives_interval": 15,
"keepalives_count": 8,
"sslmode": "disable"
}
my_async_args = {
"ssl": False
}
overrideConfiguration:
database:
sql_alchemy_connect_args: airflow_local_settings.my_args
sql_alchemy_connect_args_async: airflow_local_settings.my_async_args
The problem is that this mount is currently missing from the wait-for-db-migrations init containers and from the default worker pod template. At the same time the overrideConfiguration value is being correctly passed everywhere and this leads to errors when it has references to the missing local settings file.
The mount references the same emptyDir volume so I would argue it is related but I can certainly split this into a second PR if you deem it necessary.
Description of the change
This PR makes it possible to customize the properties of the currently hardcoded
empty-dirvolume used to hold temporary Airflow data (writeable venv, DAGs fetched from git repositories, temporary files, etc.). In addition to a regularemptyDirvolume (current behavior) it also provides a choice of a memory-backedemptyDirand a generic ephemeral volume backed by a PVC.The PR also adds a couple of currently missing
empty-dirmounts for the Airflow local settings file (airflow_local_settings.py).Benefits
Using emptyDir volumes to store what might be a non-negligible amount of data is sometimes not desirable. This change keeps the old behavior by default but provides more options for ephemeral data storage.
Possible drawbacks
Applicable issues
Additional information
Checklist
Chart.yamlaccording to semver. This is not necessary when the changes only affect README.md files.README.mdusing readme-generator-for-helm