Add `on_finish_action` to `KubernetesPodOperator` #30718

hussein-awala · 2023-04-18T18:25:56Z

Add a new argument on_finish_action for KubernetesPodOperator to determine how to deal with the finished pods. There are 3 possible values:

delete_pod: delete the pod regardless its state
delete_succeeded_pod: delete the pod if it terminate with Succeeded state
keep_pod: keep the pod and don't delete it

eladkal · 2023-04-18T19:48:53Z

airflow/providers/cncf/kubernetes/operators/pod.py

    :param is_delete_operator_pod: What to do when the pod reaches its final
        state, or the execution is interrupted. If True (default), delete the
        pod; if False, leave the pod.
+    :param delete_when_fails: If False, and is_delete_operator_pod is set to True


This feels a bit complicated.
I think we should consider replace is_delete_operator with:
action_on_finish that supports several modes. For example: delete_pod, delete_pod_success

That way you dont need to work hard to understand the combinationa of flags, the mode is self explanatory.

WDYT?

My first thought was doing something similar to #29394, but I hesitated to deprecate is_delete_operator_pod, but yes, these flags may confuse users. I'll implement the argument you propose.

cc @jedcunningham WDYT?

Yep. Agree with @eladkal simple flag is better. I made similar "keep old flag" approaches in the past and it led to really bad complexity/

potiuk

I like this. Needs conflict resolving though. @jedcunningham @dstandish ?

airflow/providers/cncf/kubernetes/operators/pod.py

jedcunningham

System test examples need updating, particularly as they get sucked into the docs.

Are you planning to change the operators based on KPO in a followup PR? Just want to make sure it isn't missed. For example, the eks pod operator.

airflow/providers/cncf/kubernetes/operators/pod.py

tests/providers/cncf/kubernetes/operators/test_pod.py

tests/providers/cncf/kubernetes/triggers/test_pod.py

Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>

hussein-awala · 2023-04-23T17:47:00Z

Are you planning to change the operators based on KPO in a followup PR? Just want to make sure it isn't missed. For example, the eks pod operator.

@jedcunningham yes, in this PR, I have ensured that it is fully backward compatible. This means that users can update the cncf.kubernetes provider version and still use it with older versions of Amazon or Google providers:

airflow/airflow/providers/cncf/kubernetes/operators/pod.py

Lines 387 to 393 in a99e3ba

    
               self.on_finish_action = ( 
        
                   OnFinishAction.DELETE_POD if is_delete_operator_pod else OnFinishAction.KEEP_POD 
        
               ) 
        
               self.is_delete_operator_pod = is_delete_operator_pod 
        
           else: 
        
               self.on_finish_action = OnFinishAction(on_finish_action) 
        
               self.is_delete_operator_pod = self.on_finish_action == OnFinishAction.DELETE_POD

Once this PR is merged, I'll create a separate PR for each provider to deprecate the argument is_delete_operator_pod and use the new one.

eladkal · 2023-04-23T17:52:18Z

Once this PR is merged, I'll create a separate PR for each provider to deprecate the argument is_delete_operator_pod and use the new one.

We can do it in this PR if you prefer.. no reason to keep it in seperated PRs unless that is how you prefer.

hussein-awala · 2023-04-23T18:05:32Z

@eladkal we have this message added 15 months ago in the version 3.0.0 of Amazon provider, and we are releasing the version 8.0.0 without doing anything

:param is_delete_operator_pod: What to do when the pod reaches its final
state, or the execution is interrupted. If True, delete the
pod; if False, leave the pod. Current default is False, but this will be
changed in the next major release of this provider.

Also we have a warning on the None is_delete_operator_pod, what is the policy for the providers deprecation?

hussein-awala · 2023-04-23T18:08:09Z

Same warning added to Google provider in 6.4.0, and we are releasing 10.0.0

potiuk · 2023-04-23T18:08:29Z

Also we have a warning on the None is_delete_operator_pod, what is the policy for the providers deprecation?

We don't have provider deprecation policy - we remove deprecated things when we see that they woudl be helpful in cleaning up the code.

potiuk · 2023-04-23T18:09:46Z

BTW. @eladkal is big proponent of removal of all deprecations when we release a breaking version. I am a bit more cautiouss, but I have no problem with that.

hussein-awala · 2023-04-23T18:15:18Z

