tests: Add make-mirror test implementation

[ronaldngounou]

Context

This PR contributes to #20612 . The goal is to migrate tests/e2e/ctl_v3_make_mirror_test.go to the common testing framework

co-author: @yagikota

While we were able to migrate the e2e tests, we would like to have a discussion with the maintenairs on how we can migrate the integration tests as providing guidance would require more context about the source code.
cc @ahrtr @serathius

Testing

go test -v -count=1 -tags=e2e . -run TestMakeMirror

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.11%. Comparing base (1b2ed5a) to head (bc6f293).
⚠️ Report is 70 commits behind head on main.

Additional details and impacted files

see 30 files with indirect coverage changes

@@            Coverage Diff             @@
##             main   #20908      +/-   ##
==========================================
- Coverage   69.21%   69.11%   -0.11%     
==========================================
  Files         422      422              
  Lines       34831    34841      +10     
==========================================
- Hits        24108    24080      -28     
- Misses       9327     9353      +26     
- Partials     1396     1408      +12     

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1b2ed5a...bc6f293. Read the comment docs.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[serathius]

Please fix

common/make_mirror_test.go:130:2: undefined: configureMirrorDestTLS

[ronaldngounou]

Fixed, thanks.

[serathius]

Can you fix pull-etcd-verify? Just run make fix.

../tests/framework/e2e/config.go:44:1: File is not properly formatted (gci)
	Version ClusterVersion
^
../tests/common/make_mirror_test.go:134:10: testMirror$1 - opts is unused (unparam)
	go func(opts config.MakeMirrorOptions) {
	        ^
../tests/common/make_mirror_test.go:32:96: unnecessary leading newline (whitespace)
			t.Run(fmt.Sprintf("Source=%s/Destination=%s", srcTC.name, destTC.name), func(t *testing.T) {
			                                                                                            ^
../tests/common/make_mirror_test.go:51:46: unnecessary leading newline (whitespace)
		for _, destTC := range clusterTestCases() {
		                                           ^
../tests/common/make_mirror_test.go:82:4: unnecessary trailing newline (whitespace)
			})
			^
../tests/common/make_mirror_test.go:102:4: unnecessary trailing newline (whitespace)
			})
			^
