client: do not timeout when wait is true #4254

gyuho · 2016-01-21T21:04:38Z

Current V2 watch waits by encoding URL with wait=true.
When a client sets 'no-sync', it requests directly to
proxy and the proxy redirects it by cloning the request
object, which leads to cancel the original request when
it times out and the cloned request gets closed prematurely.

This fixes #3894 by querying
the original client request in order to not use context timeout
when 'wait=true'.

heyitsanthony · 2016-01-21T21:13:44Z

would it be worthwhile to have an e2e test for this / proxying?

gyuho · 2016-01-21T21:14:31Z

@heyitsanthony Good idea. Will add e2e test for this case. Thanks,

heyitsanthony · 2016-01-21T21:19:47Z

client/client.go

@@ -442,9 +443,16 @@ func (c *simpleHTTPClient) Do(ctx context.Context, act httpAction) (*http.Respon
 		return nil, nil, err
 	}

+	isWait := false
+	if req != nil {
+		if req.URL != nil {


&& instead of nesting if's

ok will fix. thanks

heyitsanthony · 2016-01-21T21:26:55Z

lgtm

gyuho · 2016-01-21T23:36:56Z

@heyitsanthony Just added e2e test that only fails without this patch.

--- FAIL: TestBasicOpsV2CtlWatchWithProxyNoSync (6.91s)
    etcd_test.go:190: failed watch (Error:  client: etcd cluster is unav)ilable or misconfigured
FAIL
exit status 1

With this patch, e2e test passes:

PTAL. Thanks!

heyitsanthony · 2016-01-21T23:53:08Z

e2e/etcd_test.go

@@ -42,6 +44,7 @@ func TestBasicOpsNoTLS(t *testing.T) {
 			isPeerTLS:    false,
 			initialToken: "new",
 		},
+		etcdProcessBasePort,


why have a new base port argument? the same ports were reusable across tests

It seemed like test was hanging for watch cases when using the same ports.

sounds broken; a process might be hanging

xiang90 · 2016-01-22T00:18:59Z

@gyuho This change seems to be OK. But please update the documentation accordingly.

defer to @heyitsanthony on the testing changes.

gyuho · 2016-01-22T00:20:33Z

@xiang90 Now understand what you were talking about. I will make the comment clearer. And will update tests addressing Anthony's feedback.

Thanks,

heyitsanthony · 2016-01-22T00:22:48Z

e2e/etcd_test.go

+	go func() {
+		defer wg.Done()
+		args = append(args, "watch", key)
+		proc, err := spawnCmd(args)


this needs a proc.Close() (spawnWithExpect will do it for you)

xiang90 · 2016-01-22T00:25:35Z

@gyuho @heyitsanthony

Basically the whole long polling behavior is broken for the proxy case when proxy does not forward response header or response in time. The request header is designed for watch request in the first place. Or we have no way to know if the remote machine is hang or not. I am not even sure if we still need request header timeout or not.

heyitsanthony · 2016-01-22T00:29:13Z

e2e/etcd_test.go

+	}()
+
+	endpoint, be := "", ""
+	for _, p := range epc.procs {


this loop should probably be replaced by calling methods like (e *EtcdProcessCluster) Members() []*etcdProcess and (e *EtcdProcessCluster) Proxies() []*etcdProcess

Current V2 watch waits by encoding URL with wait=true. When a client sets 'no-sync', it requests directly to proxy and the proxy redirects it by cloning the request object, which leads to cancel the original request when it times out and the cloned request gets closed prematurely. This fixes coreos#3894 by querying the original client request in order to not use context timeout when 'wait=true'.

gyuho · 2016-01-22T02:48:17Z

I think e2e test is currently broken. #4257
I locally tested the e2e and confirmed that this fixes the issue #3894.

Let's fix the e2e issue in a separate PR.
Once we figure out the goroutine leak issue, I will rewrite the test.

gyuho · 2016-01-22T02:50:59Z

@heyitsanthony Could you take a look at client.go again? I will write the end to end test in a separate PR. Need to figure out why it was hanging. Thanks!

heyitsanthony · 2016-01-22T02:58:39Z

lgtm

gyuho · 2016-01-22T02:59:01Z

Thanks!

client: do not timeout when wait is true

gyuho changed the title ~~client: do not timeout when wait is true~~ [WIP] client: do not timeout when wait is true Jan 21, 2016

gyuho changed the title ~~[WIP] client: do not timeout when wait is true~~ client: do not timeout when wait is true Jan 21, 2016

gyuho force-pushed the check_wait branch from c1bc6be to 2427044 Compare January 21, 2016 21:13

gyuho changed the title ~~client: do not timeout when wait is true~~ *: do not timeout when wait is true Jan 21, 2016

heyitsanthony reviewed Jan 21, 2016
View reviewed changes

gyuho force-pushed the check_wait branch from 2427044 to 8d233bf Compare January 21, 2016 23:35

gyuho force-pushed the check_wait branch from 8d233bf to 982c901 Compare January 21, 2016 23:47

heyitsanthony reviewed Jan 21, 2016
View reviewed changes

heyitsanthony reviewed Jan 22, 2016
View reviewed changes

gyuho force-pushed the check_wait branch 2 times, most recently from c595075 to bc58090 Compare January 22, 2016 02:24

gyuho changed the title *: do not timeout when wait is true [wip] *: do not timeout when wait is true Jan 22, 2016

gyuho force-pushed the check_wait branch from bc58090 to 56a5faf Compare January 22, 2016 02:31

gyuho force-pushed the check_wait branch from 56a5faf to bbb7fb5 Compare January 22, 2016 02:45

gyuho changed the title ~~[wip] *: do not timeout when wait is true~~ *: do not timeout when wait is true Jan 22, 2016

gyuho changed the title *: do not timeout when wait is true client: do not timeout when wait is true Jan 22, 2016

gyuho added a commit that referenced this pull request Jan 22, 2016

Merge pull request #4254 from gyuho/check_wait

6413c96

client: do not timeout when wait is true

gyuho merged commit 6413c96 into etcd-io:master Jan 22, 2016

This was referenced Jan 22, 2016

etcdctl watch to a etcd in proxy mode with TLS to cluster fails #3894

Closed

e2e: test for etcdctl operations #4259

Closed

gyuho deleted the check_wait branch January 31, 2016 00:43

jeromegn mentioned this pull request Sep 28, 2016

Allow proper use of etcd proxies. kelseyhightower/confd#329

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

client: do not timeout when wait is true #4254

client: do not timeout when wait is true #4254

gyuho commented Jan 21, 2016

heyitsanthony commented Jan 21, 2016

gyuho commented Jan 21, 2016

heyitsanthony Jan 21, 2016

gyuho Jan 21, 2016

heyitsanthony commented Jan 21, 2016

gyuho commented Jan 21, 2016

heyitsanthony Jan 21, 2016

gyuho Jan 21, 2016

heyitsanthony Jan 22, 2016

xiang90 commented Jan 22, 2016

gyuho commented Jan 22, 2016

heyitsanthony Jan 22, 2016

xiang90 commented Jan 22, 2016

heyitsanthony Jan 22, 2016

gyuho commented Jan 22, 2016

gyuho commented Jan 22, 2016

heyitsanthony commented Jan 22, 2016

gyuho commented Jan 22, 2016

client: do not timeout when wait is true #4254

client: do not timeout when wait is true #4254

Conversation

gyuho commented Jan 21, 2016

heyitsanthony commented Jan 21, 2016

gyuho commented Jan 21, 2016

heyitsanthony Jan 21, 2016

Choose a reason for hiding this comment

gyuho Jan 21, 2016

Choose a reason for hiding this comment

heyitsanthony commented Jan 21, 2016

gyuho commented Jan 21, 2016

heyitsanthony Jan 21, 2016

Choose a reason for hiding this comment

gyuho Jan 21, 2016

Choose a reason for hiding this comment

heyitsanthony Jan 22, 2016

Choose a reason for hiding this comment

xiang90 commented Jan 22, 2016

gyuho commented Jan 22, 2016

heyitsanthony Jan 22, 2016

Choose a reason for hiding this comment

xiang90 commented Jan 22, 2016

heyitsanthony Jan 22, 2016

Choose a reason for hiding this comment

gyuho commented Jan 22, 2016

gyuho commented Jan 22, 2016

heyitsanthony commented Jan 22, 2016

gyuho commented Jan 22, 2016