You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "amoghrajesh (via GitHub)" <gi...@apache.org> on 2023/02/28 15:33:44 UTC
[GitHub] [airflow] amoghrajesh opened a new pull request, #29809: Adding configuration to control retry parameters for k8s api client
amoghrajesh opened a new pull request, #29809:
URL: https://github.com/apache/airflow/pull/29809
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
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](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
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](https://github.com/apache/airflow/tree/main/newsfragments).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] hussein-awala commented on a diff in pull request #29809: Adding configuration to control retry parameters for k8s api client
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #29809:
URL: https://github.com/apache/airflow/pull/29809#discussion_r1139851471
##########
airflow/kubernetes/kube_client.py:
##########
@@ -107,23 +110,36 @@ def get_kube_client(
if conf.getboolean("kubernetes_executor", "enable_tcp_keepalive"):
_enable_tcp_keepalive()
+ new_client_config = _get_default_configuration()
+ api_client_retry_configuration = conf.getjson("kubernetes", "api_client_retry_configuration", fallback={})
+
+ if not conf.getboolean("kubernetes_executor", "verify_ssl"):
+ _disable_verify_ssl()
+
+ if isinstance(api_client_retry_configuration, dict):
+ new_client_config.retries = urllib3.util.Retry(**api_client_retry_configuration)
+ elif isinstance(api_client_retry_configuration, unittest.mock.MagicMock):
Review Comment:
You cannot do this to fix unit tests, you should add the config in all the failed tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on a diff in pull request #29809: Adding configuration to control retry parameters for k8s api client
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29809:
URL: https://github.com/apache/airflow/pull/29809#discussion_r1125697080
##########
airflow/kubernetes/kube_client.py:
##########
@@ -105,16 +107,28 @@ def get_kube_client(
if conf.getboolean("kubernetes_executor", "enable_tcp_keepalive"):
_enable_tcp_keepalive()
+ new_client_config = Configuration.get_default_copy()
+ api_client_retry_configuration = conf.getjson("kubernetes", "api_client_retry_configuration", fallback={})
+
+ if not conf.getboolean("kubernetes_executor", "verify_ssl"):
+ _disable_verify_ssl()
+
+ if isinstance(api_client_retry_configuration, dict) and api_client_retry_configuration != {}:
+ new_client_config.retries = urllib3.util.Retry(**api_client_retry_configuration)
+ elif isinstance(api_client_retry_configuration, dict) and api_client_retry_configuration == {}:
+ pass
Review Comment:
Thanks for the idea. Made the required changes and pushed it in my latest commit
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on a diff in pull request #29809: Adding configuration to control retry parameters for k8s api client
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29809:
URL: https://github.com/apache/airflow/pull/29809#discussion_r1128406711
##########
airflow/kubernetes/kube_client.py:
##########
@@ -105,16 +107,28 @@ def get_kube_client(
if conf.getboolean("kubernetes_executor", "enable_tcp_keepalive"):
_enable_tcp_keepalive()
+ new_client_config = Configuration.get_default_copy()
+ api_client_retry_configuration = conf.getjson("kubernetes", "api_client_retry_configuration", fallback={})
+
+ if not conf.getboolean("kubernetes_executor", "verify_ssl"):
+ _disable_verify_ssl()
+
+ if isinstance(api_client_retry_configuration, dict) and api_client_retry_configuration != {}:
+ new_client_config.retries = urllib3.util.Retry(**api_client_retry_configuration)
+ elif isinstance(api_client_retry_configuration, dict) and api_client_retry_configuration == {}:
+ pass
Review Comment:
@hussein-awala without these changes, we cannot get the VC working. Should I revert back this portion?
```
FAILED [100%]
tests/kubernetes/test_client.py:42 (TestClient.test_load_config_disable_ssl)
self = <tests.kubernetes.test_client.TestClient object at 0x10c1b2280>
conf = <MagicMock name='conf' id='4500751600'>
config = <MagicMock name='config' id='4500993264'>
@mock.patch("airflow.kubernetes.kube_client.config")
@mock.patch("airflow.kubernetes.kube_client.conf")
def test_load_config_disable_ssl(self, conf, config):
conf.getboolean.return_value = False
> get_kube_client(in_cluster=False)
test_client.py:47:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
in_cluster = False, cluster_context = None, config_file = None
def get_kube_client(
in_cluster: bool = conf.getboolean("kubernetes_executor", "in_cluster"),
cluster_context: str | None = None,
config_file: str | None = None,
) -> client.CoreV1Api:
"""
Retrieves Kubernetes client.
:param in_cluster: whether we are in cluster
:param cluster_context: context of the cluster
:param config_file: configuration file
:return kubernetes client
:rtype client.CoreV1Api
"""
if not has_kubernetes:
raise _import_err
if conf.getboolean("kubernetes_executor", "enable_tcp_keepalive"):
_enable_tcp_keepalive()
new_client_config = Configuration.get_default_copy()
api_client_retry_configuration = conf.getjson("kubernetes", "api_client_retry_configuration", fallback={})
if not conf.getboolean("kubernetes_executor", "verify_ssl"):
_disable_verify_ssl()
if isinstance(api_client_retry_configuration, dict):
new_client_config.retries = urllib3.util.Retry(**api_client_retry_configuration)
else:
> raise ValueError("api_client_retry_configuration should be a dictionary")
E ValueError: api_client_retry_configuration should be a dictionary
../../airflow/kubernetes/kube_client.py:119: ValueError
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on pull request #29809: Adding configuration to control retry parameters for k8s api client
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #29809:
URL: https://github.com/apache/airflow/pull/29809#issuecomment-1509092413
Thank you for assisting with the test case failure @hussein-awala
Was caught up with personal priority issues
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #29809: Adding configuration to control retry parameters for k8s api client
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29809:
URL: https://github.com/apache/airflow/pull/29809#issuecomment-1478425969
Tests need to be fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on a diff in pull request #29809: Adding configuration to control retry parameters for k8s api client
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29809:
URL: https://github.com/apache/airflow/pull/29809#discussion_r1141886925
##########
tests/kubernetes/test_client.py:
##########
@@ -81,3 +85,19 @@ def test_disable_verify_ssl(self):
else:
configuration = Configuration()
assert not configuration.verify_ssl
+
+ def test_api_client_retry_configuration_default(self):
+ conf = initialize_config()
+ api_client_retry_configuration = conf.getjson(
+ "kubernetes", "api_client_retry_configuration", fallback={}
+ )
+
+ assert api_client_retry_configuration == {}
Review Comment:
Good catch. Removing the test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on a diff in pull request #29809: Adding configuration to control retry parameters for k8s api client
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29809:
URL: https://github.com/apache/airflow/pull/29809#discussion_r1141886630
##########
airflow/kubernetes/kube_client.py:
##########
@@ -107,23 +109,34 @@ def get_kube_client(
if conf.getboolean("kubernetes_executor", "enable_tcp_keepalive"):
_enable_tcp_keepalive()
+ new_client_config = _get_default_configuration()
Review Comment:
Thank you. I still see the same number of changes, however pushing the patch for this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] hussein-awala commented on a diff in pull request #29809: Adding configuration to control retry parameters for k8s api client
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #29809:
URL: https://github.com/apache/airflow/pull/29809#discussion_r1166065617
##########
tests/kubernetes/test_client.py:
##########
@@ -81,3 +84,11 @@ def test_disable_verify_ssl(self):
else:
configuration = Configuration()
assert not configuration.verify_ssl
+
+ @mock.patch("kubernetes.config.incluster_config.InClusterConfigLoader")
+ @conf_vars({("kubernetes", "api_client_retry_configuration"): '{"total": 3, "backoff_factor": 0.5}'})
+ def test_api_client_retry_configuration_correct_values(self, mock_in_cluster_loader):
+ get_kube_client(in_cluster=True)
+ client_configuration = mock_in_cluster_loader().load_and_set.call_args.args[0]
Review Comment:
I think this can solve the problem for python3.7
```suggestion
client_configuration = mock_in_cluster_loader().load_and_set.call_args[0][0]
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #29809: Adding configuration to control retry parameters for k8s api client
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29809:
URL: https://github.com/apache/airflow/pull/29809#issuecomment-1454826706
Tests need fixing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on a diff in pull request #29809: Adding configuration to control retry parameters for k8s api client
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29809:
URL: https://github.com/apache/airflow/pull/29809#discussion_r1125697158
##########
tests/kubernetes/test_client.py:
##########
@@ -78,3 +80,23 @@ def test_disable_verify_ssl(self):
else:
configuration = Configuration()
assert not configuration.verify_ssl
+
+ def test_api_client_retry_configuration_default(self):
+ conf = initialize_config()
+ api_client_retry_configuration = conf.getjson(
+ "kubernetes", "api_client_retry_configuration", fallback={}
+ )
+
+ assert api_client_retry_configuration == {}
+
+ @mock.patch("airflow.kubernetes.kube_client.conf")
+ @conf_vars({("kubernetes", "api_client_retry_configuration"): '{"total": 3, "backoff_factor": 0.5}'})
+ def test_api_client_retry_configuration_correct_values(self, conf):
+ get_kube_client(in_cluster=False)
+ conf.getboolean.assert_called_with("kubernetes", "api_client_retry_configuration")
+
+ api_client_retry_configuration = conf.getjson(
+ "kubernetes", "api_client_retry_configuration", fallback={}
+ )
+
+ assert api_client_retry_configuration == {"total": 3, "backoff_factor": 0.5}
Review Comment:
Thank you. Was not aware of this. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on pull request #29809: Adding configuration to control retry parameters for k8s api client
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #29809:
URL: https://github.com/apache/airflow/pull/29809#issuecomment-1481296626
@hussein-awala I am not sure why the tests are failing in the CI but not in my local development environment.
See the below screenshot that shows the variable: `client_configuration`
<img width="1791" alt="image" src="https://user-images.githubusercontent.com/35884252/227234223-d1251334-1807-4b49-818a-82490cc6b682.png">
We can also see that that `client_configuration.retries` is a valid object as opposed by the CI
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] hterik commented on a diff in pull request #29809: Adding configuration to control retry parameters for k8s api client
Posted by "hterik (via GitHub)" <gi...@apache.org>.
hterik commented on code in PR #29809:
URL: https://github.com/apache/airflow/pull/29809#discussion_r1135041256
##########
airflow/kubernetes/kube_client.py:
##########
@@ -105,16 +107,26 @@ def get_kube_client(
if conf.getboolean("kubernetes_executor", "enable_tcp_keepalive"):
_enable_tcp_keepalive()
+ new_client_config = Configuration.get_default_copy()
+ api_client_retry_configuration = conf.getjson("kubernetes", "api_client_retry_configuration", fallback={})
+
+ if not conf.getboolean("kubernetes_executor", "verify_ssl"):
+ _disable_verify_ssl()
+
+ if isinstance(api_client_retry_configuration, dict) and api_client_retry_configuration != {}:
+ new_client_config.retries = urllib3.util.Retry(**api_client_retry_configuration)
+ else:
+ raise ValueError("api_client_retry_configuration should be a dictionary")
Review Comment:
Disagree, better fail fast than expect operators to read every single line of the log. What is the use case of allowing a partially invalid config file?
But if airflow already has some convention on how it deals with this in other places then that is better to follow.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on a diff in pull request #29809: Adding configuration to control retry parameters for k8s api client
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29809:
URL: https://github.com/apache/airflow/pull/29809#discussion_r1141617303
##########
airflow/kubernetes/kube_client.py:
##########
@@ -107,23 +110,36 @@ def get_kube_client(
if conf.getboolean("kubernetes_executor", "enable_tcp_keepalive"):
_enable_tcp_keepalive()
+ new_client_config = _get_default_configuration()
+ api_client_retry_configuration = conf.getjson("kubernetes", "api_client_retry_configuration", fallback={})
+
+ if not conf.getboolean("kubernetes_executor", "verify_ssl"):
+ _disable_verify_ssl()
+
+ if isinstance(api_client_retry_configuration, dict):
+ new_client_config.retries = urllib3.util.Retry(**api_client_retry_configuration)
+ elif isinstance(api_client_retry_configuration, unittest.mock.MagicMock):
Review Comment:
Yeah, we can do cleaner. Thanks for the feedback. Made the required changes and fixed the UTs
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a diff in pull request #29809: Adding configuration to control retry parameters for k8s api client
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on code in PR #29809:
URL: https://github.com/apache/airflow/pull/29809#discussion_r1141851658
##########
tests/kubernetes/test_client.py:
##########
@@ -81,3 +85,19 @@ def test_disable_verify_ssl(self):
else:
configuration = Configuration()
assert not configuration.verify_ssl
+
+ def test_api_client_retry_configuration_default(self):
+ conf = initialize_config()
+ api_client_retry_configuration = conf.getjson(
+ "kubernetes", "api_client_retry_configuration", fallback={}
+ )
+
+ assert api_client_retry_configuration == {}
Review Comment:
Remove this test, It's not testing anything other than the upstream `fallback` behaviour.
##########
airflow/kubernetes/kube_client.py:
##########
@@ -107,23 +109,34 @@ def get_kube_client(
if conf.getboolean("kubernetes_executor", "enable_tcp_keepalive"):
_enable_tcp_keepalive()
+ new_client_config = _get_default_configuration()
Review Comment:
Nit: you can make the configuration smaller by keeping this variable
```suggestion
configuration= _get_default_configuration()
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk closed pull request #29809: Adding configuration to control retry parameters for k8s api client
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk closed pull request #29809: Adding configuration to control retry parameters for k8s api client
URL: https://github.com/apache/airflow/pull/29809
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] hussein-awala commented on a diff in pull request #29809: Adding configuration to control retry parameters for k8s api client
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #29809:
URL: https://github.com/apache/airflow/pull/29809#discussion_r1125584719
##########
tests/kubernetes/test_client.py:
##########
@@ -78,3 +80,23 @@ def test_disable_verify_ssl(self):
else:
configuration = Configuration()
assert not configuration.verify_ssl
+
+ def test_api_client_retry_configuration_default(self):
+ conf = initialize_config()
+ api_client_retry_configuration = conf.getjson(
+ "kubernetes", "api_client_retry_configuration", fallback={}
+ )
+
+ assert api_client_retry_configuration == {}
+
+ @mock.patch("airflow.kubernetes.kube_client.conf")
+ @conf_vars({("kubernetes", "api_client_retry_configuration"): '{"total": 3, "backoff_factor": 0.5}'})
+ def test_api_client_retry_configuration_correct_values(self, conf):
+ get_kube_client(in_cluster=False)
+ conf.getboolean.assert_called_with("kubernetes", "api_client_retry_configuration")
+
+ api_client_retry_configuration = conf.getjson(
+ "kubernetes", "api_client_retry_configuration", fallback={}
+ )
+
+ assert api_client_retry_configuration == {"total": 3, "backoff_factor": 0.5}
Review Comment:
You can replace it by this test:
```suggestion
@mock.patch("kubernetes.config.incluster_config.InClusterConfigLoader")
@conf_vars({("kubernetes", "api_client_retry_configuration"): '{"total": 3, "backoff_factor": 0.5}'})
def test_api_client_retry_configuration_correct_values(self, mock_in_cluster_loader):
get_kube_client(in_cluster=True)
client_configuration = mock_in_cluster_loader().load_and_set.call_args.args[0]
assert client_configuration.retries.total == 3
assert client_configuration.retries.backoff_factor == 0.5
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on a diff in pull request #29809: Adding configuration to control retry parameters for k8s api client
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29809:
URL: https://github.com/apache/airflow/pull/29809#discussion_r1121507594
##########
airflow/config_templates/config.yml:
##########
@@ -2465,6 +2465,13 @@ kubernetes_executor:
previous_name: kubernetes
version: 2.5.0
options:
+ api_client_retry_configuration:
+ description: |
+ Settings to configure the urllib3.Retry hook, consumed by Kubernetes client.
+ version_added: 2.5.2
Review Comment:
Good catch. Made this change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] hterik commented on a diff in pull request #29809: Adding configuration to control retry parameters for k8s api client
Posted by "hterik (via GitHub)" <gi...@apache.org>.
hterik commented on code in PR #29809:
URL: https://github.com/apache/airflow/pull/29809#discussion_r1135675814
##########
airflow/kubernetes/kube_client.py:
##########
@@ -105,16 +107,26 @@ def get_kube_client(
if conf.getboolean("kubernetes_executor", "enable_tcp_keepalive"):
_enable_tcp_keepalive()
+ new_client_config = Configuration.get_default_copy()
+ api_client_retry_configuration = conf.getjson("kubernetes", "api_client_retry_configuration", fallback={})
+
+ if not conf.getboolean("kubernetes_executor", "verify_ssl"):
+ _disable_verify_ssl()
+
+ if isinstance(api_client_retry_configuration, dict) and api_client_retry_configuration != {}:
+ new_client_config.retries = urllib3.util.Retry(**api_client_retry_configuration)
+ else:
+ raise ValueError("api_client_retry_configuration should be a dictionary")
Review Comment:
Why would you want to do that? If the config is structurally invalid it is better to notify the admin sooner rather than allow the misconfiguration to slip through and only be noticed much later, if it's noticed at all.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #29809: Adding configuration to control retry parameters for k8s api client
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29809:
URL: https://github.com/apache/airflow/pull/29809#issuecomment-1485098906
> 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`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] hterik commented on a diff in pull request #29809: Adding configuration to control retry parameters for k8s api client
Posted by "hterik (via GitHub)" <gi...@apache.org>.
hterik commented on code in PR #29809:
URL: https://github.com/apache/airflow/pull/29809#discussion_r1135695629
##########
airflow/kubernetes/kube_client.py:
##########
@@ -105,16 +107,26 @@ def get_kube_client(
if conf.getboolean("kubernetes_executor", "enable_tcp_keepalive"):
_enable_tcp_keepalive()
+ new_client_config = Configuration.get_default_copy()
+ api_client_retry_configuration = conf.getjson("kubernetes", "api_client_retry_configuration", fallback={})
+
+ if not conf.getboolean("kubernetes_executor", "verify_ssl"):
+ _disable_verify_ssl()
+
+ if isinstance(api_client_retry_configuration, dict) and api_client_retry_configuration != {}:
+ new_client_config.retries = urllib3.util.Retry(**api_client_retry_configuration)
+ else:
+ raise ValueError("api_client_retry_configuration should be a dictionary")
Review Comment:
Do you have example of how the unit test is failing? I can't figure out how to find the github actions for prior commits of a PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] hussein-awala commented on a diff in pull request #29809: Adding configuration to control retry parameters for k8s api client
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #29809:
URL: https://github.com/apache/airflow/pull/29809#discussion_r1120948866
##########
airflow/config_templates/config.yml:
##########
@@ -2465,6 +2465,13 @@ kubernetes_executor:
previous_name: kubernetes
version: 2.5.0
options:
+ api_client_retry_configuration:
+ description: |
+ Settings to configure the urllib3.Retry hook, consumed by Kubernetes client.
+ version_added: 2.5.2
Review Comment:
```suggestion
version_added: 2.6.0
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on a diff in pull request #29809: Adding configuration to control retry parameters for k8s api client
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29809:
URL: https://github.com/apache/airflow/pull/29809#discussion_r1125597806
##########
tests/kubernetes/test_client.py:
##########
@@ -78,3 +80,23 @@ def test_disable_verify_ssl(self):
else:
configuration = Configuration()
assert not configuration.verify_ssl
+
+ def test_api_client_retry_configuration_default(self):
+ conf = initialize_config()
+ api_client_retry_configuration = conf.getjson(
+ "kubernetes", "api_client_retry_configuration", fallback={}
+ )
+
+ assert api_client_retry_configuration == {}
+
+ @mock.patch("airflow.kubernetes.kube_client.conf")
+ @conf_vars({("kubernetes", "api_client_retry_configuration"): '{"total": 3, "backoff_factor": 0.5}'})
+ def test_api_client_retry_configuration_correct_values(self, conf):
+ get_kube_client(in_cluster=False)
+ conf.getboolean.assert_called_with("kubernetes", "api_client_retry_configuration")
+
+ api_client_retry_configuration = conf.getjson(
+ "kubernetes", "api_client_retry_configuration", fallback={}
+ )
+
+ assert api_client_retry_configuration == {"total": 3, "backoff_factor": 0.5}
Review Comment:
Added the changes, thanks.
However, I would like to understand what `@mock.patch("kubernetes.config.incluster_config.InClusterConfigLoader")` does @hussein-awala
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on pull request #29809: Adding configuration to control retry parameters for k8s api client
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #29809:
URL: https://github.com/apache/airflow/pull/29809#issuecomment-1470782793
@hussein-awala i think we should go back to throwing an exception when the type is invalid for the retry parameters.
I need some help in figuring out a way to fix the other failing unit tests due to the exception. The first few unit tests in the test_client.py fail because they have the mock dict type which doesn't enter the if condition and hence returns an exception.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] hussein-awala commented on pull request #29809: Adding configuration to control retry parameters for k8s api client
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on PR #29809:
URL: https://github.com/apache/airflow/pull/29809#issuecomment-1484355798
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on a diff in pull request #29809: Adding configuration to control retry parameters for k8s api client
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29809:
URL: https://github.com/apache/airflow/pull/29809#discussion_r1125220194
##########
airflow/kubernetes/kube_client.py:
##########
@@ -105,16 +107,24 @@ def get_kube_client(
if conf.getboolean("kubernetes_executor", "enable_tcp_keepalive"):
_enable_tcp_keepalive()
+ new_client_config = Configuration.get_default_copy()
+ api_client_retry_configuration = conf.getjson("kubernetes", "api_client_retry_configuration", fallback={})
+
+ if not conf.getboolean("kubernetes_executor", "verify_ssl"):
+ _disable_verify_ssl()
+
+ if api_client_retry_configuration != {}:
+ new_client_config.retries = urllib3.util.Retry(api_client_retry_configuration)
Review Comment:
Made this change, thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #29809: Adding configuration to control retry parameters for k8s api client
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29809:
URL: https://github.com/apache/airflow/pull/29809#issuecomment-1484236170
Try it in Breeze. Breeze is the "common" environment for everyone - if you build the image and run tests in it, it should - in principle fail in the same way. Usually if you have difference vs your local venv and breeze it means that you have **some** difference vs. the curent environment. You need to track what it is (or simply make sure your tests bass in Breeze - which is the same as CI environment. It is preisely targeted to avoid this kind of situatiobn which are known as "works for me" syndrom. Part of your task is to make sure that it not only works for you but also for everyone else.
You can even (see the error message as output. Run a command line to pull and execute the exact very same image that was used to run the test in CI (and fail). Try to do it - the error message even tells you how to run a breeze command to not only pull the image but also enter it in an interactive session so that you can run the test and iterate with it (and inspect it- for exmple comparing which versions of libraries you have).
You need to investigate following it this way,
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #29809: Adding configuration to control retry parameters for k8s api client
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29809:
URL: https://github.com/apache/airflow/pull/29809#issuecomment-1484237730
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #29809: Adding configuration to control retry parameters for k8s api client
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29809:
URL: https://github.com/apache/airflow/pull/29809#issuecomment-1485100810
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #29809: Adding configuration to control retry parameters for k8s api client
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29809:
URL: https://github.com/apache/airflow/pull/29809#issuecomment-1485102113
Closed/Reopened to rebuild with the "full tests needed" label
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on a diff in pull request #29809: Adding configuration to control retry parameters for k8s api client
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29809:
URL: https://github.com/apache/airflow/pull/29809#discussion_r1135090818
##########
airflow/kubernetes/kube_client.py:
##########
@@ -105,16 +107,26 @@ def get_kube_client(
if conf.getboolean("kubernetes_executor", "enable_tcp_keepalive"):
_enable_tcp_keepalive()
+ new_client_config = Configuration.get_default_copy()
+ api_client_retry_configuration = conf.getjson("kubernetes", "api_client_retry_configuration", fallback={})
+
+ if not conf.getboolean("kubernetes_executor", "verify_ssl"):
+ _disable_verify_ssl()
+
+ if isinstance(api_client_retry_configuration, dict) and api_client_retry_configuration != {}:
+ new_client_config.retries = urllib3.util.Retry(**api_client_retry_configuration)
+ else:
+ raise ValueError("api_client_retry_configuration should be a dictionary")
Review Comment:
No such use case, but we can log an error and fallback to using the default settings instead. Will that be a better idea @hussein-awala @hterik?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on pull request #29809: Adding configuration to control retry parameters for k8s api client
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #29809:
URL: https://github.com/apache/airflow/pull/29809#issuecomment-1450143569
@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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] hussein-awala commented on a diff in pull request #29809: Adding configuration to control retry parameters for k8s api client
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #29809:
URL: https://github.com/apache/airflow/pull/29809#discussion_r1134606251
##########
airflow/kubernetes/kube_client.py:
##########
@@ -105,16 +107,26 @@ def get_kube_client(
if conf.getboolean("kubernetes_executor", "enable_tcp_keepalive"):
_enable_tcp_keepalive()
+ new_client_config = Configuration.get_default_copy()
+ api_client_retry_configuration = conf.getjson("kubernetes", "api_client_retry_configuration", fallback={})
+
+ if not conf.getboolean("kubernetes_executor", "verify_ssl"):
+ _disable_verify_ssl()
+
+ if isinstance(api_client_retry_configuration, dict) and api_client_retry_configuration != {}:
+ new_client_config.retries = urllib3.util.Retry(**api_client_retry_configuration)
+ else:
+ raise ValueError("api_client_retry_configuration should be a dictionary")
Review Comment:
I think no need to raise an exception, we can just log an error message and skip configuring the client
```suggestion
log.error("api_client_retry_configuration should be a dictionary.")
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] hterik commented on a diff in pull request #29809: Adding configuration to control retry parameters for k8s api client
Posted by "hterik (via GitHub)" <gi...@apache.org>.
hterik commented on code in PR #29809:
URL: https://github.com/apache/airflow/pull/29809#discussion_r1135729101
##########
airflow/kubernetes/kube_client.py:
##########
@@ -105,16 +107,26 @@ def get_kube_client(
if conf.getboolean("kubernetes_executor", "enable_tcp_keepalive"):
_enable_tcp_keepalive()
+ new_client_config = Configuration.get_default_copy()
+ api_client_retry_configuration = conf.getjson("kubernetes", "api_client_retry_configuration", fallback={})
+
+ if not conf.getboolean("kubernetes_executor", "verify_ssl"):
+ _disable_verify_ssl()
+
+ if isinstance(api_client_retry_configuration, dict) and api_client_retry_configuration != {}:
+ new_client_config.retries = urllib3.util.Retry(**api_client_retry_configuration)
+ else:
+ raise ValueError("api_client_retry_configuration should be a dictionary")
Review Comment:
Put a breakpoint where it is raising the exception or add the actual type+value as part of the exception message, to help with the debugging and see what you actually get back.
I suspect it's the `and api_client_retry_configuration != {}` that should be exempt from the exception-condition. Because that is the value you get as fallback when the config is not provided. Missing value is still a valid configuration and should be allowed with fallback, not the same as value-present-but-invalid :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on a diff in pull request #29809: Adding configuration to control retry parameters for k8s api client
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29809:
URL: https://github.com/apache/airflow/pull/29809#discussion_r1139820472
##########
airflow/kubernetes/kube_client.py:
##########
@@ -105,16 +107,26 @@ def get_kube_client(
if conf.getboolean("kubernetes_executor", "enable_tcp_keepalive"):
_enable_tcp_keepalive()
+ new_client_config = Configuration.get_default_copy()
+ api_client_retry_configuration = conf.getjson("kubernetes", "api_client_retry_configuration", fallback={})
+
+ if not conf.getboolean("kubernetes_executor", "verify_ssl"):
+ _disable_verify_ssl()
+
+ if isinstance(api_client_retry_configuration, dict) and api_client_retry_configuration != {}:
+ new_client_config.retries = urllib3.util.Retry(**api_client_retry_configuration)
+ else:
+ raise ValueError("api_client_retry_configuration should be a dictionary")
Review Comment:
Hi @hterik the problem is that, for unit tests such as `test_load_config_disable_ssl`, a mock `conf` is used which is of type: `<class 'unittest.mock.MagicMock'>` resulting in the condition `isinstance(api_client_retry_configuration, dict)` being false. Maybe I should have a check for unit test so that this portion is ok. Let me try a simple solution here so that mock class as well as regular valid value is accepted
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] hussein-awala commented on a diff in pull request #29809: Adding configuration to control retry parameters for k8s api client
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #29809:
URL: https://github.com/apache/airflow/pull/29809#discussion_r1166074222
##########
airflow/kubernetes/kube_client.py:
##########
@@ -125,5 +138,5 @@ def get_kube_client(
if ssl_ca_cert:
configuration.ssl_ca_cert = ssl_ca_cert
- api_client = client.ApiClient(configuration=configuration)
+ api_client = client.ApiClient(configuration)
Review Comment:
Using keyword arguments is always recommended
```suggestion
api_client = client.ApiClient(configuration=configuration)
```
##########
airflow/config_templates/config.yml:
##########
@@ -2570,6 +2570,13 @@ kubernetes_executor:
previous_name: kubernetes
version: 2.5.0
options:
+ api_client_retry_configuration:
+ description: |
+ Settings to configure the Retry hook, consumed by Kubernetes client.
+ version_added: 2.6.0
Review Comment:
```suggestion
Kwargs to override the default urllib3 Retry used in the kubernetes API client
version_added: 2.6.1
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] hussein-awala merged pull request #29809: Adding configuration to control retry parameters for k8s api client
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala merged PR #29809:
URL: https://github.com/apache/airflow/pull/29809
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] hussein-awala commented on a diff in pull request #29809: Adding configuration to control retry parameters for k8s api client
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #29809:
URL: https://github.com/apache/airflow/pull/29809#discussion_r1125695860
##########
tests/kubernetes/test_client.py:
##########
@@ -78,3 +80,23 @@ def test_disable_verify_ssl(self):
else:
configuration = Configuration()
assert not configuration.verify_ssl
+
+ def test_api_client_retry_configuration_default(self):
+ conf = initialize_config()
+ api_client_retry_configuration = conf.getjson(
+ "kubernetes", "api_client_retry_configuration", fallback={}
+ )
+
+ assert api_client_retry_configuration == {}
+
+ @mock.patch("airflow.kubernetes.kube_client.conf")
+ @conf_vars({("kubernetes", "api_client_retry_configuration"): '{"total": 3, "backoff_factor": 0.5}'})
+ def test_api_client_retry_configuration_correct_values(self, conf):
+ get_kube_client(in_cluster=False)
+ conf.getboolean.assert_called_with("kubernetes", "api_client_retry_configuration")
+
+ api_client_retry_configuration = conf.getjson(
+ "kubernetes", "api_client_retry_configuration", fallback={}
+ )
+
+ assert api_client_retry_configuration == {"total": 3, "backoff_factor": 0.5}
Review Comment:
This line patches the class `InClusterConfigLoader` (which is used to load k8s config when in_cluster is True) and replaces it by a mock.
You can check the mock [documentation](https://docs.python.org/3/library/unittest.mock.html#the-patchers) for more info.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] hussein-awala commented on a diff in pull request #29809: Adding configuration to control retry parameters for k8s api client
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #29809:
URL: https://github.com/apache/airflow/pull/29809#discussion_r1125696401
##########
airflow/kubernetes/kube_client.py:
##########
@@ -105,16 +107,28 @@ def get_kube_client(
if conf.getboolean("kubernetes_executor", "enable_tcp_keepalive"):
_enable_tcp_keepalive()
+ new_client_config = Configuration.get_default_copy()
+ api_client_retry_configuration = conf.getjson("kubernetes", "api_client_retry_configuration", fallback={})
+
+ if not conf.getboolean("kubernetes_executor", "verify_ssl"):
+ _disable_verify_ssl()
+
+ if isinstance(api_client_retry_configuration, dict) and api_client_retry_configuration != {}:
+ new_client_config.retries = urllib3.util.Retry(**api_client_retry_configuration)
+ elif isinstance(api_client_retry_configuration, dict) and api_client_retry_configuration == {}:
+ pass
Review Comment:
I think no need to check if `api_client_retry_configuration` is empty or not, where in the two cases it will use the `Retry` default values:
```suggestion
if isinstance(api_client_retry_configuration, dict):
new_client_config.retries = urllib3.util.Retry(**api_client_retry_configuration)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] hussein-awala commented on a diff in pull request #29809: Adding configuration to control retry parameters for k8s api client
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #29809:
URL: https://github.com/apache/airflow/pull/29809#discussion_r1123866188
##########
airflow/kubernetes/kube_client.py:
##########
@@ -105,16 +107,24 @@ def get_kube_client(
if conf.getboolean("kubernetes_executor", "enable_tcp_keepalive"):
_enable_tcp_keepalive()
+ new_client_config = Configuration.get_default_copy()
+ api_client_retry_configuration = conf.getjson("kubernetes", "api_client_retry_configuration", fallback={})
+
+ if not conf.getboolean("kubernetes_executor", "verify_ssl"):
+ _disable_verify_ssl()
+
+ if api_client_retry_configuration != {}:
+ new_client_config.retries = urllib3.util.Retry(api_client_retry_configuration)
Review Comment:
```suggestion
if isinstance(api_client_retry_configuration, dict) and api_client_retry_configuration != {}:
new_client_config.retries = urllib3.util.Retry(**api_client_retry_configuration)
else:
raise ValueError("api_client_retry_configuration should be a dictionary")
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on a diff in pull request #29809: Adding configuration to control retry parameters for k8s api client
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29809:
URL: https://github.com/apache/airflow/pull/29809#discussion_r1135680124
##########
airflow/kubernetes/kube_client.py:
##########
@@ -105,16 +107,26 @@ def get_kube_client(
if conf.getboolean("kubernetes_executor", "enable_tcp_keepalive"):
_enable_tcp_keepalive()
+ new_client_config = Configuration.get_default_copy()
+ api_client_retry_configuration = conf.getjson("kubernetes", "api_client_retry_configuration", fallback={})
+
+ if not conf.getboolean("kubernetes_executor", "verify_ssl"):
+ _disable_verify_ssl()
+
+ if isinstance(api_client_retry_configuration, dict) and api_client_retry_configuration != {}:
+ new_client_config.retries = urllib3.util.Retry(**api_client_retry_configuration)
+ else:
+ raise ValueError("api_client_retry_configuration should be a dictionary")
Review Comment:
Makes sense. What do you suggest @hussein-awala @hterik. I would think that having an exception was the best way but the unit tests dont align. Any thoughts? I am not able to figure out how to get the UTs working too with the exception code
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on pull request #29809: Adding configuration to control retry parameters for k8s api client
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #29809:
URL: https://github.com/apache/airflow/pull/29809#issuecomment-1473257025
@hussein-awala @hterik I have fixed the unit tests on this one by checking on the class. If it is a mock class, we take no action and just pass without an exception.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #29809: Adding configuration to control retry parameters for k8s api client
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29809:
URL: https://github.com/apache/airflow/pull/29809#issuecomment-1485243365
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] amoghrajesh commented on a diff in pull request #29809: Adding configuration to control retry parameters for k8s api client
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #29809:
URL: https://github.com/apache/airflow/pull/29809#discussion_r1135721266
##########
airflow/kubernetes/kube_client.py:
##########
@@ -105,16 +107,26 @@ def get_kube_client(
if conf.getboolean("kubernetes_executor", "enable_tcp_keepalive"):
_enable_tcp_keepalive()
+ new_client_config = Configuration.get_default_copy()
+ api_client_retry_configuration = conf.getjson("kubernetes", "api_client_retry_configuration", fallback={})
+
+ if not conf.getboolean("kubernetes_executor", "verify_ssl"):
+ _disable_verify_ssl()
+
+ if isinstance(api_client_retry_configuration, dict) and api_client_retry_configuration != {}:
+ new_client_config.retries = urllib3.util.Retry(**api_client_retry_configuration)
+ else:
+ raise ValueError("api_client_retry_configuration should be a dictionary")
Review Comment:
@hterik If I make the code to have an exception (state before the last commit), the tests look like this
```
/Users/adesai/.pyenv/versions/airflow-env/bin/python "/Users/adesai/Library/Application Support/JetBrains/GoLand2021.3/plugins/python-ce/helpers/pycharm/_jb_pytest_runner.py" --target test_client.py::TestClient
Testing started at 8:45 PM ...
Launching pytest with arguments test_client.py::TestClient --no-header --no-summary -q in /Users/adesai/Documents/OpenSource/Airflow/airflow/tests/kubernetes
============================= test session starts ==============================
collecting ... collected 7 items
test_client.py::TestClient::test_load_cluster_config ========================= AIRFLOW ==========================
Home of the user: /Users/adesai
Airflow home /Users/adesai/airflow
Skipping initializing of the DB as it was initialized already.
You can re-initialize the database by adding --with-db-init flag when running tests.
FAILED [ 14%]
tests/kubernetes/test_client.py:30 (TestClient.test_load_cluster_config)
self = <tests.kubernetes.test_client.TestClient object at 0x112250df0>
config = <MagicMock name='config' id='4602299968'>
@mock.patch("airflow.kubernetes.kube_client.config")
def test_load_cluster_config(self, config):
> get_kube_client(in_cluster=True)
test_client.py:33:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
in_cluster = True, cluster_context = None, config_file = None
def get_kube_client(
in_cluster: bool = conf.getboolean("kubernetes_executor", "in_cluster"),
cluster_context: str | None = None,
config_file: str | None = None,
) -> client.CoreV1Api:
"""
Retrieves Kubernetes client.
:param in_cluster: whether we are in cluster
:param cluster_context: context of the cluster
:param config_file: configuration file
:return kubernetes client
:rtype client.CoreV1Api
"""
if not has_kubernetes:
raise _import_err
if conf.getboolean("kubernetes_executor", "enable_tcp_keepalive"):
_enable_tcp_keepalive()
new_client_config = Configuration.get_default_copy()
api_client_retry_configuration = conf.getjson("kubernetes", "api_client_retry_configuration", fallback={})
if not conf.getboolean("kubernetes_executor", "verify_ssl"):
_disable_verify_ssl()
if isinstance(api_client_retry_configuration, dict) and api_client_retry_configuration != {}:
new_client_config.retries = urllib3.util.Retry(**api_client_retry_configuration)
else:
> raise ValueError("api_client_retry_configuration should be a dictionary")
E ValueError: api_client_retry_configuration should be a dictionary
../../airflow/kubernetes/kube_client.py:119: ValueError
FAILED [ 28%]
tests/kubernetes/test_client.py:36 (TestClient.test_load_file_config)
self = <tests.kubernetes.test_client.TestClient object at 0x112250d00>
config = <MagicMock name='config' id='4603967328'>
@mock.patch("airflow.kubernetes.kube_client.config")
def test_load_file_config(self, config):
> get_kube_client(in_cluster=False)
test_client.py:39:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
in_cluster = False, cluster_context = None, config_file = None
def get_kube_client(
in_cluster: bool = conf.getboolean("kubernetes_executor", "in_cluster"),
cluster_context: str | None = None,
config_file: str | None = None,
) -> client.CoreV1Api:
"""
Retrieves Kubernetes client.
:param in_cluster: whether we are in cluster
:param cluster_context: context of the cluster
:param config_file: configuration file
:return kubernetes client
:rtype client.CoreV1Api
"""
if not has_kubernetes:
raise _import_err
if conf.getboolean("kubernetes_executor", "enable_tcp_keepalive"):
_enable_tcp_keepalive()
new_client_config = Configuration.get_default_copy()
api_client_retry_configuration = conf.getjson("kubernetes", "api_client_retry_configuration", fallback={})
if not conf.getboolean("kubernetes_executor", "verify_ssl"):
_disable_verify_ssl()
if isinstance(api_client_retry_configuration, dict) and api_client_retry_configuration != {}:
new_client_config.retries = urllib3.util.Retry(**api_client_retry_configuration)
else:
> raise ValueError("api_client_retry_configuration should be a dictionary")
E ValueError: api_client_retry_configuration should be a dictionary
../../airflow/kubernetes/kube_client.py:119: ValueError
FAILED [ 42%]
tests/kubernetes/test_client.py:42 (TestClient.test_load_config_disable_ssl)
self = <tests.kubernetes.test_client.TestClient object at 0x112250e20>
conf = <MagicMock name='conf' id='4604005248'>
config = <MagicMock name='config' id='4604090400'>
@mock.patch("airflow.kubernetes.kube_client.config")
@mock.patch("airflow.kubernetes.kube_client.conf")
def test_load_config_disable_ssl(self, conf, config):
conf.getboolean.return_value = False
> get_kube_client(in_cluster=False)
test_client.py:47:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
in_cluster = False, cluster_context = None, config_file = None
def get_kube_client(
in_cluster: bool = conf.getboolean("kubernetes_executor", "in_cluster"),
cluster_context: str | None = None,
config_file: str | None = None,
) -> client.CoreV1Api:
"""
Retrieves Kubernetes client.
:param in_cluster: whether we are in cluster
:param cluster_context: context of the cluster
:param config_file: configuration file
:return kubernetes client
:rtype client.CoreV1Api
"""
if not has_kubernetes:
raise _import_err
if conf.getboolean("kubernetes_executor", "enable_tcp_keepalive"):
_enable_tcp_keepalive()
new_client_config = Configuration.get_default_copy()
api_client_retry_configuration = conf.getjson("kubernetes", "api_client_retry_configuration", fallback={})
if not conf.getboolean("kubernetes_executor", "verify_ssl"):
_disable_verify_ssl()
if isinstance(api_client_retry_configuration, dict) and api_client_retry_configuration != {}:
new_client_config.retries = urllib3.util.Retry(**api_client_retry_configuration)
else:
> raise ValueError("api_client_retry_configuration should be a dictionary")
E ValueError: api_client_retry_configuration should be a dictionary
../../airflow/kubernetes/kube_client.py:119: ValueError
FAILED [ 57%]
tests/kubernetes/test_client.py:55 (TestClient.test_enable_tcp_keepalive)
self = <tests.kubernetes.test_client.TestClient object at 0x112250100>
def test_enable_tcp_keepalive(self):
socket_options = [
(socket.SOL_SOCKET, socket.SO_KEEPALIVE, 1),
> (socket.IPPROTO_TCP, socket.TCP_KEEPIDLE, 120),
(socket.IPPROTO_TCP, socket.TCP_KEEPINTVL, 30),
(socket.IPPROTO_TCP, socket.TCP_KEEPCNT, 6),
]
E AttributeError: module 'socket' has no attribute 'TCP_KEEPIDLE'
test_client.py:59: AttributeError
PASSED [ 71%]PASSED [ 85%][2023-03-14T20:45:44.378+0530] {configuration.py:1372} INFO - Reading default test configuration from /Users/adesai/Documents/OpenSource/Airflow/airflow/airflow/config_templates/default_test.cfg
[2023-03-14T20:45:44.380+0530] {configuration.py:1375} INFO - Reading test configuration from /Users/adesai/airflow/unittests.cfg
PASSED [100%]
test_client.py::TestClient::test_load_file_config
test_client.py::TestClient::test_load_config_disable_ssl
test_client.py::TestClient::test_enable_tcp_keepalive
test_client.py::TestClient::test_disable_verify_ssl
test_client.py::TestClient::test_api_client_retry_configuration_default
test_client.py::TestClient::test_api_client_retry_configuration_correct_values
========================= 4 failed, 3 passed in 0.89s ==========================
Process finished with exit code 1
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org