Skip to content

Conversation

@sean-rose
Copy link
Contributor

@sean-rose sean-rose commented Apr 18, 2025

closes: #49441
Currently the on_pod_cleanup callbacks are only called if the cleanup method completes successfully, but the cleanup method raises exceptions if the task failed. This changes KubernetesPodOperator to call the on_pod_cleanup callbacks from within the cleanup method before it raises such exceptions.


^ 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 airflow-core/newsfragments.

@eladkal eladkal force-pushed the k8s-pod-cleanup-callbacks-for-failed-tasks branch from f6bc1cc to ae51de7 Compare May 20, 2025 08:17
@eladkal eladkal requested a review from romsharon98 May 20, 2025 08:17
@romsharon98
Copy link
Contributor

can you change the description of the PR to reference the correct issue? (you referencing now to the current PR :))

@romsharon98
Copy link
Contributor

romsharon98 commented May 21, 2025

according to those lines I think the skipping on the on_pod_cleanup is on purpose.

@sean-rose
Copy link
Contributor Author

according to those lines I think the skipping on the on_pod_cleanup is on purpose.

My interpretation of those comments is that cleanup code should be skipped when the task has been killed, which is still the case with this change.

@romsharon98
Copy link
Contributor

romsharon98 commented May 22, 2025

Got it — looks good!
To help prevent regressions, what do you think about adding a test to ensure that callbacks.on_pod_cleanup is called even when the pod fails or self.skip_on_exit_code is set to True?

@github-actions
Copy link

github-actions bot commented Jul 7, 2025

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.

@github-actions github-actions bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Jul 7, 2025
@sean-rose
Copy link
Contributor Author

To help prevent regressions, what do you think about adding a test to ensure that callbacks.on_pod_cleanup is called even when the pod fails or self.skip_on_exit_code is set to True?

That sounds reasonable. I'm about to go on vacation, but will try to get back to this when I return.

@github-actions github-actions bot removed the stale Stale PRs per the .github/workflows/stale.yml policy file label Jul 11, 2025
@github-actions
Copy link

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.

@github-actions github-actions bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Aug 26, 2025
@sean-rose
Copy link
Contributor Author

Sorry, Git bot, I've been busy. 😛

Still hoping to find time to complete this at some point.

@github-actions github-actions bot removed the stale Stale PRs per the .github/workflows/stale.yml policy file label Aug 30, 2025
@kev-datams
Copy link

Hi @sean-rose, we also observed this behavior. Thank you for contributing 👍
Do you have an ETA for the fix ? 👼

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 (k8s) provider related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants