-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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
Add tests to serializable operations validation #17918
Add tests to serializable operations validation #17918
Conversation
2c83949
to
6d0acff
Compare
/retest |
Hmm, it's timeout error |
/retest |
|
||
if diff := cmp.Diff(response.EtcdResponse.Range, expectResp.Range); diff != "" { | ||
lg.Error("Failed validating serializable operation", zap.Any("request", request), zap.String("diff", diff)) | ||
return fmt.Errorf("response didn't match expected") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make the error a constant so it is easier to check in tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not a huge priority. Feel free to open a followup PR.
if request.Type == model.Range && request.Range.Revision != 0 { | ||
resp = append(resp, op) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a question: iiuc, all range requests are linearized by default, so:
(1) how are we doing serialized ranges? I see how we implement them in our model, but not sure of the distinction.
(2) how is this if
condition telling us that an operation is a serializable operation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry naming is hard. Serializable is an overloaded term in etcd.
Name serializable comes from dropping linearizable guarantee from consistency model. Etcd normally provides both Serializable and Linearizable, which combined is called Strict Serializable https://jepsen.io/consistency. By dropping Linearizable we get Serializable and Sequential, which doesn't really have a name. Etcd calls it just Serializable.
Etcd supports serializable reads by allowing client set a field. Such reads will latest state of local member (they are Sequential because member data is not expected to go back), without checking with the whole cluster, so we such reads cannot be linearized. In robustness test we don't support this exact type of request, as it's not used by Kubernetes, however Kubernetes something we could call a "exact stale read".
A exact stale read request state of data from some revision, usually older state of database. Those kind of request are still Serializable, but we don't validate them the same way as Linearizable request. We cannot validate the response data, because it model doesn't include historical data, that would be too much data resulting in horrible performance. For those kind we do the following validation:
- Linearization validates that revision in request and response is correct. Lower than the current revision and larger than compact revision (implementation pending in Implement compaction support in robustness test #17833)
- Use the persisted requests to replay the etcd state to the requested revision and validate if the response contents match.
Name validateSerializableRead still makes sense as it can validate the response data of both "exact stale read" and "etcd serializable read",
Signed-off-by: Marek Siarkowicz <siarkowicz@google.com>
6d0acff
to
b883f83
Compare
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
ping @MadhavJivrajani |
@MadhavJivrajani is on vacation. |
cc @fuweid @siyuanfoundation @MadhavJivrajani