raft node notifies configure when confChanged

[chaochn47]

Fix #15528

The PR is built on top of #15703

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

PTAL @ahrtr @serathius, thanks!

[ahrtr]

Can #15528 be easily reproduced? If not, then we need to add an integration test; we can intentionally inject a sleep failpoint before r.Advanced()

[chaochn47]

Can #15528 be easily reproduced? If not, then we need to add an integration test; we can intentionally inject a sleep failpoint before r.Advanced()

Not easily. Inject a sleep should do the trick.

[fuweid]

It's hard to reproduce in few requests. It needs to send request one by one without pause.
FYI: You can use the following test code to reproduce it.

package verify

import (
        "context"
        "strings"
        "testing"
        "time"

        integration2 "go.etcd.io/etcd/tests/v3/framework/integration"
        "go.etcd.io/etcd/tests/v3/integration"
)

var lazyCluster = integration.NewLazyClusterWithConfig(
        integration2.ClusterConfig{
                Size:                        3,
                WatchProgressNotifyInterval: 200 * time.Millisecond,
                DisableStrictReconfigCheck:  true})

func TestVerify(t *testing.T) {
        defer lazyCluster.Terminate()

        clus := lazyCluster.Cluster()
        leaderIdx := clus.WaitLeader(t)
        members := clus.Members

        followerIdx := (leaderIdx + 1) % 3
        cli := clus.Client(leaderIdx)

        // promoting any of the voting members in cluster should fail
        expectedErrKeywords := "can only promote a learner member"
        var err error
        for i := 0; i < 10000; i++ {
                ctx, cancel := context.WithTimeout(context.TODO(), 1*time.Minute)
                _, err = cli.MemberPromote(ctx, uint64(members[followerIdx].ID()))
                cancel()
                if err == nil {
                        t.Fatalf("expect promoting voting member to fail, got no error")
                }

                if strings.Contains(err.Error(), expectedErrKeywords) {
                        continue
                }

                t.Fatalf("unexpected error: %v", err)
        }
}

[serathius]

This PR not only changes MemberPromote, but introduces a big change for all membership changes. We should double check that all configuration changes are properly tested. If not we should add a large set of e2e tests that:

• Grow cluster from 1 to 3 members (without learners)
• Grow cluster from 1 to 3 members (with learners) in both 1 at the time and two at one time scenario
• Reduce cluster from 3 to 1 member
  etc

I would not want to rush this change, so I would recommend #15528 independently.

[chaochn47]

Can #15528 be easily reproduced? If not, then we need to add an integration test; we can intentionally inject a sleep failpoint before r.Advanced()

Hi @ahrtr could you please help clarify how would integration test inject a sleep failpoint? I know with etcd binary built with gofail enabled it is achievable.

etcd/tests/framework/e2e/etcd_process.go

Lines 298 to 318 in b754994

	func (f *BinaryFailpoints) Setup(ctx context.Context, failpoint, payload string) error {
	host := fmt.Sprintf("127.0.0.1:%d", f.member.Config().GoFailPort)
	failpointUrl := url.URL{
	Scheme: "http",
	Host: host,
	Path: failpoint,
	}
	r, err := http.NewRequestWithContext(ctx, "PUT", failpointUrl.String(), bytes.NewBuffer([]byte(payload)))
	if err != nil {
	return err
	}
	resp, err := httpClient.Do(r)
	if err != nil {
	return err
	}
	defer resp.Body.Close()
	if resp.StatusCode != http.StatusNoContent {
	return fmt.Errorf("bad status code: %d", resp.StatusCode)
	}
	return nil
	}

Thanks to @serathius as part of the robustness test work.

[ahrtr]

Hi @ahrtr could you please help clarify how would integration test inject a sleep failpoint? I know with etcd binary built with gofail enabled it is achievable.

Please refer to the followings,

1. the section "Test" in https://docs.google.com/document/d/1U9hAcZQp3Y36q_JFiw2VBJXVAo2dK2a-8Rsbqv3GgDo/edit#heading=h.wwktogcu1ekk.
2. https://github.com/etcd-io/bbolt/blob/97d2cc34ac943a03ca596d15e0629b163468f59f/Makefile#L77

[chaochn47]

This PR not only changes MemberPromote, but introduces a big change for all membership changes. We should double check that all configuration changes are properly tested. If not we should add a large set of e2e tests that:

* Grow cluster from 1  to 3 members (without learners)

* Grow cluster from 1 to 3 members (with learners) in both 1 at the time and two at one time scenario

* Reduce cluster from 3 to 1 member
  etc

I would not want to rush this change, so I would recommend #15528 independently.

Hi @serathius I audited all the membership reconfiguration tests.

1. e2e/ctl_v3_member_test.go
2. tests/common/member_test.go
3. tests/integration/clientv3/cluster_test.go
4. tests/integration/clientv3/examples/example_cluster_test.go

Number 1 & 2 tests does not expose issue mentioned in #15528 because it applies configuration change only once. common tests cover all the e2e test cases so there are some duplicates.

Test case TestMemberPromoteMemberNotLearner in Number 3 test covers the scenario that back to back member promote will timeout while it does not cover MemberAdd, MemberUpdate, MemberRemove.

Number 4 example test cases share a lazy cluster that could have back to back membership reconfigurations. So that's why #12983 and #14040 is flaky.

So My proposal is to enhance tests/integration/clientv3/cluster_test.go with failpoint injected with sleep 100ms on

etcd/server/etcdserver/raft.go

Line 310 in d81d3c3

r.Advance()

and exercise MemberAdd, MemberUpdate, MemberRemove, MemberPromote repeatedly, what do you think? @ahrtr @serathius

[chaochn47]

Ping @serathius @ahrtr @fuweid @tjungblu @jmhbnz for review ~

[jmhbnz]

A comment to respond to review ping. I've read through this pr a few times and at a code level it looks good, however I'm abstaining from hitting approve as I don't understand overall system implications well enough.

[tjungblu]

/lgtm (non-binding)

Thanks for finally fixing this!

[fuweid]

LGTM

Thanks!