Repository navigation
Can't configure Kubernetes and Celery workers in Helm Chart #28880
Description
Activity
Why not - if someoene would like to pick it, i marked it as good first issue.
I could take this one. How about this proposal? values defined at the "workers" level will be taken by both celery & k8s.
workers: safeToEvict: false celery: resources: limits: cpu: 1 memory: 1Gi requests: cpu: 1 memory: 1Gi kubernetes: resources: limits: cpu: 240m memory: 875Mi requests: cpu: 240m memory: 875Mi
Reacted by SKisContentJust make a PR - we (or others) can easier discuss it there.
Reacted by Carlos Sánchez Páez@potiuk if nobody is actively working on this, I would like to take it up.
Feel free
Is this problem still relevant? It's mostly about
CeleryKubernetesExecutorwhich is no longer needed after AIP-61.Yes this is still an issue because this is more of an issue with the helm chart than it is about the specific executor. If someone wants to configure both a Celery executor and a Kubernetes executor after 2.10 releases they will still encounter this issue.
@eladkal HI, I don't know what the reference to AIP-61 would mean, but we are using CeleryKubernetesExecutor in Airflow v2.8.3. We don't want to fork our own version of the Helm chart, but the existing chart does not permit us to specify separate resource requirements for the Celery and Kubernetes workers. Since the Helm values file schema does not permit any additional sections, the accommodation needs to happen upstream. Here is a PR for that: #41628
@eladkal HI, I don't know what the reference to AIP-61 would mean, but we are using CeleryKubernetesExecutor in Airflow v2.8.3.
AIP-61 means Hybrid Executors. It was released in Airflow 2.10 (see release notes)
This means that we don't need another dedicated executor for any 2 types of executors like:CeleryExecutor+KubernetesExecutor= newCeleryKubernetesExecutor. You can simply have a list of executors as[CeleryExecutor, KubernetesExecutor]
See docs: https://airflow.apache.org/docs/apache-airflow/stable/core-concepts/executor/index.html#statically-coded-hybrid-executorsI slowly started working on resolving this issue by introducing
workers.celeryandworkers.kubernetessections. Below is the list of all commands underworkersand their status (41/41 merged):- replicas - Add workers.celery.replicas field #59730
- revisionHistoryLimit - Add workers.celery.revisionHistoryLimit field #60056
- command - Add workers.celery.command & workers.kubernetes.command #60067
- args - Add workers.celery.args field #60163
- livenessProbe - Add workers.celery.livenessProbe section #60186
- updateStrategy - Add workers.celery.updateStrategy field #60351
- strategy - Add workers.celery.strategy field #60354
- podManagementPolicy - Add workers.celery.podManagementPolicy field #60359
- securityContext - will not be migrated as this field is already deprecated
- securityContexts - Add workers.celery.securityContexts & workers.kubernetes.securityContexts #60396
- containerLifecycleHooks - Add workers.celery.containerLifecycleHooks & workers.kubernetes.containerLifecycleHooks #61369
- podDisruptionBudget - Add workers.celery.podDisruptionBudget #61414
- serviceAccount - Add workers.celery.serviceAccount & workers.kubernetes.serviceAccount #64730
- keda - Add workers.celery.keda section #61820
- hpa - Add workers.celery.hpa #64734
- persistence - Add workers.celery.persistence section #60238
- kerberosSidecar - Add workers.celery.kerberosSidecar & workers.kubernetes.kerberosSidecar sections #61881
- kerberosInitContainer - Add workers.celery.kerberosInitContainer field #60427, Finish workers.celery.kerberosInitContainer & add workers.kubernetes.kerberosInitContainer #60751
- resources - Add workers.celery.resources & workers.kubernetes.resources #61890
- terminationGracePeriodSeconds - Add workers.celery.terminationGracePeriodSeconds & workers.kubernetes.terminationGracePeriodSeconds #61892
- safeToEvict - Add workers.celery.safeToEvict & workers.kubernetes.safeToEvict #61915
- extraContainers - Add workers.celery.extraContainers & workers.kubernetes.extraContainers #64739
- extraInitContainers - Add workers.celery.extraInitContainers & workers.kubernetes.extraInitContainers #64741
- extraVolumes - Add workers.celery.extraVolumes & workers.kubernetes.extraVolumes #64746
- extraVolumeMounts - Add workers.celery.extraVolumeMounts & workers.kubernetes.extraVolumeMounts #65059
- extraPorts - Add workers.celery.extraPorts #61919
- nodeSelector - Add workers.celery.nodeSelector & workers.kubernetes.nodeSelector #61957
- runtimeClassName - Add workers.celery.runtimeClassName & workers.kubernetes.runtimeClassName #61962
- priorityClassName - Add workers.celery.priorityClassName & workers.kubernetes.priorityClassName #61961
- affinity - Add workers.celery.affinity & workers.kubernetes.affinity #64860
- tolerations - Add workers.celery.tolerations & workers.kubernetes.tolerations #64976
- topologySpreadConstraints - Add workers.celery.topologySpreadConstraints & workers.kubernetes.topologySpreadConstraints #64980
- hostAliases - Add workers.celery.hostAliases & workers.kubernetes.hostAliases #61960
- annotations - Add workers.celery.annotations field #64982
- podAnnotations - Add workers.celery.podAnnotations & workers.kubernetes.podAnnotations #65027
- labels - Add workers.celery.labels & workers.kubernetes.labels #65030
- logGroomerSidecar - Add workers.celery.logGroomerSidecar section #65033
- waitForMigrations - Add workers.celery.waitForMigrations section #62054
- env - Add workers.celery.env & workers.kubernetes.env #65056
- volumeClaimTemplates - Add workers.celery.volumeClaimTemplates #62048
- schedulerName - Add workers.celery.schedulerName & workers.kubernetes.schedulerName #62030
As most of these changes will be overlapping and in one huge PR, it could be hard to merge and review (I prepared some time ago one big PR with a change #51460), I plan to do the next PR with the next field after the merge of the previous one.
Reacted by Jens Scheffler and Nils BadtkeI slowly started working on resolving this issue by introducing
workers.celeryandworkers.kubernetessections. Below is the list of all commands underworkersand their status (12/41 merged):- replicas - Add workers.celery.replicas field #59730[x] revisionHistoryLimit - Add workers.celery.revisionHistoryLimit field #60056[x] command - Add workers.celery.command & workers.kubernetes.command #60067[x] args - Add workers.celery.args field #60163[x] livenessProbe - Add workers.celery.livenessProbe section #60186[x] updateStrategy - Add workers.celery.updateStrategy field #60351[x] strategy - Add workers.celery.strategy field #60354[x] podManagementPolicy - Add workers.celery.podManagementPolicy field #60359[x] securityContext - will not be migrated as this field is already deprecated[x] securityContexts - Add workers.celery.securityContexts & workers.kubernetes.securityContexts #60396[ ] containerLifecycleHooks[ ] podDisruptionBudget[x] serviceAccount - Separate workers service accounts #52357[ ] keda[ ] hpa[x] persistence - Add workers.celery.persistence section #60238[ ] kerberosSidecar[ ] kerberosInitContainer[ ] resources[ ] terminationGracePeriodSeconds[ ] safeToEvict[ ] extraContainers[ ] extraInitContainers[ ] extraVolumes[ ] extraVolumeMounts[ ] extraPorts[ ] nodeSelector[ ] runtimeClassName[ ] priorityClassName[ ] affinity[ ] tolerations[ ] topologySpreadConstraints[ ] hostAliases[ ] annotations[ ] podAnnotations[ ] labels[ ] logGroomerSidecar[ ] waitForMigrations[ ] env[ ] volumeClaimTemplates[ ] schedulerName
As most of these changes will be overlapping and in one huge PR, it could be hard to merge and review (I prepared some time ago one big PR with a change #51460), I plan to do the next PR with the next field after the merge of the previous one.
This looks like a huge workload. Do you mind if I pick up some of these files to help lighten the load?
Not at all. Feel free to take some. Just let me know when you will take something. I will link it in the comment above and do a review too
I linked it above. I wasn't quick enough to make a review before the merge tho. Could you take a look at my comments? I believe we should make changes consistent, in terms of behaviour, within the whole
workers->workers.celery/kubernetesmoveReacted by Nils BadtkeReacted by Henry ChenI linked it above. I wasn't quick enough to make a review before the merge tho. Could you take a look at my comments? I believe we should make changes consistent, in terms of behaviour, within the whole
workers->workers.celery/kubernetesmoveSure! Appreciate your reviewing
Not at all. Feel free to take some. Just let me know when you will take something. I will link it in the comment above and do a review too
Can I take logGroomerSidecar, labels, if they are still avaliable？
To be honest, not sure really. In general, they are available, as I didn't start creating PRs for them, but it is mostly because there are many PRs already created, which are waiting for one-by-one merge. I think after the 1.19 release and dropping support for <2.11 versions, we will be going forward with merging them slowly, but it is hard to estimate when merging e.g. logGroomerSidecar would be the best (in some of the already created PRs, I needed to refactor some e.g. test cases to smoothen further changes, but until the merge of particular PR, basically creating a PR for aftected fields would be a work which would need to be repeated after it).
I have some things to do along with this one issue, so if that would be ok with you, we could do it in a way that when we get to the end with merging already created PRs, and somewhere along this way, I will not have a time to create next PRs (due to these different things to do), you could take this up (I would let you know here). What do you think?
Reacted by Henry ChenTo be honest, not sure really. In general, they are available, as I didn't start creating PRs for them, but it is mostly because there are many PRs already created, which are waiting for one-by-one merge. I think after the 1.19 release and dropping support for <2.11 versions, we will be going forward with merging them slowly, but it is hard to estimate when merging e.g. logGroomerSidecar would be the best (in some of the already created PRs, I needed to refactor some e.g. test cases to smoothen further changes, but until the merge of particular PR, basically creating a PR for aftected fields would be a work which would need to be repeated after it).
I have some things to do along with this one issue, so if that would be ok with you, we could do it in a way that when we get to the end with merging already created PRs, and somewhere along this way, I will not have a time to create next PRs (due to these different things to do), you could take this up (I would let you know here). What do you think?
Makes total sense. It's better to avoid repeated work due to refactoring in other PRs. I'm happy to wait for the current queue to clear up a bit. Just ping me here whenever you'd like me to jump in, and I'll take it from there. Thanks for the heads-up!
Reacted by Przemysław MirowskiWow, cool!
@Miretpl the goat :D thank you <3
Official Helm Chart version
1.7.0 (latest released)
Apache Airflow version
2.4.3
Kubernetes Version
1.25
Helm Chart configuration
No response
Docker Image customizations
No response
What happened
Current Helm chart uses the "workers" section to configure Kubernetes or Celery workers parameters (resources, affinity, etc.).
However, when using CeleryKubernetesExecutor, the "workers" section is used to configure the Celery ones, making Kubernetes workers settings available only via podTemplateFile, which can be difficult to manage.
I suggest splitting "workers" into "kubernetesWorkers" (used in pod_template_file) and "celeryWorkers" (used by the celery ones)
What you think should happen instead
Helm Chart should allow to configure both kind of workers in an easy way
How to reproduce
No response
Anything else
No response
Are you willing to submit PR?
Code of Conduct