Change flow to succeeded pods

[wolvery]

WHAT

Changes the flow of delete_succeeded_pods to keep only failed pods when this option is enabled.

WHY

Failed pods are important for troubleshooting purposes, especially when observability has a lower collection frequency (for example, 30 seconds for OpenTelemetry Collectors).

Removing other pods helps prevent orphaned pods from remaining in the cluster, which could consume resources and negatively affect the Airflow retry process.

[wolvery]

Hey @jscheffl I created this one with the motivation wrote here. We would like to remove the non failed pods in the delete_succeeded_pods to be able to have observability and without orphan pods not being monitores by Airflow.
Please, if you could consider this approach would be very helpful for us.

[jscheffl]

Thanks. That are convincing arguments. Sorry, I was only reading the PR description, not the associated bug ticket. I should have maybe.

Can you fix the tests?

[wolvery]

Thanks. That are convincing arguments. Sorry, I was only reading the PR description, not the associated bug ticket. I should have maybe.

Can you fix the tests?

Sure. I believe it is fixed, now. Thank you.

[SameerMesiah97]

@wolvery

Thanks for opening a PR to address this issue. Had I known you were already working on a solution, I would have deferred to you, as you naturally understand your use case better than I do. That said, I am concerned that changing the behaviour of an existing cleanup mode could introduce a risk of breaking current deployments.

My understanding of the intended semantics of the cleanup modes is:

1. delete_pod: The most aggressive option, and the default. This is best suited for “happy path” scenarios where there is no expectation of needing pods for post-mortem debugging.
2. delete_succeeded_pod: Cleans up pods that have completed successfully and whose logs are no longer required. These pods have run to completion without issues and no longer need to remain in the namespace.
3. keep_pod: Retains all pod objects. While I would not personally recommend this in most cases, it’s provide users with maximum flexibility.
4. delete_active_pod: A new mode I introduced based on my interpretation of your issue. This removes pods that are consuming (or may consume) resources—i.e. pods in RUNNING or PENDING states—while retaining completed pods for debugging. This avoids scenarios where resource-heavy pods continue running after the task has terminated, without losing succeeded/failed pods that may be useful for investigation. If my understanding is correct, this should cover your use case: retaining completed pods for debugging while cleaning up resource-hogging ones.

We could alternatively change the behaviour of delete_succeeded_pod, but that would require redefining what the flag represents. Doing so risks breaking existing deployments and effectively deprecates the current semantics. Keeping the flag name while changing its behaviour would also be confusing: a mode called delete_succeeded_pod would then delete running and pending pods as well. While this may seem pedantic, many users will not read docstrings closely and could inadvertently choose a setting that deletes pods they intended to keep.

If you are insistent on requiring a mode that deletes all pods save for the ones that have not failed, I believe it would be better to add a new flag for this with an appropriate name such as delete_non_failed_pod. But would this new flag provide additional useful functionality that delete_active_pod does not? I understand you might want succeeded pods to be gone with the running/pending pods but I am not sure of what purpose that would serve? In post-mortem debugging, I believe it would be more useful to have logs for both successful and failing states rather than just the latter.

I think it would be best to have someone who owns this provider look at this and offer their comments. Even though this is a seemingly trivial change, it has subtle risks that require consideration. That being said I am happy that you raised the issue. The absence of a specific flag to delete running/pending pods on task termination was definitely a gap that needed to be addressed.

@hussein-awala @jedcunningham

[wolvery]

@wolvery

Thanks for opening a PR to address this issue. Had I known you were already working on a solution, I would have deferred to you, as you naturally understand your use case better than I do. That said, I am concerned that changing the behaviour of an existing cleanup mode could introduce a risk of breaking current deployments.

My understanding of the intended semantics of the cleanup modes is:

1. delete_pod: The most aggressive option, and the default. This is best suited for “happy path” scenarios where there is no expectation of needing pods for post-mortem debugging.
2. delete_succeeded_pod: Cleans up pods that have completed successfully and whose logs are no longer required. These pods have run to completion without issues and no longer need to remain in the namespace.
3. keep_pod: Retains all pod objects. While I would not personally recommend this in most cases, it’s provide users with maximum flexibility.
4. delete_active_pod: A new mode I introduced based on my interpretation of your issue. This removes pods that are consuming (or may consume) resources—i.e. pods in RUNNING or PENDING states—while retaining completed pods for debugging. This avoids scenarios where resource-heavy pods continue running after the task has terminated, without losing succeeded/failed pods that may be useful for investigation. If my understanding is correct, this should cover your use case: retaining completed pods for debugging while cleaning up resource-hogging ones.

We could alternatively change the behaviour of delete_succeeded_pod, but that would require redefining what the flag represents. Doing so risks breaking existing deployments and effectively deprecates the current semantics. Keeping the flag name while changing its behaviour would also be confusing: a mode called delete_succeeded_pod would then delete running and pending pods as well. While this may seem pedantic, many users will not read docstrings closely and could inadvertently choose a setting that deletes pods they intended to keep.

