o-nikolas commented on code in PR #73014:
URL: https://github.com/apache/airflow/pull/73014#discussion_r4021584237
##########
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/kube_client.py:
##########
Review Comment:
What about `get_async_kube_client()`? If the k8s executor is using async pod
creation, they won't get the client factory and that might fail silently back
to a kube config (or fail loudly if nothing else provides auth).
##########
providers/cncf/kubernetes/tests/unit/cncf/kubernetes/test_kube_client.py:
##########
@@ -16,14 +16,115 @@
# under the License.
from __future__ import annotations
+import os
+import subprocess
+import sys
+import textwrap
from unittest import mock
import pytest
-from airflow.providers.cncf.kubernetes.kube_client import
_TimeoutAsyncK8sApiClient, get_async_kube_client
+from airflow.providers.cncf.kubernetes.kube_client import (
+ _TimeoutAsyncK8sApiClient,
+ get_async_kube_client,
+ get_kube_client,
+)
+from airflow.providers.common.compat.sdk import AirflowConfigException
from tests_common.test_utils.config import conf_vars
+FACTORY_MARKER = "client-from-factory"
+
+
+def build_fake_client():
+ return FACTORY_MARKER
+
+
+class TestKubeClientFactory:
Review Comment:
All five tests exercise `get_kube_client` directly. If someone drops the
newly added `use_client_factory=True` from `KubernetesExecutor.start`,
`get_streaming_task_log`, or
`KubernetesJobWatcher.run`, the feature silently stops working and the suite
will stay green. Can you add a small test asserting each of those call sites
requests the
factory?
##########
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/kube_client.py:
##########
@@ -168,15 +168,25 @@ def get_kube_client(
in_cluster: bool | None = None,
cluster_context: str | None = None,
config_file: str | None = None,
+ use_client_factory: bool = False,
) -> client.CoreV1Api:
"""
Retrieve Kubernetes client.
:param in_cluster: whether we are in cluster
:param cluster_context: context of the cluster
:param config_file: configuration file
+ :param use_client_factory: whether to honor the ``client_factory``
setting; only the
+ KubernetesExecutor passes this, so other callers are unaffected by the
setting
:return: kubernetes client
"""
+ # An import path rather than a callable, so that KubernetesJobWatcher can
re-resolve it in
+ # its own process, where the spawn start method would not carry a callable
over.
+ if use_client_factory and (
+ client_factory := conf.getimport("kubernetes_executor",
"client_factory", fallback=None)
Review Comment:
Should this be team aware? Will different teams want to use different
clients?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
