fix: own pipeline cleanup job by its ISB Service to avoid deleting a recreated pipeline's buffers#3552
Conversation
|
Hey! Feel free to ignore this for now while I take a look at Claude's attempt. Would be interested in your thoughts on the general vibe of this solution though. Was this intentionally not assigned this owner for a reason that I don't understand? |
…recreated pipeline's buffers The isbsvc-delete "cln" job is created during Pipeline finalizer processing and must outlive the Pipeline, so its owner references were cleared. This left it fully orphaned: if a same-named Pipeline (and same-named ISB Service) is recreated while the job is still pending - e.g. blue/green tooling that recycles instance names - the job deletes the new Pipeline's freshly created buffers and OT buckets, and every vertex crash-loops on "stream not found" with no self-healing, because the controller's buffer diff never re-runs the create job. Owning the job by the ISB Service instance it cleans (bound by UID) scopes its lifetime correctly: a long-lived shared ISB keeps the job exactly as before, while deleting or replacing the ISB Service garbage collects the pending job instead of letting it fire at buffers it was never scoped to. This also removes the stuck errored cleanup pods described in numaproj#2759 when the ISB Service and Pipeline are deleted together. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Cameron Matthew <cameron@playerdata.com>
eae6a8a to
06b1439
Compare
Every buildISBBatchJob caller now passes the job's owner instead of the builder hardcoding Pipeline ownership and the cleanup call site mutating it afterwards. The cre/del jobs keep their Pipeline controller reference; the cln job passes the ISB Service reference introduced in the previous commit. Removes the only build-then-mutate call site. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Cameron Matthew <cameron@playerdata.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3552 +/- ##
==========================================
+ Coverage 83.13% 83.20% +0.06%
==========================================
Files 310 310
Lines 81995 81997 +2
==========================================
+ Hits 68168 68225 +57
+ Misses 13234 13176 -58
- Partials 593 596 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
We made the cln jobs not owned by anybody intentionally - it's not right for them to be owned by any objects. I'm trying to understand your problem, are you doing A/B deployment back to back in one namespace, and using same pipeline? or something else? |
|
Something like that. I think is "less" right to leave orphan jobs that are cleaning up for a Pipeline. Why is it not right to have them as childs of the pipeline resources? I think it would be much cleaner if the pipeline object was to wait for the cleaning tasks to be done before getting deleted. |
We are doing A/B deployments. We alternate between namespaces. So if we have two deployments in quick succession we do: --- 1st Deployment ---
--- 2nd Deployment ---
The orphaned cleanup jobs from B down then interfere with bringing B back up. As the gate between the two jobs is the B application being marked as removed. But B can be marked as removed while these cleanup jobs remain. |
3 participants
What this PR does
Makes the pipeline buffer-cleanup Job (
isbsvc-delete, job typecln) owned by the InterStepBufferService it cleans up, instead of clearing its OwnerReferences entirely. Two commits: the behavioral fix (with the matching unit-test update), and a small follow-up refactor that has everybuildISBBatchJobcaller pass the job's owner explicitly, removing the build-then-mutate call site.The problem
We do an A/B deployment that alternates namespaces. Each deployment cycle brings up a new set of pipelines and if they are healthy we drain and tear down the old set.
If we do two A/B deployments back-to-back, we see a race where these cleanup jobs hang around after the application is marked as deleted, and then when the new deployment is brought back, up they delete resources on the next generation of the application.
The root cause of this is that numaflow pipelines don't clean up after themselves properly. They don't clean up after themselves properly because this cleanup job doesn't have the right owner.
Not sure what you guys think about this particular solution. Happy to take suggestions.
We will probably work around this by not reusing the same namespace for new generations of deployment. But we think it is probably a gap worth filling nevertheless.