You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "SameerMesiah97 (via GitHub)" <gi...@apache.org> on 2026/06/13 14:05:26 UTC
[PR] KubernetesPodOperator: make pod retention patching explicit [airflow]
SameerMesiah97 opened a new pull request, #68507:
URL: https://github.com/apache/airflow/pull/68507
**Description**
This change is a small cleanup that was originally intended for PR #61637 but was omitted when that PR was closed.
**Rationale**
The pod retention logic has been made more explicit to clearly distinguish between pods that are deleted and pods that are intentionally retained due to phase-based deletion rules. No functional behavior is changed.
`Backwards Compatibility`
No existing defaults are modified, and no public APIs have beem removed or altered.
--
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
Re: [PR] KubernetesPodOperator: make pod retention patching explicit [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl merged PR #68507:
URL: https://github.com/apache/airflow/pull/68507
--
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
Re: [PR] KubernetesPodOperator: make pod retention patching explicit [airflow]
Posted by "SameerMesiah97 (via GitHub)" <gi...@apache.org>.
SameerMesiah97 commented on code in PR #68507:
URL: https://github.com/apache/airflow/pull/68507#discussion_r3408643789
##########
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/operators/pod.py:
##########
@@ -1228,12 +1228,20 @@ def cleanup(
istio_enabled = self.is_istio_enabled(remote_pod)
pod_phase = remote_pod.status.phase if hasattr(remote_pod, "status") else None
- # if the pod fails or success, but we don't want to delete it
- if (
- pod_phase != PodPhase.SUCCEEDED
- or self.on_finish_action == OnFinishAction.KEEP_POD
- or self.on_finish_action == OnFinishAction.DELETE_ACTIVE_POD
- ):
+ should_delete = (
+ self.on_finish_action == OnFinishAction.DELETE_POD
+ or (
+ self.on_finish_action == OnFinishAction.DELETE_SUCCEEDED_POD
+ and pod_phase == PodPhase.SUCCEEDED
+ )
+ or (
+ self.on_finish_action == OnFinishAction.DELETE_ACTIVE_POD
+ and pod_phase in {PodPhase.RUNNING, PodPhase.PENDING}
+ )
+ )
+
+ # Patch the pod unless it is a succeeded pod that will be deleted.
+ if not (pod_phase == PodPhase.SUCCEEDED and should_delete):
Review Comment:
Done. Though it makes the PR rather trivial.
I still think the existing check for patching the pod was difficult to understand. This should make it a bit more readable.
--
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
Re: [PR] KubernetesPodOperator: make pod retention patching explicit [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on code in PR #68507:
URL: https://github.com/apache/airflow/pull/68507#discussion_r3409292802
##########
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/operators/pod.py:
##########
@@ -1228,12 +1228,20 @@ def cleanup(
istio_enabled = self.is_istio_enabled(remote_pod)
pod_phase = remote_pod.status.phase if hasattr(remote_pod, "status") else None
- # if the pod fails or success, but we don't want to delete it
- if (
- pod_phase != PodPhase.SUCCEEDED
- or self.on_finish_action == OnFinishAction.KEEP_POD
- or self.on_finish_action == OnFinishAction.DELETE_ACTIVE_POD
- ):
+ should_delete = (
+ self.on_finish_action == OnFinishAction.DELETE_POD
+ or (
+ self.on_finish_action == OnFinishAction.DELETE_SUCCEEDED_POD
+ and pod_phase == PodPhase.SUCCEEDED
+ )
+ or (
+ self.on_finish_action == OnFinishAction.DELETE_ACTIVE_POD
+ and pod_phase in {PodPhase.RUNNING, PodPhase.PENDING}
+ )
+ )
+
+ # Patch the pod unless it is a succeeded pod that will be deleted.
+ if not (pod_phase == PodPhase.SUCCEEDED and should_delete):
Review Comment:
Trivial is sometimes good - exactly because later the code is better readable. Looks better to me now as I can understand on the first read.
--
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
Re: [PR] KubernetesPodOperator: make pod retention patching explicit [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on code in PR #68507:
URL: https://github.com/apache/airflow/pull/68507#discussion_r3408587722
##########
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/operators/pod.py:
##########
@@ -1228,12 +1228,20 @@ def cleanup(
istio_enabled = self.is_istio_enabled(remote_pod)
pod_phase = remote_pod.status.phase if hasattr(remote_pod, "status") else None
- # if the pod fails or success, but we don't want to delete it
- if (
- pod_phase != PodPhase.SUCCEEDED
- or self.on_finish_action == OnFinishAction.KEEP_POD
- or self.on_finish_action == OnFinishAction.DELETE_ACTIVE_POD
- ):
+ should_delete = (
+ self.on_finish_action == OnFinishAction.DELETE_POD
+ or (
+ self.on_finish_action == OnFinishAction.DELETE_SUCCEEDED_POD
+ and pod_phase == PodPhase.SUCCEEDED
+ )
+ or (
+ self.on_finish_action == OnFinishAction.DELETE_ACTIVE_POD
+ and pod_phase in {PodPhase.RUNNING, PodPhase.PENDING}
+ )
+ )
+
+ # Patch the pod unless it is a succeeded pod that will be deleted.
+ if not (pod_phase == PodPhase.SUCCEEDED and should_delete):
Review Comment:
I was reading 3 times and it is both the existing as well as the new code hard to de-chipher. Can you attempt to make it w/o negation? For example like:
```suggestion
if pod_phase != PodPhase.SUCCEEDED or should_keep):
```
--
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