Move KubernetesPodTrigger pod cleanup from cleanup() to on_kill()#65741
Move KubernetesPodTrigger pod cleanup from cleanup() to on_kill()#65741amoghrajesh merged 5 commits intoapache:mainfrom
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide
|
jscheffl
left a comment
There was a problem hiding this comment.
Sorry, need to request changes on this. This PR deletes too much.
Unfortunaltely is it not (yet) time to remove all Airflow 2.x support. The K8s provider package is still and needs still to be suoported with Airflow 2.11.x. Please revert the removal.
And yes this adds a bot of complexity but: The new on_kill() will only be available for Airflow 3.3.0++
This means the existing logic also needs to stay for all Airflow <3.3.0.
|
Hi @jscheffl , Thanks for the feedback, i have addressed your valuable feedback, please do review. |
Will do a proper review after comments, no blocker atm
There was a problem hiding this comment.
Pull request overview
Updates KubernetesPodTrigger to perform pod deletion on user-initiated cancellation via BaseTrigger.on_kill() (Airflow 3.3+), and avoids deleting pods during triggerer restarts/redistribution by making cleanup() a no-op on 3.3+.
Changes:
- Add
KubernetesPodTrigger.on_kill()to delete pods on user kills (guarded byAIRFLOW_V_3_3_PLUS). - Update
cleanup()to skip pod deletion on Airflow 3.3+ while keeping the legacysafe_to_cancel()behavior for older Airflow versions. - Update unit tests to assert
on_kill()behavior (and legacycleanup()behavior for Airflow < 3.3).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/triggers/pod.py | Adds on_kill() and gates pod deletion away from cleanup() on Airflow 3.3+ to prevent restart-triggered deletion. |
| providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/version_compat.py | Introduces AIRFLOW_V_3_3_PLUS for provider-side feature gating. |
| providers/cncf/kubernetes/tests/unit/cncf/kubernetes/triggers/test_pod.py | Migrates/extends tests to validate on_kill() on 3.3+ and legacy cleanup() behavior on < 3.3. |
|
@raushanprabhakar1 — There are 2 unresolved review threads on this PR from @jscheffl. Could you either push a fix or reply in each thread explaining why the feedback doesn't apply? Once you believe the feedback is addressed, mark the thread as resolved so the reviewer isn't re-pinged needlessly. Thanks! Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this two-stage triage process so that our maintainers' limited time is spent where it matters most: the conversation with you. |
…essts to pytests pytest.mark.skipif with AIRFLOW_V_3_3_PLUS
There was a problem hiding this comment.
Looks good for me. Second pair of eyes would be good.
@raushanprabhakar1 please run prek run -a ruff and commit changes to fix fails in static checks.
…netes/triggers/test_pod.py
|
Hi @jscheffl , fixed the lint issues. Thanks |
|
I will take a look too |
amoghrajesh
left a comment
There was a problem hiding this comment.
Just one comment or LGTM
|
Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions. |
KubernetesPodTriggernow cancels external work only throughBaseTrigger.on_kill()instead of the previouscleanup()path that usedsafe_to_cancel()to distinguish user kills from triggerer restarts. Pod deletion on user-initiated kill is unchanged in intent: it still respects_fired_event,on_kill_action, andtermination_grace_period; triggerer shutdown or redistribution no longer goes through pod-deletion logic in this trigger.Unit tests were updated to assert
on_killbehavior instead ofcleanup/safe_to_cancel.related: #65733
Was generative AI tooling used to co-author this PR?
{pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.