vincbeck commented on code in PR #75251:
URL: https://github.com/apache/airflow/pull/75251#discussion_r4337598962
##########
providers/cncf/kubernetes/tests/unit/cncf/kubernetes/test_kube_client.py:
##########
@@ -118,6 +162,24 @@ async def
test_team_async_factory_takes_precedence_over_the_global_one(self, mon
assert client == FACTORY_MARKER
+ @pytest.mark.skipif(not AIRFLOW_V_3_2_PLUS, reason="team config requires
Airflow 3.2+")
+ @pytest.mark.asyncio
+ async def test_async_factory_is_told_the_team(self, monkeypatch):
+ monkeypatch.setenv(
+ "AIRFLOW__TEAM_A___KUBERNETES_EXECUTOR__ASYNC_CLIENT_FACTORY",
f"{__name__}.build_team_client"
+ )
+
+ assert await _get_executor_async_kube_client(team_name="team_a") ==
(FACTORY_MARKER, "team_a")
+
+ @pytest.mark.asyncio
+ async def
test_executor_async_factory_takes_precedence_over_the_config(self):
+ with conf_vars({("kubernetes_executor", "async_client_factory"):
f"{__name__}.build_other_client"}):
Review Comment:
Not a big deal but we usually use the decorator
##########
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/cli/kubernetes_command.py:
##########
@@ -148,7 +149,8 @@ def _get_pod_completion_time(pod):
@providers_configuration_loaded
def cleanup_pods(args):
"""Clean up k8s pods in evicted/failed/succeeded/pending states."""
- namespace = args.namespace
+ team_name = args.team
+ namespace = args.namespace or conf.get("kubernetes_executor", "namespace",
**_team_kwargs(team_name))
Review Comment:
Another one from my agent:
> No Airflow 3.2 guard on cleanup-pods --team (kubernetes_command.py:153).
generate_pod_yaml checks AIRFLOW_V_3_2_PLUS, but this command doesn't. On
Airflow 2.11 to 3.1, conf.get(team_name=...) will either raise TypeError or
quietly ignore the team.
##########
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/cli/kubernetes_command.py:
##########
@@ -148,7 +149,8 @@ def _get_pod_completion_time(pod):
@providers_configuration_loaded
def cleanup_pods(args):
Review Comment:
And another one :)
> cleanup-pods --team deletes every team's pods in a shared namespace,
because the label selector has no team filter.
##########
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/cli/kubernetes_command.py:
##########
@@ -148,7 +149,8 @@ def _get_pod_completion_time(pod):
@providers_configuration_loaded
def cleanup_pods(args):
Review Comment:
From my agent:
> cleanup-pods --team can hit the wrong cluster
(cli/kubernetes_command.py:183). It builds its client with get_kube_client and
skips the team's client_factory. For a team on AwsEksExecutor with no
in_cluster/config_file set, it falls back to in_cluster=True. It then deletes
pods on the scheduler's own cluster, not the team's.
--
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]
