Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

avoid retrying after KubernetesPodOperator has been marked as failed #36749

Merged

Conversation

Lee-W
Copy link
Member

@Lee-W Lee-W commented Jan 12, 2024

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-cyborg boring-cyborg bot added area:providers provider:cncf-kubernetes Kubernetes provider related issues labels Jan 12, 2024
@Lee-W Lee-W force-pushed the fix-kpo-retry-even-after-marking-it-as-failed branch from a7573f7 to 6b0f18f Compare January 12, 2024 15:32
Copy link
Contributor

@dirrao dirrao left a comment

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.

@Lee-W Lee-W force-pushed the fix-kpo-retry-even-after-marking-it-as-failed branch 2 times, most recently from 4f3b55b to 141b7a8 Compare January 15, 2024 00:45
@Lee-W Lee-W force-pushed the fix-kpo-retry-even-after-marking-it-as-failed branch from 141b7a8 to c10f762 Compare January 15, 2024 07:18
Copy link
Contributor

@amoghrajesh amoghrajesh left a comment

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?

Copy link
Member

@jedcunningham jedcunningham left a comment

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.

@eladkal eladkal linked an issue Jan 16, 2024 that may be closed by this pull request
2 tasks
@Lee-W Lee-W force-pushed the fix-kpo-retry-even-after-marking-it-as-failed branch from b3eeab9 to 1eeeb3e Compare January 17, 2024 08:40
@Lee-W Lee-W requested a review from dirrao January 17, 2024 08:41
Copy link
Contributor

@dirrao dirrao left a comment

Choose a reason for hiding this comment

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

LGTM

@Lee-W Lee-W requested a review from jedcunningham January 18, 2024 13:35
@Lee-W Lee-W force-pushed the fix-kpo-retry-even-after-marking-it-as-failed branch from 1eeeb3e to a7f25ed Compare January 19, 2024 00:12
Copy link
Member

@jedcunningham jedcunningham left a comment

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.

if the task has been killed, do not cleanup again
@Lee-W Lee-W force-pushed the fix-kpo-retry-even-after-marking-it-as-failed branch from f7f9935 to ca60adc Compare January 20, 2024 07:52
@eladkal eladkal merged commit d3b4a91 into apache:main Jan 20, 2024
59 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers provider:cncf-kubernetes Kubernetes provider related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Task is retried when it is setting failed manually
6 participants