On Mon, Mar 18, 2024 at 05:52:12PM +0100, Ilya Maximets wrote: > On 3/18/24 17:15, Felix Huettner wrote: > > On Fri, Mar 15, 2024 at 09:14:49PM +0100, Ilya Maximets wrote: > >> Each cluster member typically always transfers leadership to the same > >> other member, which is the first in their list of servers. This may > >> result in two servers in a 3-node cluster to transfer leadership to > >> each other and never to the third one. > >> > >> Randomizing the selection to make the load more evenly distributed. > > > > Hi Ilya, > > Hi, Felix. Thanks for the comments! > > > > > just out of curiosity: since basically only one of the 3 members is > > active at any point in time, is balancing the load even relevant. It > > will always only be on one of the 3 members anyway. > It is not very important, I agree. What I observed in practice is > that sometimes if, for example, compactions happen in approximately > similar time, the server we transfer the leadership to may send it > right back, while the first server is busy compacting. This is > less of a problem today as well, since we have parallel compaction, > but it may still be annoying if that happens every time. > > I'm mostly making this patch for the purpose of better testing below. > > > > >> > >> This also makes cluster failure tests cover more scenarios as servers > >> will transfer leadership to servers they didn't before. This is > >> important especially for cluster joining tests. > >> > >> Ideally, we would transfer to a random server with a highest apply > >> index, but not trying to implement this for now. > >> > >> Signed-off-by: Ilya Maximets <i.maxim...@ovn.org> > >> --- > >> ovsdb/raft.c | 6 +++++- > >> 1 file changed, 5 insertions(+), 1 deletion(-) > >> > >> diff --git a/ovsdb/raft.c b/ovsdb/raft.c > >> index f463afcb3..25f462431 100644 > >> --- a/ovsdb/raft.c > >> +++ b/ovsdb/raft.c > >> @@ -1261,8 +1261,12 @@ raft_transfer_leadership(struct raft *raft, const > >> char *reason) > >> return; > >> } > >> > >> + size_t n = hmap_count(&raft->servers) * 3; > >> struct raft_server *s; > >> - HMAP_FOR_EACH (s, hmap_node, &raft->servers) { > >> + > >> + while (n--) { > >> + s = CONTAINER_OF(hmap_random_node(&raft->servers), > >> + struct raft_server, hmap_node); > >> if (!uuid_equals(&raft->sid, &s->sid) > >> && s->phase == RAFT_PHASE_STABLE) { > >> struct raft_conn *conn = raft_find_conn_by_sid(raft, &s->sid); > > > > i think this has the risk of never selecting one server out of the list of > > cluster members. Suppose you have a 3 node cluster where one of them > > members is down. In this case there is a single member the leadership > > can be transfered to. > > This means for a single iteration of the while loop has a chance of 2/3 > > to select a member that can not be used. Over the 9 iterations this > > would do this would give a chance of (2/3)^9 to always choose an > > inappropriate member. This equals to a chance of 0.026% of never > > selecting the single appropriate target member. > > > > Could we instead rather start iterating the hmap at some random offset? > > That should give a similar result while giving the guarantee that we > > visit each member once. > > I don't think it's very important, because we're transferring leadership > without verifying if the other side is alive and we're not checking if we > actually transferred it or not. So, retries are basically just for the > server not hitting itself or servers that didn't join yet. We will transfer > to the first other server that already joined regardless of the iteration > method. > > The way to mostly fix the issue with dead servers is, as I mentioned, to > transfer only to the servers with the highest apply index, i.e. the servers > that acknowledged the most amount of changes. This will ensure that we > at least heard something from the server in the recent past. But that's > a separate feature to implement. > > Also, the leadership transfer is just an optimization to speed up elections, > so it's not necessary for correctness for this operation to be successful. > If we fail to transfer or transfer to the dead server, the rest of the > cluster will notice the absence of the leader and initiate election by > the timeout. > > Best regards, Ilya Maximets.
Hi Ilya, thanks for the clarifications. Then i guess the 0.026% chance is not too relevant. Thanks Felix _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev