Skip to content

Commit

Permalink
storage: fix "inverted range" panic
Browse files Browse the repository at this point in the history 
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 cockroachdb#6027
  • Loading branch information
@bdarnell
bdarnell committed Apr 19, 2016
1 parent 250d0af commit ea97f13
Show file tree
Hide file tree
Showing 6 changed files with 187 additions and 21 deletions.
132 changes: 132 additions & 0 deletions kv/dist_sender_server_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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
Expand Down Expand Up @@ -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)
}
}
}
36 changes: 18 additions & 18 deletions kv/dist_sender_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}),
}
Expand Down Expand Up @@ -359,20 +359,20 @@ 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
}
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
}
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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")
}
Expand Down Expand Up @@ -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.
Expand All @@ -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
}
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand All @@ -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
}
Expand Down Expand Up @@ -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,
Expand All @@ -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
}
Expand Down Expand Up @@ -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
}
Expand Down
5 changes: 5 additions & 0 deletions roachpb/errors.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
10 changes: 10 additions & 0 deletions roachpb/metadata.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
6 changes: 5 additions & 1 deletion storage/replica.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down
19 changes: 17 additions & 2 deletions storage/store.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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.
Expand Down

0 comments on commit ea97f13

Please sign in to comment.