Skip to content

Adding configuration to control retry parameters for k8s api client - #29809

Merged
hussein-awala merged 30 commits into
apache:mainfrom
amoghrajesh:k8sApiServerRetry
Apr 14, 2023
Merged

Adding configuration to control retry parameters for k8s api client#29809
hussein-awala merged 30 commits into
apache:mainfrom
amoghrajesh:k8sApiServerRetry

Conversation

@amoghrajesh

Copy link
Copy Markdown
Contributor

K8s api server doesn't have the option to configure the retry parameters. PR adds support to add these parameters in the default config file as well as config.yml.

closes: #24748

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

@hussein-awala hussein-awala left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you run the pre-commit to fix the static checks (How to prepare a PR), and check why the tests failed?

Comment thread airflow/config_templates/config.yml Outdated
@amoghrajesh
amoghrajesh requested review from hussein-awala and removed request for dstandish and jedcunningham March 1, 2023 11:36
@amoghrajesh

Copy link
Copy Markdown
Contributor Author

@hussein-awala I fixed all the unit tests but I think the staticchecks is still failing and I cannot reason why it is happening.
Can you help in establishing the solution to that?

@hussein-awala hussein-awala left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

you need to add some tests for the provided configs, here is an example to how to mock Airflow config.

Comment thread airflow/kubernetes/kube_client.py Outdated
@potiuk

potiuk commented Mar 4, 2023

Copy link
Copy Markdown
Member

Tests need fixing.

Comment thread tests/kubernetes/test_client.py Outdated
Comment thread airflow/kubernetes/kube_client.py Outdated
@potiuk

potiuk commented Mar 26, 2023

Copy link
Copy Markdown
Member

Or at least it used to write the instructions- I need to see why it did not do it before.

But the detailed instructions are here https://github.com/apache/airflow/blob/main/CI.rst#replicating-the-ci-jobs-locally

@hussein-awala

Copy link
Copy Markdown
Member

It seems like there is an issue in the mock lib (or unittest) installed in the CI image. I had the same problem with breeze, but when I installed mock==5.0.1 in the CI image, and I replaced from unittest import mock by import mock, the test passed without any problem.

I'll check it again, and open a separate PR to fix the issue.

@potiuk

potiuk commented Mar 27, 2023

Copy link
Copy Markdown
Member

I had the same problem with breeze, but when I installed mock==5.0.1 in the CI image, and I replaced from unittest import mock by import mock, the test passed without any problem.

Interesting. We have no non-standard mock installed at all in Airlfow - https://github.com/apache/airflow/blob/constraints-main/constraints-3.7.txt - we are using standard mock that comes built-in in Python. Maybe there is a different behaviour of the python version you use @amoghrajesh and that's why you can't reproduce it ? Or some other mocking library is changing the mock behaviour.

Note that in Breeze we are using (automatically - it gest upgraded behind the scenes right after it gets released and tests pass) the latest version of released Python in given Python line for example currently it is. For example for Python 3.7 it is.

root@ee68a99cdb58:/opt/airflow# python --version
Python 3.7.16
root@ee68a99cdb58:/opt/airflow# 

But maybe you are not using Python 3.7 at all (the tests in PR use the lowest supported Python version - which is 3.7 and maybe this is simply difference vs. 3.7 and 3.8 that you need to handle differently ? We still suport 3.7 till July: https://github.com/apache/airflow/blob/main/README.md#support-for-python-and-kubernetes-versions

So maybe the way to reproduce it locally is build your Python venv from latest 3.7 and make sure you have no extra "mock" installed (if you have it installed in your environment @amoghrajesh ) and then you will be able to reproduce.

If that's the case that you can reproduce it this way, then it's the classic "works for me" case and precisely what breeze/ci of ours is supposed to catch before it makes it into main.

@potiuk

potiuk commented Mar 27, 2023

Copy link
Copy Markdown
Member

BTW. I can test the Python 3.7 hypothesis. I can add "full tests needed" to the PR and then it will run a matrix of tests with different Python versions and we will see if this is the reason.

@potiuk potiuk added the full tests needed We need to run full set of tests for this PR to merge label Mar 27, 2023
@potiuk potiuk closed this Mar 27, 2023
@potiuk potiuk reopened this Mar 27, 2023
@potiuk

potiuk commented Mar 27, 2023

Copy link
Copy Markdown
Member

Closed/Reopened to rebuild with the "full tests needed" label

@potiuk

potiuk commented Mar 27, 2023

Copy link
Copy Markdown
Member

Yep. This was a good hypothesis. Seems that this tests does not work for Python 3.7 only You should fix it so it also works there. Switching to Python 3.7 in your local venv should give you reproducible case. Classic "works for me" problem as I thought.

Comment thread tests/kubernetes/test_client.py Outdated
Comment thread airflow/config_templates/config.yml Outdated
Comment thread airflow/kubernetes/kube_client.py Outdated
@hussein-awala hussein-awala added this to the Airflow 2.6.1 milestone Apr 13, 2023
@hussein-awala
hussein-awala requested review from ashb and hterik April 13, 2023 23:49
@potiuk potiuk modified the milestones: Airflow 2.6.1, Airflow 2.6.0 Apr 14, 2023
@hussein-awala
hussein-awala merged commit dcffbb4 into apache:main Apr 14, 2023
ephraimbuddy pushed a commit that referenced this pull request Apr 14, 2023
…29809)

* Adding configuration to control retry parameters for k8s api client

* Handling review comments

* Fixing code bug

* Fixing failing tests

* Temporary commit with UT wip

* Fixing unit test

* Fixing the strict checks

* Handling review comments from Hussein

* Revert "Handling review comments from Hussein"

This reverts commit fa3bc26.

* Fixing failing ut

* Reverting bad hack

* Updating logic in kube_client.py

Co-authored-by: Hussein Awala <hussein@awala.fr>

* Fixing unit tests

* Fixing unit tests

* Handling review comments from Ash

* Fix loading mock call args for python3.7

* Apply suggestions from code review

* fix static check

* add in 2.6.0

---------

Co-authored-by: Amogh <adesai@cloudera.com>
Co-authored-by: Hussein Awala <houssein.awala.96@gmail.com>
(cherry picked from commit dcffbb4)
@amoghrajesh

Copy link
Copy Markdown
Contributor Author

Thank you for assisting with the test case failure @hussein-awala
Was caught up with personal priority issues

wookiist pushed a commit to wookiist/airflow that referenced this pull request Apr 19, 2023
…pache#29809)

* Adding configuration to control retry parameters for k8s api client

* Handling review comments

* Fixing code bug

* Fixing failing tests

* Temporary commit with UT wip

* Fixing unit test

* Fixing the strict checks

* Handling review comments from Hussein

* Revert "Handling review comments from Hussein"

This reverts commit fa3bc26.

* Fixing failing ut

* Reverting bad hack

* Updating logic in kube_client.py

Co-authored-by: Hussein Awala <hussein@awala.fr>

* Fixing unit tests

* Fixing unit tests

* Handling review comments from Ash

* Fix loading mock call args for python3.7

* Apply suggestions from code review

* fix static check

* add in 2.6.0

---------

Co-authored-by: Amogh <adesai@cloudera.com>
Co-authored-by: Hussein Awala <houssein.awala.96@gmail.com>
@ephraimbuddy ephraimbuddy added the type:new-feature Changelog: New Features label May 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

full tests needed We need to run full set of tests for this PR to merge provider:cncf-kubernetes Kubernetes (k8s) provider related issues type:new-feature Changelog: New Features

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Configuring retry policy of the the kubernetes CoreV1Api ApiClient

6 participants