dirrao commented on code in PR #39694:
URL: https://github.com/apache/airflow/pull/39694#discussion_r1605947085
##########
airflow/providers/cncf/kubernetes/operators/pod.py:
##########
@@ -842,16 +838,14 @@ def cleanup(self, pod: k8s.V1Pod, remote_pod: k8s.V1Pod):
if self._killed or not remote_pod:
return
- istio_enabled = self.is_istio_enabled(remote_pod)
pod_phase = remote_pod.status.phase if hasattr(remote_pod, "status")
else None
# if the pod fails or success, but we don't want to delete it
if pod_phase != PodPhase.SUCCEEDED or self.on_finish_action ==
OnFinishAction.KEEP_POD:
self.patch_already_checked(remote_pod, reraise=False)
- failed = (pod_phase != PodPhase.SUCCEEDED and not istio_enabled) or (
- istio_enabled and not container_is_succeeded(remote_pod,
self.base_container_name)
- )
+ # Consider pod failed if the pod has not succeeded and the base
container within the pod has not succeeded
+ failed = pod_phase != PodPhase.SUCCEEDED and not
container_is_succeeded(remote_pod, self.base_container_name)
Review Comment:
Looks like we are not handling the case when it has sidecars. sidecars are
still running event after base container exit. We can't judge the task state
based on the pod phase. Why can't we use base container state alone?
`failed = not container_is_succeeded(remote_pod, self.base_container_name)`
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
