Re: [PR] Fix Kubernetes executor not to discard successful XCom when sidecar kill fails [airflow]

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

potiuk commented on code in PR #72068:
URL: https://github.com/apache/airflow/pull/72068#discussion_r4057360677


##########
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -986,7 +986,10 @@ def extract_xcom(self, pod: V1Pod) -> str:
             result = self.extract_xcom_json(pod)
             return result
         finally:
-            self.extract_xcom_kill(pod)
+            try:
+                self.extract_xcom_kill(pod)
+            except PodCommandException:
+                self.log.warning("Failed to kill xcom sidecar container, skipping")

Review Comment:
   The warning drops the failure reason and the pod name, and doesn't say what the consequence is. Suggest:
   
   ```python
   except PodCommandException as e:
       self.log.warning(
           "Failed to kill xcom sidecar container in pod %s, leaving it running: %s",
           pod.metadata.name,
           e,
       )
   ```
   
   Otherwise users debugging the `Permission denied` case have to correlate this line with the earlier `stderr from command` output, and with `on_finish_action=keep_pod` they'd want to know the sidecar is still up.



##########
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -986,7 +986,10 @@ def extract_xcom(self, pod: V1Pod) -> str:
             result = self.extract_xcom_json(pod)
             return result
         finally:
-            self.extract_xcom_kill(pod)
+            try:
+                self.extract_xcom_kill(pod)
+            except PodCommandException:

Review Comment:
   The sidecar loops forever (`while true; do sleep 1; done`) until it is killed, so swallowing the failure here leaves it running. That's fine for `KubernetesPodOperator` (the pod is awaited on the base container and deleted by default), but `KubernetesJobOperator.execute` calls `extract_xcom` and then `self.hook.wait_until_job_complete(...)`, which polls with no timeout until the Job reports Complete/Failed — which can't happen while the sidecar container is alive. Before this change that scenario failed the task loudly; after it, the task hangs until `activeDeadlineSeconds` or the Airflow task timeout kicks in. Please either scope the swallow so the Job path still fails (or handles the still-running sidecar explicitly), or make that path cope, and cover whichever behaviour you choose with a test.



-- 
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