etcdctl: use user specified timeout value

[mitake]

etcdctl should use a timeout value specified with the option --timeout

Fixes #3517

If the current behavior is intentional, please ignore it.

[mitake]

On the second thought, c.GlobalDuration("timeout") seems to be passed to the transport layer correctly. Is just using context.Background() or context.WithCancel() in etcdctl commands enough?

[xiang90]

After reading through the code, I think etcdctl correctly set the timeout to every client.

That timeout is a pre request timeout. etcdctl might send out multiple requests if it fails to connection the first few endpoints.

What you have changed is a total timeout for the RPC. We have not defined the flag for that timeout yet.

[mitake]

Yes, per request timeout seems to be configured correctly. However, current etcdctl use 1s as total timeout for RPC and it causes unreasonable timeout (I found this issue with testing with this tool: https://github.com/osrg/earthquake , the tool can test various execution pattern of distributed systems. I'll share the test code later). It means even user passes --timeout 5s, etcdctl causes timeout in 1s.

Therefore I think providing a flag for configuring timeout of whole RPC or not configuring timeout would be reasonable. How do you think?

[yichengq]

It means even user passes --timeout 5s, etcdctl causes timeout in 1s.

The client.DefaultRequestTimeout is 5s, so etcdctl should cause timeout in 5s. Do you observe 1s behavior?

[xiang90]

@yichengq But we should support total-timeout right?

[yichengq]

@xiang90 Yes. 5s could be a default value, and users could customize the total timeout.

[mitake]

@yichengq sorry, the concrete value of the timeout would be different. However, I observed unexpected behavior of timeout during my testing.

@xiang90 How do you think about a new flag like --total-timeout for this purpose?

[mitake]

Or should etcdctl remove total timeout (allow unbounded timeout for total execution time)?

[xiang90]

@mitake I would suggest a 5 second default total-timeout and make the total-timeout also a flag and configurable.

[mitake]

@xiang90 @yichengq updated tihs PR, added --total-timeout for entire RPC (client API call) execution

[xiang90]

5 * time.Second

[xiang90]

timeout for the command execution (except watch)

[mitake]

5 * time.Second

ok, I'll fix.

timeout for the command execution (except watch)

Actually the timeout value corresponds to one RPC (functions defined in client/). If we call the RPC (potentially which can consist multiple requests?) as command, your description is right. However, I'm not sure it is suitable or not.

Or should we use one context object per one command execution? I think this direction would be natural for users.

[xiang90]

Or should we use one context object per one command execution? I think this direction would be natural for users.

I think so.

[mitake]

I created a new PR for this problem: #3530

Could you review the new one? The contents of two PR are so different.