../tests/framework/e2e/etcdctl.go:749:115: unnecessary leading newline (whitespace)
func (ctl *EtcdctlV3) MakeMirror(ctx context.Context, destEndpoints string, opts config.MakeMirrorOptions) error {
                                                                                                                  ^
../tests/framework/e2e/etcdctl.go:794:1: unnecessary trailing newline (whitespace)

[serathius]

/retest

[ronaldngounou]

/retest

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ronaldngounou, serathius

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [serathius]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[serathius]

/cc @fuweid @ahrtr

[fuweid]

This test case only works for e2e. I think we should move that into e2e folder instead of common.

[yagikota]

@fuweid @ronaldngounou

This test case only works for e2e. I think we should move that into e2e folder instead of common.

The motivation of this PR is to migrate the following tests to common

• e2e: https://github.com/etcd-io/etcd/blob/main/tests/e2e/ctl_v3_make_mirror_test.go
• integration
  • https://github.com/etcd-io/etcd/blob/main/tests/integration/clientv3/mirror_auth_test.go
  • https://github.com/etcd-io/etcd/blob/main/tests/integration/clientv3/mirror_test.go

As you say, the current tests only work for e2e since MakeMirror implementation for integration test is just a mock.

etcd/tests/framework/integration/integration.go

Lines 480 to 482 in 952c177

	func (c integrationClient) MakeMirror(ctx context.Context, destEndpoints string, opts config.MakeMirrorOptions) error {
	return errors.New("errors")
	}

To make the tests work for integration, we need to complete the MakeMirror function.
The logic to be implemented is here:

etcd/etcdctl/ctlv3/command/make_mirror_command.go

Lines 142 to 240 in 952c177

	func makeMirror(ctx context.Context, c *clientv3.Client, dc *clientv3.Client) error {
	total := int64(0)
	// if destination prefix is specified and remove destination prefix is true return error
	if mmnodestprefix && len(mmdestprefix) > 0 {
	cobrautl.ExitWithError(cobrautl.ExitBadArgs, errors.New("`--dest-prefix` and `--no-dest-prefix` cannot be set at the same time, choose one"))
	}
	go func() {
	for {
	time.Sleep(30 * time.Second)
	fmt.Println(atomic.LoadInt64(&total))
	}
	}()
	startRev := mmrev - 1
	if startRev < 0 {
	startRev = 0
	}
	s := mirror.NewSyncer(c, mmprefix, startRev)
	// If a rev is provided, then do not sync the whole key space.
	// Instead, just start watching the key space starting from the rev
	if startRev == 0 {
	rc, errc := s.SyncBase(ctx)
	// if remove destination prefix is false and destination prefix is empty set the value of destination prefix same as prefix
	if !mmnodestprefix && len(mmdestprefix) == 0 {
	mmdestprefix = mmprefix
	}
	for r := range rc {
	for _, kv := range r.Kvs {
	_, err := dc.Put(ctx, modifyPrefix(string(kv.Key)), string(kv.Value))
	if err != nil {
	return err
	}
	atomic.AddInt64(&total, 1)
	}
	}
	err := <-errc
	if err != nil {
	return err
	}
	}
	wc := s.SyncUpdates(ctx)
	for wr := range wc {
	if wr.CompactRevision != 0 {
	return rpctypes.ErrCompacted
	}
	var lastRev int64
	var ops []clientv3.Op
	for _, ev := range wr.Events {
	nextRev := ev.Kv.ModRevision
	if lastRev != 0 && nextRev > lastRev {
	_, err := dc.Txn(ctx).Then(ops...).Commit()
	if err != nil {
	return err
	}
	ops = []clientv3.Op{}
	}
	lastRev = nextRev
	if len(ops) == int(mmmaxTxnOps) {
	_, err := dc.Txn(ctx).Then(ops...).Commit()
	if err != nil {
	return err
	}
	ops = []clientv3.Op{}
	}
	switch ev.Type {
	case mvccpb.PUT:
	ops = append(ops, clientv3.OpPut(modifyPrefix(string(ev.Kv.Key)), string(ev.Kv.Value)))
	atomic.AddInt64(&total, 1)
	case mvccpb.DELETE:
	ops = append(ops, clientv3.OpDelete(modifyPrefix(string(ev.Kv.Key))))
	atomic.AddInt64(&total, 1)
	default:
	panic("unexpected event type")
	}
	}
	if len(ops) != 0 {
	_, err := dc.Txn(ctx).Then(ops...).Commit()
	if err != nil {
	return err
	}
	}
	}
	return nil
	}

But I'm not sure whether this approach is good or not...

[fuweid]

Hi @yagikota yeah. It's ok to move that into common. But that change on option doesn't make senses to me, like WithTCPClient. I think we revert that change. Other than that, It looks good.

[yagikota]

Hi @yagikota yeah. It's ok to move that into common. But that change on option doesn't make senses to me, like WithTCPClient. I think we revert that change. Other than that, It looks good.

Thanks for sharing your thoughts. Let's wait for updates from @ronaldngounou.

[ronaldngounou]

/test pull-etcd-e2e-386

[k8s-ci-robot]

@ronaldngounou: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-etcd-e2e-arm64	bc6f293	link	true	/test pull-etcd-e2e-arm64
pull-etcd-coverage-report	bc6f293	link	true	/test pull-etcd-coverage-report
pull-etcd-e2e-amd64	bc6f293	link	true	/test pull-etcd-e2e-amd64
pull-etcd-e2e-386	bc6f293	link	true	/test pull-etcd-e2e-386

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

[fuweid]

do we still need this one? I don't see any invoker for this one.

[yagikota]

Not necessary since it's redundant with

etcd/tests/framework/e2e/config.go

Line 77 in 852e4d8

func ensureE2EClusterContext(c *config.ClusterConfig) *ClusterContext {

[yagikota]

cc @ronaldngounou