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()

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.

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)
        }
}

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.

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.

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

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

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