migrate e2e & integration role_test to common #14020

chaochn47 · 2022-05-08T04:38:07Z

Partially fix #13637

tests/e2e/ctl_v3_role_test.go

chaochn47 · 2022-05-08T05:09:15Z

tests/e2e/ctl_v3_role_test.go

 func TestCtlV3RoleAddTimeout(t *testing.T) { testCtl(t, roleAddTest, withDialTimeout(0)) }

-func TestCtlV3RoleGrant(t *testing.T) { testCtl(t, roleGrantTest) }
-
 func roleAddTest(cx ctlCtx) {


Called from TestCtlV3RoleAddTimeout. So it was left un-deleted.

chaochn47 · 2022-05-08T05:10:32Z

tests/e2e/ctl_v3_role_test.go

-	cmdArgs = append(cmdArgs, args...)
-
-	return e2e.SpawnWithExpects(cmdArgs, cx.envMap, expStr...)
-}
 func ctlV3Role(cx ctlCtx, args []string, expStr string) error {


Line 52 and below are retained only in this PR. After auth_test are migrated to common, they should be cleaned up.

ahrtr · 2022-05-08T05:57:35Z

tests/framework/e2e/etcdctl.go

+	err = json.Unmarshal([]byte(line), &resp)
+	return &resp, err
+}
+


Almost all methods have similar implementation. I think it makes more sense to wrap them into 1 ~ 2 common methods so as to simplify the overall implementation.

It's a good idea, however let's try not to mix in to many changes into PR.

tests/e2e/ctl_v3_role_test.go

serathius · 2022-05-08T17:15:20Z

tests/common/role_test.go

+				if err == nil || !strings.Contains(err.Error(), rpctypes.ErrRoleAlreadyExist.Error()) {
+					t.Fatalf("want (%v) error, but got (%v)", rpctypes.ErrRoleAlreadyExist, err)
+				}
+				_, err = cc.RoleAdd("")


This looks like a separate test, like TestRoleError was merged with other test.

codecov-commenter · 2022-05-09T17:36:21Z

Codecov Report

Merging #14020 (e004c91) into main (545f04f) will decrease coverage by 0.46%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main   #14020      +/-   ##
==========================================
- Coverage   75.12%   74.65%   -0.47%     
==========================================
  Files         448      448              
  Lines       37193    37193              
==========================================
- Hits        27941    27768     -173     
- Misses       7482     7626     +144     
- Partials     1770     1799      +29