IMHO we can merge this PR without updating the operators GKEStartPodOperator and EksPodOperator, and I can create two PRs which do the same thing for these two operators and change the default value for pod_deletion to be consistent with KPO, and we can add them to the next major release of each provider with a breaking change note. WDYT?

eladkal · 2023-04-23T18:18:56Z

IMHO we can merge this PR without updating the operators GKEStartPodOperator and EksPodOperator, and I can create two PRs which do the same thing for these two operators and change the default value for pod_deletion to be consistent with KPO, and we can add them to the next major release of each provider with a breaking change note. WDYT?

It's OK to update multiple providers in one PR.

I see no reason to break backward compatibility in any provider.
This PR add new parameter and deprecate the old one.
We should do the same for the other providers that inherit from KPO.

hussein-awala · 2023-04-23T18:21:13Z

Okay, I'll add them.

hussein-awala · 2023-05-25T13:00:27Z

The issue with these operators is that the default is_delete_operator_pod is not aligned with the KPO. Therefore, we need to handle this parameter individually for each subclass operator.

Using on_finish_action as an argument for the super function won't work with the older version of the KPO. Additionally, providing is_delete_operator_pod will result in a deprecation warning being logged, even if the user provides on_finish_action for the GKE/EKS operator.
IMO, the best solution is to check the KPO version until modifying the default value of is_delete_operator_pod for the GKE/EKS operator, which, btw, is already deprecated.

…ning

kand617 · 2023-05-31T03:57:07Z

Any update on this?
Our usecase is to keep pods that have failed and delete ones that completed successfully.
Can we opt for a simple solution? A new param perhaps?
is_delete_operator_pod_failed is_delete_operator_pod_success

if is_delete_operator_pod=True without extra params => delete pod always.
if either new params are defined, then ignore is_delete_operator_pod?

In some cases, Its unclear why pod has been removed. Cant tell from the output

dstandish · 2023-05-31T08:38:49Z

i am late to the party here. that said, what about the following...

keep is_delete_pod_operator and let it be bool | OnFinishAction (or some other better name such as DeletePodCondition)

then it maintains a little more continuity and the name stays more tailored to what is actually being controlled, and we don't have to add any more deprecations. then it could be is_delete_operator_pod=DeletePodCondition.TaskFailure

one thing that pops out for me re the name on_finish_action... it kind of sounds like it's a more elaborate "callback" feature.

anyway, just a suggestion

eladkal · 2023-06-02T11:51:24Z

Using on_finish_action as an argument for the super function won't work with the older version of the KPO. Additionally, providing is_delete_operator_pod will result in a deprecation warning being logged, even if the user provides on_finish_action for the GKE/EKS operator.

Is this a concern only for google provider?
currently google provider depended on

airflow/airflow/providers/google/provider.yaml

Lines 1121 to 1123 in dc5bf3f

    
           - name: cncf.kubernetes 
        
             dependencies: 
        
               - apache-airflow-providers-cncf-kubernetes>=6.2.0

Should we consider bumping the dependency apache-airflow-providers-cncf-kubernetes>7.0.0 ?

hussein-awala · 2023-06-18T20:33:48Z

Should we consider bumping the dependency apache-airflow-providers-cncf-kubernetes>7.0.0 ?

@eladkal +1, bu we should add the same dependency to AWS provider, is that possible?

eladkal · 2023-06-29T08:09:25Z

airflow/providers/amazon/provider.yaml

      - aiobotocore[boto3]>=2.2.0
+  - name: cncf.kubernetes
+    dependencies:
+      - apache-airflow-providers-cncf-kubernetes>=8.0.0


I don't think 8.0.0 is right?
There is no such version for cncf.kubernetes

I believe it should be:
apache-airflow-providers-cncf-kubernetes>7.0.0

