etcdctl: use user specified timeout value for entire command execution

[mitake]

etcdctl should be capable to use a user specified timeout value for
total command execution, not only per request timeout. This commit
adds a new option --total-timeout to the command. The value passed via
this option is used as a timeout value of entire command execution.

Fixes coreos#3517

This is the new version of the PR #3518 . This new one is so different from the old one, so I made a new PR.

[mitake]

ping? @xiang90 @yichengq (if you already noticed this pr, sorry for noise)

[yichengq]

I don't like current approach because it introduces new parameter in each function call, and it has one more level indirection.

Could we use
ctx, cancel := context.WithTimeout(context.Background(), c.GlobalDuration("total-timeout")) in each function and use the same ctx for the whole function to achieve this?

[mitake]

@yichengq thanks for your review. However, current approach can reduce possibility of using wrong timeout value for each context (the problem this PR is trying to fix). So I thought introducing a new wrapper function actionWithTotalTimeout() is valuable. And commands which doesn't require total timeout aren't changed.

Of course, if you prefer declaration of ctx in each function, I'll pick your approach.

[yichengq]

I still prefer to declare ctx. Current solution makes the code hard to read, and the indirection could be replaced by one-line declaration.

possibility of using wrong timeout value for each context

We could add a function to return overall context and its cancel.

And commands which doesn't require total timeout aren't changed.

I think other commands need to have total timeout too. They are using context.TODO now.

[mitake]

@yichengq ok, I'll change this PR based on your policy later

[mitake]

@yichengq updated based on the policy

[yichengq]

I would defer cancel() here. It is reasonable because the ctx is for the whole function.

[mitake]

defer cancel() seems nice, I'll fix it.

[mitake]

@yichengq updated based on your comments

[mitake]

updated the commit log a little bit

[yichengq]

The per request timeout is used at https://github.com/coreos/etcd/blob/master/etcdctl/command/util.go#L251. It is the timeout for each HTTP request(ref: http://godoc.org/github.com/coreos/etcd/client#Config). When we call mAPI.Add, it may include several HTTP requests, so we cannot set per request timeout as its context.

It is fine that we just use totalCtx for all requests in the this function IMO.

[mitake]

Yes, I misused client.DefaultRequestTimeout :( Thanks for pointing. I'll fix it.

[mitake]

@yichengq updated for using correct context and timeout

[yichengq]

LGTM. Do you have plan to fix the left context.Background() ones? For example, https://github.com/coreos/etcd/blob/master/etcdctl/command/set_command.go#L58

[mitake]

@yichengq thanks for your review. Sure, I'd like to work on other parts. Should I fix them in this PR?

[yichengq]

No. this one is good enough.

Thanks for the contribution! ❤️