Skip to content

fix: job deletion propagation policy - #64

Open
gilbsgilbs wants to merge 2 commits into
snakemake:mainfrom
gilbsgilbs:fix-propagation-policy
Open

fix: job deletion propagation policy#64
gilbsgilbs wants to merge 2 commits into
snakemake:mainfrom
gilbsgilbs:fix-propagation-policy

Conversation

@gilbsgilbs

@gilbsgilbs gilbsgilbs commented Dec 15, 2025

Copy link
Copy Markdown

The previous way of setting the propagation policy had no effect. As a result, child resources such as pods (among others) would not be properly destroyed.

see kubernetes-client/python#234

Summary by CodeRabbit

  • Bug Fixes
    • Simplified job-deletion behavior to rely on Kubernetes for background cleanup, improving reliability of resource removal.
    • Maintains existing error handling (continues to ignore already-missing jobs) so external error responses remain unchanged.

✏️ Tip: You can customize this high-level summary in your review settings.

The previous way of setting the propagation policy had no effect. As a
result, child resources such as pods (among others) would not be
properly destroyed.

see kubernetes-client/python#234
@coderabbitai

coderabbitai Bot commented Dec 15, 2025

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

Updated safe_delete_job to remove explicit pod deletion and construct a V1DeleteOptions body with propagation_policy="Background", then call delete_namespaced_job passing only (jobid, namespace, body); 404 ApiException behavior remains unchanged.

Changes

Cohort / File(s) Summary
Kubernetes job deletion logic
snakemake_executor_plugin_kubernetes/__init__.py
Removed manual pod enumeration and deletion; changed V1DeleteOptions to use propagation_policy="Background" in the delete body; adjusted delete_namespaced_job call to omit the propagation_policy parameter and pass only (jobid, namespace, body); preserved existing 404-ignore ApiException handling.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Areas requiring attention:
    • Confirm that propagation_policy="Background" in the delete options body is accepted by the Kubernetes client and API server in this usage pattern
    • Verify that background propagation results in expected asynchronous cleanup of associated pods and does not leave orphaned resources in practice
    • Ensure the 404 ApiException handling still triggers correctly with the revised delete request structure

Possibly related PRs

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The pull request title 'fix: job deletion propagation policy' clearly and concisely identifies the main change: adjusting how the Kubernetes job deletion propagation policy is applied to ensure proper cleanup of child resources.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

It seems that sometimes, propagation policy does not apply well, and
some Volcano podgroups remain orphaned in `InQueue` state, consuming
resources while have no more pods attached.

This commit is one desperate attempt to workaround this problem once
again.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant