From 0544f3324876e965cc9fb7d0f983882cd4881108 Mon Sep 17 00:00:00 2001 From: Tobias Schottdorf Date: Tue, 25 Feb 2020 12:45:45 +0100 Subject: [PATCH] raft: clarify ApplyConfChange contract for rejected conf changes Apps typically maintain the raft configuration as part of the state machine. As a result, they want to be able to reject configuration change entries at apply time based on the state on which the entry is supposed to be applied. When this happens, the app should not call ApplyConfChange, but the comments did not make this clear. As a result, it was tempting to pass an empty pb.ConfChange or it's V2 version instead of not calling ApplyConfChange. However, an empty V1 or V2 proto aren't noops when the configuration is joint: an empty V1 change is treated internally as a single configuration change for NodeID zero and will cause a panic when applied in a joint state. An empty V2 proto is treated as a signal to leave a joint state, which means that the app's config and raft's would diverge. The comments updated in this commit now ask users to not call ApplyConfState when they reject a conf change. Apps that never use joint consensus can keep their old behavior since the distinction only matters when in a joint state, but we don't want to encourage that. --- raft/node.go | 4 +++- raft/raft.go | 4 +++- raft/rafttest/interaction_env_handler_deliver_msgs.go | 2 +- raft/rawnode.go | 4 +++- raft/testdata/confchange_v2_add_double_auto.txt | 3 ++- 5 files changed, 12 insertions(+), 5 deletions(-) diff --git a/raft/node.go b/raft/node.go index ab6185b99..28579c9ca 100644 --- a/raft/node.go +++ b/raft/node.go @@ -168,7 +168,9 @@ type Node interface { Advance() // ApplyConfChange applies a config change (previously passed to // ProposeConfChange) to the node. This must be called whenever a config - // change is observed in Ready.CommittedEntries. + // change is observed in Ready.CommittedEntries, except when the app decides + // to reject the configuration change (i.e. treats it as a noop instead), in + // which case it must not be called. // // Returns an opaque non-nil ConfState protobuf which must be recorded in // snapshots. diff --git a/raft/raft.go b/raft/raft.go index 90eedfe44..d63ee7f03 100644 --- a/raft/raft.go +++ b/raft/raft.go @@ -1606,7 +1606,9 @@ func (r *raft) abortLeaderTransfer() { // If the new entries would exceed the limit, the method returns false. If not, // the increase in uncommitted entry size is recorded and the method returns // true. -// Configuration changes are never refused. +// +// Empty payloads are never refused. This is used both for appending an empty +// entry at a new leader's term, as well as leaving a joint configuration. func (r *raft) increaseUncommittedSize(ents []pb.Entry) bool { var s uint64 for _, e := range ents { diff --git a/raft/rafttest/interaction_env_handler_deliver_msgs.go b/raft/rafttest/interaction_env_handler_deliver_msgs.go index 2f59df9bf..82a5af36d 100644 --- a/raft/rafttest/interaction_env_handler_deliver_msgs.go +++ b/raft/rafttest/interaction_env_handler_deliver_msgs.go @@ -86,7 +86,7 @@ func (env *InteractionEnv) DeliverMsgs(rs ...Recipient) int { } toIdx := int(msg.To - 1) if err := env.Nodes[toIdx].Step(msg); err != nil { - env.Output.WriteString(err.Error()) + fmt.Fprintln(env.Output, err) } } } diff --git a/raft/rawnode.go b/raft/rawnode.go index 90eb69493..3ee52e0bb 100644 --- a/raft/rawnode.go +++ b/raft/rawnode.go @@ -98,7 +98,9 @@ func (rn *RawNode) ProposeConfChange(cc pb.ConfChangeI) error { return rn.raft.Step(m) } -// ApplyConfChange applies a config change to the local node. +// ApplyConfChange applies a config change to the local node. The app must call +// this when it applies a configuration change, except when it decides to reject +// the configuration change, in which case no call must take place. func (rn *RawNode) ApplyConfChange(cc pb.ConfChangeI) *pb.ConfState { cs := rn.raft.applyConfChange(cc.AsV2()) return &cs diff --git a/raft/testdata/confchange_v2_add_double_auto.txt b/raft/testdata/confchange_v2_add_double_auto.txt index 9583102ed..4a58e5f0a 100644 --- a/raft/testdata/confchange_v2_add_double_auto.txt +++ b/raft/testdata/confchange_v2_add_double_auto.txt @@ -403,5 +403,6 @@ stabilize INFO 3 switched to configuration voters=(1) > 1 receiving messages 2->1 MsgAppResp Term:1 Log:0/9 - raft: cannot step as peer not found3->1 MsgAppResp Term:1 Log:0/9 + raft: cannot step as peer not found + 3->1 MsgAppResp Term:1 Log:0/9 raft: cannot step as peer not found