From 5aeac1abf9acf0a2ca01a0b505235763681a2381 Mon Sep 17 00:00:00 2001 From: Pavel Kalinnikov Date: Fri, 27 Sep 2024 13:29:05 +0100 Subject: [PATCH] apply: fix initial snapshot assertion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The broad assertion here is that all the entries composing a snapshot must have been applied by someone before (at least the sender of the snapshot has done it, otherwise it couldn’t have sent this snapshot). And then there is an additional assertion (which failed occasionally before this PR) that there can’t be a snapshot without at least one entry applied. This is generally true, except that there are entries not registered by the "apply" stack: particularly, empty entries that raft leader appends at the start of its term. There is a comment to this extent in the `asserter.go` code: https://github.com/cockroachdb/cockroach/blob/5b0371b44009ac2ae3f58ea7ed95b290dd4e8227/pkg/kv/kvserver/apply/asserter.go#L254-L259 This commit modifies the snapshot assertion to correctly handle this case. It is possible that an initial snapshot sent from leader to followers contains only this dummy entry, and there were no "real" proposals applied. Epic: none Release note: none --- pkg/kv/kvserver/apply/asserter.go | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/pkg/kv/kvserver/apply/asserter.go b/pkg/kv/kvserver/apply/asserter.go index d0da9cba565b..19ac02122e29 100644 --- a/pkg/kv/kvserver/apply/asserter.go +++ b/pkg/kv/kvserver/apply/asserter.go @@ -380,17 +380,23 @@ func (r *rangeAsserter) applySnapshot( r.Lock() defer r.Unlock() - // INVARIANT: we can't have a snapshot without any applied log entries. - if len(r.log) == 0 { - fail("snapshot before any log entries applied") - } - // INVARIANT: a snapshot must progress the replica's applied index. if ri := r.replicaAppliedIndex[replicaID]; index <= ri { fail("replica applied index regression: %d -> %d", ri, index) } r.replicaAppliedIndex[replicaID] = index + // We can't have a snapshot without any applied log entries, except when this + // is an initial snapshot. It's possible that the initial snapshot follows an + // empty entry appended by the raft leader at the start of this term. Since we + // don't register this entry as applied, r.log can be empty here. + // + // See the comment in r.apply() method, around the empty cmdID check, and the + // comment for r.log saying that there can be gaps in the observed applies. + if len(r.log) == 0 { + return + } + // INVARIANT: a snapshot must match the applied state at the given Raft index. // // The snapshot may point beyond the log or to a gap in our log because of