From 1511367ac7ad0e8d345c46ad7663e4b38ab23491 Mon Sep 17 00:00:00 2001 From: Ben Darnell Date: Tue, 19 Apr 2016 00:17:15 -0400 Subject: [PATCH] storage: fix "inverted range" panic The store now returns NotLeaderError instead of a malformed RangeKeyMismatchError when a request is addressed to an uninitialized replica. The replica that caused the creation of the uninitialized replica is used as our best guess for the current leader. Guard against regressions by checking the validity of the descriptor passed to NewRangeKeyMismatchError. Also add an admittedly complex regression test. Fixes #6027 --- kv/dist_sender_server_test.go | 132 ++++++++++++++++++++++++++++++++++ kv/dist_sender_test.go | 36 +++++----- roachpb/errors.go | 5 ++ roachpb/metadata.go | 10 +++ storage/replica.go | 6 +- storage/store.go | 19 ++++- 6 files changed, 187 insertions(+), 21 deletions(-) diff --git a/kv/dist_sender_server_test.go b/kv/dist_sender_server_test.go index eb3b418b55f6..57da1396bc00 100644 --- a/kv/dist_sender_server_test.go +++ b/kv/dist_sender_server_test.go @@ -28,6 +28,7 @@ import ( "github.com/cockroachdb/cockroach/kv" "github.com/cockroachdb/cockroach/roachpb" "github.com/cockroachdb/cockroach/server" + "github.com/cockroachdb/cockroach/storage" "github.com/cockroachdb/cockroach/storage/engine" "github.com/cockroachdb/cockroach/testutils" "github.com/cockroachdb/cockroach/testutils/storageutils" @@ -36,6 +37,7 @@ import ( "github.com/cockroachdb/cockroach/util/leaktest" "github.com/cockroachdb/cockroach/util/log" "github.com/cockroachdb/cockroach/util/uuid" + "github.com/coreos/etcd/raft/raftpb" ) // NOTE: these tests are in package kv_test to avoid a circular @@ -663,3 +665,133 @@ func TestPropagateTxnOnPushError(t *testing.T) { } } } + +// TestRequestToUninitializedRange tests the behavior when a request +// is sent to a node which should be a replica of the correct range +// but has not yet received its initial snapshot. This would +// previously panic due to a malformed error response from the server, +// as seen in https://github.com/cockroachdb/cockroach/issues/6027. +// +// Prior to the other changes in the commit that introduced it, this +// test would reliable trigger the panic from #6027. However, it +// relies on some hacky tricks to both trigger the panic and shut down +// cleanly. If this test needs a lot of maintenance in the future we +// should be willing to get rid of it. +func TestRequestToUninitializedRange(t *testing.T) { + defer leaktest.AfterTest(t)() + s := server.TestServer{StoresPerNode: 2} + if err := s.Start(); err != nil { + t.Fatalf("Could not start server: %v", err) + } + defer s.Stop() + + // Choose a range ID that is much larger than any that would be + // created by initial splits. + const rangeID = roachpb.RangeID(1000) + + // Set up a range with replicas on two stores of the same node. This + // ensures that the DistSender will consider both replicas healthy + // and will try to talk to both (so we can get a non-retryable error + // from the second store). + replica1 := roachpb.ReplicaDescriptor{ + NodeID: 1, + StoreID: 1, + ReplicaID: 1, + } + replica2 := roachpb.ReplicaDescriptor{ + NodeID: 1, + StoreID: 2, + ReplicaID: 2, + } + + // HACK: remove the second store from the node to generate a + // non-retryable error when we try to talk to it. + store2, err := s.Stores().GetStore(2) + if err != nil { + t.Fatal(err) + } + s.Stores().RemoveStore(store2) + + // Create the uninitialized range by sending an isolated raft + // message to the first store. + conn, err := s.RPCContext().GRPCDial(s.ServingAddr()) + if err != nil { + t.Fatal(err) + } + raftClient := storage.NewMultiRaftClient(conn) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + stream, err := raftClient.RaftMessage(ctx) + if err != nil { + t.Fatal(err) + } + msg := storage.RaftMessageRequest{ + GroupID: rangeID, + ToReplica: replica1, + FromReplica: replica2, + Message: raftpb.Message{ + Type: raftpb.MsgApp, + To: 1, + }, + } + if err := stream.Send(&msg); err != nil { + t.Fatal(err) + } + + // Make sure the replica was created. + store1, err := s.Stores().GetStore(1) + if err != nil { + t.Fatal(err) + } + util.SucceedsSoon(t, func() error { + if replica, err := store1.GetReplica(rangeID); err != nil { + return util.Errorf("failed to look up replica: %s", err) + } else if replica.IsInitialized() { + return util.Errorf("expected replica to be uninitialized") + } + return nil + }) + + // Create our own DistSender so we can force some requests to the + // bogus range. The DistSender needs to be in scope for its own + // MockRangeDescriptorDB closure. + var sender *kv.DistSender + sender = kv.NewDistSender(&kv.DistSenderContext{ + Clock: s.Clock(), + RPCContext: s.RPCContext(), + RangeDescriptorDB: kv.MockRangeDescriptorDB( + func(key roachpb.RKey, considerIntents, useReverseScan bool, + ) ([]roachpb.RangeDescriptor, []roachpb.RangeDescriptor, *roachpb.Error) { + if key.Equal(roachpb.RKeyMin) { + // Pass through requests for the first range to the real sender. + desc, pErr := sender.FirstRange() + if pErr != nil { + return nil, nil, pErr + } + return []roachpb.RangeDescriptor{*desc}, nil, nil + } + return []roachpb.RangeDescriptor{{ + RangeID: rangeID, + StartKey: roachpb.RKey(keys.Meta2Prefix), + EndKey: roachpb.RKeyMax, + Replicas: []roachpb.ReplicaDescriptor{replica1, replica2}, + }}, nil, nil + }), + }, s.Gossip()) + // Only inconsistent reads triggered the panic in #6027. + hdr := roachpb.Header{ + ReadConsistency: roachpb.INCONSISTENT, + } + req := roachpb.NewGet(roachpb.Key("asdf")) + // Repeat the test a few times: due to the randomization between the + // two replicas, each attempt only had a 50% chance of triggering + // the panic. + for i := 0; i < 5; i++ { + _, pErr := client.SendWrappedWith(sender, context.Background(), hdr, req) + // Each attempt fails with "store 2 not found" because that is the + // non-retryable error. + if !testutils.IsPError(pErr, "store 2 not found") { + t.Fatal(pErr) + } + } +} diff --git a/kv/dist_sender_test.go b/kv/dist_sender_test.go index 91264468de25..ff4ce8a67ae2 100644 --- a/kv/dist_sender_test.go +++ b/kv/dist_sender_test.go @@ -286,7 +286,7 @@ func TestSendRPCOrder(t *testing.T) { ctx := &DistSenderContext{ RPCSend: testFn, - RangeDescriptorDB: mockRangeDescriptorDB(func(roachpb.RKey, bool, bool) ([]roachpb.RangeDescriptor, []roachpb.RangeDescriptor, *roachpb.Error) { + RangeDescriptorDB: MockRangeDescriptorDB(func(roachpb.RKey, bool, bool) ([]roachpb.RangeDescriptor, []roachpb.RangeDescriptor, *roachpb.Error) { return []roachpb.RangeDescriptor{descriptor}, nil, nil }), } @@ -359,12 +359,12 @@ func TestSendRPCOrder(t *testing.T) { } } -type mockRangeDescriptorDB func(roachpb.RKey, bool, bool) ([]roachpb.RangeDescriptor, []roachpb.RangeDescriptor, *roachpb.Error) +type MockRangeDescriptorDB func(roachpb.RKey, bool, bool) ([]roachpb.RangeDescriptor, []roachpb.RangeDescriptor, *roachpb.Error) -func (mdb mockRangeDescriptorDB) RangeLookup(key roachpb.RKey, _ *roachpb.RangeDescriptor, considerIntents, useReverseScan bool) ([]roachpb.RangeDescriptor, []roachpb.RangeDescriptor, *roachpb.Error) { +func (mdb MockRangeDescriptorDB) RangeLookup(key roachpb.RKey, _ *roachpb.RangeDescriptor, considerIntents, useReverseScan bool) ([]roachpb.RangeDescriptor, []roachpb.RangeDescriptor, *roachpb.Error) { return mdb(stripMeta(key), considerIntents, useReverseScan) } -func (mdb mockRangeDescriptorDB) FirstRange() (*roachpb.RangeDescriptor, *roachpb.Error) { +func (mdb MockRangeDescriptorDB) FirstRange() (*roachpb.RangeDescriptor, *roachpb.Error) { rs, _, err := mdb.RangeLookup(nil, nil, false /* considerIntents */, false /* useReverseScan */) if err != nil || len(rs) == 0 { return nil, err @@ -372,7 +372,7 @@ func (mdb mockRangeDescriptorDB) FirstRange() (*roachpb.RangeDescriptor, *roachp return &rs[0], nil } -var defaultMockRangeDescriptorDB = mockRangeDescriptorDB(func(key roachpb.RKey, _, _ bool) ([]roachpb.RangeDescriptor, []roachpb.RangeDescriptor, *roachpb.Error) { +var defaultMockRangeDescriptorDB = MockRangeDescriptorDB(func(key roachpb.RKey, _, _ bool) ([]roachpb.RangeDescriptor, []roachpb.RangeDescriptor, *roachpb.Error) { if bytes.HasPrefix(key, keys.Meta2Prefix) { return []roachpb.RangeDescriptor{testMetaRangeDescriptor}, nil, nil } @@ -532,7 +532,7 @@ func TestRetryOnDescriptorLookupError(t *testing.T) { ctx := &DistSenderContext{ RPCSend: testFn, - RangeDescriptorDB: mockRangeDescriptorDB(func(key roachpb.RKey, _, _ bool) ([]roachpb.RangeDescriptor, []roachpb.RangeDescriptor, *roachpb.Error) { + RangeDescriptorDB: MockRangeDescriptorDB(func(key roachpb.RKey, _, _ bool) ([]roachpb.RangeDescriptor, []roachpb.RangeDescriptor, *roachpb.Error) { // Return next error and truncate the prefix of the errors array. var pErr *roachpb.Error if key != nil { @@ -839,7 +839,7 @@ func TestSendRPCRetry(t *testing.T) { } ctx := &DistSenderContext{ RPCSend: testFn, - RangeDescriptorDB: mockRangeDescriptorDB(func(key roachpb.RKey, _, _ bool) ([]roachpb.RangeDescriptor, []roachpb.RangeDescriptor, *roachpb.Error) { + RangeDescriptorDB: MockRangeDescriptorDB(func(key roachpb.RKey, _, _ bool) ([]roachpb.RangeDescriptor, []roachpb.RangeDescriptor, *roachpb.Error) { if bytes.HasPrefix(key, keys.Meta2Prefix) { return []roachpb.RangeDescriptor{testMetaRangeDescriptor}, nil, nil } @@ -942,7 +942,7 @@ func TestMultiRangeMergeStaleDescriptor(t *testing.T) { } ctx := &DistSenderContext{ RPCSend: testFn, - RangeDescriptorDB: mockRangeDescriptorDB(func(key roachpb.RKey, _, _ bool) ([]roachpb.RangeDescriptor, []roachpb.RangeDescriptor, *roachpb.Error) { + RangeDescriptorDB: MockRangeDescriptorDB(func(key roachpb.RKey, _, _ bool) ([]roachpb.RangeDescriptor, []roachpb.RangeDescriptor, *roachpb.Error) { if bytes.HasPrefix(key, keys.Meta2Prefix) { return []roachpb.RangeDescriptor{testMetaRangeDescriptor}, nil, nil } @@ -983,7 +983,7 @@ func TestRangeLookupOptionOnReverseScan(t *testing.T) { ctx := &DistSenderContext{ RPCSend: testFn, - RangeDescriptorDB: mockRangeDescriptorDB(func(key roachpb.RKey, considerIntents, useReverseScan bool) ([]roachpb.RangeDescriptor, []roachpb.RangeDescriptor, *roachpb.Error) { + RangeDescriptorDB: MockRangeDescriptorDB(func(key roachpb.RKey, considerIntents, useReverseScan bool) ([]roachpb.RangeDescriptor, []roachpb.RangeDescriptor, *roachpb.Error) { if len(key) > 0 && !useReverseScan { t.Fatalf("expected UseReverseScan to be set") } @@ -1070,7 +1070,7 @@ func TestTruncateWithSpanAndDescriptor(t *testing.T) { t.Fatal(err) } - // Fill mockRangeDescriptorDB with two descriptors. When a + // Fill MockRangeDescriptorDB with two descriptors. When a // range descriptor is looked up by key "b", return the second // descriptor whose range is ["a", "c") and partially overlaps // with the first descriptor's range. @@ -1096,7 +1096,7 @@ func TestTruncateWithSpanAndDescriptor(t *testing.T) { }, }, } - descDB := mockRangeDescriptorDB(func(key roachpb.RKey, _, _ bool) ([]roachpb.RangeDescriptor, []roachpb.RangeDescriptor, *roachpb.Error) { + descDB := MockRangeDescriptorDB(func(key roachpb.RKey, _, _ bool) ([]roachpb.RangeDescriptor, []roachpb.RangeDescriptor, *roachpb.Error) { if bytes.HasPrefix(key, keys.Meta2Prefix) { return []roachpb.RangeDescriptor{testMetaRangeDescriptor}, nil, nil } @@ -1177,7 +1177,7 @@ func TestTruncateWithLocalSpanAndDescriptor(t *testing.T) { t.Fatal(err) } - // Fill mockRangeDescriptorDB with two descriptors. + // Fill MockRangeDescriptorDB with two descriptors. var descriptor1 = roachpb.RangeDescriptor{ RangeID: 1, StartKey: roachpb.RKeyMin, @@ -1212,7 +1212,7 @@ func TestTruncateWithLocalSpanAndDescriptor(t *testing.T) { }, } - descDB := mockRangeDescriptorDB(func(key roachpb.RKey, _, _ bool) ([]roachpb.RangeDescriptor, []roachpb.RangeDescriptor, *roachpb.Error) { + descDB := MockRangeDescriptorDB(func(key roachpb.RKey, _, _ bool) ([]roachpb.RangeDescriptor, []roachpb.RangeDescriptor, *roachpb.Error) { switch { case bytes.HasPrefix(key, keys.Meta2Prefix): return []roachpb.RangeDescriptor{testMetaRangeDescriptor}, nil, nil @@ -1361,7 +1361,7 @@ func TestSequenceUpdateOnMultiRangeQueryLoop(t *testing.T) { } - // Fill mockRangeDescriptorDB with two descriptors. + // Fill MockRangeDescriptorDB with two descriptors. var descriptor1 = roachpb.RangeDescriptor{ RangeID: 1, StartKey: roachpb.RKeyMin, @@ -1384,7 +1384,7 @@ func TestSequenceUpdateOnMultiRangeQueryLoop(t *testing.T) { }, }, } - descDB := mockRangeDescriptorDB(func(key roachpb.RKey, _, _ bool) ([]roachpb.RangeDescriptor, []roachpb.RangeDescriptor, *roachpb.Error) { + descDB := MockRangeDescriptorDB(func(key roachpb.RKey, _, _ bool) ([]roachpb.RangeDescriptor, []roachpb.RangeDescriptor, *roachpb.Error) { if bytes.HasPrefix(key, keys.Meta2Prefix) { return []roachpb.RangeDescriptor{testMetaRangeDescriptor}, nil, nil } @@ -1492,7 +1492,7 @@ func TestMultiRangeSplitEndTransaction(t *testing.T) { } - // Fill mockRangeDescriptorDB with two descriptors. + // Fill MockRangeDescriptorDB with two descriptors. var descriptor1 = roachpb.RangeDescriptor{ RangeID: 1, StartKey: roachpb.RKeyMin, @@ -1515,7 +1515,7 @@ func TestMultiRangeSplitEndTransaction(t *testing.T) { }, }, } - descDB := mockRangeDescriptorDB(func(key roachpb.RKey, _, _ bool) ([]roachpb.RangeDescriptor, []roachpb.RangeDescriptor, *roachpb.Error) { + descDB := MockRangeDescriptorDB(func(key roachpb.RKey, _, _ bool) ([]roachpb.RangeDescriptor, []roachpb.RangeDescriptor, *roachpb.Error) { if bytes.HasPrefix(key, keys.Meta2Prefix) { return []roachpb.RangeDescriptor{testMetaRangeDescriptor}, nil, nil } @@ -1596,7 +1596,7 @@ func TestCountRanges(t *testing.T) { } // Mock out descriptor DB and sender function. - descDB := mockRangeDescriptorDB(func(key roachpb.RKey, _, _ bool) ([]roachpb.RangeDescriptor, []roachpb.RangeDescriptor, *roachpb.Error) { + descDB := MockRangeDescriptorDB(func(key roachpb.RKey, _, _ bool) ([]roachpb.RangeDescriptor, []roachpb.RangeDescriptor, *roachpb.Error) { if bytes.HasPrefix(key, keys.Meta2Prefix) { return []roachpb.RangeDescriptor{testMetaRangeDescriptor}, nil, nil } diff --git a/roachpb/errors.go b/roachpb/errors.go index d8282ef0b156..93368eb56571 100644 --- a/roachpb/errors.go +++ b/roachpb/errors.go @@ -299,6 +299,11 @@ var _ ErrorDetailInterface = &RangeNotFoundError{} // NewRangeKeyMismatchError initializes a new RangeKeyMismatchError. func NewRangeKeyMismatchError(start, end Key, desc *RangeDescriptor) *RangeKeyMismatchError { + if desc != nil && !desc.IsInitialized() { + // We must never send uninitialized ranges back to the client (nil + // is fine) guard against regressions of #6027. + panic("descriptor is not initialized") + } return &RangeKeyMismatchError{ RequestStartKey: start, RequestEndKey: end, diff --git a/roachpb/metadata.go b/roachpb/metadata.go index 0c095f0c0c6c..a0c02c99d710 100644 --- a/roachpb/metadata.go +++ b/roachpb/metadata.go @@ -125,6 +125,16 @@ func (r RangeDescriptor) FindReplica(storeID StoreID) (int, *ReplicaDescriptor) return -1, nil } +// IsInitialized returns false if this descriptor represents an +// uninitialized range. +// TODO(bdarnell): unify this with Validate(). +func (r RangeDescriptor) IsInitialized() bool { + if len(r.EndKey) == 0 { + return false + } + return true +} + // Validate performs some basic validation of the contents of a range descriptor. func (r RangeDescriptor) Validate() error { if r.NextReplicaID == 0 { diff --git a/storage/replica.go b/storage/replica.go index e3d6857897c1..2e38f33a4ec6 100644 --- a/storage/replica.go +++ b/storage/replica.go @@ -188,6 +188,10 @@ type Replica struct { systemDBHash []byte // sha1 hash of the system config @ last gossip abortCache *AbortCache // Avoids anomalous reads after abort + // creatingReplica is set when a replica is created as uninitialized + // via a raft message. + creatingReplica *roachpb.ReplicaDescriptor + // Held in read mode during read-only commands. Held in exclusive mode to // prevent read-only commands from executing. Acquired before the embedded // RWMutex. @@ -618,7 +622,7 @@ func (r *Replica) IsInitialized() bool { // to an incoming message but we are waiting for our initial snapshot. // isInitializedLocked requires that the replica lock is held. func (r *Replica) isInitializedLocked() bool { - return len(r.mu.desc.EndKey) > 0 + return r.mu.desc.IsInitialized() } // Desc returns the range's descriptor. diff --git a/storage/store.go b/storage/store.go index 7259763dbd63..19e67acd16b3 100644 --- a/storage/store.go +++ b/storage/store.go @@ -1636,6 +1636,19 @@ func (s *Store) Send(ctx context.Context, ba roachpb.BatchRequest) (br *roachpb. pErr = roachpb.NewError(err) return nil, pErr } + if !rng.IsInitialized() { + // If we have an uninitialized copy of the range, then we are + // probably a valid member of the range, we're just in the + // process of getting our snapshot. If we returned + // RangeNotFoundError, the client would invalidate its cache, + // but we can be smarter: the replica that caused our + // uninitialized replica to be created is most likely the + // leader. + return nil, roachpb.NewError(&roachpb.NotLeaderError{ + RangeID: ba.RangeID, + Leader: rng.creatingReplica, + }) + } rng.assert5725(ba) br, pErr = rng.Send(ctx, ba) if pErr == nil { @@ -1803,7 +1816,8 @@ func (s *Store) handleRaftMessage(req *RaftMessageRequest) error { s.cacheReplicaDescriptorLocked(req.GroupID, req.FromReplica) s.cacheReplicaDescriptorLocked(req.GroupID, req.ToReplica) // Lazily create the group. - r, err := s.getOrCreateReplicaLocked(req.GroupID, req.ToReplica.ReplicaID) + r, err := s.getOrCreateReplicaLocked(req.GroupID, req.ToReplica.ReplicaID, + req.FromReplica) // TODO(bdarnell): is it safe to release the store lock here? // It deadlocks to hold s.Mutex while calling raftGroup.Step. s.mu.Unlock() @@ -1925,7 +1939,7 @@ func (s *Store) processRaft() { // getOrCreateReplicaLocked returns a replica for the given RangeID, // creating an uninitialized replica if necessary. The caller must // hold the store's lock. -func (s *Store) getOrCreateReplicaLocked(groupID roachpb.RangeID, replicaID roachpb.ReplicaID) (*Replica, error) { +func (s *Store) getOrCreateReplicaLocked(groupID roachpb.RangeID, replicaID roachpb.ReplicaID, creatingReplica roachpb.ReplicaDescriptor) (*Replica, error) { r, ok := s.mu.replicas[groupID] if ok { if err := r.setReplicaID(replicaID); err != nil { @@ -1955,6 +1969,7 @@ func (s *Store) getOrCreateReplicaLocked(groupID roachpb.RangeID, replicaID roac if err != nil { return nil, err } + r.creatingReplica = &creatingReplica // Add the range to range map, but not rangesByKey since // the range's start key is unknown. The range will be // added to rangesByKey later when a snapshot is applied.