If you are insistent on requiring a mode that deletes all pods save for the ones that have not failed, I believe it would be better to add a new flag for this with an appropriate name such as delete_non_failed_pod. But would this new flag provide additional useful functionality that delete_active_pod does not? I understand you might want succeeded pods to be gone with the running/pending pods but I am not sure of what purpose that would serve? In post-mortem debugging, I believe it would be more useful to have logs for both successful and failing states rather than just the latter.

I think it would be best to have someone who owns this provider look at this and offer their comments. Even though this is a seemingly trivial change, it has subtle risks that require consideration. That being said I am happy that you raised the issue. The absence of a specific flag to delete running/pending pods on task termination was definitely a gap that needed to be addressed.

@hussein-awala @jedcunningham

I was clear about the issue I raised: we had pods running for long periods without Airflow Workers monitoring them. In my opinion, we expected that the Airflow Worker would not keep these kinds of pods at all. This led to increased costs, and in our situation we also had resource constraints in the cluster. As a result, we reached the cluster limits, which affected the entire Airflow environment.

We need to remove successful pods because we have components that read pods at startup, and keeping them is critical. I don’t see value in having another option, since we already have one for deleting all. We don't use it because of observability for failures.

I don’t understand why we should keep this behavior and let others face the same issue. I opened this issue based on an incident we experienced. Other people might face the same problem if they rely on the KubernetesPodOperator. We are here to contribute, so even if you haven’t experienced the same incident, let’s try to have a mindset of being helpful so others don’t run into the same issues.

Sometimes we are not able to properly review PRs, and I apologize for that. However, I opened this issue and this PR because I believe this is the correct approach. All details are in the PR comment, so, take your time and read it. You can read the issue, again. You already made your comments on the PR, but I disagree with the proposed solution that you raised. Please, I don't see reason to have a long talk on this topic. The issue lies on this option and we should build a strong tool for others.

[potiuk]

I quite agree with @SameerMesiah97 here. It's wrong to delete RUNNING and PENDING pods when your flag is "delete_succeeded_pods". When there are some pods running or pending, they might - given enough time turn into either
"succeeded" or "failed". That would be a breaking change as well - as explained in the comment from @SameerMesiah97 .

If you want to implement your change, you should add a new on finish action enum. and when your condition is != Failed - I think the right is "delete_non_failed".

This is purely a matter of consistency and clarity. When you say that this is unexpected behaviour, then - as somoene who does not know that much about KPO internals - just occasionally look here - it would be absolutely surprising when I chose "delete_succeeded" deleting also PENDING and RUNNING - this is simply not what the name of the action says.

However, if I would see that I can choose among:

• delete_pod (default)
• delete_succeeded
• delete_active
• delete_non_failed (new one that you should add)

Then it's absolultely crystal clear what options I have.

Our job is not to guess intentions of people and change options to what we think will be more popular and reasonable, but to leave the decision in the hands of the users - presenting them clear and unambiguous options they can choose from - and let them be in control what happens.

Please add the new option instead of changing semantics of the exsisting one.

[wolvery]

I quite agree with @SameerMesiah97 here. It's wrong to delete RUNNING and PENDING pods when your flag is "delete_succeeded_pods". When there are some pods running or pending, they might - given enough time turn into either "succeeded" or "failed". That would be a breaking change as well - as explained in the comment from @SameerMesiah97 .

If you want to implement your change, you should add a new on finish action enum. and when your condition is != Failed - I think the right is "delete_non_failed".

This is purely a matter of consistency and clarity. When you say that this is unexpected behaviour, then - as somoene who does not know that much about KPO internals - just occasionally look here - it would be absolutely surprising when I chose "delete_succeeded" deleting also PENDING and RUNNING - this is simply not what the name of the action says.

However, if I would see that I can choose among:

• delete_pod (default)
• delete_succeeded
• delete_active
• delete_non_failed (new one that you should add)

Then it's absolultely crystal clear what options I have.

Our job is not to guess intentions of people and change options to what we think will be more popular and reasonable, but to leave the decision in the hands of the users - presenting them clear and unambiguous options they can choose from - and let them be in control what happens.

Please add the new option instead of changing semantics of the exsisting one.

Ok.

I'll work on adding it to the documentation too: We could mention that this option can lead to unsupervised pods in the cluster: pods in a Running or Pending state may be ignored, which can result in tasks being marked as successful even though the option is intended to delete only successful pods.

What would be the steps to deprecate delete_succeeded_pods after introducing delete_non_failed_pods?
Could I add a recommendation/deprecation warning?

[potiuk]

What would be the steps to deprecate delete_succeeded_pods after introducing delete_non_failed_pods?
Could I add a recommendation/deprecation warning?

Why deprecating it? It's just another option and someone might want to use it. I don't see any reason to deprecate it.

[potiuk]

