[GitHub] [airflow] jedcunningham commented on a diff in pull request #34579: Fix KubernetesPodTrigger startup timeout

["jedcunningham (via GitHub)" <gi...@apache.org>]

jedcunningham commented on code in PR #34579:
URL: https://github.com/apache/airflow/pull/34579#discussion_r1339224522


##########
tests/providers/cncf/kubernetes/triggers/test_pod.py:
##########
@@ -334,3 +335,37 @@ def test_define_container_state_should_execute_successfully(
         )
 
         assert expected_state == trigger.define_container_state(pod)
+
+    @pytest.mark.asyncio
+    @pytest.mark.parametrize("container_state", [ContainerState.WAITING, ContainerState.UNDEFINED])
+    @mock.patch(f"{TRIGGER_PATH}.define_container_state")
+    @mock.patch(f"{TRIGGER_PATH}._get_async_hook")
+    async def test_run_loop_return_timeout_event(
+        self, mock_hook, mock_method, trigger, caplog, container_state
+    ):
+        trigger.trigger_start_time = TRIGGER_START_TIME - datetime.timedelta(minutes=2)
+        mock_hook.return_value.get_pod.return_value = self._mock_pod_result(
+            mock.MagicMock(
+                status=mock.MagicMock(
+                    phase=PodPhase.PENDING,
+                )
+            )
+        )
+        mock_method.return_value = container_state
+
+        caplog.set_level(logging.INFO)
+
+        generator = trigger.run()
+        actual = await generator.asend(None)
+        assert (
+            TriggerEvent(
+                {
+                    "name": POD_NAME,
+                    "namespace": NAMESPACE,
+                    "status": "timeout",
+                    "message": f"Pod took longer than {STARTUP_TIMEOUT_SECS} seconds to start."
+                    f" Check the pod events in kubernetes to determine why.",

Review Comment:
   ```suggestion
                       " Check the pod events in kubernetes to determine why.",
   ```
   
   nit



##########
airflow/providers/cncf/kubernetes/operators/pod.py:
##########
@@ -679,7 +679,10 @@ def execute_complete(self, context: Context, event: dict, **kwargs):
                     return xcom_sidecar_output
         finally:
             istio_enabled = self.is_istio_enabled(pod)
-            pod = self.pod_manager.await_pod_completion(pod, istio_enabled, self.base_container_name)
+            # Skip await_pod_completion when the event is 'timeout' due to the pod can hang
+            # on the ErrImagePull or ContainerCreating step and it will never complete
+            if event["status"] != "timeout":

Review Comment:
   We don't have test coverage that we skip the await, right?



-- 
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-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org