Merge cockroachdb#100401

100401: raftlog: correctly decode empty commands  r=erikgrinaker a=erikgrinaker

When unquiescing a Raft group, an empty command is proposed to wake the Raft leader. During application, empty entries are considered noops, but only if they don't have a command ID. However, Raft entry decoding would preserve the command ID of an empty payload. `CheckForcedErr()` thus no longer considered this a noop, instead returning an error because the `ProposerLeaseSequence` of 0 was older than the current lease. Construction and processing of this error had a non-negligible cost when unquiescing a large number of ranges.

This patch makes sure such empty commands are decoded as empty entries, without a command ID, and thus considered noops.

Resolves cockroachdb#100400.

Epic: none
Release note: None

Co-authored-by: Erik Grinaker <[email protected]>

pkg/kv/kvserver/kvserverbase/forced_error.go

@@ -95,11 +95,11 @@ func CheckForcedErr(
 		requestedLease = *raftCmd.ReplicatedEvalResult.State.Lease
 	}
 	if idKey == "" {
-		// This is an empty Raft command (which is sent by Raft after elections
-		// to trigger reproposals or during concurrent configuration changes).
-		// Nothing to do here except making sure that the corresponding batch
-		// (which is bogus) doesn't get executed (for it is empty and so
-		// properties like key range are undefined).
+		// This is an empty Raft command, which is sent by Raft after elections to
+		// trigger reproposals, during concurrent configuration changes, or to
+		// unquiesce the Raft group. Nothing to do here except making sure that the
+		// corresponding batch (which is bogus) doesn't get executed (for it is
+		// empty and so properties like key range are undefined).
 		return ForcedErrResult{
 			LeaseIndex:  leaseIndex,
 			Rejection:   ProposalRejectionPermanent,

pkg/kv/kvserver/raftlog/entry.go

@@ -183,10 +183,15 @@ func (e *Entry) load() error {
 		}
 		e.ID = kvserverbase.CmdIDKey(e.ConfChangeContext.CommandID)
 		raftCmdBytes = e.ConfChangeContext.Payload
+	} else if len(raftCmdBytes) == 0 {
+		// Empty commands may be proposed to wake the leader, e.g. during
+		// unquiescence. Ignore them during application by clearing the command ID
+		// and other fields (see CheckForcedErr), similarly to those submitted by
+		// Raft on leader changes (see EntryEncodingEmpty).
+		*e = Entry{Entry: e.Entry}
+		return nil
 	}
 
-	// TODO(tbg): can len(payload)==0 if we propose an empty command to wake up leader?
-	// If so, is that a problem here?
 	return errors.Wrap(protoutil.Unmarshal(raftCmdBytes, &e.Cmd), "unmarshalling RaftCommand")
 }
 

pkg/kv/kvserver/raftlog/entry_test.go

@@ -18,17 +18,46 @@ import (
 	"go.etcd.io/raft/v3/raftpb"
 )
 
-func TestLoadInvalidEntry(t *testing.T) {
-	invalidEnt := raftpb.Entry{
-		Term:  1,
-		Index: 1,
-		Data: EncodeRaftCommand(
-			// It would be nice to have an "even more invalid" command here but it
-			// turns out that DecodeRaftCommand "handles" errors via panic().
-			EntryEncodingStandardWithAC, "foobarzz", []byte("definitely not a protobuf"),
-		),
+func TestNewEntry(t *testing.T) {
+	// TODO(replication): Add more cases.
+	testcases := map[string]struct {
+		data        []byte
+		expectEmpty bool
+		expectErr   bool
+	}{
+		// Proposed by Raft on leader change.
+		"empty entry": {data: nil, expectEmpty: true},
+		// Proposed by CRDB on unquiescence.
+		"empty payload": {
+			data:        EncodeRaftCommand(EntryEncodingStandardWithoutAC, "00000000", nil),
+			expectEmpty: true,
+		},
+		"invalid": {
+			data:      EncodeRaftCommand(EntryEncodingStandardWithAC, "00000000", []byte("not a protobuf")),
+			expectErr: true,
+		},
+	}
+	for name, tc := range testcases {
+		t.Run(name, func(t *testing.T) {
+			ent, err := NewEntry(raftpb.Entry{
+				Term:  1,
+				Index: 1,
+				Data:  tc.data,
+			})
+			if tc.expectErr {
+				require.Error(t, err)
+				return
+			}
+			require.NoError(t, err)
+
+			// Clear out the passed Raft entry, and only assert on the decoded entry.
+			require.NotNil(t, ent)
+			ent.Entry = raftpb.Entry{}
+			if tc.expectEmpty {
+				require.Zero(t, *ent)
+			} else {
+				require.NotZero(t, *ent)
+			}
+		})
 	}
-	ent, err := NewEntry(invalidEnt)
-	require.Error(t, err) // specific error doesn't matter
-	require.Zero(t, ent)
 }