fix: use ownership instead of revision to guard teardown deletion#542
fix: use ownership instead of revision to guard teardown deletion#542erdii wants to merge 3 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
Walkthrough
ChangesOwner-aware object teardown flow
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
erdii
force-pushed
the
drop-revision-comparison-on-teardown-orphan
branch
from
43dae05 to
f3558d8
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #542 +/- ##
==========================================
+ Coverage 82.68% 82.70% +0.01%
==========================================
Files 32 32
Lines 2737 2740 +3
==========================================
+ Hits 2263 2266 +3
Misses 346 346
Partials 128 128 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
When an Owner is provided, check whether we are the controller via detectOwner rather than comparing revision annotations. Owner refs are the authoritative signal for ownership and correctly handle orphaning deletions where the revision annotation may have been updated by a different reconciliation pass. Callers without an Owner fall back to the existing revision comparison.
erdii
force-pushed
the
drop-revision-comparison-on-teardown-orphan
branch
from
f3558d8 to
90da9d2
Compare
| @@ -173,6 +164,16 @@ func (e *ObjectEngine) Teardown( | |||
| // TODO should we check if the patch differs from actualObject before firing the request? | |||
| return true, e.writer.Patch(ctx, patch, client.MergeFrom(actualObject)) | |||
There was a problem hiding this comment.
i think using a merge patch here is dangerous and could result in accidentally overwriting the object.
There was a problem hiding this comment.
Maybe sending an empty SSA intent with resourceVersion set to the observer version from actualObject instead is safe?
This would:
- give us 100% certainty that we know what we're overwriting because of optimistic locking
- remove all field values that we still own from the object including the owner reference
There's a pitfall though: Other actors in the system would have to deliberately own all fields that they want to keep when we tear down the object.
There was a problem hiding this comment.
Do we actually have to remove the owner ref at all?
Asumme:
- we have
owner-oldandowner-new. owner-newtakes the controlling owner reference and demotes our owner reference to non controlling. (It actually doesn't matter if the owner ref is demoted or not.)- teardown of the
owner-oldis triggered
then:
- we skip deletion of the managed object during teardown
- we remove the
owner-old - the owner ref to
owner-oldstarts dangling - the kubernetes garbage collector removes the danling owner ref
- the managed object still exists.
There was a problem hiding this comment.
So the case where ownership is transfered to a new owner is handled.
I think the only dangerous case is where our owner ref is demoted from controller: true to controller: false without adding a new owner.
If that's the case and we don't remove our owner ref then the garbage collector would still remove the managed object after the owner is deleted.
pbabic-redhat
left a comment
pbabic-redhat
left a comment
There was a problem hiding this comment.
Couple of small testing comments.
| afterAssert: func(t *testing.T, writer *testutil.CtrlClient) { | ||
| t.Helper() | ||
| writer.AssertNotCalled(t, "Delete") | ||
| }, |
There was a problem hiding this comment.
Won't the call to "Delete" fail if the mock was not setup with it (i.e., not called w.On("Delete",...)) anyway? If so, afterAssert might be unnecessary.
| if tc.afterAssert != nil { | ||
| tc.afterAssert(t, writer) | ||
| } |
There was a problem hiding this comment.
I would instead call AssertExpectations with all the mocks. I've seen so many tests that still don't have it. It can cause weird silent failures.
There was a problem hiding this comment.
Nice observation! Totally agree that AssertExpectations should be used everywhere. I've pushed a commit that addresses your concerns atleast for the teardown tests.
Let me know what you think and if you like it I'll squash the commits together.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@machinery/objects_test.go`:
- Around line 1254-1255: Remove the stale commented-out mock expectations for
the writer.On Delete method calls that are triggering dupword lint failures.
Delete the entire commented blocks containing writer.On("Delete", mock.Anything,
mock.Anything, mock.Anything) and its associated Panic statement at both
locations mentioned (lines 1254-1255 and lines 1285-1286) as these expectations
are no longer needed and cause CI failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: a1d991d3-a3c8-444f-bab5-b6bfbf9bc494
📒 Files selected for processing (1)
machinery/objects_test.go
2 participants
When an Owner is provided, check whether we are the controller via
detectOwner rather than comparing revision annotations. Owner refs
are the authoritative signal for ownership and correctly handle
orphaning deletions where the revision annotation may have been
updated by a different reconciliation pass. Callers without an
Owner fall back to the existing revision comparison.
Summary
Change Type
Check List Before Merging
Additional Information