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.

Sign up for GitHub

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

Jump to bottom

Make *client.Client safe for concurrent use #43738

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Make *client.Client safe for concurrent use #43738

wants to merge 3 commits into from
+2,348 −1,458

Conversation

corhere
Copy link
Contributor

@corhere corhere commented Jun 23, 2022 •
edited

- What I did

  • Added concurrency control and synchronization to version negotiation to eliminate data races and duplicate pings
  • Modified how versioning is handled in the client so that changing the client's API version is atomic: each request will use the same API version to construct and validate the request, process the response and handle errors, even if the client API version is changed mid-request
  • Updated all the client unit tests to construct clients using the exported NewClientWithOpts constructor instead of a struct literal
  • Modified version negotiation to disregard the negotiation attempt and try negotiating again on the next API call instead of falling back to v1.24 if the ping request fails due to network conditions or the request context is canceled mid-ping

- How I did it
Way too much tedious and repetitive semi-manual editing.

- How to verify it
New tests to exercise concurrent client usage for the race detector and to verify the new negotiation retry behaviour.

- Description for the changelog

  • *client.Client instances can now safely be used concurrently by multiple goroutines in all cases. Older client versions are not safe for concurrent use when the WithAPIVersionNegotation option is used.
  • The WithAPIVersionNegotiation option for *client.Client has been made more robust. API version negotiation will now be retried if the first attempt timed out or could not be completed due to network conditions.

- A picture of a cute animal (not mandatory but encouraged)

@corhere
client: test using public Client constructor
6ccceb6 
Signed-off-by: Cory Snider <csnider@mirantis.com>
@corhere
client: make safe for concurrent use
632e667 
The API version is the only stateful thing in the client. It has
historically had no locking or synchronization, making the client de
facto unsafe for concurrent use. Making the client concurrency-safe is
not exactly a trivial undertaking, however: the client uses the API
version when building requests and handling the responses, so care must
be taken to ensure that the responses to any requests be handled with
the semantics of the same API version that was used when building the
request, even if the client's API version is changed while the request
is in-flight. Merely converting all accesses to the version field to
atomic loads and stores would not be sufficient. The client also has an
optional feature to lazily auto-negotiate the API version on the first
API request, so care also needs to be taken to suppress redundant
auto-negotiation attempts while an auto-negotiation is in progress.

Introduce the concept of a "versioned client" to the client internals
which encapsulates a point-in-time snapshot of the client's configured
API version. Refactor the client methods to perform all version-
dependent operations through a versioned-client instance to ensure that
they always operate on an immutable snapshot of the version. Move
version auto-negotiation into the versioned-client constructor and
synchronize accesses to the version state to prevent data races and
redundant auto-negotation attempts. Exclusive access to the version
state is held by the goroutine throughout the entire duration of the
auto-negotiation process, blocking the construction of new versioned
clients or starting any concurrent negotiation processes until the
auto-negotiation process completes.

With auto-negotiation now performed explicitly in the versioned-client
constructor rather than implicitly in the high-level HTTP request helper
methods, it is now possible to implement Ping() in terms of those high-
level helpers. Simplify the Ping implementation through the use of an
"unversioned" versioned client (i.e. with an empty version) and the
aforementioned high-level helpers.

Signed-off-by: Cory Snider <csnider@mirantis.com>
@corhere corhere added area/api kind/enhancement Enhancements are not bugs or new features but can improve usability or performance. labels Jul 4, 2022
@corhere corhere marked this pull request as ready for review July 4, 2022 22:12
@corhere
client: retry version negotiation after error
2d4806a 
When a client is configured for API version negotiation, it would try
exactly once to ping the server, then unconditionally configure the
client API version and set the negotiated flag. Any failure to ping the
server would result in the client "negotiating" the fallback API version
v1.24, even if the request never reached the daemon. Transient network
issues or simply calling a client method with an already-canceled
context would be sufficient to cause this to happen.

Modify the client's negotiation process to retry negotiation on each
subsequent request until it receives a ping response from the daemon.
Only responses with HTTP status codes in the range [1, 500] are
considered to be ping responses from the daemon as HTTP status codes
>= 501 (e.g. 502 Gateway Timeout, 503 Service Unavailable) could be
returned by intermediate hops.

Signed-off-by: Cory Snider <csnider@mirantis.com>
@corhere corhere requested review from thaJeztah and ndeloof July 5, 2022 17:18
@ndeloof
Copy link
Contributor

ndeloof commented Jul 6, 2022

Sounds to me a damn complicated way to address this issue while API version negociation could just take place once within NewClient. If some usage require this to be lazy-set, then they could just lazy-create a client

@corhere
Copy link
Contributor Author

corhere commented Jul 6, 2022

Sounds to me a damn complicated way to address this issue while API version negociation could just take place once within NewClient. If some usage require this to be lazy-set, then they could just lazy-create a client

Nearly all of the complexity will still be required to make the client safe for concurrent use so long as func (*Client) NegotiateAPIVersion(context.Context) and func (*Client) NegotiateAPIVersionPing(types.Ping) exist. And removing them would be a breaking change. Suppose we made a client/v2 which removes these methods and eagerly negotiates API version within NewClient. Someone wanting lazily-created clients would have to implement very similar synchronization and retry logic to deal with the case of NewClient returning an error.

There are more ways to implement API version negotiation than just eager and lazy pinging. If I have time someday, I would like to experiment with what I like to call optimistic version negotiation: do the user's request initially using the highest API version the client supports. If the response is a 400 Bad Request, downgrade the client's API version using the API-Version response header and replay the request. This would not alter the existing lazy-negotiation semantics and so could be released as a backwards-compatible update to the current client package. The same could not be released as a backwards-compatible update to an eagerly-negotiating client/v2: it would alter the semantics of NewClient such that err == nil would no longer imply that the daemon is reachable and responsive to pings.

@thediveo

This comment was marked as off-topic.

Sign in to view
@gearnode gearnode mentioned this pull request Jun 29, 2023
Open
@vvoland vvoland mentioned this pull request Sep 19, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thaJeztah thaJeztah Awaiting requested review from thaJeztah

@ndeloof ndeloof Awaiting requested review from ndeloof

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
area/api kind/enhancement Enhancements are not bugs or new features but can improve usability or performance.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[go client] negotiateAPIVersionPing triggers Go race detector
3 participants
@corhere @ndeloof @thediveo