shoemoney commented on PR #72068:
URL: https://github.com/apache/airflow/pull/72068#issuecomment-5751061575
Thanks for the review. All five points are addressed in
c3e8cc219a946d0cd41d2b6173306a25c6eef672. The branch is also rebased onto
current main.
1. Regression test added. I took your test next to
`test_extract_xcom_success` as
`test_extract_xcom_returns_result_when_sidecar_kill_fails`, and added
`test_extract_xcom_reraises_kill_failure_when_not_ignored` for the scoped path.
Both fail on the pre-fix source: the first with `PodCommandException: Command
failed with stderr: Permission denied`, the second with `TypeError:
PodManager.extract_xcom() got an unexpected keyword argument
'ignore_kill_failure'`.
2. Scoped the swallow rather than making the Job path cope. `extract_xcom`
takes a keyword-only `ignore_kill_failure: bool = True`. The default keeps the
new behaviour for `KubernetesPodOperator`. `KubernetesJobOperator.execute` now
calls `self.extract_xcom(pod=pod, ignore_kill_failure=False)`, so that path
fails exactly as loudly as it did before this PR and never reaches the untimed
`wait_until_job_complete` poll. `KubernetesPodOperator.extract_xcom` threads
the argument through.
I chose this over re-checking `container_is_running` on the sidecar after
extraction. That check reads pod state from the API and a successful `kill -s
SIGINT 1` does not make the container report terminated instantly, so it would
fail the happy path intermittently. Keying on whether the kill actually raised
is race-free and reads as the same decision the caller is making.
3. Warning now uses your wording, including the pod name, the reason and the
consequence.
4. Retitled to "Fix KubernetesPodOperator discarding successful XCom when
sidecar kill fails". The commit headline matches.
5. Template restored from `.github/PULL_REQUEST_TEMPLATE.md` with the
existing description kept inside it. Generative AI tooling was used for
drafting and refactoring. Tool: Claude Code.
Test evidence, run locally against a venv built from `airflow-core`,
`task-sdk`, `devel-common` and `providers/cncf/kubernetes` on Python 3.12.
RED, source reverted to the pre-fix state with the new tests in place:
```
FAILED
.../utils/test_pod_manager.py::TestPodManager::test_extract_xcom_returns_result_when_sidecar_kill_fails
FAILED
.../utils/test_pod_manager.py::TestPodManager::test_extract_xcom_reraises_kill_failure_when_not_ignored
2 failed, 5 passed, 135 deselected
```
RED for the Job path, with only the `ignore_kill_failure=False` argument
removed from `job.py`:
```
FAILED
.../operators/test_job.py::TestKubernetesJobOperator::test_xcom_sidecar_kill_failure_fails_job_instead_of_hanging
1 failed, 90 deselected
```
The failure there is the swallow letting execution run on past
`extract_xcom`, so `pytest.raises(PodCommandException)` never sees the
exception.
GREEN, full run of `test_pod_manager.py`, `test_job.py` and `test_pod.py`:
```
4 failed, 477 passed, 1 warning, 13 errors
```
Same three files on unmodified main give `4 failed, 474 passed, 1 warning,
13 errors`. The failures and errors are identical in both runs and are
pre-existing (`test_fetch_container_with_valid_since_time`,
`test_write_logs_*_since_time`, the `TestSuppress` and
`TestKubernetesPodOperatorAsync` setup errors). The delta is exactly the three
new passing tests.
`ruff check` and `ruff format --check` are clean on all five touched files.
--
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]
