Repository: pingcap/pd
Pull request #2161 (2020-02-26 10:14:26)

checker: fix rule checker panic (#2160)
cherry-pick #2160 to release-3.1

---

Signed-off-by: disksing <i@disksing.com>

### What problem does this PR solve? <!--add the issue link with summary if it exists-->
when no more store to replace, it panics:
```
[2020/02/26 11:46:45.244 +08:00] [FATAL] [log.go:294] [panic] [recover="\"invalid memory address or nil pointer dereference\""] [stack="github.com/pingcap/log.Fatal\n\t/home/birdstorm/go/pkg/mod/github.com/pingcap/log@v0.0.0-2020011      7041106-d28c14d3b1cd/global.go:59\ngithub.com/pingcap/pd/v3/pkg/logutil.LogPanic\n\t/home/birdstorm/go/src/github.com/pingcap/pd/pkg/logutil/log.go:294\nruntime.gopanic\n\t/usr/local/go/src/runtime/panic.go:679\nruntime.panicmem\n\t      /usr/local/go/src/runtime/panic.go:199\nruntime.sigpanic\n\t/usr/local/go/src/runtime/signal_unix.go:394\ngithub.com/pingcap/kvproto/pkg/metapb.(*Store).GetId\n\t/home/birdstorm/go/pkg/mod/github.com/pingcap/kvproto@v0.0.0-202002130      74014-83e827908584/pkg/metapb/metapb.pb.go:221\ngithub.com/pingcap/pd/v3/server/core.DistinctScore\n\t/home/birdstorm/go/src/github.com/pingcap/pd/server/core/store.go:453\ngithub.com/pingcap/pd/v3/server/schedule/checker.(*RuleChec      ker).fixBetterLocation\n\t/home/birdstorm/go/src/github.com/pingcap/pd/server/schedule/checker/rule_checker.go:187\ngithub.com/pingcap/pd/v3/server/schedule/checker.(*RuleChecker).fixRulePeer\n\t/home/birdstorm/go/src/github.com/pin      gcap/pd/server/schedule/checker/rule_checker.go:110\ngithub.com/pingcap/pd/v3/server/schedule/checker.(*RuleChecker).Check\n\t/home/birdstorm/go/src/github.com/pingcap/pd/server/schedule/checker/rule_checker.go:59\ngithub.com/pingca      p/pd/v3/server/schedule.(*CheckerController).CheckRegion\n\t/home/birdstorm/go/src/github.com/pingcap/pd/server/schedule/checker_controller.go:58\ngithub.com/pingcap/pd/v3/server/cluster.(*coordinator).patrolRegions\n\t/home/birdsto      rm/go/src/github.com/pingcap/pd/server/cluster/coordinator.go:125"]
```

### What is changed and how it works?
- check nil
- add unit test

### Check List <!--REMOVE the items that are not applicable-->

Tests <!-- At least one of them must be included. -->
 - Unit test

Related changes
 - Need to cherry-pick to the release branch (release-3.1)


codecov-io: # [Codecov](https://codecov.io/gh/pingcap/pd/pull/2161?src=pr&el=h1) Report
> Merging [#2161](https://codecov.io/gh/pingcap/pd/pull/2161?src=pr&el=desc) into [release-3.1](https://codecov.io/gh/pingcap/pd/commit/824ae7fd417041b7c2f299fb7933749e05118c06?src=pr&el=desc) will **decrease** coverage by `0.07%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/pingcap/pd/pull/2161/graphs/tree.svg?width=650&token=J1yRL471OI&height=150&src=pr)](https://codecov.io/gh/pingcap/pd/pull/2161?src=pr&el=tree)

```diff
@@               Coverage Diff               @@
##           release-3.1    #2161      +/-   ##
===============================================
- Coverage        74.94%   74.87%   -0.08%     
===============================================
  Files              194      194              
  Lines            19982    19985       +3     
===============================================
- Hits             14975    14963      -12     
- Misses            3875     3891      +16     
+ Partials          1132     1131       -1
```


| [Impacted Files](https://codecov.io/gh/pingcap/pd/pull/2161?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [server/schedule/checker/rule\_checker.go](https://codecov.io/gh/pingcap/pd/pull/2161/diff?src=pr&el=tree#diff-c2VydmVyL3NjaGVkdWxlL2NoZWNrZXIvcnVsZV9jaGVja2VyLmdv) | `76.07% <100%> (+0.44%)` | :arrow_up: |
| [server/region\_syncer/client.go](https://codecov.io/gh/pingcap/pd/pull/2161/diff?src=pr&el=tree#diff-c2VydmVyL3JlZ2lvbl9zeW5jZXIvY2xpZW50Lmdv) | `81.03% <0%> (-3.45%)` | :arrow_down: |
| [server/kv/etcd\_kv.go](https://codecov.io/gh/pingcap/pd/pull/2161/diff?src=pr&el=tree#diff-c2VydmVyL2t2L2V0Y2Rfa3YuZ28=) | `72.72% <0%> (-2.6%)` | :arrow_down: |
| [server/core/storage.go](https://codecov.io/gh/pingcap/pd/pull/2161/diff?src=pr&el=tree#diff-c2VydmVyL2NvcmUvc3RvcmFnZS5nbw==) | `71.85% <0%> (-1.01%)` | :arrow_down: |
| [server/config/option.go](https://codecov.io/gh/pingcap/pd/pull/2161/diff?src=pr&el=tree#diff-c2VydmVyL2NvbmZpZy9vcHRpb24uZ28=) | `87.44% <0%> (-0.87%)` | :arrow_down: |
| [server/grpc\_service.go](https://codecov.io/gh/pingcap/pd/pull/2161/diff?src=pr&el=tree#diff-c2VydmVyL2dycGNfc2VydmljZS5nbw==) | `56.65% <0%> (-0.86%)` | :arrow_down: |
| [pkg/btree/btree.go](https://codecov.io/gh/pingcap/pd/pull/2161/diff?src=pr&el=tree#diff-cGtnL2J0cmVlL2J0cmVlLmdv) | `86.84% <0%> (-0.81%)` | :arrow_down: |
| [server/server.go](https://codecov.io/gh/pingcap/pd/pull/2161/diff?src=pr&el=tree#diff-c2VydmVyL3NlcnZlci5nbw==) | `64.32% <0%> (-0.72%)` | :arrow_down: |
| [server/handler.go](https://codecov.io/gh/pingcap/pd/pull/2161/diff?src=pr&el=tree#diff-c2VydmVyL2hhbmRsZXIuZ28=) | `51.61% <0%> (+0.43%)` | :arrow_up: |
| [server/member/leader.go](https://codecov.io/gh/pingcap/pd/pull/2161/diff?src=pr&el=tree#diff-c2VydmVyL21lbWJlci9sZWFkZXIuZ28=) | `76.53% <0%> (+1.02%)` | :arrow_up: |
| ... and [2 more](https://codecov.io/gh/pingcap/pd/pull/2161/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pingcap/pd/pull/2161?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pingcap/pd/pull/2161?src=pr&el=footer). Last update [824ae7f...75db5d4](https://codecov.io/gh/pingcap/pd/pull/2161?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

nolouch: /merge
sre-bot: /run-all-tests

Patch:
diff --git a/server/schedule/checker/rule_checker.go b/server/schedule/checker/rule_checker.go
index d8590fb1b..f7caff727 100644
--- a/server/schedule/checker/rule_checker.go
+++ b/server/schedule/checker/rule_checker.go
@@ -182,6 +182,10 @@ func (c *RuleChecker) fixBetterLocation(region *core.RegionInfo, fit *placement.
 	}
 	oldPeer := region.GetStorePeer(oldPeerStore.GetID())
 	newPeerStore := SelectStoreToReplacePeerByRule("rule-checker", c.cluster, region, fit, rf, oldPeer)
+	if newPeerStore == nil {
+		log.Debug("no replacement store", zap.Uint64("region-id", region.GetID()))
+		return nil, nil
+	}
 	stores = getRuleFitStores(c.cluster, removePeerFromRuleFit(rf, oldPeer))
 	oldScore := core.DistinctScore(rf.Rule.LocationLabels, stores, oldPeerStore)
 	newScore := core.DistinctScore(rf.Rule.LocationLabels, stores, newPeerStore)


Test patch:
diff --git a/server/schedule/checker/rule_checker_test.go b/server/schedule/checker/rule_checker_test.go
index 48c1fc243..d463dc88c 100644
--- a/server/schedule/checker/rule_checker_test.go
+++ b/server/schedule/checker/rule_checker_test.go
@@ -15,6 +15,7 @@ package checker
 
 import (
 	"encoding/hex"
+
 	. "github.com/pingcap/check"
 	"github.com/pingcap/kvproto/pkg/metapb"
 	"github.com/pingcap/kvproto/pkg/pdpb"
@@ -179,3 +180,21 @@ func (s *testRuleCheckerSuite) TestBetterReplacement(c *C) {
 	op = s.rc.Check(s.cluster.GetRegion(1))
 	c.Assert(op, IsNil)
 }
+
+func (s *testRuleCheckerSuite) TestNoBetterReplacement(c *C) {
+	s.cluster.AddLabelsStore(1, 1, map[string]string{"host": "host1"})
+	s.cluster.AddLabelsStore(2, 1, map[string]string{"host": "host1"})
+	s.cluster.AddLabelsStore(3, 1, map[string]string{"host": "host2"})
+	s.cluster.AddLeaderRegionWithRange(1, "", "", 1, 2, 3)
+	s.ruleManager.SetRule(&placement.Rule{
+		GroupID:        "pd",
+		ID:             "test",
+		Index:          100,
+		Override:       true,
+		Role:           placement.Voter,
+		Count:          3,
+		LocationLabels: []string{"host"},
+	})
+	op := s.rc.Check(s.cluster.GetRegion(1))
+	c.Assert(op, IsNil)
+}