Repository: etcd-io/etcd
File: server/etcdserver/api/membership/cluster.go (Go)

Code before:
			zap.Strings("recovered-remote-peer-urls", m.PeerURLs),
			zap.Bool("recovered-remote-peer-is-learner", m.IsLearner),
		)
	}
	if c.version != nil {
		c.lg.Info(
			"set cluster version from store",
			zap.String("cluster-version", version.Cluster(c.version.String())),
		)
	}
}

// ValidateConfigurationChange takes a proposed ConfChange and
// ensures that it is still valid.
func (c *RaftCluster) ValidateConfigurationChange(cc raftpb.ConfChange, shouldApplyV3 ShouldApplyV3) error {
	var membersMap map[types.ID]*Member
	var removedMap map[types.ID]bool

	if shouldApplyV3 {
		membersMap, removedMap = c.be.MustReadMembersFromBackend()
	}

	id := types.ID(cc.NodeID)
	if removedMap[id] {
		return ErrIDRemoved
	}
	switch cc.Type {
	case raftpb.ConfChangeAddNode, raftpb.ConfChangeAddLearnerNode:
		confChangeContext := new(ConfigChangeContext)
		if err := json.Unmarshal(cc.Context, confChangeContext); err != nil {
			c.lg.Panic("failed to unmarshal confChangeContext", zap.Error(err))
		}

		if confChangeContext.IsPromote { // promoting a learner member to voting member
			if membersMap[id] == nil {
				return ErrIDNotFound
			}
			if !membersMap[id].IsLearner {
				return ErrMemberNotLearner
			}
		} else { // adding a new member
			if membersMap[id] != nil {
				return ErrIDExists
			}

			var members []*Member
			urls := make(map[string]bool)
			for _, m := range membersMap {
				members = append(members, m)
				for _, u := range m.PeerURLs {
					urls[u] = true

Review comment (bug):
This is the root cause that causes the unit test failure.

When you reapply the entries, `shouldApplyV3` is false, so it doesn't load memberMap and removedMap at all. So it fails to apply the second `ConfChangeRemoveNode`, so node is set to None (0).


Proposal, we don't call `ValidateConfigurationChange` at all if `shouldApplyV3` is false. We already removed v2store, we don't need to worry any inconsistence between v3store and v2store.

Code after:
			zap.Strings("recovered-remote-peer-urls", m.PeerURLs),
			zap.Bool("recovered-remote-peer-is-learner", m.IsLearner),
		)
	}
	if c.version != nil {
		c.lg.Info(
			"set cluster version from store",
			zap.String("cluster-version", version.Cluster(c.version.String())),
		)
	}
}

// ValidateConfigurationChange takes a proposed ConfChange and
// ensures that it is still valid.
func (c *RaftCluster) ValidateConfigurationChange(cc raftpb.ConfChange, shouldApplyV3 ShouldApplyV3) error {
	if !shouldApplyV3 {
		return nil
	}

	membersMap, removedMap := c.be.MustReadMembersFromBackend()

	id := types.ID(cc.NodeID)
	if removedMap[id] {
		return ErrIDRemoved
	}
	switch cc.Type {
	case raftpb.ConfChangeAddNode, raftpb.ConfChangeAddLearnerNode:
		confChangeContext := new(ConfigChangeContext)
		if err := json.Unmarshal(cc.Context, confChangeContext); err != nil {
			c.lg.Panic("failed to unmarshal confChangeContext", zap.Error(err))
		}

		if confChangeContext.IsPromote { // promoting a learner member to voting member
			if membersMap[id] == nil {
				return ErrIDNotFound
			}
			if !membersMap[id].IsLearner {
				return ErrMemberNotLearner
			}
		} else { // adding a new member
			if membersMap[id] != nil {
				return ErrIDExists
			}

			var members []*Member
			urls := make(map[string]bool)
			for _, m := range membersMap {
				members = append(members, m)
				for _, u := range m.PeerURLs {
					urls[u] = true
				}