Copilot commented on code in PR #63737:
URL: https://github.com/apache/airflow/pull/63737#discussion_r3066472614
##########
providers/docker/src/airflow/providers/docker/operators/docker.py:
##########
@@ -529,6 +529,8 @@ def on_kill(self) -> None:
self.log.info("Not attempting to kill container as it was not
created")
return
self.cli.stop(self.container["Id"])
+ if self.auto_remove == "force":
+ self.cli.remove_container(self.container["Id"], force=True)
Review Comment:
`on_kill` should be best-effort and ideally idempotent.
`remove_container(...)` can raise (e.g., container already removed externally
or by a concurrent cleanup), which would cause `on_kill` itself to error and
potentially interfere with task termination handling. Consider catching the
specific Docker client exception for missing containers (and possibly API
errors) and logging at info/debug level instead of raising.
```suggestion
try:
self.cli.remove_container(self.container["Id"],
force=True)
except APIError as e:
self.log.info(
"Failed to remove docker container %s during
on_kill; it may already be gone: %s",
self.container["Id"],
e,
)
```
##########
providers/docker/src/airflow/providers/docker/operators/docker.py:
##########
@@ -529,6 +529,8 @@ def on_kill(self) -> None:
self.log.info("Not attempting to kill container as it was not
created")
return
self.cli.stop(self.container["Id"])
+ if self.auto_remove == "force":
+ self.cli.remove_container(self.container["Id"], force=True)
Review Comment:
This change introduces a new behavioral branch in `on_kill`. Add/extend a
unit test to assert that when `auto_remove == \"force\"`, `on_kill()` calls
`remove_container(..., force=True)` after stopping the container, and does not
call it for other `auto_remove` values.
--
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]
