thesuperzapper
commented
1 year ago @iSWATxJOKERi Thanks for the PR, however, I am still not sold that there is a legitimate use case that needs us to expose the lifecycle configs to the user. For example, in your ca-certificates use case, a much safer (and already available option) is to use extraInitContainers, and possibly also shareProcessNamespace.
Furthermore, using lifecycle hooks might result in unexpected behavior, specifically:
preStopcommands are not guaranteed by Kubernetes to be run before the containers ENTRYPOINT, so there is a possible race condition- the hooks can actually be delivered MORE than once!
While I can sort-of imagine (very niche) use cases for postStart hooks, if we choose to expose their functionality to the user, we must put strong warnings about possible unexpected behavior, and recommend using init-containers in any use case that needs something to run BEFORE the pod starts.
If we were to implement a feature like this, here are my initial thoughts on your PR
- You have only added this ability to the
airflow.kubernetesPodTemplatepods, we should add values for all the deployments/pods and probably also have a "global" one under anairflow.xxxxvalue - Technically the correct link for the "spec" of
Lifecycle v1 coreishttps://kubernetes.io/docs/reference/generated/kubernetes-api/v1.20/#lifecycle-v1-core, so that's a better link for the docstring.
I also want to note that we currently use the preStop hook in the workers.celery.gracefullTermination feature for worker pods, so allowing users to set their own lifecycle would not be possible alongside this feature. Given this, I think it's probably best to not expose the full lifecycle spec, but rather create more focused xxxx.postStartScript and xxxx.preStopScript values (that take a string), so that we can do whatever we want in the chart and then run the users extra script (if they have provided one).
We can create these values for all of our deployments, but also make one called airflow.postStartScript and airflow.preStopScript that will set the script on all our airflow deployments/pods. (NOTE: I am not sure if airflow.preStopScript should override workers.preStopScript, or be run in addition to it)
For situations when we have our own commands that we want to run (like in the workers.celery.gracefullTermination feature), we would run whatever our chart-provided command was, and then run the user's one AFTER.
An example of what a workers.preStopScript value might look like:
workers:
preStopScript: |
#!/usr/bin/env bash
echo "Hello world!"
iSWATxJOKERi
stale[bot]
What issues does your PR fix? Sometimes it is necessary to perform a certain action/do something when a container has started. Other's have also had this problem: #463
In my team's case for example, after we've read secrets from Vault and mounted them in a directory, let's say,
usr/local/share/ca-certificates, we want to add these certificates files to the trust store inetc/ssl/certs/ca-certificates.crtby runningupdate-ca-certificates.The PR allows you to define the container lifecycle hook in the kubernetesPodTemplate, something that was previously only possible by adding in the whole of the pod template via the airflow.kubernetesPodTemplate.stringOverride variable which can be very difficult to do (didn't successfully do it) because it's hard to tell exactly what needs to go into the full pod template of the
stringOverridekey in order to replicate what you get when you look at the output of a pod spec as-is with-o yaml.What does your PR do? Adds the airflow.kubernetesPodTemplate.lifecycle value to set container lifecycle hooks on the KubernetesExecutor pod_template. Other's have also had this problem
Checklist
For all Pull Requests
For releasing ONLY