raft: error on ignored proposal and wait for EntryConfChange results

[lavacat]

fixes: #43

[lavacat]

there might be some other side effects of returning ErrProposalDropped. If there is interest in merging this PR, I can do some more digging/testing.

[fuweid]

Switching to normal entry is trying to increase applied index so that the next confChange can be applicable. It seems that it is right move to return dropped if we have a way to update pendingConfIndex.

[chaochn47]

Switching to normal entry is trying to increase applied index so that the next confChange can be applicable.

I don't think switching to normal entry will make a difference here to increase applied index. The advance of applied index is triggered by etcd. But I agree returning proposal dropped early is helpful.

[fuweid]

You are right. The raft will update applied index even if the apply fails. I was thinking that the normal is used to catch up with pendingConfIndex so that it won't impact the following confChange.

[chaochn47]

aha. Thanks for explanation! I think next ProposeConfChange() will succeed once the raft.Applied is advanced by etcd raftNode r.Advance()

[lavacat]

For reference, the use of normal entry was added here
I think the idea was to support mixed list of entries (ConfChanges and Normal) in proposal, but is it even possible to get mixed list right now?
When I check for creation of pb.Message{Type: pb.MsgProp, there are two distinct places:

1. regular Propose
2. confChangeToMsg - used in ProposeConfChange and raft.appliedTo

[chaochn47]

I don't know the side effect of dropping other entries in the same m.Entries for loop.

How about moving it til the end of stepLeader function, especially after bcastAppend so other entries handling are not impacted just to be safe?

[lavacat]

the loop is only processing config changes, see if cc != nil {. So, I think it's ok to drop here. Also, my understanding is that we have to drop full proposal, there is no per entry ErrProposalDropped.

[chaochn47]

I see, thanks for the explanation!

[chaochn47]

etcd only uses so-called deprecated pb.ConfChange, but I am wondering how this change will impact pb.ConfChangeV2 handling.

raft/node.go

Lines 141 to 153 in 3cbcd6e

	// ProposeConfChange proposes a configuration change. Like any proposal, the
	// configuration change may be dropped with or without an error being
	// returned. In particular, configuration changes are dropped unless the
	// leader has certainty that there is no prior unapplied configuration
	// change in its log.
	//
	// The method accepts either a pb.ConfChange (deprecated) or pb.ConfChangeV2
	// message. The latter allows arbitrary configuration changes via joint
	// consensus, notably including replacing a voter. Passing a ConfChangeV2
	// message is only allowed if all Nodes participating in the cluster run a
	// version of this library aware of the V2 API. See pb.ConfChangeV2 for
	// usage details and semantics.
	ProposeConfChange(ctx context.Context, cc pb.ConfChangeI) error

cc @ahrtr @serathius for insights~

[ahrtr]

etcd only uses so-called deprecated pb.ConfChange, but I am wondering how this change will impact pb.ConfChangeV2 handling.

raft will automatically convert it to V2, see node.go#L533.

m.Entries[i] = pb.Entry{Type: pb.EntryNormal}

The intention is just to ignore the confChange, as the log "ignoring conf change" mentions.

[chaochn47]

etcd only uses so-called deprecated pb.ConfChange, but I am wondering how this change will impact pb.ConfChangeV2 handling.

To be clear, I am not sure what's the impact of fail fast in case of pb.ConfChangeV2 handling

raft/raft.go

Lines 1222 to 1226 in 7357439

	} else if alreadyJoint && !wantsLeaveJoint {
	refused = "must transition out of joint config first"
	} else if !alreadyJoint && wantsLeaveJoint {
	refused = "not in joint state; refusing empty conf change"
	}

Is there an unknown reason that designs like this? @tbg for consulting..

[lavacat]

cc @nvanbenschoten, noticed some of your recent comments here

raft/raft.go

Lines 698 to 704 in 7357439

	// NB: this proposal can't be dropped due to size, but can be
	// dropped if a leadership transfer is in progress. We'll keep
	// checking this condition on each applied entry, so either the
	// leadership transfer will succeed and the new leader will leave
	// the joint configuration, or the leadership transfer will fail,
	// and we will propose the config change on the next advance.
	if err := r.Step(m); err != nil {

can you take a look at this PR?

[nvanbenschoten]

@lavacat thanks for the ping. I don't see any issue with the changes made to stepLeader. There's now a decent amount of coupling between the rejection logic in stepLeader and the auto-leave proposal logic in appliedTo, but it appears to be correct.