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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
