Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Manually implement client.Wait backoffs #4200

Merged
merged 2 commits into from
Sep 7, 2023
Merged

Conversation

jedevc
Copy link
Member

@jedevc jedevc commented Sep 5, 2023

🛠️ Fixes regression #4164, while preserving the spirit of #4015.
◀️ Reverts #4015.

When calling client.Wait, we want to avoid the default backoff behavior, because we want to achieve a quick response back once the server becomes active.

To do this, without modifying the entire client's exponential backoff configuration, we can use conn.ResetConnectBackoff, while attempting to reconnect every second.

Here are some common scenarios under the new client.Wait implementation:

  • Server is listening: the call to Info succeeds quickly, and we return.
  • Server is listening, but is behind several proxies and so latency is high: the call to Info succeeds slowly (up to minConnectTimeout=20s), and we return.
    • Note: if the server cannot be reached in fewer than minConnectTimeout, then client.Wait will never return.
  • Server is not listening and gets "connection refused": the call to Info fails quickly, and we wait a second before retrying.
  • Server is not listening and does not respond (e.g. firewall dropping packets): the call to Info fails slowly (by default after minConnectTimeout=20s). After the call fails, we wait a second before retrying.

This PR is split into two commits:

This reverts commit 1aef766.

Signed-off-by: Justin Chadwell <me@jedevc.com>
Copy link
Member

@crazy-max crazy-max left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The automatic retries for transient errors and recover gracefully when the service is unavailable temporarily looks good enough but I wonder if we should consider a maximum retry limit as well.

client/client.go Outdated Show resolved Hide resolved
@jedevc
Copy link
Member Author

jedevc commented Sep 5, 2023

I wonder if we should consider a maximum retry limit as well.

I think the --timeout flag controls a maximum duration of the context that eventually gets passed through into client.Wait. I think that makes more sense than an explicit number of retries?

@crazy-max
Copy link
Member

I wonder if we should consider a maximum retry limit as well.

I think the --timeout flag controls a maximum duration of the context that eventually gets passed through into client.Wait. I think that makes more sense than an explicit number of retries?

Looks good 👍

client/client.go Outdated Show resolved Hide resolved
When calling client.Wait, we want to avoid the default backoff behavior,
because we want to achieve a quick response back once the server becomes
active.

To do this, without modifying the entire client's exponential backoff
configuration, we can use conn.ResetConnectBackoff, while attempting to
reconnect every second.

Here are some common scenarios:
- Server is listening: the call to Info succeeds quickly, and we return.
- Server is listening, but is behind several proxies and so latency is
  high: the call to Info succeeds slowly (up to minConnectTimeout=20s),
  and we return.
- Server is not listening and gets "connection refused": the
  call to Info fails quickly, and we wait a second before retrying.
- Server is not listening and does not respond (e.g. firewall dropping
  packets): the call to Info fails slowly (by default after
  minConnectTimeout=20s). After the call fails, we wait a second before
  retrying.

Signed-off-by: Justin Chadwell <me@jedevc.com>
@jedevc jedevc merged commit 97e0efa into moby:master Sep 7, 2023
56 checks passed
@jedevc jedevc deleted the fix-client-wait branch September 7, 2023 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

After upgrade local CLI to 0.12 remote build stop working
3 participants