etcd-tester: add retry logic on retriving lease info #6788

fanminshi · 2016-11-02T16:59:01Z

getting lease and keys info through raw rpcs rarely experience error such as EOF. This is considered as a failure and causes tester to clean up.
however, they are just transient problem with temporary connection issue which should not br considered as a testing failure. so we add retry logic in case of transient failure.

FIX #6754

fanminshi · 2016-11-02T17:02:41Z

I suspect the EOF issue has nothing to do with our internal server code but it is a issue with the http library in which we are using. I think the simplest way to get around it is through retrying on client side. since connection errors are expected and client should take care of those.

cc @xiang90 @gyuho @heyitsanthony

heyitsanthony · 2016-11-02T17:05:10Z

If it's errors from a disrupted connection, why not issue the RPC calls with FailFast=false?

fanminshi · 2016-11-02T17:23:09Z

@heyitsanthony
getting lease info is using failfast=false. we are still experiencing EOF.

func (ls *leaseStresser) getLeaseByID(ctx context.Context, leaseID int64) (*pb.LeaseTimeToLiveResponse, error) {
    ltl := &pb.LeaseTimeToLiveRequest{ID: leaseID, Keys: true}
    return ls.lc.LeaseTimeToLive(ctx, ltl, grpc.FailFast(false))
}

heyitsanthony · 2016-11-02T17:31:46Z

@fanminshi ok, that's a server-side issue. There needs to be an actual error code returned by the server that can be detected here instead of dropping the error on the floor.

fanminshi · 2016-11-02T17:38:34Z

@heyitsanthony I dig into the lease code a bit. I think error is thrown from this line.
https://github.com/coreos/etcd/blob/master/lease/leasehttp/http.go#L187
I suspect that the http req failed due to EOF, and then that error is escalated to the client side.
I wonder how would the server side handles this kind of connection related error?

fanminshi · 2016-11-02T17:43:02Z

The syndrome is similar to that of http://stackoverflow.com/questions/17714494/golang-http-request-results-in-eof-errors-when-making-multiple-requests-successi.

However, according to the post, that issue is patched in the go version 1.6. So we might experiencing something a bit different.

xiang90 · 2016-11-02T22:31:06Z

tools/functional-tester/etcd-tester/lease_stresser.go

-	plog.Debugf("hasLeaseExpired %v resp %v error (%v)", leaseID, resp, err)
-	if rpctypes.Error(err) == rpctypes.ErrLeaseNotFound {
-		return true, nil
+	for ctx.Err() == nil {


this for condition seems strange. how about checking the ctx.Err at the end of the loop instead?

also i think we only need to retry on the keepalive path, not here?

@xiang90 I wonder which keepalive path are you talking about?

sorry. my mistake. i was confused.

xiang90 · 2016-11-03T16:38:18Z

tools/functional-tester/etcd-tester/checks.go

@@ -54,14 +54,14 @@ func (hc *hashChecker) checkRevAndHashes() (err error) {
 	for i := 0; i < retries; i++ {
 		revs, hashes, err = hc.hrg.getRevisionHash()
 		if err != nil {
-			plog.Warningf("retry %i. failed to retrieve revison and hash (%v)", i, err)
+			plog.Warningf("retry %d. failed to retrieve revison and hash (%v)", i, err)


can we fix this in another pr? thanks!

yep. will do

xiang90 · 2016-11-03T16:39:11Z

tools/functional-tester/etcd-tester/lease_stresser.go

-		return true, nil
+	// keep retrying until lease's state is known or ctx is being canceled
+	for ctx.Err() == nil {
+		resp, err := ls.getLeaseByID(ctx, leaseID)


the error is not expected, right? probably we should print out warning here if there is an error other than lease not found.

yeah, there shouldn't be error the first place. a warning makes sense.

xiang90 · 2016-11-03T16:39:25Z

Let's clean this up. Then lgtm

xiang90 · 2016-11-03T17:03:13Z

tools/functional-tester/etcd-tester/lease_stresser.go

+	// keep retrying until lease's state is known or ctx is being canceled
+	for ctx.Err() == nil {
+		resp, err := ls.getLeaseByID(ctx, leaseID)
+		plog.Warningf("hasLeaseExpired %v resp %v error (%v)", leaseID, resp, err)


move this line after 270. we do not want to print it out if:

there is no error

the error is not found

getting lease and keys info through raw rpcs rarely experience error such as EOF. This is considered as a failure and causes tester to clean up. however, they are just transient problem with temporary connection issue which should not be considered as a testing failure. so we add retry logic in case of transient failure. FIX etcd-io#6754

fanminshi · 2016-11-03T17:07:15Z

All fixed. PTAL @xiang90

xiang90 · 2016-11-03T17:07:29Z

@fanminshi LGTM. Thanks a lot!

fanminshi added the area/functional-testing label Nov 2, 2016

fanminshi force-pushed the lease_http_eof_fix branch from e72e57c to e13f1b9 Compare November 2, 2016 17:00

xiang90 reviewed Nov 2, 2016

View reviewed changes

fanminshi force-pushed the lease_http_eof_fix branch 2 times, most recently from accdb3b to c0a7c00 Compare November 2, 2016 22:47

xiang90 reviewed Nov 3, 2016

View reviewed changes

fanminshi force-pushed the lease_http_eof_fix branch from c0a7c00 to 4c0c2bf Compare November 3, 2016 17:02

xiang90 reviewed Nov 3, 2016

View reviewed changes

fanminshi force-pushed the lease_http_eof_fix branch from 4c0c2bf to 649fe7f Compare November 3, 2016 17:05

fanminshi merged commit b7ab5c6 into etcd-io:master Nov 3, 2016

fanminshi deleted the lease_http_eof_fix branch November 3, 2016 17:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

etcd-tester: add retry logic on retriving lease info #6788

etcd-tester: add retry logic on retriving lease info #6788

fanminshi commented Nov 2, 2016

fanminshi commented Nov 2, 2016

heyitsanthony commented Nov 2, 2016

fanminshi commented Nov 2, 2016

heyitsanthony commented Nov 2, 2016

fanminshi commented Nov 2, 2016

fanminshi commented Nov 2, 2016

xiang90 Nov 2, 2016

xiang90 Nov 2, 2016

fanminshi Nov 2, 2016

xiang90 Nov 2, 2016

xiang90 Nov 3, 2016

fanminshi Nov 3, 2016

xiang90 Nov 3, 2016

fanminshi Nov 3, 2016

xiang90 commented Nov 3, 2016

xiang90 Nov 3, 2016

fanminshi commented Nov 3, 2016

xiang90 commented Nov 3, 2016

etcd-tester: add retry logic on retriving lease info #6788

etcd-tester: add retry logic on retriving lease info #6788

Conversation

fanminshi commented Nov 2, 2016

fanminshi commented Nov 2, 2016

heyitsanthony commented Nov 2, 2016

fanminshi commented Nov 2, 2016

heyitsanthony commented Nov 2, 2016

fanminshi commented Nov 2, 2016

fanminshi commented Nov 2, 2016

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

Choose a reason for hiding this comment

xiang90 commented Nov 3, 2016

Choose a reason for hiding this comment

fanminshi commented Nov 3, 2016

xiang90 commented Nov 3, 2016