Skip to content
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

Add KubernetesPodOperatorAsync #28523

Closed
wants to merge 6 commits into from

Conversation

phanikumv
Copy link
Contributor

@phanikumv phanikumv commented Dec 21, 2022

This PR donates the KubernetesPodOperatorAsync from astronomer-providers repo to Airflow.

Astronomer and its customers have been running this in production since last 6 months


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

@boring-cyborg boring-cyborg bot added provider:cncf-kubernetes Kubernetes provider related issues area:providers labels Dec 21, 2022
@phanikumv phanikumv marked this pull request as ready for review December 21, 2022 12:12
@phanikumv phanikumv changed the title Add KubernetesPodOperatorAsync Add KubernetesPodOperatorAsync Dec 21, 2022
Copy link
Contributor

@dimberman dimberman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks good to me. My only major question is whether we need to asynchronously load the configuration. otherwise great job!

Comment on lines 479 to 528
await config.load_kube_config(
config_file=kubeconfig_path,
client_configuration=self.client_configuration,
context=cluster_context,
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this function really one that requires async? I imagine loading a kube config would be pretty quick?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're referring to async def _load_config(self) right?

well it does do file operations which are technically blocking (even though they might be fast)

so you might as well handle them asyncly ... just cus... why not?

but now that you mention it one thing that is a bit curious is ... how are we able to await await config.load_kube_config becaus it look like that is not an async method... i must be missing something

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup, @phanikumv this config should come from from kubernetes_asyncio import config instead of from kubernetes import config like it is doing right now.

Please check https://github.com/astronomer/astronomer-providers/blob/main/astronomer/providers/cncf/kubernetes/hooks/kubernetes.py

"You can only use one option at a time."
)
if in_cluster:
self.log.debug("loading kube_config from: in_cluster configuration")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be place more descriptive comment here to describe what does actually in_cluster parameter mean?

Comment on lines 479 to 528
await config.load_kube_config(
config_file=kubeconfig_path,
client_configuration=self.client_configuration,
context=cluster_context,
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're referring to async def _load_config(self) right?

well it does do file operations which are technically blocking (even though they might be fast)

so you might as well handle them asyncly ... just cus... why not?

but now that you mention it one thing that is a bit curious is ... how are we able to await await config.load_kube_config becaus it look like that is not an async method... i must be missing something

airflow/providers/cncf/kubernetes/hooks/kubernetes.py Outdated Show resolved Hide resolved
@kaxil
Copy link
Member

kaxil commented Dec 21, 2022

#28230 was created first, lets get that one merged and than rebase this on top of it to add more features

@phanikumv phanikumv force-pushed the kubernetes_async branch 2 times, most recently from 36d7187 to 04e81e2 Compare January 16, 2023 05:56
@phanikumv
Copy link
Contributor Author

#28230 was created first, lets get that one merged and than rebase this on top of it to add more features

I am working to rebase it and add more features

@phanikumv
Copy link
Contributor Author

Closing as the additional feature on periodic fetching of logs has been implemented in #27758

@phanikumv phanikumv closed this Feb 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers provider:cncf-kubernetes Kubernetes provider related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants