[GitHub] [airflow] hterik commented on a diff in pull request #26710: Add config to control Kubernetes Client retry behaviour

[GitBox <gi...@apache.org>]

hterik commented on code in PR #26710:
URL: https://github.com/apache/airflow/pull/26710#discussion_r981342716


##########
airflow/kubernetes/kube_client.py:
##########
@@ -19,25 +19,18 @@
 
 import logging
 
+import urllib3
+
 from airflow.configuration import conf
 
 log = logging.getLogger(__name__)
 
 try:
     from kubernetes import client, config
-    from kubernetes.client import Configuration
+    from kubernetes.client import ApiClient, Configuration
     from kubernetes.client.rest import ApiException
 
     has_kubernetes = True
-
-    def _disable_verify_ssl() -> None:
-        if hasattr(Configuration, 'get_default_copy'):
-            configuration = Configuration.get_default_copy()
-        else:
-            configuration = Configuration()
-        configuration.verify_ssl = False
-        Configuration.set_default(configuration)

Review Comment:
   Is it ok to remove this `set_default` and only rely on every other code path going through `get_kube_client`?
   
   I see in `pod_generator` and `TaskInstance` creates `ApiClient` in many places, but only uses it for offline operations.
   
   It's also created by `hooks.kubernetes`, haven't looked into what that does yet.
   
   -----
   
   Maybe it's safer to keep it this way and incorporate the new config using this same method?



##########
airflow/kubernetes/kube_client.py:
##########
@@ -104,16 +97,25 @@ def get_kube_client(
     if conf.getboolean('kubernetes', 'enable_tcp_keepalive'):
         _enable_tcp_keepalive()
 
+    client_config = Configuration.get_default_copy()
+
+    retryparams = conf.getjson('kubernetes', 'client_retry_configuration_kwargs', fallback={})
+    if retryparams != {}:
+        client_config.retries = urllib3.util.Retry(**retryparams)

Review Comment:
   Is this level of configuration granularity good? Or is it enough to only expose the backoff and number?
   I could even go as far as saying some kind of backoff should be enabled by default, without 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