Read replicas of a master the cluster has declared failed

A replica cut off from its master falls further behind only while
somebody writes to the shard. Once the cluster has agreed the master is
down, nobody does until a new master is elected, so the replicas' data
is the freshest the shard can offer no matter how long ago their links
broke. Excluding them after ReplicaLinkDownTolerance turns a stalled
failover into a read outage for no gain.

The master's state comes from CLUSTER NODES fetched with every reload.
Only the agreed flag counts: a node's own suspicion (fail?) does not,
and without a readable CLUSTER NODES the tolerance alone decides. A
replica that has never synced since it started stays excluded either
way.

Author: g7r

rediscluster/cluster.go

@@ -117,8 +117,10 @@ type Opts struct {
 	// WeightProvider - enables to explicitly set weights of replicas (has higher priority than LatencyOrientedRR)
 	WeightProvider WeightProvider
 	// ReplicaLinkDownTolerance - a replica whose link with its master is down keeps receiving
-	// reads while its master_link_down_since_seconds stays below this value. A replica that
-	// has never synced since it started is never read regardless of it.
+	// reads while its master_link_down_since_seconds stays below this value. Once the cluster
+	// declares the master failed, its replicas are read for as long as it stays failed:
+	// nobody writes to the shard until a new master is elected. A replica that has never
+	// synced since it started is never read regardless of both.
 	// default: 60 seconds; negative: a replica with a broken link is never read
 	ReplicaLinkDownTolerance time.Duration
 	// Enable connection with TLS
@@ -171,10 +173,11 @@ type clusterConfig struct {
 }
 
 type shard struct {
-	rr          uint32
-	good        uint32
-	addr        []string
-	pingWeights []uint32
+	rr           uint32
+	good         uint32
+	masterFailed uint32
+	addr         []string
+	pingWeights  []uint32
 }
 type shardMap map[uint16]*shard
 type masterMap map[string]uint16
@@ -389,11 +392,11 @@ func (c *Cluster) control() {
 }
 
 func (c *Cluster) reloadMapping() error {
-	nodes, err := c.slotRangesAndInternalMasterOnly()
+	nodes, failedMasters, err := c.slotRangesAndInternalMasterOnly()
 	if err != nil {
 		return err
 	}
-	c.updateMappings(nodes)
+	c.updateMappings(nodes, failedMasters)
 	return nil
 }
 

rediscluster/cluster_test.go

@@ -518,6 +518,47 @@ func (s *Suite) TestFallbackToSlaveTimeout() {
 
 }
 
+func (s *Suite) TestReadFromReplicaWhileMasterStaysFailed() {
+	opts := longcheckopts
+	opts.RoundRobinSeed = alwaysZero{}
+	opts.ReplicaLinkDownTolerance = -1
+	cl, err := NewCluster(s.ctx, []string{"127.0.0.1:21100"}, opts)
+	s.r().Nil(err)
+	defer cl.Close()
+
+	sconn := redis.SyncCtx{cl.WithPolicy(MasterAndSlaves)}
+
+	key := slotkey("frozen", s.keys[1], "read")
+	s.r().Equal("OK", sconn.Do(s.ctx, "SET", key, "1"))
+	s.waitReplicated(1, 30*time.Second)
+
+	s.cl.Node[3].DoSure("CONFIG", "SET", "cluster-replica-no-failover", "yes")
+	defer s.cl.Node[3].DoSure("CONFIG", "SET", "cluster-replica-no-failover", "no")
+	s.cl.Node[0].Stop()
+	defer func() {
+		s.cl.Node[0].Start()
+		s.cl.WaitClusterOk()
+	}()
+
+	// The first failed read forces a reload, which marks the replica down.
+	var res interface{}
+	for deadline := time.Now().Add(5 * time.Second); time.Now().Before(deadline); time.Sleep(50 * time.Millisecond) {
+		if res = sconn.Do(s.ctx, "GET", key); redis.AsError(res) != nil {
+			break
+		}
+	}
+	s.r().True(s.AsError(res).IsOfType(ErrNoAliveConnection), "expected no_alive_connection, got %v", res)
+
+	// The testbed runs with cluster-node-timeout of one second, so the cluster agrees the
+	// master failed within a few seconds and the next reload picks that up.
+	for deadline := time.Now().Add(15 * time.Second); time.Now().Before(deadline); time.Sleep(200 * time.Millisecond) {
+		if res = sconn.Do(s.ctx, "GET", key); redis.AsError(res) == nil {
+			break
+		}
+	}
+	s.Equal([]byte("1"), res)
+}
+
 func (s *Suite) TestGetMoved() {
 	cl, err := NewCluster(s.ctx, []string{"127.0.0.1:21100"}, longcheckopts)
 	s.r().Nil(err)

rediscluster/redisclusterutil/cluster.go

@@ -133,12 +133,14 @@ func parseHostname(metadata []interface{}) string {
 
 // InstanceInfo represents line of CLUSTER NODES result.
 type InstanceInfo struct {
-	Uuid   string
-	Addr   string
-	IP     string
-	Port   int
-	Port2  int
-	Fail   bool
+	Uuid  string
+	Addr  string
+	IP    string
+	Port  int
+	Port2 int
+	Fail  bool
+	// PFail: the reporting node suspects the instance (fail?), the cluster has not agreed yet.
+	PFail  bool
 	MySelf bool
 	// NoAddr means that node were missed due to misconfiguration.
 	// More probably, redis instance with other UUID were started on the same port.
@@ -347,6 +349,7 @@ func ParseClusterNodes(res interface{}) (InstanceInfos, error) {
 		node.Port2, _ = strconv.Atoi(ipp[1])
 
 		node.Fail = strings.Contains(parts[2], "fail")
+		node.PFail = strings.Contains(parts[2], "fail?")
 		if strings.Contains(parts[2], "slave") {
 			node.SlaveOf = parts[3]
 		}

rediscluster/redisclusterutil/cluster_test.go

@@ -269,3 +269,21 @@ func TestParseSlotsInfo_NoHostname(t *testing.T) {
 
 	assert.Equal(t, expectedSlots, slots)
 }
+
+func TestParseClusterNodes_FailFlags(t *testing.T) {
+	nodes := "0000000000000000000000000000000000000001 127.0.0.1:21100@31100 master,fail - 0 1789410223036 34 connected 0-5499\n" +
+		"0000000000000000000000000000000000000002 127.0.0.1:21101@31101 master,fail? - 0 1789410223136 23 connected 5500-10999\n" +
+		"0000000000000000000000000000000000000003 127.0.0.1:21102@31102 myself,master - 0 0 24 connected 11000-16383\n"
+	infos, err := ParseClusterNodes([]byte(nodes))
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(infos) != 3 {
+		t.Fatalf("parsed %d nodes, want 3", len(infos))
+	}
+	for i, want := range []struct{ fail, pfail bool }{{true, false}, {true, true}, {false, false}} {
+		if infos[i].Fail != want.fail || infos[i].PFail != want.pfail {
+			t.Errorf("node %d: Fail=%v PFail=%v, want Fail=%v PFail=%v", i, infos[i].Fail, infos[i].PFail, want.fail, want.pfail)
+		}
+	}
+}

rediscluster/replica_health_internal_test.go

@@ -11,10 +11,11 @@ func TestReplicaHealthy(t *testing.T) {
 		return []byte("# Replication\r\nrole:slave\r\n" + strings.Join(lines, "\r\n") + "\r\n# Persistence\r\nloading:0\r\n")
 	}
 	for _, tc := range []struct {
-		name      string
-		info      []byte
-		tolerance time.Duration
-		want      bool
+		name         string
+		info         []byte
+		tolerance    time.Duration
+		masterFailed bool
+		want         bool
 	}{
 		{name: "link up", info: info("master_link_status:up"), tolerance: time.Minute, want: true},
 		{name: "link down for a while", info: info("master_link_status:down", "master_link_down_since_seconds:3"), tolerance: time.Minute, want: true},
@@ -23,10 +24,14 @@ func TestReplicaHealthy(t *testing.T) {
 		{name: "link down without duration", info: info("master_link_status:down"), tolerance: time.Minute, want: false},
 		{name: "negative tolerance", info: info("master_link_status:down", "master_link_down_since_seconds:0"), tolerance: -1, want: false},
 		{name: "loading", info: []byte("# Replication\r\nmaster_link_status:up\r\n# Persistence\r\nloading:1\r\n"), tolerance: time.Minute, want: false},
+		{name: "master failed, link down beyond tolerance", info: info("master_link_status:down", "master_link_down_since_seconds:3600"), tolerance: time.Minute, masterFailed: true, want: true},
+		{name: "master failed, negative tolerance", info: info("master_link_status:down", "master_link_down_since_seconds:5"), tolerance: -1, masterFailed: true, want: true},
+		{name: "master failed, never synced since start", info: info("master_link_status:down", "master_link_down_since_seconds:-1"), tolerance: time.Minute, masterFailed: true, want: false},
+		{name: "master failed, loading", info: []byte("# Replication\r\nmaster_link_status:down\r\nmaster_link_down_since_seconds:5\r\n# Persistence\r\nloading:1\r\n"), tolerance: time.Minute, masterFailed: true, want: false},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
-			if got := replicaHealthy(tc.info, tc.tolerance); got != tc.want {
-				t.Errorf("replicaHealthy(%q, %v) = %v, want %v", tc.info, tc.tolerance, got, tc.want)
+			if got := replicaHealthy(tc.info, tc.tolerance, tc.masterFailed); got != tc.want {
+				t.Errorf("replicaHealthy(%q, %v, %v) = %v, want %v", tc.info, tc.tolerance, tc.masterFailed, got, tc.want)
 			}
 		})
 	}

rediscluster/slotrange.go

@@ -13,17 +13,19 @@ import (
 
 const masterOnlyFlag = 0x4000
 
-func (c *Cluster) slotRangesAndInternalMasterOnly() ([]redisclusterutil.SlotsRange, error) {
+func (c *Cluster) slotRangesAndInternalMasterOnly() ([]redisclusterutil.SlotsRange, map[string]struct{}, error) {
 	nodes := c.getConfig().nodes
 
 	var ranges []redisclusterutil.SlotsRange
+	var failedMasters map[string]struct{}
 	var err error
 Outter:
 	for _, node := range nodes {
 		for _, conn := range node.conns {
 			resp := redis.Sync{conn}.Do("CLUSTER SLOTS")
 			ranges, err = redisclusterutil.ParseSlotsInfo(resp)
 			if err == nil {
+				failedMasters = failedMastersOf(redis.Sync{conn}.Do("CLUSTER NODES"))
 				break Outter
 			}
 			c.report(LogClusterSlotsError{Conn: conn, Error: err})
@@ -32,7 +34,7 @@ Outter:
 	}
 	if err != nil {
 		c.report(LogSlotRangeError{})
-		return nil, c.err(ErrClusterSlots)
+		return nil, nil, c.err(ErrClusterSlots)
 	}
 
 	// look for reminder about future migrations
@@ -43,10 +45,28 @@ Outter:
 	}
 	c.m.Unlock()
 
-	return ranges, nil
+	return ranges, failedMasters, nil
 }
 
-func (c *Cluster) updateMappings(slotRanges []redisclusterutil.SlotsRange) {
+// failedMastersOf lists the masters the cluster agrees are down. A node's own
+// suspicion (fail?) does not count. Without a readable CLUSTER NODES nothing counts,
+// and the link-down tolerance alone decides.
+func failedMastersOf(res interface{}) map[string]struct{} {
+	infos, err := redisclusterutil.ParseClusterNodes(res)
+	if err != nil {
+		return nil
+	}
+	failed := map[string]struct{}{}
+	for i := range infos {
+		ii := &infos[i]
+		if ii.IsMaster() && ii.Fail && !ii.PFail && ii.HasAddr() {
+			failed[ii.Addr] = struct{}{}
+		}
+	}
+	return failed
+}
+
+func (c *Cluster) updateMappings(slotRanges []redisclusterutil.SlotsRange, failedMasters map[string]struct{}) {
 	shards := make(map[string][]string)
 	for _, r := range slotRanges {
 		shards[r.Addrs[0]] = r.Addrs
@@ -122,19 +142,23 @@ func (c *Cluster) updateMappings(slotRanges []redisclusterutil.SlotsRange) {
 			return sh
 		}()
 
-		if oldshard != nil {
-			newConfig.shards[shardno] = oldshard
-		} else {
-			shard := &shard{
+		sh := oldshard
+		if sh == nil {
+			sh = &shard{
 				addr:        addrs,
 				good:        (uint32(1) << uint(len(addrs))) - 1,
 				pingWeights: make([]uint32, len(addrs)),
 			}
-			newConfig.shards[shardno] = shard
-			for i := range shard.pingWeights {
-				shard.pingWeights[i] = 1
+			for i := range sh.pingWeights {
+				sh.pingWeights[i] = 1
 			}
 		}
+		masterFailed := uint32(0)
+		if _, ok := failedMasters[master]; ok {
+			masterFailed = 1
+		}
+		atomic.StoreUint32(&sh.masterFailed, masterFailed)
+		newConfig.shards[shardno] = sh
 		newConfig.masters[addrs[0]] = shardno
 		random = shardno
 	}
@@ -230,7 +254,7 @@ func (s *shard) setReplicaInfo(res interface{}, n uint64, tolerance time.Duratio
 	} else if buf, ok := res.([]byte); !ok {
 		haserr = true
 	} else {
-		haserr = !replicaHealthy(buf, tolerance)
+		haserr = !replicaHealthy(buf, tolerance, atomic.LoadUint32(&s.masterFailed) != 0)
 	}
 	for {
 		oldstate := atomic.LoadUint32(&s.good)
@@ -252,7 +276,9 @@ func (s *shard) setReplicaInfo(res interface{}, n uint64, tolerance time.Duratio
 // replicaHealthy tells whether INFO output describes a replica worth reading from.
 // master_link_down_since_seconds is -1 for a replica that has never synced since it
 // started, and its dataset is then anything from empty to the RDB it booted from.
-func replicaHealthy(info []byte, tolerance time.Duration) bool {
+// A replica cut off from a master the cluster has declared failed cannot fall further
+// behind: nobody accepts writes for the shard until a new master is elected.
+func replicaHealthy(info []byte, tolerance time.Duration, masterFailed bool) bool {
 	if bytes.Contains(info, []byte("loading:1")) {
 		return false
 	}
@@ -263,7 +289,7 @@ func replicaHealthy(info []byte, tolerance time.Duration) bool {
 	if !ok || since < 0 {
 		return false
 	}
-	return time.Duration(since)*time.Second < tolerance
+	return masterFailed || time.Duration(since)*time.Second < tolerance
 }
 
 func infoInt(info []byte, field string) (int64, bool) {