Flag	Coverage Δ
all	`74.65% <ø> (-0.47%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
server/proxy/grpcproxy/register.go	`69.76% <0.00%> (-20.94%)`	⬇️
server/etcdserver/api/rafthttp/peer_status.go	`87.87% <0.00%> (-12.13%)`	⬇️
server/auth/simple_token.go	`80.00% <0.00%> (-8.47%)`	⬇️
server/etcdserver/api/rafthttp/peer.go	`87.01% <0.00%> (-8.45%)`	⬇️
client/pkg/v3/fileutil/lock_linux.go	`72.22% <0.00%> (-8.34%)`	⬇️
etcdctl/ctlv3/command/printer_simple.go	`55.55% <0.00%> (-5.56%)`	⬇️
raft/rafttest/node.go	`95.00% <0.00%> (-5.00%)`	⬇️
server/storage/wal/file_pipeline.go	`90.69% <0.00%> (-4.66%)`	⬇️
server/etcdserver/api/v3rpc/watch.go	`83.55% <0.00%> (-4.37%)`	⬇️
client/v3/leasing/txn.go	`88.09% <0.00%> (-3.18%)`	⬇️
... and 25 more

Continue to review full report at Codecov.

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

chaochn47 · 2022-05-09T18:47:27Z

All comments are addressed. PTAL, thx! @ahrtr @serathius

serathius · 2022-05-09T19:02:36Z

client/v3/maintenance.go

+		header := ar.Header
 		for _, am := range ar.Alarms {
 			dresp, derr := m.AlarmDisarm(ctx, (*AlarmMember)(am))
 			if derr != nil {
 				return nil, toErr(ctx, derr)
 			}
 			ret.Alarms = append(ret.Alarms, dresp.Alarms...)
+			header = dresp.Header
 		}
+		ret.Header = header


Suggested change

header := ar.Header

for _, am := range ar.Alarms {

dresp, derr := m.AlarmDisarm(ctx, (*AlarmMember)(am))

if derr != nil {

return nil, toErr(ctx, derr)

}

ret.Alarms = append(ret.Alarms, dresp.Alarms...)

header = dresp.Header

}

ret.Header = header

ret.Header = ar.Header

for _, am := range ar.Alarms {

dresp, derr := m.AlarmDisarm(ctx, (*AlarmMember)(am))

if derr != nil {

return nil, toErr(ctx, derr)

}

ret.Alarms = append(ret.Alarms, dresp.Alarms...)

ret.Header = dresp.Header

}

This looks like a reasonable fix for a bug, however I would want to make sure that it's properly reviewed. Can you send it in separate PR?

serathius · 2022-05-09T19:17:19Z

tests/common/user_test.go

@@ -301,7 +302,8 @@ func TestUserDelete(t *testing.T) {
 				if err == nil {
 					t.Fatalf("deleting a non-existent user should fail")
 				}
-				assert.Contains(t, err.Error(), "user name not found")
+				expectErrMsg := "user name not found"


Looks like unrelated change.

Not related.

serathius · 2022-05-09T19:20:17Z

tests/framework/e2e/etcd_spawn.go

 	"go.etcd.io/etcd/pkg/v3/expect"
 	"go.uber.org/zap"
 )

 func SpawnCmd(args []string, envVars map[string]string) (*expect.ExpectProcess, error) {
 	return SpawnCmdWithLogger(zap.NewNop(), args, envVars)
 }
+
+// SpawnCmdWithJsonPrinter spawns an etcdctl process with provided command line arguments and environment variables


This function has strong assumptions about working only for etcdctl, this makes both name and location (etcd_spawn.go) not match expectations. Please consider making this a helper method for EtcdctlV3 struct below.

serathius · 2022-05-09T19:22:04Z

tests/framework/e2e/etcd_spawn.go

+	return cmd.ExpectFunc(func(txt string) bool {
+		// lease response does not have header substring
+		// member add response does not have revision substring
+		return strings.Contains(txt, "header") || strings.Contains(txt, "revision")


I consider using or here a workaround for tests, however I think more important here is to find out why we have such inconsistency and consider fixing it. Could you give exact examples of Response protos in https://github.com/etcd-io/etcd/blob/main/api/etcdserverpb/rpc.proto that are missing some fields?

Yeah, let me raise a issue instead of fixing it in the tests migration PR.

It's not grpc client implementation missed some fields but client v3.

Examples:

header substring is missing because of embedding.

etcd/client/v3/lease.go

Lines 71 to 75 in b7be91f

// LeaseLeasesResponse wraps the protobuf message LeaseLeasesResponse.

type LeaseLeasesResponse struct {

*pb.ResponseHeader

Leases []LeaseStatus `json:"leases"`

}

revision is not populated in

etcd/server/etcdserver/api/v3rpc/member.go

Lines 108 to 110 in b7be91f

func (cs *ClusterServer) header() *pb.ResponseHeader {

return &pb.ResponseHeader{ClusterId: uint64(cs.cluster.ID()), MemberId: uint64(cs.server.ID()), RaftTerm: cs.server.Term()}

}

I guess #2 is intended as etcd member reconfiguration does not going though MVCC but just stored in memory.

Thanks for raising this.

For lease, we need some big change on this. There is an issue discussing this. The problem mentioned here is lease response indeed has the header info, but it isn't included under the "header" section. For example,

$ etcdctl lease list -w json | jq { "cluster_id": 14841639068965180000, "member_id": 10276657743932975000, "revision": 1, "raft_term": 2, "leases": [] } $ etcdctl get k1 -w json | jq { "header": { "cluster_id": 14841639068965180000, "member_id": 10276657743932975000, "revision": 1, "raft_term": 2 } }

With regard to the missing "revision" in the memer related commands, I think we might need to fix it. Feel free to raise a PR to send out for review.

$ etcdctl member list -w json | jq { "header": { "cluster_id": 14841639068965180000, "member_id": 10276657743932975000, "raft_term": 2 }, "members": [ { "ID": 10276657743932975000, "name": "default", "peerURLs": [ "http://localhost:2380" ], "clientURLs": [ "http://localhost:2379" ] } ] }

The problem mentioned here is lease response indeed has the header info, but it isn't included under the "header" section

Exactly. I think may be a json tag added to the LeaseLeasesResponse struct should help. It's a relatively small change.

I just realized we cannot update client v3 directly as it would break the library users. So I will leave them as they are for now.

Technically speaking, it's a small change. But it has impact on user experience. So we should be cautious about this. Let's track this in the issue I mentioned above.

Feel free to fix the missing revision in member related commands for now, although it might also change the user experience, but I think the impact should be minimal.

Yeah, 100% agree. I just realized it like kube-apiserver will be impacted by this small change so I dropped the drafted PR.

serathius · 2022-05-09T19:22:59Z

tests/framework/e2e/etcdctl.go

@@ -120,8 +121,7 @@ func (ctl *EtcdctlV3) Put(key, value string, opts config.PutOptions) error {
 }

 func (ctl *EtcdctlV3) Delete(key string, o config.DeleteOptions) (*clientv3.DeleteResponse, error) {
-	args := ctl.cmdArgs()
-	args = append(args, "del", key, "-w", "json")
+	args := ctl.cmdArgs("del", key)


Thanks for refactor, however I'm worried that this might make a breaking change for othere when they migrate. Could we move it to separate PR so it can be quickly reviewed and merged?

Make sense.

serathius · 2022-05-09T19:25:37Z

tests/framework/integration.go

 	integrationCfg.QuotaBackendBytes = cfg.QuotaBackendBytes
+	integrationCfg.ClientTLS, err = tlsInfo(t, cfg.ClientTLS)


Not sure this is related.

Not related. Just refactor.

serathius · 2022-05-09T19:29:21Z

tests/framework/interface.go

@@ -39,6 +39,7 @@ type Member interface {
 	Stop()
 }

+// Client is a wrapper on top of "go.etcd.io/etcd/client/v3".Client interface


Not sure what what wrapper on top of ... interface means. I would set that this interface is meant as a subset of clientv3.Client interface implemented by common framework, however this is also not true as options are passed differently to be more expressive.

Overall this is still work in progress, so I would propose that you skip adding documentation in this PR.

chaochn47 · 2022-05-09T20:19:23Z

Thanks for the review. I will keep the current PR for the minimal changes of role_test test migration.

I agree the issues/bugs exposed by refactoring with etcdctl json printer should be addressed in a separate PR.

serathius · 2022-05-10T08:16:26Z

tests/framework/e2e/etcdctl.go

+		return "", err
+	}
+	return cmd.ExpectFunc(func(txt string) bool {
+		// header substring is missing because of embedding in LeaseLeasesResponse.


This PR doesn't touch LeaseLeasesResponse, let's skip this for now.

serathius · 2022-05-10T14:34:08Z

tests/framework/e2e/etcdctl.go

+func (ctl *EtcdctlV3) RoleDelete(role string) (*clientv3.AuthRoleDeleteResponse, error) {
+	line, err := SpawnCmdWithJsonPrinter(ctl.cmdArgs("role", "delete", role), nil)
+	if err != nil {
+		return nil, err
+	}
+	var resp clientv3.AuthRoleDeleteResponse
+	err = json.Unmarshal([]byte(line), &resp)
+	return &resp, err
+}
+
+// SpawnCmdWithJsonPrinter spawns an etcdctl process with provided command line arguments and environment variables
+// with json printer output and capture header line as output.
+func SpawnCmdWithJsonPrinter(args []string, envVars map[string]string) (string, error) {
+	args = append(args, "-w", "json")
+	cmd, err := SpawnCmd(args, envVars)
+	if err != nil {
+		return "", err
+	}
+	return cmd.ExpectFunc(func(txt string) bool {
+		// header substring is missing because of embedding in LeaseLeasesResponse.
+		// revision substring is missing because ClusterServer.header() does not fill revision in the header
+		return strings.Contains(txt, "header") || strings.Contains(txt, "revision")
+	})
+}


Suggested change

func (ctl *EtcdctlV3) RoleDelete(role string) (*clientv3.AuthRoleDeleteResponse, error) {

line, err := SpawnCmdWithJsonPrinter(ctl.cmdArgs("role", "delete", role), nil)

if err != nil {

return nil, err

}

var resp clientv3.AuthRoleDeleteResponse

err = json.Unmarshal([]byte(line), &resp)

return &resp, err

}

// SpawnCmdWithJsonPrinter spawns an etcdctl process with provided command line arguments and environment variables

// with json printer output and capture header line as output.

func SpawnCmdWithJsonPrinter(args []string, envVars map[string]string) (string, error) {

args = append(args, "-w", "json")

cmd, err := SpawnCmd(args, envVars)

if err != nil {

return "", err

}

return cmd.ExpectFunc(func(txt string) bool {

// header substring is missing because of embedding in LeaseLeasesResponse.

// revision substring is missing because ClusterServer.header() does not fill revision in the header

return strings.Contains(txt, "header") || strings.Contains(txt, "revision")

})

}

func (ctl *EtcdctlV3) RoleDelete(role string) (*clientv3.AuthRoleDeleteResponse, error) {

var resp clientv3.AuthRoleDeleteResponse

err := ctl.spawnJsonCmd(&resp, "role", "delete", role)

return &resp, err

}

func (ctl *EtcdctlV3) spawnJsonCmd(output interface{}, args ...string) error {

args = append(args, "-w", "json")

cmd, err := SpawnCmd(append(ctl.cmdArgs(), args...), nil)

if err != nil {

return err

}

line, err := cmd.Expect("header")

if err != nil {

return err

}

err = json.Unmarshal([]byte(line), output)

if err != nil {

return err

}

return nil

}

SpawnCmdWithJsonPrinter you proposed doesn't work for any cmd, it depends directly on -w json parameters available only for some of the etcdctl commands. As so it should not be a function, but a method on EtcdctlV3 struct.

I also proposed to make one additional step to include json parsing. It make sense that if we provide commands used to force json output, then we should always be able to parse json.

Thanks for the suggestions!! Much appreciate it.

chaochn47 commented May 8, 2022

View reviewed changes

ahrtr reviewed May 8, 2022

View reviewed changes

serathius reviewed May 8, 2022

View reviewed changes

chaochn47 force-pushed the role_test branch from 3a1196e to f6506ff Compare May 9, 2022 16:58

chaochn47 force-pushed the role_test branch from f6506ff to 1b4ed0b Compare May 9, 2022 17:53

serathius reviewed May 9, 2022

View reviewed changes

chaochn47 force-pushed the role_test branch from 1b4ed0b to a90c638 Compare May 9, 2022 21:52

chaochn47 mentioned this pull request May 9, 2022

Fix member response header missing revision #14026

Closed

ahrtr mentioned this pull request May 9, 2022

Make leases linearizable (excluding TTL) #13915

Open

serathius reviewed May 10, 2022

View reviewed changes

migrate e2e & integration role_test to common

e004c91

chaochn47 force-pushed the role_test branch from a90c638 to e004c91 Compare May 10, 2022 17:18

chaochn47 requested a review from serathius May 10, 2022 17:36

serathius approved these changes May 10, 2022

View reviewed changes

serathius merged commit 066e510 into etcd-io:main May 10, 2022

chaochn47 deleted the role_test branch May 10, 2022 22:02

chaochn47 mentioned this pull request May 11, 2022

tests: Refactor spawn json command #14029

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

migrate e2e & integration role_test to common #14020

migrate e2e & integration role_test to common #14020

chaochn47 commented May 8, 2022

chaochn47 May 8, 2022 •

edited

Loading

chaochn47 May 8, 2022

ahrtr May 8, 2022

serathius May 9, 2022

serathius May 8, 2022

codecov-commenter commented May 9, 2022 •

edited

Loading

chaochn47 commented May 9, 2022

serathius May 9, 2022

serathius May 9, 2022

serathius May 9, 2022

chaochn47 May 9, 2022

serathius May 9, 2022

serathius May 9, 2022

chaochn47 May 9, 2022

chaochn47 May 9, 2022 •

edited

Loading

ahrtr May 9, 2022

chaochn47 May 9, 2022 •

edited

Loading

ahrtr May 9, 2022 •

edited

Loading

chaochn47 May 9, 2022

serathius May 9, 2022

chaochn47 May 9, 2022

serathius May 9, 2022

chaochn47 May 9, 2022

serathius May 9, 2022

chaochn47 commented May 9, 2022 •

edited

Loading

serathius May 10, 2022

serathius May 10, 2022

chaochn47 May 10, 2022

	// LeaseLeasesResponse wraps the protobuf message LeaseLeasesResponse.
	type LeaseLeasesResponse struct {
	*pb.ResponseHeader
	Leases []LeaseStatus `json:"leases"`
	}

	func (cs ClusterServer) header() pb.ResponseHeader {
	return &pb.ResponseHeader{ClusterId: uint64(cs.cluster.ID()), MemberId: uint64(cs.server.ID()), RaftTerm: cs.server.Term()}
	}

		integrationCfg.QuotaBackendBytes = cfg.QuotaBackendBytes
		integrationCfg.ClientTLS, err = tlsInfo(t, cfg.ClientTLS)

migrate e2e & integration role_test to common #14020

migrate e2e & integration role_test to common #14020

Conversation

chaochn47 commented May 8, 2022

chaochn47 May 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented May 9, 2022 • edited Loading

Codecov Report

chaochn47 commented May 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chaochn47 May 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chaochn47 May 9, 2022 • edited Loading

Choose a reason for hiding this comment

ahrtr May 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chaochn47 commented May 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chaochn47 May 8, 2022 •

edited

Loading

codecov-commenter commented May 9, 2022 •

edited

Loading

chaochn47 May 9, 2022 •

edited

Loading

chaochn47 May 9, 2022 •

edited

Loading

ahrtr May 9, 2022 •

edited

Loading

chaochn47 commented May 9, 2022 •

edited

Loading