Bug for list_peers

[linyxy]

Hi devs,

My team owns a proudct that uses braft. We recently discovered a bug in the butil::Status list_peers(std::vector<PeerId>* peers); in node.

The implementation of list_peers function returns the result _conf.conf.list_peers, while we believe it should reutrn _conf.list_peers.

// node.cpp
butil::Status NodeImpl::list_peers(std::vector<PeerId>* peers) {
    BAIDU_SCOPED_LOCK(_mutex);
    if (_state != STATE_LEADER) {
        return butil::Status(EPERM, "Not leader");
    }
    _conf.conf.list_peers(peers);
    return butil::Status::OK();
}

During a reconfiguration, the current api would only return peers from new configuration instead of both old+new configuration. Since our product depends on this api for making decisions on cluster management, this bug limits our ability to safely doing reconfigurations and adds a lot of overhead to manage our cluster.

Looking forward to hearing back soon.
Thank you.

[linyxy]

There are 3 phases of the raft group during a consensus as described in the original paper as: C_old, C_old_new, and C_new. In each phase, the ‘ListPeers’ api, which gives us the current group configuration, should give us the 3 configurations respectively. However, in braft’s code, it returns C_new for both 2nd phase and 3rd phase.

This behavior for ListPeers api is wrong because it would return a configuration that has not been reached yet. It aggressively provides information about the raft group when the reconfiguration is still in progress. An analogy to this is setting the read ready bit to true before copying over the data in a concurrent queue. So it breaks the correctness of this api.

This behavior might have minimal impact for those systems that rely on an external system to manage the raft group assignment or a config based system. This behavior is problematic for us because our product utilizes dynamic raft member allocation and does not rely on another external system to manage raft membership. We build our product to be self-healing and self-maintaining, which requires the ListPeers api always returns the correct configuration.

[linyxy]

How would this break us

Imagine we are trying to reconfigure the raft group from the original C_old [a,b,c] to the new group C_new [d,e,f]. In this reconfiguration, we change all members of the raft group, which is an extreme case for changing the majority of a configuration.

T0
a* is leader. Configuration planer wants to reassign to [d,e,f]
a* broadcasts a,b,c,d,e,f as the expanded set in broadcasts
Original group a*,b,c propose reconfiguration: C_new [d,e,f] as the new peer set.
a* waits d,e,f to catchup as follower first (braft’s implementation)
a* creates C_old_new (Old: a,b,c. New: d,e,f) and apply the C_old_new

T1
The raft group is in stage_joint and a failover happened for a
d* selected leader (allowed by paper).
list_peers in d give out C_new [d,e,f]
broadcasts becomes C_new [d,e,f]. And a,b,c will shutdown state machine when they receive this broadcasts
If the raft group is still in stage_joint, agreement cannot be reached.
The raft group stalls until we can bring back the state machine for the majority of C_old.

If the ListPeers api is correct, when broker d gets selected as leader in stage_joint, it would return C_old_new for the list peer. Then the broadcasts value from d is C_old_new, and C_old would not close their state machine. So the raft group could reach consensus and move forward.