dabla commented on code in PR #76330:
URL: https://github.com/apache/airflow/pull/76330#discussion_r4479329418
##########
providers/microsoft/azure/src/airflow/providers/microsoft/azure/operators/container_instances.py:
##########
@@ -563,6 +566,18 @@ def _monitor_logging(self, resource_group: str, name: str)
-> int:
time.sleep(1)
+ def _log_container_output(self) -> list | None:
+ """Write the container group's logs to the task log and return them."""
+ try:
+ logs = self.hook.get_logs(self.resource_group, self.name)
+ except Exception as e:
+ self.log.warning(
+ "Failed to fetch logs for container group %s/%s: %s",
self.resource_group, self.name, e
+ )
+ return None
Review Comment:
**[warning]** On success with `xcom_all` set, a failed log fetch now pushes
`[]` to XCom and the task still succeeds.
Swallowing the exception here makes sense on the error path: the container's
own error should be the one that surfaces. On the success path, though, `logs`
is also what goes to XCom. `AzureContainerInstanceHook.get_logs` never returns
`None` (it returns `[]` when there is no content), so before this change `logs
is None` could not happen and a failing `get_logs` failed the task. Now, with
`xcom_all=True` or `False`, a fetch error (auth, throttling, a group removed by
another process) is logged as a warning, `[]` is pushed under `logs`, and the
task returns `0`. A downstream task then reads "no output" when the output was
actually never read. That breaks "Errors should never pass silently" ([PEP
20](https://peps.python.org/pep-0020/)).
One way to keep both behaviours: on the success path, when XCom needs the
logs and the first fetch failed, fetch them again without the guard, so the
error propagates as it did before:
```python
if self.xcom_all is not None:
if logs is None:
logs = self.hook.get_logs(self.resource_group, self.name)
context["ti"].xcom_push(key="logs", value=logs if self.xcom_all else
logs[-1:])
```
A test for success + `xcom_all=True` + a failing `get_logs` would pin this.
`test_execute_complete_log_fetch_failure_keeps_container_error` could also
assert that the warning was logged: today it would pass even if the helper
logged nothing.
--
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]