I think >=8.0.0 is fine, and we have to release those three packages together. There should be 8.0.0 release of cncf.kubernetes coming together with releasing of those two providers (the changes in cncf.kubernetes it brings are breaking, so that's expected to be bumped to 8.0.0.

Comment: The "additional" dependencies are pretty special because they are mostly "informatory". Almost no-one will install apache-airflow-providers-amazon[cncf.kubernetes]==8.3.0 - what is mainly the purpose of those is to be document the dependencies.

w8.. I'm missing something here.
In this PR we add a feature to apache-airflow-providers-cncf-kubernetes that feature is what needed for amazon and google provider so what we are after is setting dependency for the next version of apache-airflow-providers-cncf-kubernetes thus the condition should be:

apache-airflow-providers-cncf-kubernetes>=7.2.0

Currently stable version of cncf.kubernetes is 7.1.0

In this PR we add a feature to apache-airflow-providers-cncf-kubernetes that feature is what needed for amazon and google provider so what we are after is setting dependency for the next version of apache-airflow-providers-cncf-kubernetes thus the condition should be:

Ah ok. If that's not a breaking change, then yes. I have not looked in detail enough and badly classified upcoming cncf.kubernetes version as MAJOR upgrade.

This is really the result of our release process and why in the past I was leaving all the version upgrades to the release manager "documentation preparation" - because literally we do not know if the next version will be MINOR or MAJOR upgrade until we merge last change. I think it's next to impossible to change it unless we would have a much more active RM role upfront (RM would have to make versioning decisions upfront and for example hold breaking changes from being merged if the next release is non-breaking, but I feel that is quite impossible with our volunteer-driven organisation).

Ideally we should have in this case >= NEXT_CNCF_KUBERNETES_PROVIDER_VERSION that could be automatically changed when the release documentation is prepared - because this is what we have here, really. But it's yet another automation complexity and feels like a bit of an edge-case that can be handled manually.

And in edge-cases like that it's hard to have a process to follow for every single case like that, so it's case-by-case basis.

Side note. Assuming that we do not know if the next release is MINOR or MAJOR we could add >=MINOR_UPGRADE always. It's a bit limiting and slightly confusing for the users if we do MAJOR release instead, but technically it would work.

For example in this case we could agree to have >=7.2.0 here independently if the next version is MAJOR or MINOR. Even if we release 8.0.0, that would still technicaly work (8.0.0 >= 7.2.0), but there would be no 7.2.0 release at all., However (except confusing the users) there is also a side effect that it would not alow us (we don't do it anyway but techniclaly there is such a possiblity with mixed-governance model) to release 7.2.0 after the 8.0.0. Technically (and purely theoretically because we've never done so) we could have it and then the >=7.2.0 would be wrong if the features needed will not be cherry-picked to the v7 branch of the provider.

But I think it's better to get it right even if it requires a bit of discussion and "humans-in-the-loop" agreeing on details of such versioning question (as we do now).

Side note. Assuming that we do not know if the next release is MINOR or MAJOR we could add >=MINOR_UPGRADE always. It's a bit limiting and slightly confusing for the users if we do MAJOR release instead, but technically it would work.

I like this idea, I added 8.0.0 because I didn't know if there is any breaking changes on the other PRs, but yes 7.2.0 will work for both minor and major releases 👍

potiuk · 2023-06-29T15:47:07Z

>=7.2.0 @hussein-awala ?

airflow/providers/amazon/provider.yaml

airflow/providers/google/provider.yaml

Co-authored-by: Jarek Potiuk <jarek@potiuk.com>

eladkal · 2023-06-30T08:01:41Z

Great work @hussein-awala !

Add a new arg for KPO to only delete the pod when it doesn't fail

66e7c20

hussein-awala requested a review from jedcunningham as a code owner April 18, 2023 18:25

boring-cyborg bot added provider:cncf-kubernetes Kubernetes (k8s) provider related issues area:providers labels Apr 18, 2023

hussein-awala added the type:new-feature Changelog: New Features label Apr 18, 2023

eladkal reviewed Apr 18, 2023

View reviewed changes

hussein-awala added 4 commits April 19, 2023 01:07

deprecate is_delete_operator_pod and add on_finish_action

b2eddbe

Add deprecated properties and fix unit tests

c245594

Merge branch 'main' into kpo_delete_when_fails

af4e93d

add missing attribute

64bff31

eladkal removed the type:new-feature Changelog: New Features label Apr 19, 2023

potiuk approved these changes Apr 22, 2023

View reviewed changes

Merge branch 'main' into kpo_delete_when_fails

074b75e

eladkal reviewed Apr 23, 2023

View reviewed changes

airflow/providers/cncf/kubernetes/operators/pod.py Show resolved Hide resolved

jedcunningham reviewed Apr 23, 2023

View reviewed changes

airflow/providers/cncf/kubernetes/operators/pod.py Outdated Show resolved Hide resolved

tests/providers/cncf/kubernetes/operators/test_pod.py Outdated Show resolved Hide resolved

tests/providers/cncf/kubernetes/triggers/test_pod.py Outdated Show resolved Hide resolved

Apply suggestions from code review

a99e3ba

Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>

hussein-awala added 3 commits April 28, 2023 12:07

Merge branch 'main' into kpo_delete_when_fails

366dc52

update GKEStartPodOperator to be consistent with KPO

b14e190

update EksPodOperator to be consistent with KPO

6cd01c4

hussein-awala requested a review from o-nikolas as a code owner May 21, 2023 14:13

update unit tests and the method used to check the kpo compatibility

e72a5ae

hussein-awala added 2 commits May 25, 2023 15:03

Merge branch 'main' into kpo_delete_when_fails

a0a6b6b

warn with AirflowProviderDeprecationWarning instead of DeprecationWar…

25543de

…ning

eladkal mentioned this pull request Jun 15, 2023

Add istio support to k8s operator #31389

Closed

hussein-awala added 3 commits June 28, 2023 23:25

Merge branch 'main' into kpo_delete_when_fails

6073020

Bump KPO min version in GCP provider and add a new one to AWS provider

3d9a8fa

Add the new param to the GKE trigger

38c45a6

hussein-awala requested review from eladkal and jedcunningham June 28, 2023 23:26

eladkal approved these changes Jun 29, 2023

View reviewed changes

eladkal reviewed Jun 29, 2023

View reviewed changes

potiuk approved these changes Jun 29, 2023

View reviewed changes

potiuk reviewed Jun 29, 2023

View reviewed changes

airflow/providers/amazon/provider.yaml Outdated Show resolved Hide resolved

potiuk reviewed Jun 29, 2023

View reviewed changes

airflow/providers/google/provider.yaml Outdated Show resolved Hide resolved

Apply suggestions from code review

d0da97d

Co-authored-by: Jarek Potiuk <jarek@potiuk.com>

eladkal approved these changes Jun 30, 2023

View reviewed changes

eladkal changed the title ~~Add a new arg for KPO to only delete the pod when it doesn't fail~~ Add on_finish_action to KubernetesPodOperator Jun 30, 2023

eladkal merged commit dd937e5 into apache:main Jun 30, 2023

This was referenced Jun 30, 2023

Allow creating unique pod and hostname for KubernetesPodOperator from task job_id #32278

Closed

Status of testing Providers that were prepared on July 06, 2023 #32389

Closed

eladkal mentioned this pull request Jul 9, 2023

Status of testing Providers that were prepared on July 09, 2023 #32460

Closed

32 tasks

hussein-awala mentioned this pull request Nov 9, 2023

Add support for catchup="ignore_first" to disable catchup for the first DagRun #35392

Closed

eladkal mentioned this pull request May 1, 2024

Add ability to disable a running DAG only after after it's in a finished state #22006

Open

2 tasks

eladkal mentioned this pull request Nov 15, 2024

Add wait_policy option to EmrCreateJobFlowOperator. #44055

Merged

Add on_finish_action to KubernetesPodOperator #30718

Add on_finish_action to KubernetesPodOperator #30718

Uh oh!

Conversation

hussein-awala commented Apr 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

potiuk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jedcunningham left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hussein-awala commented Apr 23, 2023

Uh oh!

eladkal commented Apr 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hussein-awala commented Apr 23, 2023

Uh oh!

hussein-awala commented Apr 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

potiuk commented Apr 23, 2023

Uh oh!

potiuk commented Apr 23, 2023

Uh oh!

hussein-awala commented Apr 23, 2023

Uh oh!

eladkal commented Apr 23, 2023

Uh oh!

hussein-awala commented Apr 23, 2023

Uh oh!

hussein-awala commented May 25, 2023

Uh oh!

kand617 commented May 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dstandish commented May 31, 2023

Uh oh!

eladkal commented Jun 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hussein-awala commented Jun 18, 2023

Uh oh!

eladkal Jun 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

potiuk commented Jun 29, 2023

Uh oh!

Uh oh!

Uh oh!

eladkal commented Jun 30, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Add `on_finish_action` to `KubernetesPodOperator` #30718

Add `on_finish_action` to `KubernetesPodOperator` #30718

hussein-awala commented Apr 18, 2023 •

edited

Loading

eladkal commented Apr 23, 2023 •

edited

Loading

hussein-awala commented Apr 23, 2023 •

edited

Loading

kand617 commented May 31, 2023 •

edited

Loading

eladkal commented Jun 2, 2023 •

edited

Loading

eladkal Jun 29, 2023 •

edited

Loading