Copilot commented on code in PR #59948:
URL: https://github.com/apache/airflow/pull/59948#discussion_r3066481047
##########
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -864,8 +864,18 @@ def read_pod(self, pod: V1Pod) -> V1Pod:
"""Read POD information."""
try:
return self._client.read_namespaced_pod(pod.metadata.name,
pod.metadata.namespace)
+ except ApiException as e:
+ if e.status == 404:
+ self.log.warning("Pod %s not found (404)", pod.metadata.name)
+ raise
+
+ raise KubernetesApiException(
+ f"There was an error reading the kubernetes API: {e}"
+ ) from e
Review Comment:
Catching ApiException and wrapping all non-404 cases in
KubernetesApiException changes retry/exception semantics: generic_api_retry
will now retry on *any* non-404 ApiException (including 400/401/403), and
callers that rely on ApiException.status (e.g. credential refresh paths) will
no longer see the ApiException. Consider letting ApiException propagate
unchanged (so generic_api_retry can apply status-based transient retries), and
only special-case 404 into a dedicated exception/message.
```suggestion
raise
```
##########
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -31,7 +31,7 @@
import pendulum
from kubernetes import client, watch
-from kubernetes.client.rest import ApiException
+from kubernetes.client.exceptions import ApiException
Review Comment:
This switches the sync Kubernetes ApiException import to
kubernetes.client.exceptions.ApiException, but the retry predicate in
kubernetes_helper_functions._should_retry_api checks
kubernetes.client.rest.ApiException. If these are not the same class in the
installed kubernetes client, retries may stop working as intended. Prefer using
the same ApiException type consistently (or update the retry predicate to
recognize both).
```suggestion
from kubernetes.client.rest import ApiException
```
##########
providers/common/ai/www-hash.txt:
##########
@@ -0,0 +1 @@
+109120dcbfbc188a78b043c1212d9ab54ea105a68fc36bd6fb2d321017c2ba90
Review Comment:
This new hash file appears unrelated to the PR’s stated goal (404 handling
in PodManager.read_pod). If it isn’t required by the build/release process for
this PR, it should be removed to keep the change focused.
##########
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -864,8 +864,18 @@ def read_pod(self, pod: V1Pod) -> V1Pod:
"""Read POD information."""
try:
return self._client.read_namespaced_pod(pod.metadata.name,
pod.metadata.namespace)
+ except ApiException as e:
+ if e.status == 404:
+ self.log.warning("Pod %s not found (404)", pod.metadata.name)
+ raise
+
Review Comment:
The PR description says 404s should raise a clearer, actionable exception,
but this branch only logs a warning and re-raises the original ApiException.
Consider raising PodNotFoundException (including pod name/namespace and hinting
that the pod may have been preempted/deleted) and chaining the original
exception for context.
##########
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -864,8 +864,18 @@ def read_pod(self, pod: V1Pod) -> V1Pod:
"""Read POD information."""
try:
return self._client.read_namespaced_pod(pod.metadata.name,
pod.metadata.namespace)
+ except ApiException as e:
+ if e.status == 404:
Review Comment:
PodManager.read_pod now has new ApiException branching (404 special-case and
wrapping others), but there are no unit tests covering these new behaviors.
Since this module already has unit tests for read_pod retry behavior, please
add tests for (a) 404 handling and (b) ensuring non-transient ApiException
statuses are not retried/rewrapped unexpectedly.
--
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]
