anmolxlight commented on code in PR #66650:
URL: https://github.com/apache/airflow/pull/66650#discussion_r3215047437
##########
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/triggers/pod.py:
##########
@@ -508,6 +508,18 @@ def define_container_state(self, pod: V1Pod) ->
ContainerState:
return ContainerState.TERMINATED if state_obj.exit_code == 0
else ContainerState.FAILED
return ContainerState.UNDEFINED
+ def define_pod_state(self, pod: V1Pod) -> ContainerState:
Review Comment:
Renamed the method to `define_pod_container_state()` and renamed local
variables to `pod_container_state` so the combined pod/container check is
explicit.
##########
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/triggers/pod.py:
##########
@@ -508,6 +508,18 @@ def define_container_state(self, pod: V1Pod) ->
ContainerState:
return ContainerState.TERMINATED if state_obj.exit_code == 0
else ContainerState.FAILED
return ContainerState.UNDEFINED
+ def define_pod_state(self, pod: V1Pod) -> ContainerState:
+ if pod.status is None:
+ return ContainerState.UNDEFINED
+
+ if pod.status.phase == PodPhase.SUCCEEDED:
+ return ContainerState.TERMINATED
+
+ if pod.status.phase == PodPhase.FAILED:
+ return ContainerState.FAILED
+
+ return self.define_container_state(pod)
Review Comment:
Added explicit coverage for `Succeeded` pod phase and non-terminal pod phase
fallback to `define_container_state()`.
##########
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/triggers/pod.py:
##########
@@ -307,7 +307,7 @@ async def _wait_for_container_completion(self) ->
TriggerEvent:
time_get_more_logs = time_begin +
datetime.timedelta(seconds=self.logging_interval)
while True:
pod = await self._get_pod()
- container_state = self.define_container_state(pod)
+ container_state = self.define_pod_state(pod)
if container_state == ContainerState.TERMINATED:
return TriggerEvent(
{
Review Comment:
Renamed the loop variable to `pod_container_state` and updated the
failure/debug wording to reflect pod-or-container state.
##########
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/operators/pod.py:
##########
@@ -925,7 +925,7 @@ def invoke_defer_method(
logging_interval=self.logging_interval,
trigger_kwargs=self.trigger_kwargs,
)
- container_state = trigger.define_container_state(self.pod) if self.pod
else None
+ container_state = trigger.define_pod_state(self.pod) if self.pod else
None
if context and (
container_state == ContainerState.TERMINATED or container_state ==
ContainerState.FAILED
):
Review Comment:
Renamed the operator pre-deferral variable and related test mock to
`pod_container_state`/`mocked_pod_container_state`; event message wording now
also reflects pod-or-container state.
--
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]