We could mention that this option can lead to unsupervised pods in the cluster: pods in a Running or Pending state may be ignored, which can result in tasks being marked as successful even though the option is intended to delete only successful pods.

Yes. That's a good idea.

[wolvery]

What would be the steps to deprecate delete_succeeded_pods after introducing delete_non_failed_pods?
Could I add a recommendation/deprecation warning?

Why deprecating it? It's just another option and someone might want to use it. I don't see any reason to deprecate it.

I believe this option could negatively impact other users, as it did us. That’s why I opened this issue: I think this option is somewhat risky when there are resource or namespace constraints in a Kubernetes cluster.

We experienced situations where pods kept running after the Airflow worker finished and skipped them, which eventually led to no available machines to schedule new pods in our Kubernetes cluster. This situation persisted for a long time due to the Airflow retries we had in place and the time to provision the node in the cluster, for example.

I understand that the option itself is clear of the intent, but for users who enable it, it may not be clear that the Airflow worker can leave pods unsupervised in a Pending or Running state, and appear succesfull pods after their execution.

[potiuk]

I understand that the option itself is clear of the intent, but for users who enable it, it may not be clear that the Airflow worker can leave pods unsupervised in a Pending or Running state, and appear succesfull pods after their execution.

That's why your idea of explaining what it does is good. Do it.

The best way for optional choice where there is another choice that is well named and all of that is consistent, and - as you proposed - described including the context and consequences using it, this is fine.

There might be good reasons why somoene wants to only remove success pods. maybe they already have automated cleanup of pods that are running or pending too long for example and they will be cleaned regardless for example.

We cannot make all decisions for our users, but we can let them decide based on what we told them.

This would be totally dfferent story if it was default behaviour. But it is not. You have to deliberately make your choice of the way you deal with Pods - and it's very clearly telling what it does. I don't think there is a need to deprecate this option.

[Nataneljpwd]

I have to agree with what @potiuk and @SameerMesiah97 said here, we would not want such a change where all pods are deleted, even if they are orphan pods, maybe a good idea might be to allow to configure the states of the pods to be deleted rather than choose for the client, I think it can be done without much complexity while preserving current behavior

[SameerMesiah97]

I have to agree with what @potiuk and @SameerMesiah97 said here, we would not want such a change where all pods are deleted, even if they are orphan pods, maybe a good idea might be to allow to configure the states of the pods to be deleted rather than choose for the client, I think it can be done without much complexity while preserving current behavior

So you would want to replace the existing flags with a set of flags for the deletion of the pods in each state? Like this:

1.delete_running_pod
2.delete_pending_pod
3.delete_failed_pod
4.delete_succeeded_pod

I believe a user could replicate the behaviour of the existing pods using these flags but if your suggestion involves deprecating the existing cleanup modes, then that would be a breaking change. This is not too much of a concern for delete_active_pod as that was recently introduced but delete_pod, keep_pod and succeeded_pod are likely being used by many deployments.

I think what you suggested is the ideal set of options as it allows maximum configurability by users and avoids us having to decide what combination of pods to delete for each mode is appropriate. If you have an approach that minimises parameter bloat and preserves backwards compatibility, I’m all ears.

One possible approach would be to add just one parameter (maybe call it delete_pods_in_phase), which accepts a list/set/dict of valid pod phases. Pods that are in any of the valid phases are deleted, regardless of the cleanup mode selected. Naturally, this would be an override parameter meant for advanced users.

[Nataneljpwd]

I have to agree with what @potiuk and @SameerMesiah97 said here, we would not want such a change where all pods are deleted, even if they are orphan pods, maybe a good idea might be to allow to configure the states of the pods to be deleted rather than choose for the client, I think it can be done without much complexity while preserving current behavior

So you would want to replace the existing flags with a set of flags for the deletion of the pods in each state? Like this:

1.delete_running_pod
2.delete_pending_pod
3.delete_failed_pod
4.delete_succeeded_pod

I believe a user could replicate the behaviour of the existing pods using these flags but if your suggestion involves deprecating the existing cleanup modes, then that would be a breaking change. This is not too much of a concern for delete_active_pod as that was recently introduced but delete_pod, keep_pod and succeeded_pod are likely being used by many deployments.

No, I do not propose deprecating current behavior, rather, I proposed to add a delete_by_states method which allows to delete pods by the respective states which the user provides

I think what you suggested is the ideal set of options as it allows maximum configurability by users and avoids us having to decide what combination of pods to delete for each mode is appropriate. If you have an approach that minimises parameter bloat and preserves backwards compatibility, I’m all ears.

As written above, I have added clarity to my proposes approach

One possible approach would be to add just one parameter (maybe call it delete_pods_in_phase), which accepts a list/set/dict of valid pod phases. Pods that are in any of the valid phases are deleted, regardless of the cleanup mode selected. Naturally, this would be an override parameter meant for advanced users.

Yes, this is exactly what I proposed

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.