Re: [PR] Fix KubernetesPodOperator discarding successful XCom when sidecar kill fails [airflow]

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

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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org