Skip to content

Add kubernetes_resources to differentiate from Celery worker resources - #41628

Closed
SKisContent wants to merge 9 commits into
apache:mainfrom
SKisContent:main
Closed

SKisContent wants to merge 9 commits into
apache:mainfrom
SKisContent:main

Conversation

@SKisContent

@SKisContent SKisContent commented Aug 21, 2024 •

Copy link
Copy Markdown

This adds a kubernetes_resources setting to the pod template so that those using the CeleryKubernetesExecutor can specify different resource settings for the Celery workers and the Kubernetes workers. In practice the Celery workers will be few in number and probably need more CPU and memory since they will handle multiple tasks, while the Kubernetes workers will be many in number and need much less CPU and memory. However, with the current one-size-fits-both approach, Kubernetes worker pods have to wait to get scheduled because of high initial resource requests.

closes: #28880


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@nevcohen

Copy link
Copy Markdown
Contributor

Now that there is a hybrid executor, we might want an entirely separate set of values ​​for the pod-template? Or it will just be duplication of values?

@SKisContent

Copy link
Copy Markdown
Author

@nevcohen I may not be understanding your question. The proposed change allows for a separate set of resource specifications for the Kubernetes worker pod. In my case, my values.yaml file under the workers section contains this:

  resources:
    limits:
      memory: 8500Mi
    requests:
      cpu: 2000m
      memory: 4500Mi

  # Additional resource requests for the KubernetesExecutor pods
  kubernetes_resources:
   limits:
    memory: 512Mi
   requests:
    cpu: 200m
    memory: 128Mi

Thus the Celery worker pods are scheduled on nodes that have at least 2 vCPUs available, and the Kubernetes worker pods get scheduled on nodes that have 0.2 vCPUs.

@nevcohen

Copy link
Copy Markdown
Contributor

I mean why not add values.podTemplate? Guess in the future there will be more differences that won't be the same in the workers of the celery executor and in the pods of the k8s executor.

I'm just asking, maybe I'm wrong..

@SKisContent

Copy link
Copy Markdown
Author

@nevcohen Are you asking why not use the existing values.podTemplate section? Writing out a whole template seems overkill, requires the user to spend a lot more time on the task, and if in the future the default template changes, the user would have to be aware of that and update the inline template.

The solution in this PR seems like a quick fix for a problem that several people are facing (see related issue), I'm not aware of other parts of the manifest that people want to change.

@nevcohen

nevcohen commented Aug 23, 2024 •

Copy link
Copy Markdown
Contributor

OK, I understand, agree with you for now.

In the future, when more people use CeleryKubernetesExecutor we will need something like this:

worker:
    ...
    resurce:
        ...
    ...
podTemplate:
    ...
    resurce:
        ...
    ...

And the pod-template will look like this:

    ...
      resources: {{- toYaml (or .Values.podTemplate.resources .Values.workers.resources) | nindent 8 }}
    ...

And continue like this for most of the other values of the worker that should also be in the pod-template.

@SKisContent

Copy link
Copy Markdown
Author

Hi, @dstandish @hussein-awala @jedcunningham
Would it be possible to get a yea/nay on this PR so that my org can make a decision on how to go forward?

@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

@github-actions github-actions Bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Oct 22, 2024
@github-actions github-actions Bot closed this Oct 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:helm-chart Airflow Helm Chart stale Stale PRs per the .github/workflows/stale.yml policy file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Can't configure Kubernetes and Celery workers in Helm Chart

2 participants