storage: fix nil pointer panic during replica changes #40961
Conversation
|
This change is |
There was a problem hiding this comment.
Thanks for the fix. I wish this stuff were easier to test.
Reviewed 3 of 3 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @nvanbenschoten)
pkg/storage/client_replica_test.go, line 2311 at r1 (raw file):
// We're going to have one goroutine trying to ADD the 4th node. // and another goroutine trying to move out of a joint config on both // sides and then
Sentence needs an end.
pkg/storage/client_replica_test.go, line 2320 at r1 (raw file):
require.NoError(t, err) // Manually construct the batch because the (*DB).AdminChangeReplicas does
It does, but you need this magical hack
ajwerner
force-pushed
the
ajwerner/fix-nil-pointer-in-change-replicas-when-racing-with-merge
branch
from
c060a7b
to
f50f5d8
Compare
There was a problem hiding this comment.
I realized that there's a bad case if we read a range descriptor for a different range. I added testing for that too.
Reviewable status:
pkg/storage/client_replica_test.go, line 2311 at r1 (raw file):
Previously, tbg (Tobias Grieger) wrote…
Sentence needs an end.
Done.
pkg/storage/client_replica_test.go, line 2320 at r1 (raw file):
Previously, tbg (Tobias Grieger) wrote…
It does, but you need this magical hack
Done.
ajwerner
force-pushed
the
ajwerner/fix-nil-pointer-in-change-replicas-when-racing-with-merge
branch
from
f50f5d8
to
ffccfde
Compare
Before this PR there was a bug whereby a command attempting to move a range out of a joint config would fail to find a RangeDescriptor for a range because it was racing with a merge which destroyed that range. The code used to panic. This fix is simply detecting the nil and not attempting to move out of a joint config on a range that no longer exists. Another slightly less problematic but still wrong case was if the merged range had been resplit at the same key. In this case we might attempt to modify a different range. This PR is almost exclusively testing. Fixes cockroachdb#40877. Release justification: Fixes a panic. Release note: None
ajwerner
force-pushed
the
ajwerner/fix-nil-pointer-in-change-replicas-when-racing-with-merge
branch
from
ffccfde
to
1f55bb1
Compare
There was a problem hiding this comment.
Reviewed 2 of 2 files at r2.
Reviewable status:
pkg/storage/replica_command.go, line 1547 at r2 (raw file):
// NB: We might fail to find the range if the range has been merged away // in which case we definitely want to fail the check below. if kvDesc != nil && kvDesc.RangeID == referenceDesc.RangeID && chgs.leaveJoint() {
Good catch.
|
TFTR! bors r+ |
40961: storage: fix nil pointer panic during replica changes r=ajwerner a=ajwerner Before this PR there was a bug whereby a command attempting to move a range out of a joint config would fail to find a RangeDescriptor for a range because it was racing with a merge which destroyed that range. The code used to panic. This fix is simply detecting the nil and not attempting to move out of a joint config on a range that no longer exists. This PR is almost exclusively testing. Fixes #40877. Release justification: Fixes a panic. Release note: None Co-authored-by: Andrew Werner <ajwerner@cockroachlabs.com>
Build succeeded |
Before this PR there was a bug whereby a command attempting to move a range out
of a joint config would fail to find a RangeDescriptor for a range because it
was racing with a merge which destroyed that range. The code used to panic.
This fix is simply detecting the nil and not attempting to move out of a
joint config on a range that no longer exists.
This PR is almost exclusively testing.
Fixes #40877.
Release justification: Fixes a panic.
Release note: None