cli: set timeout connection ping on sockets as well - #3722
Conversation
Note that this does not fully fix the referenced issue, but at least makes sure that API clients don't hang forever on the initialization step. See: docker#3652 Signed-off-by: Nick Santos <nick.santos@docker.com> Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
| if strings.HasPrefix(cli.DockerEndpoint().Host, "tcp://") { | ||
| if !strings.HasPrefix(cli.DockerEndpoint().Host, "ssh://") { | ||
| // @FIXME context.WithTimeout doesn't work with connhelper / ssh connections | ||
| // time="2020-04-10T10:16:26Z" level=warning msg="commandConn.CloseWrite: commandconn: failed to wait: signal: killed" |
There was a problem hiding this comment.
I know it isn't a part of this PR, but maybe you remember how does context.WithTimeout has anything to do with ssh being used? The PR that introduced this doesn't mention any details here.
There was a problem hiding this comment.
Just curious - so if you remember then just some quick tldr will be enough 😄
There was a problem hiding this comment.
I'm not sure how much time I spent at the time digging into the exact cause; the timeout was helping with an issue (see #2424), and I'm not sure I was able to consistently reproduce the ssh problem locally (or only in CI?). Ultimately decided that the SSH connections were less common, so to keep diving into the issue for later (there's some other issues remaining with the ssh connection helper as well).
In case useful;
- ssh implementation; support SSH connection #1014
- multiplexing change that we reverted due to CI issues (but should look into to have it merged again) revert "connhelper: use ssh multiplexing" #2303
There was a problem hiding this comment.
It may be just missing functionality in the ssh connecting helper (it not handling context cancelation), but yeah, never found the time really to get myself familiar with that code or to dig into the issue.
|
Let me bring this one in |
|
I don't know why docker login alway fail! |
|
is this a way to deloy a server side component ? |
|
docker company's business model is so stupid. |
|
@WillisZhang you're commenting on a 3 Year old pull request; if you suspect there's a bug at hand that's causing login to fail, please open a ticket instead, and provide details that are requested in the ticket template, and steps to reproduce. |
Run a single bounded Version() ping before resolving secrets, so an unreachable engine fails `pass run` fast instead of hanging resolution indefinitely. The ping only runs when the client request timeout is indefinite; a timeout configured via WithTimeout already bounds the wait. The Docker CLI bounds its daemon connection ping the same way (docker/cli#3722, fixing the unreachable-daemon hang in docker/cli#3652). Also expose WithResponseTimeout and WithSocketPath run options so embedders can override the client's 1s response-header timeout and the engine socket path; the socket option also lets tests point the preflight ping at a dead socket. Signed-off-by: Johannes Großmann <grossmann.johannes@t-online.de>
GetSecrets on a client without a request timeout now pings the engine's version endpoint first, bounded to 2 seconds to match the Docker CLI's defaultInitTimeout (docker/cli#3722), so an unreachable engine fails fast instead of blocking resolution indefinitely. Clients with a request timeout skip the ping and fail on their own. This moves the preflight out of the pass run command into the client, so every unbounded consumer gets the fail-fast behavior. As a side effect, pass run no longer contacts the engine when the environment holds no se:// references. Signed-off-by: Johannes Großmann <grossmann.johannes@t-online.de>
4 participants
Note that this does not fully fix the referenced issue, but
at least makes sure that API clients don't hang forever on
the initialization step.
See: #3652
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)