pkg/utils/etcdutil: reload watcher after compaction#11033
Conversation
Reload from a consistent etcd snapshot when the requested watch revision has been compacted, so changes in the compacted interval are not skipped. Keep the first load callback error and only advance the watch revision after a successful reload. Signed-off-by: Ryan Leung <rleungx@gmail.com>
ti-chi-bot
Bot
added
release-note
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
ti-chi-bot
Bot
added
the
size/L
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughLoopWatcher now reloads consistent etcd snapshots after compaction, retries with bounded backoff, reconciles deleted keys, and preserves callback errors. TSO, scheduling, routing, and service-primary watchers enable deleted-key reconciliation, with expanded tests and rule-watcher checker injection. ChangesLoopWatcher consistency
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant LoopWatcher
participant etcd
participant WatchCallbacks
LoopWatcher->>etcd: Watch from required revision
etcd-->>LoopWatcher: Compacted revision response
LoopWatcher->>etcd: Reload snapshot at consistent revision
etcd-->>LoopWatcher: Snapshot keys and loaded revision
LoopWatcher->>WatchCallbacks: Apply snapshot callbacks
LoopWatcher->>etcd: Resume watch after loaded revision
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #11033 +/- ##
==========================================
- Coverage 79.25% 79.25% -0.01%
==========================================
Files 541 541
Lines 76037 76134 +97
==========================================
+ Hits 60262 60337 +75
- Misses 11534 11549 +15
- Partials 4241 4248 +7
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
/retest |
Signed-off-by: Ryan Leung <rleungx@gmail.com>
ti-chi-bot
Bot
added
size/XXL
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
| // SetReconcileDeletedKeys enables deletion reconciliation for full loads. | ||
| // It must be called before StartWatchLoop. Since this keeps one entry per | ||
| // watched key, callers should only enable it for prefixes with bounded key cardinality. | ||
| func (lw *LoopWatcher) SetReconcileDeletedKeys() { |
There was a problem hiding this comment.
Should we also opt the ResourceManager metadata watcher into this mechanism in this PR? It maintains cached resource-group settings and service limits and has a non-no-op delete callback, but its factory exposes only StartWatchLoop and WaitLoad, so it currently cannot call this setter. I verified the generic behavior with a focused unit test: a reload without this opt-in retains a deleted key, while TestWatcherReconcilesDeletedKeysAfterCompaction passes with it enabled. If this watcher is intentionally out of scope, could we document or narrow the stated coverage?
Signed-off-by: Ryan Leung <rleungx@gmail.com>
|
@rleungx: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| } | ||
| } | ||
| if loadCompleted && err == nil && lw.reconcileDeletedKeys { | ||
| lw.loadedKeys = snapshotKeys |
There was a problem hiding this comment.
This all-or-nothing replacement loses partial reconciliation progress. reconcileLoadedKeys can successfully apply deletion A, then fail deletion B, while a successful postEventsFn still commits A. A remains in loadedKeys, so the retry invokes A delete again. That is unsafe for existing non-idempotent callbacks, including the rule watcher after its first commit removes the local RuleStorage entry, and the TSO watcher when the service-registry map entry is gone. A transient B failure can therefore leave compaction reload retrying forever.
I reproduced this on 03ed7ea: a two-key focused test with one successful delete and one one-time delete failure fails the second reload with key was already deleted; the same test passes on 492c88c. Please record successful operations at the same successful post-callback commit boundary, or make the callback contract explicitly idempotent, and add a regression test such as:
func (suite *loopWatcherTestSuite) TestWatcherRetriesPartialReconciliationDelete() {
re := suite.Require()
const prefix = "TestWatcherRetriesPartialReconciliationDelete/"
firstKey, secondKey := prefix+"first", prefix+"second"
suite.put(re, firstKey, "first")
suite.put(re, secondKey, "second")
cache := make(map[string]string)
transientErr := errors.New("transient delete error")
alreadyDeletedErr := errors.New("key was already deleted")
failSecondDelete := true
watcher := NewLoopWatcher(
suite.ctx, &suite.wg, suite.client, "test", prefix,
func([]*clientv3.Event) error { return nil },
func(kv *mvccpb.KeyValue) error {
cache[string(kv.Key)] = string(kv.Value)
return nil
},
func(kv *mvccpb.KeyValue) error {
key := string(kv.Key)
if key == secondKey && failSecondDelete {
failSecondDelete = false
return transientErr
}
if _, ok := cache[key]; !ok {
return alreadyDeletedErr
}
delete(cache, key)
return nil
},
func([]*clientv3.Event) error { return nil },
true,
)
watcher.SetReconcileDeletedKeys()
_, err := watcher.load(suite.ctx)
re.NoError(err)
_, err = suite.client.Delete(suite.ctx, firstKey)
re.NoError(err)
_, err = suite.client.Delete(suite.ctx, secondKey)
re.NoError(err)
_, err = watcher.load(suite.ctx)
re.ErrorIs(err, transientErr)
_, err = watcher.load(suite.ctx)
re.NoError(err)
re.Empty(cache)
}
| etcdClient: client, | ||
| ruleStorage: storage, | ||
| regionLabeler: labelerManager, | ||
| checkerController: checkerController, |
There was a problem hiding this comment.
Minor: could this helper close rw after the assertion instead of only canceling its context? Watcher.Close() waits for the label watcher and clears the injected checker controller suspect ranges. BenchmarkLoadLargeRules now reuses one controller across iterations, so without that cleanup later samples inherit the previous 16K ranges and can overlap with the previous watcher teardown. A defer rw.Close() after successful initialization would make each iteration independent.
2 participants
What problem does this PR solve?
Issue Number: Close #11032
When an etcd watch revision has been compacted, LoopWatcher resumes from the
compact revision without reloading current state. Changes in the compacted
interval are not replayed, so consumer caches can remain stale.
Paginated loads can also observe different etcd revisions, and an earlier
callback error can be overwritten by a later successful callback.
What is changed and how does it work?
after a successful load.
consistency, and callback error propagation.
Check List
Tests
Release note
Summary by CodeRabbit