Skip to content

avoid retrying after KubernetesPodOperator has been marked as failed - #36749

Merged
eladkal merged 5 commits into
apache:mainfrom
astronomer:fix-kpo-retry-even-after-marking-it-as-failed
Jan 20, 2024
Merged

avoid retrying after KubernetesPodOperator has been marked as failed#36749
eladkal merged 5 commits into
apache:mainfrom
astronomer:fix-kpo-retry-even-after-marking-it-as-failed

Conversation

@Lee-W

@Lee-WLee-W commented Jan 12, 2024•
edited
Loading

Copy link
Copy Markdown
Member

After marking the task as failed, KPO still has a finally section to run, which causes #36471. In this PR, I tried to check whether on_killed is called and do not raise exception in the finally section as it'll overwrite the "Mark as failed" behavior

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@boring-cyborgboring-cyborgBot added area:providers provider:cncf-kubernetes Kubernetes (k8s) provider related issues labels Jan 12, 2024
@Lee-W
Lee-Wforce-pushed the fix-kpo-retry-even-after-marking-it-as-failed branch from a7573f7 to 6b0f18fCompareJanuary 12, 2024 15:32
dirrao
dirrao requested changes Jan 13, 2024
View reviewed changes

@dirraodirrao left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice Work. It would be great if possible then, add the test cases.

Comment threadairflow/providers/cncf/kubernetes/operators/pod.py Outdated
Comment threadairflow/providers/cncf/kubernetes/operators/pod.py Outdated
@Lee-W
Lee-Wforce-pushed the fix-kpo-retry-even-after-marking-it-as-failed branch 2 times, most recently from 4f3b55b to 141b7a8CompareJanuary 15, 2024 00:45
amoghrajesh
amoghrajesh reviewed Jan 15, 2024
View reviewed changes
Comment threadairflow/providers/cncf/kubernetes/operators/pod.py Outdated
Comment threadairflow/providers/cncf/kubernetes/operators/pod.py Outdated
Comment threadairflow/providers/cncf/kubernetes/operators/pod.py Outdated
@Lee-W
Lee-Wforce-pushed the fix-kpo-retry-even-after-marking-it-as-failed branch from 141b7a8 to c10f762CompareJanuary 15, 2024 07:18
amoghrajesh
amoghrajesh approved these changes Jan 16, 2024
View reviewed changes

@amoghrajeshamoghrajesh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Pending comments from @dirrao.
@hussein-awala WDYT?

@Lee-WLee-W mentioned this pull request Jan 16, 2024
Closed
2 tasks
dstandish
dstandish reviewed Jan 16, 2024
View reviewed changes
Comment threadairflow/providers/cncf/kubernetes/operators/pod.py Outdated
jedcunningham
jedcunningham reviewed Jan 16, 2024
View reviewed changes

@jedcunninghamjedcunningham left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also add some test coverage to make sure we don't accidentally reintroduce this.

Comment threadairflow/providers/cncf/kubernetes/operators/pod.py Outdated
Comment threadairflow/providers/cncf/kubernetes/operators/pod.py Outdated
Comment threadairflow/providers/cncf/kubernetes/operators/pod.py Outdated
Comment threadairflow/providers/cncf/kubernetes/operators/pod.py Outdated
@eladkaleladkal linked an issue Jan 16, 2024 that may be closed by this pull request
Task is retried when it is setting failed manually #36471
Closed
2 tasks
@Lee-W
Lee-Wforce-pushed the fix-kpo-retry-even-after-marking-it-as-failed branch from b3eeab9 to 1eeeb3eCompareJanuary 17, 2024 08:40
@Lee-W
Lee-W requested a review from dirraoJanuary 17, 2024 08:41
dirrao
dirrao approved these changes Jan 18, 2024
View reviewed changes

@dirraodirrao left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Lee-W
Lee-Wforce-pushed the fix-kpo-retry-even-after-marking-it-as-failed branch from 1eeeb3e to a7f25edCompareJanuary 19, 2024 00:12
jedcunningham
jedcunningham reviewed Jan 19, 2024
View reviewed changes
Comment threadairflow/providers/cncf/kubernetes/operators/pod.py Outdated
jedcunningham
jedcunningham approved these changes Jan 19, 2024
View reviewed changes

@jedcunninghamjedcunningham left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second though, I think we are okay without test coverage on this. LGTM.

@Lee-W
fix(providers/cncf): move the _killed flag to cleanup method
ca60adc
if the task has been killed, do not cleanup again
@Lee-W
Lee-Wforce-pushed the fix-kpo-retry-even-after-marking-it-as-failed branch from f7f9935 to ca60adcCompareJanuary 20, 2024 07:52
@eladkal
eladkal merged commit d3b4a91 into apache:mainJan 20, 2024
@eladkaleladkal mentioned this pull request Jan 22, 2024
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dirraodirraodirrao approved these changes
@eladkaleladkaleladkal approved these changes
@jedcunninghamjedcunninghamjedcunningham approved these changes
@hussein-awalahussein-awalaAwaiting requested review from hussein-awala
@dstandishdstandishAwaiting requested review from dstandish
@amoghrajeshamoghrajeshAwaiting requested review from amoghrajesh

Assignees

No one assigned

Labels

area:providersprovider:cncf-kubernetesKubernetes (k8s) provider related issues

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Task is retried when it is setting failed manually

6 participants

@Lee-W@dstandish@amoghrajesh@dirrao@eladkal@jedcunningham