`KubernetesPodOperator` add `container_resources` as templated field by alanatlemba · Pull Request #26982 · apache/airflow

alanatlemba · 2022-10-11T05:49:07Z

This is my first PR for a mainstream large scale open source project, let alone Airflow.
Sorry in advance if I've done anything improper, and thanks for your patience.

Thank you @nitinmuteja & @lior1990 for doing much of the work. This is following up on this closed/abandoned PR: #23531

Here is how I tested:
breeze testing tests --test-type "Providers[cncf]"
breeze k8s run-complete-tests

I'm not sure if further testing is necessary. Should I bother making sure that I see a mapped k8s pod operator dag run with the expected resources? I may need some help working through that. :)

^ 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.

To see the specific tasks where the Asana app for GitHub is being used, see below:
- https://app.asana.com/0/0/1203072091962590

boring-cyborg · 2022-10-11T05:49:11Z

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:

Pay attention to the quality of your code (flake8, mypy and type annotations). Our pre-commits will help you with that.
In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
Be sure to read the Airflow Coding style.
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: [email protected]
Slack: https://s.apache.org/airflow-slack

alanatlemba · 2022-10-12T01:39:33Z

@eladkal thanks for triggering the run before. I'm unfamiliar with the failed. I don't think I touched anything around it, and that test is succeeding on my breeze tests. Could we please re-trigger it? thanks again.

alanatlemba · 2022-10-12T01:41:25Z

perhaps I should just update the branch to re-trigger?

eladkal · 2022-10-13T12:56:38Z

airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py

        'config_file',
        'pod_template_file',
        'namespace',
+        'k8s_resources',


Did you mean container_resources?

@eladkal I wasn't sure, so I went with the comment on the previous PR on this --

#23531 (comment)

I'm also perplexed as to why I'm getting failures on google auth credentials

I don't think this was right?

What users are doing is:

compute_resources=k8s.V1ResourceRequirements( requests={ 'memory': '512Mi', 'cpu': '500m' }, limits={ 'memory': '1Gi', 'cpu': 500m } ) KubernetesPodOperator(..., container_resources=compute_resources)

But what they want to do is instead of specifying hard coded values like 512Mi they want it to be templated so something like:

requests={ 'memory': '{{ dag_run.conf["request_memory"] }} ', 'cpu': '500m' }

note that the k8s_resources is not a field exposed to the users.
It is used mainly for backward compatibility where users used to pass resources in dict rather than V1ResourceRequirements:

airflow/airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py

Line 216 in 53d6804

resources: dict[str, Any] | None = None,

airflow/airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py

Line 193 in 53d6804

container_resources: k8s.V1ResourceRequirements | None = None,

airflow/airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py

Lines 220 to 228 in 53d6804

if isinstance(resources, k8s.V1ResourceRequirements):

warnings.warn(

"Specifying resources for the launched pod with 'resources' is deprecated. "

"Use 'container_resources' instead.",

category=DeprecationWarning,

stacklevel=2,

)

container_resources = resources

resources = None

I think we are also about to remove this support for resources in the next release of the provider so the k8s_resources will no longer be needed? cc @jedcunningham @dstandish

@eladkal thanks for the detailed review. I will get to this later today!

Raised PR #27197

thanks @eladkal I will rebase and re-push this PR once that PR is merged.

#27197 is merged

Huzzah! @eladkal I'll rebase & re-push tomorrow. thx :)

alanatlemba · 2022-10-18T10:20:58Z

@jedcunningham github removed you from the reviewer list when I added @eladkal . this was not intended, and it seems I cannot add you back :(

o-nikolas · 2022-10-18T20:33:06Z

@jedcunningham github removed you from the reviewer list when I added @eladkal . this was not intended, and it seems I cannot add you back :(

I've added Jed back as a reviewer :)

potiuk · 2022-10-26T03:11:59Z

conflicts need resolution.

alanatlemba · 2022-10-26T12:40:59Z

@potiuk awaiting #27197 by @eladkal being merged/closed then I'll resolve the conflicts.

alanatlemba · 2022-11-02T00:37:02Z

I didn't mean to close this - I just updated my fork. I'll just link to this when I create a new PR

alanatlemba · 2022-11-02T01:00:26Z

#27457 raised after rebase. FYI @potiuk , @eladkal @jedcunningham

alanatlemba requested a review from jedcunningham as a code owner October 11, 2022 05:49

boring-cyborg bot added provider:cncf-kubernetes Kubernetes (k8s) provider related issues area:providers labels Oct 11, 2022

alanatlemba force-pushed the main branch from 13ef21c to 7374c3f Compare October 11, 2022 05:54

eladkal changed the title ~~#23529 - Allow KubernetesPodOperator resources to be templated.~~ KubernetesPodOperator add container_resources as templated field Oct 11, 2022

alanatlemba force-pushed the main branch from 0e52365 to 16edb45 Compare October 13, 2022 12:52

eladkal reviewed Oct 13, 2022

View reviewed changes

alanatlemba requested review from eladkal and removed request for jedcunningham October 13, 2022 14:13

o-nikolas requested a review from jedcunningham October 18, 2022 20:32

alanatlemba closed this Nov 2, 2022

alanatlemba force-pushed the main branch from 16edb45 to eda0893 Compare November 2, 2022 00:30

alanatlemba mentioned this pull request Nov 2, 2022

Add container_resources as KubernetesPodOperator templatable #27457

Merged

	if isinstance(resources, k8s.V1ResourceRequirements):
	warnings.warn(
	"Specifying resources for the launched pod with 'resources' is deprecated. "
	"Use 'container_resources' instead.",
	category=DeprecationWarning,
	stacklevel=2,
	)
	container_resources = resources
	resources = None

Comments

Conversation

alanatlemba commented Oct 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

boring-cyborg bot commented Oct 11, 2022

Uh oh!

alanatlemba commented Oct 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alanatlemba commented Oct 12, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eladkal Oct 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alanatlemba commented Oct 18, 2022

Uh oh!

o-nikolas commented Oct 18, 2022

Uh oh!

potiuk commented Oct 26, 2022

Uh oh!

alanatlemba commented Oct 26, 2022

Uh oh!

alanatlemba commented Nov 2, 2022

Uh oh!

alanatlemba commented Nov 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

alanatlemba commented Oct 11, 2022 •

edited

Loading

alanatlemba commented Oct 12, 2022 •

edited

Loading

eladkal Oct 18, 2022 •

edited

Loading