clientv3: kv ordering wrapper for serialized requests

[heyitsanthony]

Based on discussion from #7597

Check whether serialized Get/Txn responses are ordered by caching last known revision on the client side. Each Get/Txn client call will check the response's header revision is equal to or greater than the last known revision at the time of issuing the RPC.

This avoids the case where an etcd client connects to multiple endpoints, issues multiple rev=0 Gets, and receives revisions older than those already received. If this happens on a single proxy/server, it's a bug.

Two behavior notes:

1. retry on same server within some duration (endpoint might just be slow to get update)
2. if retries fail after some time, switch endpoint

[mangoslicer]

Please correct me if I am wrong, but isn't this just a band-aid over a cache-coherence issue on the proxy/server? Shouldn't we make sure that the server or caches response on the proxy doesn't return older revisions on the proxy/server rather than on the client? I'm not sure exactly how we would deal with this on the server side, but I just wanted to get your thoughts on the possibility of a server-side solution and to better understand the issue.

[heyitsanthony]

@mangoslicer there's already linearized reads for server-side ordering.

The serialized case is different since it never goes through quorum. Suppose there are three members A,B,C with C partitioned from A and B. Let a client issue a serialized read to A/B, then partition the client so it can only access C. The client will reconnect to C and if C is somewhat behind, the next serialized read will likely be stale with respect to the rest of the cluster. The client can see the response revision is older than the last seen revision and know that it's out of date (and handle it accordingly), but there's no way C would know it's behind. Even if there's no partitioning, raft only requires a quorum, so switching between members can still lead to serialized requests going backwards.

[mangoslicer]

@heyitsanthony Alright thanks for explaining. I was looking through the clientv3 src folder in order to find a reasonable place to cache the last rev. Do you think that the kv struct defined in clientv3/kv.go would be an appropriate place to add an int64 field named something like "PrevRev"? I thought that the kv struct might be an appropriate place to cache because both the txn and get responses have access to the kv struct. What do you think?

[heyitsanthony]

@mangoslicer this shouldn't modify the base client. It'd be a wrapper for KV sort of like namespace.

[mangoslicer]

@heyitsanthony

I made the initial wrapper. I have some questions about the retry and endpoint switching functionality.

Would the retry functionality be as simple as setting a time out and then sending the request again, or is there a different way that you want this to be implemented?

For the endpoint switching, I think the the implementation would involve using clientv3's Endpoints method to get the list of endpoints, removing the current endpoint from this array, and then setting the array of endpoints using clientv3's SetEndpoints method. But I'm not sure if switching the endpoint based off of a KV.Get returning a old-revision error would work, because it looks like the error from the Client.Get would be returned directly to the user without any room for the client code to interpret and respond to the data.

[heyitsanthony]

@mangoslicer the wrapper shouldn't return an old revision error, it would make the Get call, see an old revision, then reissue the read on a different endpoint; the user wouldn't see any of this.

[mangoslicer]

@heyitsanthony

Alright. Here's a link to my implementation so far. I'm not sure how to switch endpoints in the actual Get call, it doesn't seem like the KV interface has any way of switching endpoints. Do you have any suggestions on how to switch endpoints?

[heyitsanthony]

@mangoslicer possibly separate the policy (how to switch/what's retry rate) from the mechanism of detecting ordering violation? That way the action the client takes based on order violation doesn't have to be baked into the wrapper.

Something like:

type OrderViolationFunc func(op clientv3.Op, resp clientv3.OpResponse, prevRev int64)

func NewKV(kv clientv3.KV, f OrderViolationFunc) *kvOrderingCache {...}

func (kv *kvOrderingCache) SomeRPC(...) {
    ... Do() ...
    if isPastRev {
        kv.orderViolation(op, resp, prevRev)
    }
    ...
}

The switching code can be passed in as a closure:

orderingKV := NewKV(client.KV, func(...) { client.DoStuff() })

Also, can you put this up as a PR? Thanks!

[mangoslicer]

@heyitsanthony

Thanks for the suggestions. I think that should work. Right now I am trying to figure out how to implement the endpoint switching, it seems like the conn attribute in the Client struct is private and can only possibly be set in the newClient constructor. So I don't think it is possible to dial a different endpoint without creating a new client. I was thinking that in order to switch endpoints I would need to create a new client or muck around with the clientv3/client.go code to enable endpoint switching (i.e. make a setter for the conn attribute). What do you think I should do? Is there another way of switching endpoints that I have not thought of?

[heyitsanthony]

@mangoslicer the easiest way at the moment would probably be something like:

var mu sync.Mutex
violationCount := 0
handleViolation := func(...) {
    mu.Lock()
    defer mu.Unlock()
    eps := client.Endpoints()
    client.SetEndpoints([]string{eps[violationCount%len(eps)]})
    client.SetEndpoints(eps)
    violationCount++
}

[mangoslicer]

@heyitsanthony

Ok thanks for the code. I don't understand why client.SetEndpoints([]string{eps[violationCount%len(eps)]}) is immediately followed by client.SetEndpoints(eps). Can you explain that? Thanks

[heyitsanthony]

@mangoslicer otherwise the next call to client.Endpoints() will only have eps[0] even if the client was configured with multiple endpoints

[mangoslicer]

@heyitsanthony

Ok, so the client.SetEndpoints(eps) is just clean up work for after we have resolved the outdated revision issue. So would the code look more like this?

client.SetEndpoints([]string{eps[violationCount%len(eps)]})
// Do the actual GRPC call
client.SetEndpoints(eps)

[heyitsanthony]

@mangoslicer no, it's meant to cause the balancer to try to connect to another endpoint. I don't think it should try submitting the RPC while there's only one endpoint configured.

[mangoslicer]

@heyitsanthony

Ok that makes sense.

[mangoslicer]

@heyitsanthony

I pushed to the branch in this PR. Can you take a look?

I have to notes regarding to testing. In kv_test.go, I had to make the get field in the clientv3.OpResponse struct public, so that I was able to define that field in my test cases. I can remove the kv_test.go file from the PR, if changing the clientv3.OpResponse struct is not a good idea.

I also noticed that the clientv3.Client type does not implement an interface, so mocking in unit tests is not possible. I would use the client.Client interface but it doesn't seem like the clientv3.Client properly implements that interface. In the client.Client interface the SetEndpoints signature is SetEndpoints(eps []string) error while the clientv3.Client type implements SetEndpoints with the following signature: func (c *Client) SetEndpoints(eps ...string). Is this inconsistency in the clientv3.Client type and client.Client interface intentional? If I can mock the clientv3.Client I can also add a unit test for the utils.go file to the PR.

[heyitsanthony]

Fixed by #8092