etcdserver: translate EOF to ErrNoLeader for renew, timetolive #6791

gyuho · 2016-11-02T21:44:54Z

Address #6754.

In case there are network errors or unexpected EOF errors
in TimeToLive http requests to leader, we translate that into
ErrNoLeader, and expects the client to retry its request.

/cc @xiang90 @heyitsanthony @fanminshi

heyitsanthony · 2016-11-02T21:54:35Z

lgtm

xiang90 · 2016-11-02T22:28:54Z

lease/leasehttp/http.go

 		if err != nil {
+			// follower cannot reach leader at the moment


remove todo at line 185?

Done. Merging in greens. Thanks.

xiang90 · 2016-11-02T22:29:02Z

LGTM

fanminshi · 2016-11-02T22:00:23Z

lease/leasehttp/http.go

 		if err != nil {
+			// follower cannot reach leader at the moment


The error message we got in the tester is (rpc error: code = 2 desc = Post http://10.240.0.3:2380/leases/internal: EOF). is Post http://10.240.0.3:2380/leases/internal: EOF return by http call equals to either io.EOF or io.ErrUnexpectedEOF?

fanminshi · 2016-11-02T22:28:40Z

lease/leasehttp/http.go

+			// possibly from network errors
+			// translate to ErrNoLeader, expecting clients to retry
+			if err == io.EOF || err == io.ErrUnexpectedEOF {
+				err = rpctypes.ErrNoLeader


should we return grpc error ErrGRPCNoLeader instead of client side error rpctypes.ErrNoLeader

I think we also need to convert the internal error to grpc error for lease in the API layer.
https://github.com/coreos/etcd/blob/master/etcdserver/api/v3rpc/lease.go#L57

gyuho · 2016-11-02T23:03:08Z

@xiang90 @fanminshi PTAL.

I added another commit to match ErrNoLeader in togRPCError

So https://github.com/coreos/etcd/blob/master/etcdserver/api/v3rpc/lease.go#L57 would return rpctypes.ErrNoLeader

func (ls *LeaseServer) LeaseTimeToLive(ctx context.Context, rr *pb.LeaseTimeToLiveRequest) (*pb.LeaseTimeToLiveResponse, error) {
    resp, err := ls.le.LeaseTimeToLive(ctx, rr)
    if err != nil {
        return nil, togRPCError(err)
    }

And now togRPCError would translate that into rpctypes.ErrGRPCNoLeader.

So tester would have to implement retrial logic to error rpctypes.ErrGRPCNoLeader.

heyitsanthony · 2016-11-02T23:13:13Z

etcdserver/api/v3rpc/util.go

@@ -49,7 +49,7 @@ func togRPCError(err error) error {
 	case etcdserver.ErrTooManyRequests:
 		return rpctypes.ErrTooManyRequests

-	case etcdserver.ErrNoLeader:
+	case etcdserver.ErrNoLeader, rpctypes.ErrNoLeader: // ErrNoLeader from leasehttp


this is really goofy; maybe lessor.LeaseTimeToLive should still return io.EOF and v3api.LeaseTimeToLive will translate it to etcdserver.ErrNoLeader?

hmm... seems bad. Now API layer needs to know the details of the lease implementation. It now start to understand io related errors. I feel we should still put the error convert back to the HTTP handler itself.

/cc @heyitsanthony @gyuho

The api layer already calls out to a package called leasehttp, so it already has some idea of the lease implementation. I think it's OK for the API layer to know the http request may return an EOF instead of having the leasehttp package make claims about etcd leadership.

@heyitsanthony

https://github.com/gyuho/etcd/blob/162069394b103a489357a76c6584c7eb28a2b70b/etcdserver/api/v3rpc/lease.go

Where does it call leasehttp.X in this file?

Oops, OK, it's hidden through lessor, but it's still a bald error type. Putting the rpctypes error in the case still seems like it's violating some assumed design principle since it sticks out as different. Likewise, leasehttp has to import another package to get at the error . Perhaps what belongs here is a new error like lease.ErrLessorUnavailable?

@heyitsanthony

https://github.com/coreos/etcd/blob/master/etcdserver/v3_server.go#L325

I think this is the place to convert the error, right? We do not need the ErrLessorUnavailable.

Yeah, that looks like the right place to do the EOF check / conversion.

gyuho · 2016-11-02T23:22:53Z

Just fixed to handle EOF directly from v3rpc as Anthony suggested. It's cleaner now. Thanks!

fanminshi · 2016-11-03T00:08:23Z

etcdserver/api/v3rpc/lease.go

@@ -86,7 +86,7 @@ func (ls *LeaseServer) LeaseKeepAlive(stream pb.Lease_LeaseKeepAliveServer) erro
 		}

 		if err != nil {
-			return err
+			return handleLeaseEOF(err)


should we also check EOF for other lease raw RPCs?

Other RPCs should be handled by gRPC, I think. These are only needed for RPCs with raw HTTP requests.

keepAlive uses leaseRenew. i think we might want to catch EOF at that function.
https://github.com/coreos/etcd/blob/master/etcdserver/api/v3rpc/lease.go#L82

Yeah this PR catches it here https://github.com/coreos/etcd/blob/master/etcdserver/api/v3rpc/lease.go#L89

nice! i didn't recognize from the PR.

fanminshi · 2016-11-03T01:23:33Z

LGTM

xiang90 · 2016-11-03T01:58:23Z

etcdserver/api/v3rpc/lease.go

+// lease renew, timetolive requests to followers are forwarded to leader,
+// and follower might not be able to reach leader from network errors with EOFs.
+// Then translate that into ErrNoLeader, expect the client to retry its request.
+func handleLeaseEOF(err error) error {


this is really not lease specific error. handleLeaseEOF is confusing i think.

Right. Just fixed.

gyuho · 2016-11-03T04:37:52Z

@xiang90 @heyitsanthony All fixed. PTAL. Thanks!

xiang90 · 2016-11-03T05:16:10Z

etcdserver/v3_server.go

+// lease renew, timetolive requests to followers are forwarded to leader,
+// and follower might not be able to reach leader from network errors with EOFs.
+// Then translate that into ErrNoLeader, expect the client to retry its request.
+func handleEOF(err error) error {


// convertEOFToNoLeader convert ... to ... since ...
covertEOFErrortoNoLeader

xiang90 · 2016-11-03T05:16:27Z

LGTM after fixing the func name and its comments.

Address etcd-io#6754. In case there are network errors or unexpected EOF errors in TimeToLive http requests to leader, we translate that into ErrNoLeader, and expects the client to retry its request.

gyuho · 2016-11-03T05:23:10Z

Fixed. Merging in greens. Thanks all.

gyuho force-pushed the grpc-leader branch 4 times, most recently from b1506fe to 8e41ab9 Compare November 2, 2016 21:52

gyuho force-pushed the grpc-leader branch from 8e41ab9 to d5064ae Compare November 2, 2016 21:58

xiang90 reviewed Nov 2, 2016

View reviewed changes

gyuho force-pushed the grpc-leader branch from d5064ae to 62aeeb4 Compare November 2, 2016 22:30

fanminshi reviewed Nov 2, 2016

View reviewed changes

gyuho force-pushed the grpc-leader branch from 62aeeb4 to e5dd5d6 Compare November 2, 2016 22:49

heyitsanthony reviewed Nov 2, 2016

View reviewed changes

gyuho force-pushed the grpc-leader branch from 6f6d627 to 2476b64 Compare November 2, 2016 23:21

gyuho changed the title ~~lease/leasehttp: translate EOF to ErrNoLeader for clients~~ v3rpc: translate EOF to ErrNoLeader for renew, timetolive Nov 2, 2016

gyuho force-pushed the grpc-leader branch from 2476b64 to 82d985d Compare November 2, 2016 23:25

fanminshi reviewed Nov 3, 2016

View reviewed changes

xiang90 reviewed Nov 3, 2016

View reviewed changes

gyuho force-pushed the grpc-leader branch 2 times, most recently from 1620693 to c2f007a Compare November 3, 2016 04:37

gyuho changed the title ~~v3rpc: translate EOF to ErrNoLeader for renew, timetolive~~ etcdserver: translate EOF to ErrNoLeader for renew, timetolive Nov 3, 2016

xiang90 reviewed Nov 3, 2016

View reviewed changes

etcdserver: translate EOF to ErrNoLeader for renew, timetolive

cc304ac

Address etcd-io#6754. In case there are network errors or unexpected EOF errors in TimeToLive http requests to leader, we translate that into ErrNoLeader, and expects the client to retry its request.

gyuho force-pushed the grpc-leader branch from c2f007a to cc304ac Compare November 3, 2016 05:22

gyuho merged commit bbc1cda into etcd-io:master Nov 3, 2016

gyuho deleted the grpc-leader branch November 3, 2016 05:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

etcdserver: translate EOF to ErrNoLeader for renew, timetolive #6791

etcdserver: translate EOF to ErrNoLeader for renew, timetolive #6791

gyuho commented Nov 2, 2016

heyitsanthony commented Nov 2, 2016

xiang90 Nov 2, 2016

gyuho Nov 2, 2016

xiang90 commented Nov 2, 2016

fanminshi Nov 2, 2016

fanminshi Nov 2, 2016

fanminshi Nov 2, 2016

gyuho commented Nov 2, 2016 •

edited

Loading

heyitsanthony Nov 2, 2016

xiang90 Nov 3, 2016 •

edited

Loading

xiang90 Nov 3, 2016

heyitsanthony Nov 3, 2016

xiang90 Nov 3, 2016

heyitsanthony Nov 3, 2016

xiang90 Nov 3, 2016

heyitsanthony Nov 3, 2016 •

edited

Loading

gyuho commented Nov 2, 2016

fanminshi Nov 3, 2016

gyuho Nov 3, 2016 •

edited

Loading

fanminshi Nov 3, 2016

gyuho Nov 3, 2016

fanminshi Nov 3, 2016

fanminshi commented Nov 3, 2016

xiang90 Nov 3, 2016

gyuho Nov 3, 2016

gyuho commented Nov 3, 2016

xiang90 Nov 3, 2016

xiang90 commented Nov 3, 2016

gyuho commented Nov 3, 2016

		if err != nil {
		// follower cannot reach leader at the moment

etcdserver: translate EOF to ErrNoLeader for renew, timetolive #6791

etcdserver: translate EOF to ErrNoLeader for renew, timetolive #6791

Conversation

gyuho commented Nov 2, 2016

heyitsanthony commented Nov 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiang90 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

gyuho commented Nov 2, 2016 • edited Loading

Choose a reason for hiding this comment

xiang90 Nov 3, 2016 • 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

heyitsanthony Nov 3, 2016 • edited Loading

Choose a reason for hiding this comment

gyuho commented Nov 2, 2016

Choose a reason for hiding this comment

gyuho Nov 3, 2016 • 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

fanminshi commented Nov 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gyuho commented Nov 3, 2016

Choose a reason for hiding this comment

xiang90 commented Nov 3, 2016

gyuho commented Nov 3, 2016

gyuho commented Nov 2, 2016 •

edited

Loading

xiang90 Nov 3, 2016 •

edited

Loading

heyitsanthony Nov 3, 2016 •

edited

Loading

gyuho Nov 3, 2016 •

edited

Loading