🐛 Fix agent watcher store write race that erases DeletionTimestamp - #233
🐛 Fix agent watcher store write race that erases DeletionTimestamp#233ciaranRoche wants to merge 1 commit into
Conversation
A delete event applied by HandleReceivedResource while a status patch is in flight could be overwritten by the patch's final store write, because the version check and the write in ManifestWorkAgentClient.Patch are not atomic with respect to the store writers. The work then loses its DeletionTimestamp in the agent's in-memory store and the work controller recreates the deleted ManifestWork on the spoke on its next requeue. Serialize the agent watcher store writers with a store-level mutex, make HandleReceivedResource's read-modify-write atomic, and add an optional ConditionalUpdater capability (UpdateWithVersion) that performs the version check and the write in one critical section. The version check requires equality, which also rejects stale writes after the per-name versioner is reset by a delete/re-add cycle. Patch uses the capability when the store provides it and falls back to the previous behaviour otherwise, so the ClientWatcherStore interface is unchanged. Fixes open-cluster-management-io#232 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01USPzrSgidrmV9YrxRRf5tQ Signed-off-by: Ciaran Roche <croche@redhat.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ciaranRoche The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
WalkthroughThe change serializes watcher-store mutations, adds atomic conditional updates, and updates ManifestWork patching to use resource-version checks. New tests cover stale updates, deletion races, version resets, forced updates, and concurrent status patches. ChangesAtomic update flow
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
mikeshng
changed the title
mikeshng
left a comment
mikeshng
left a comment
There was a problem hiding this comment.
The good thing about this PR is only impacting users that uses the cloudevent stuffs and within that only the ManifestWork cloudevent. So most likely the impact is only Maestro so regression possibility for OCM-io kube driver (which to my knowledge 99% of users are on) is literally none.
This PR is also functionally correct but I am worry about the performance impact of introducing the lock/sync. One of the main goals of Maestro to my knowledge is performance so this might impact it depending on usage.
I will leave it to the maintainers to make the call.
/assign @jnpacker @qiujian16 @tesshuflower
| } | ||
|
|
||
| func (s *AgentInformerWatcherStore[T]) Add(resource runtime.Object) error { | ||
| s.mu.Lock() |
There was a problem hiding this comment.
I do not think work agent use this store. Do you need to update this generic store?
5 participants
Fixes #232
What this does
Closes the write race between
ManifestWorkAgentClient.Patchand the agent watcher store'sHandleReceivedResourcethat erasesDeletionTimestampfrom the agent's in-memory store and resurrects deleted ManifestWorks on the spoke (full analysis and log timeline in #232, in the wild report in open-cluster-management-io/ocm#1404).Three changes:
AgentInformerWatcherStore[T]and the work agent store now serialize their writers, andHandleReceivedResourceholds the lock for its whole read-modify-write sequence, so another writer can't be interleaved between reading the cached resource and writing it back.ConditionalUpdatercapability. A new optional interface inclients/storewith one method,UpdateWithVersion(ctx, resource, expectedResourceVersion). The work agent store implements it as a single critical section: get, compare the resource version for equality, bump the versioner, write. Equality (rather than the<check inversionCompare) also rejects stale writes after a delete/re-add cycle resets the per-name versioner, which was a second latent bug in the same code path.Patchuses the capability when available. The final store write inPatchtype-asserts forConditionalUpdaterand uses it, keeping the existing Get/versionCompare/Update as the fallback for stores that don't implement it, soClientWatcherStoreitself is unchanged and other implementations keep working as before.The CloudEvents publish stays outside the lock, so the lock is only ever held for an in-memory map operation, there's no contention on the network path. On conflict the caller retries and re-derives the patch from the fresh store state (which now includes the deletion timestamp), so nothing is lost.
Tests
TestManifestWorkAgentClient_ConcurrentStatusPatchAndDeleteEventreproduces the production failure. On current main it fails within a few iterations:With this change it passes reliably under
-race.TestManifestWorkAgentClient_Patch_DeleteEventDuringStatusPublishdeterministically interleaves a delete event while the status publish is in flight (via a publish hook) and asserts the stale patch gets a conflict and the deletion timestamp survives.TestAgentInformerWatcherStore_UpdateWithVersioncovers the CAS semantics: match, stale, force with "0", empty version, missing work, the versioner reset case, and delete-event-wins.go test -race ./pkg/cloudevents/...is green.🤖 Generated with Claude Code
https://claude.ai/code/session_01USPzrSgidrmV9YrxRRf5tQ
Summary by CodeRabbit