mvcc: restore tombstone index if it's first revision #19188
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: fuweid The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files
... and 29 files with indirect coverage changes @@ Coverage Diff @@
## main #19188 +/- ##
==========================================
- Coverage 68.83% 68.76% -0.07%
==========================================
Files 420 420
Lines 35641 35642 +1
==========================================
- Hits 24532 24511 -21
- Misses 9687 9706 +19
- Partials 1422 1425 +3 Continue to review full report in Codecov by Sentry.
|
|
How much this differs from #18089 ? |
As mentioned in #19179 (comment), it's an edge case that we missed when fixing #18089. etcdserver shouldn't silently drop any deletion event, it should either deliver the event to client or respond with an ErrCompact error. But since we missed this edge case (etcdserver crash right after finishing the compaction but before setting the |
|
/retest |
Don't know :( |
|
Overall looks good, with only two minor comments. Thanks @fuweid |
The tombstone could be the only one available revision in database. It happens when all historical revisions have been deleted in previous compactions. Since tombstone revision is still in database, we should restore it as valid key index. Otherwise, we lost that event. Signed-off-by: Wei Fu <fuweid89@gmail.com>
|
/retest |
|
@fuweid: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions 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. |
| require.NoError(t, err) | ||
| defer clus.Close() | ||
|
|
||
| c1 := newClient(t, clus.EndpointsGRPC(), cfg.Client) |
| case watchResp := <-watchChan: | ||
| require.Len(t, watchResp.Events, 1) | ||
|
|
||
| require.Equal(t, mvccpb.DELETE, watchResp.Events[0].Type) |
There was a problem hiding this comment.
I was a little surprised that we are able to restore tombstone without any information about key creation, but apparently fields like CreateRevision, Version are set to 0 on delete. Still I think it would be nice if it was obvious in the test.
Instead of comparing subset of fields to some hardcoded values, can we compare whole event to matching event from before the compaction?
// new lines
var deleteEvent *clientv3.Event
select {
case watchResp := <-c1.Watch(ctx, firstKey, clientv3.WithRev(deleteResp.Header.Revision)):
require.Len(t, watchResp.Events, 1)
deleteEvent = watchResp.Events[0]
case <-time.After(100 * time.Millisecond):
t.Fatal("timed out getting watch response")
}
// old lines
require.NoError(t, clus.Procs[0].Failpoints().SetupHTTP(ctx, "compactBeforeSetFinishedCompact", `panic`))
t.Logf("COMPACT rev=%d", deleteResp.Header.Revision)
_, err = c1.KV.Compact(ctx, deleteResp.Header.Revision, clientv3.WithCompactPhysical())
require.Error(t, err)
require.NoError(t, clus.Restart(ctx))
c2 := newClient(t, clus.EndpointsGRPC(), cfg.Client)
defer c2.Close()
watchChan := c2.Watch(ctx, firstKey, clientv3.WithRev(deleteResp.Header.Revision))
select {
case watchResp := <-watchChan:
require.Len(t, watchResp.Events, 1)
// changed lines
require.Equal(t, deleteEvent, watchResp.Events[0])
case <-time.After(100 * time.Millisecond):
// we care only about the first response, but have an
// escape hatch in case the watch response is delayed.
t.Fatal("timed out getting watch response")
}
The tombstone could be the only one available revision in database. It happens when all historical revisions have been deleted in previous compactions. Since tombstone revision is still in database, we should restore it as valid key index. Otherwise, we lost that deletion event if we try to compact on that revision.
REF: #19179
Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.