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

Replace blocking IO with async IO in AsyncKubernetesHook #35162

Merged
merged 4 commits into from Oct 26, 2023

Conversation

functicons
Copy link
Contributor

@functicons functicons commented Oct 24, 2023

Currently there are blocking IO operations in AsyncKubernetesHook, which are causing triggerer failures or high CPU usage. This change replaces them with async IO.


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.

Copy link
Member

@hussein-awala hussein-awala left a comment

Choose a reason for hiding this comment

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

I agree that this needs to be improved, I tried to implement a similar solution in the past but I had a problem with an older version of aiofiles, could you add a unit test fo this to check if it works as expected?

@functicons
Copy link
Contributor Author

I agree that this needs to be improved, I tried to implement a similar solution in the past but I had a problem with an older version of aiofiles, could you add a unit test fo this to check if it works as expected?

Thanks for the feedback! Could you give me some pointers on where to add the test?

@functicons
Copy link
Contributor Author

Some checks failed with the following error:

RuntimeError: By default one of Airflow's dependencies installs a GPL dependency (unidecode). To avoid this dependency set SLUGIFY_USES_TEXT_UNIDECODE=yes in your environment when you install or 
upgrade Airflow. To force installing the GPL version set AIRFLOW_GPL_UNIDECODE

I'm not sure if it's caused by my change? Any help is appreciated!

@Taragolis
Copy link
Contributor

I'm not sure if it's caused by my change? Any help is appreciated!

I guess this related to changes in #35099
For some reason apache-airflow downgraded up to 1.10.2 and I assume this error happen there

WARNING: apache-airflow 2.7.2 does not provide the extra 'cncf-kubernetes'
WARNING: apache-airflow 2.7.1 does not provide the extra 'cncf-kubernetes'
WARNING: apache-airflow 2.7.0 does not provide the extra 'cncf-kubernetes'
WARNING: apache-airflow 2.6.3 does not provide the extra 'cncf-kubernetes'
...
WARNING: apache-airflow 1.10.9 does not provide the extra 'cncf-kubernetes'
WARNING: apache-airflow 1.10.8 does not provide the extra 'cncf-kubernetes'
WARNING: apache-airflow 1.10.7 does not provide the extra 'cncf-kubernetes'
WARNING: apache-airflow 1.10.6 does not provide the extra 'cncf-kubernetes'
WARNING: apache-airflow 1.10.5 does not provide the extra 'cncf-kubernetes'
WARNING: apache-airflow 1.10.4 does not provide the extra 'cncf-kubernetes'
WARNING: apache-airflow 1.10.3 does not provide the extra 'cncf-kubernetes'

@Taragolis
Copy link
Contributor

I've checked again, previous error happen because we tried to install Airflow from PyPI with constraints for main branch.
If it failed it tried to install airflow without constraints.

And actual error happen in tests

  ==================================== ERRORS ====================================
  _______________ ERROR collecting test_kubernetes_pod_operator.py _______________
  ImportError while importing test module '/home/runner/work/airflow/airflow/kubernetes_tests/test_kubernetes_pod_operator.py'.
  Hint: make sure your test modules/packages have valid Python names.
  Traceback:
  /opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/importlib/__init__.py:127: in import_module
      return _bootstrap._gcd_import(name[level:], package, level)
  test_kubernetes_pod_operator.py:41: in <module>
      from airflow.providers.cncf.kubernetes.hooks.kubernetes import KubernetesHook
  ../airflow/providers/cncf/kubernetes/hooks/kubernetes.py:25: in <module>
      import aiofiles
  E   ModuleNotFoundError: No module named 'aiofiles'

I would recommend to add aiofiles into the https://github.com/apache/airflow/blob/main/scripts/ci/kubernetes/k8s_requirements.txt as workaround

Jarek @potiuk maybe you know how to deal with new dependencies for K8S Provider in tests without add it into ci requirements?

@potiuk
Copy link
Member

potiuk commented Oct 26, 2023

Hmm. This is an interesting one. The change to use apache-airflow has been implemented few days ago - in order to accomodate an interaction with #34729 PR. Honestly, it's super strange that pip started to backtrack and went back to airflow 1.10 on that one - I think this has somethign to do with this PR installing the environment from scratch (because it comes from PR) - where in the main build we use cached version of it and there airflow is already installed.

I will make a small PR to improve it and ask you to rebase @functicons affter it's merged.

@potiuk
Copy link
Member

potiuk commented Oct 26, 2023

OK. I think I figured out exactly what was going on and have a fix in #35191 that should work in all cases.

The change I implemented to make #34729 work with the new pre-installed common.io provider (I implemented a fix in #35099 ) had the bad side effect that in case of conflicting requirements between constraints and the released airflow, pip went into backtracking loop trying to resolve unresolvable dependencies (we have currently such conflicting requirements because of FAB pinning and recent PR where I upgraded FAB to newer version (#35085).

While looking at it closely I also found out that #35099 change was not really good for implementing changes to KPO (we did not test the local version of it but the released one) but also that the original way it has been implemented, also had the side effect that local testing while modifying KPO was not really using the modified version either.

The #35191 should kill all those birds with single stone. Not good it happened, here, but good it got my attention to those problems :).

@potiuk
Copy link
Member

potiuk commented Oct 26, 2023

Fixed in main. I rebased to see if it helps

@potiuk
Copy link
Member

potiuk commented Oct 26, 2023

Fixed in main. I rebased to see if it helps

It does :)

@hussein-awala
Copy link
Member

My problem was with the CI image too, I thought it was a conflict issue.

For the change, I'm using a similar solution in my operators without any issues, so it's safe to merge this one.

@hussein-awala hussein-awala merged commit d400226 into apache:main Oct 26, 2023
67 checks passed
@ephraimbuddy ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Oct 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) provider:cncf-kubernetes Kubernetes provider related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants