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

rpc: remote clock monitoring fails to recognize excess offset #4884

Closed
knz opened this Issue Mar 5, 2016 · 14 comments

Comments

Projects
None yet
5 participants
@knz
Member

knz commented Mar 5, 2016

See:

http://52.91.194.28/cgi-bin/display.py?entry=20160305T161219#20160305T161219
http://52.91.194.28/cgi-bin/display.py?entry=20160305T161642#20160305T161642
http://52.91.194.28/cgi-bin/display.py?entry=20160305T162220#20160305T162220

The error basically says that what should be a linearizable history is not linearizable. This is happening while clock skews in excess of seconds are introduced randomly on each node. The nodes do not "commit suicide" as would be necessary to prevent these errors.

@knz knz added the C-bug label Mar 5, 2016

@tschottdorf

This comment has been minimized.

Show comment
Hide comment
@tschottdorf

tschottdorf Mar 5, 2016

Member

just to clarify, this is a test which is running with --linearizable, correct?

Member

tschottdorf commented Mar 5, 2016

just to clarify, this is a test which is running with --linearizable, correct?

@tschottdorf tschottdorf changed the title from Consistency error found with Jepsen with large clock skews to rpc: remote clock monitoring fails to recognize excess offset Mar 5, 2016

@knz

This comment has been minimized.

Show comment
Hide comment
@knz

knz Mar 5, 2016

Member

It doesn't matter. Error happens both with and without --linearizable.
The examples linked above don't use --linearizable, but the following does:

http://52.91.194.28/cgi-bin/display.py?entry=20160305T171032#20160305T171032

Member

knz commented Mar 5, 2016

It doesn't matter. Error happens both with and without --linearizable.
The examples linked above don't use --linearizable, but the following does:

http://52.91.194.28/cgi-bin/display.py?entry=20160305T171032#20160305T171032

@tschottdorf

This comment has been minimized.

Show comment
Hide comment
@tschottdorf

tschottdorf Mar 5, 2016

Member

I was asking because you called this a consistency error, but you're violating the MaxOffset you've set on the cluster, so this is really not a consistency error but a failure of the clock detection/suicide trigger (which can never work 100% reliable, but should work better than it does now). Just clarifying for other readers. I re-titled the issue.

Member

tschottdorf commented Mar 5, 2016

I was asking because you called this a consistency error, but you're violating the MaxOffset you've set on the cluster, so this is really not a consistency error but a failure of the clock detection/suicide trigger (which can never work 100% reliable, but should work better than it does now). Just clarifying for other readers. I re-titled the issue.

@knz

This comment has been minimized.

Show comment
Hide comment
@knz

knz Mar 5, 2016

Member

ok thanks!

Member

knz commented Mar 5, 2016

ok thanks!

@knz

This comment has been minimized.

Show comment
Hide comment
@knz

knz Mar 14, 2016

Member

Tested again after #5107 merged in, and reducing monitorInterval to 1 second (instead of the default 30s). I confirm that the consistency error that motivated this issue happens much less often, and instead the nodes abort with an error more often.

However with the default monitoring interval of 30s there is a 30 second interval where consistency errors can happen. I can see this happen experimentally. Two questions thus remain:

  • would it be interesting to reduce the default clock offset monitoring interval?
  • what statements do we want to make about consistency in the face of drifting clock? Is there a way to qualify which types of consistency errors may or may not happen?
Member

knz commented Mar 14, 2016

Tested again after #5107 merged in, and reducing monitorInterval to 1 second (instead of the default 30s). I confirm that the consistency error that motivated this issue happens much less often, and instead the nodes abort with an error more often.

However with the default monitoring interval of 30s there is a 30 second interval where consistency errors can happen. I can see this happen experimentally. Two questions thus remain:

  • would it be interesting to reduce the default clock offset monitoring interval?
  • what statements do we want to make about consistency in the face of drifting clock? Is there a way to qualify which types of consistency errors may or may not happen?

@knz knz modified the milestones: 1.0, Beta Mar 14, 2016

@tamird

This comment has been minimized.

Show comment
Hide comment
@tamird

tamird Mar 14, 2016

Collaborator

I think the current interval-based monitoring is inherently flawed. We should really be checking the clocks in the request path and aborting eagerly.

Collaborator

tamird commented Mar 14, 2016

I think the current interval-based monitoring is inherently flawed. We should really be checking the clocks in the request path and aborting eagerly.

@tamird

This comment has been minimized.

Show comment
Hide comment
@tamird

tamird Mar 30, 2016

Collaborator

OK, this is slightly better now that #5512 is in, but the solution is still not totally robust. We need to do something adaptive with maxOffset, but it's no longer a high priority. Unassigning myself since I'm not currently working on this.

@knz perhaps it's worth closing this issue since the reports here are (hopefully) no longer relevant.

Collaborator

tamird commented Mar 30, 2016

OK, this is slightly better now that #5512 is in, but the solution is still not totally robust. We need to do something adaptive with maxOffset, but it's no longer a high priority. Unassigning myself since I'm not currently working on this.

@knz perhaps it's worth closing this issue since the reports here are (hopefully) no longer relevant.

@tamird tamird assigned knz and unassigned tamird Mar 30, 2016

@knz

This comment has been minimized.

Show comment
Hide comment
@knz

knz Apr 1, 2016

Member

Thanks @tamird for the update.
I think however that the issue is still relevant even if the original test reports are not up to date. Namely as long as the issue is not fixed there is still a risk of db consistency errors in the presence of large skews. I agree however the issue is now somewhat lower priority than initially.

There are two ways forward really:

  • either make skew detection rock solid (which may or may not be theoretically possible)
  • document exuberantly that we disclaim any consistency guarantee if the users don't run NTP or equivalent clock synchronization with guaranteed sub-maxoffset precision.

Should we move this issue to doc?

Member

knz commented Apr 1, 2016

Thanks @tamird for the update.
I think however that the issue is still relevant even if the original test reports are not up to date. Namely as long as the issue is not fixed there is still a risk of db consistency errors in the presence of large skews. I agree however the issue is now somewhat lower priority than initially.

There are two ways forward really:

  • either make skew detection rock solid (which may or may not be theoretically possible)
  • document exuberantly that we disclaim any consistency guarantee if the users don't run NTP or equivalent clock synchronization with guaranteed sub-maxoffset precision.

Should we move this issue to doc?

@tamird

This comment has been minimized.

Show comment
Hide comment
@tamird

tamird Apr 1, 2016

Collaborator

Yes, this should be a doc issue now. Improving the implementation is a
worthwhile pursuit, but it's going to be a complete rethinking and deserves
a separate issue.

On Thu, Mar 31, 2016 at 8:49 PM, kena notifications@github.com wrote:

Thanks @tamird https://github.com/tamird for the update.
I think however that the issue is still relevant even if the original test
reports are not up to date. Namely as long as the issue is not fixed there
is still a risk of db consistency errors in the presence of large skews. I
agree however the issue is now somewhat lower priority than initially.

There are two ways forward really:

  • either make skew detection rock solid (which may or may not be
    theoretically possible)
  • document exuberantly that we disclaim any consistency guarantee if
    the users don't run NTP or equivalent clock synchronization with guaranteed
    sub-maxoffset precision.

Should we move this issue to doc?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#4884 (comment)

Collaborator

tamird commented Apr 1, 2016

Yes, this should be a doc issue now. Improving the implementation is a
worthwhile pursuit, but it's going to be a complete rethinking and deserves
a separate issue.

On Thu, Mar 31, 2016 at 8:49 PM, kena notifications@github.com wrote:

Thanks @tamird https://github.com/tamird for the update.
I think however that the issue is still relevant even if the original test
reports are not up to date. Namely as long as the issue is not fixed there
is still a risk of db consistency errors in the presence of large skews. I
agree however the issue is now somewhat lower priority than initially.

There are two ways forward really:

  • either make skew detection rock solid (which may or may not be
    theoretically possible)
  • document exuberantly that we disclaim any consistency guarantee if
    the users don't run NTP or equivalent clock synchronization with guaranteed
    sub-maxoffset precision.

Should we move this issue to doc?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#4884 (comment)

@knz knz added docs-todo and removed C-bug labels Apr 17, 2016

@knz knz assigned jseldess and unassigned knz Apr 17, 2016

@tamird

This comment has been minimized.

Show comment
Hide comment
@tamird

tamird Jul 16, 2016

Collaborator

@jseldess any movement on this one?

Collaborator

tamird commented Jul 16, 2016

@jseldess any movement on this one?

@jseldess

This comment has been minimized.

Show comment
Hide comment
@jseldess

jseldess Jul 18, 2016

Contributor

@tamird, @knz, we do mention clock synchronization in our recommended production settings, but the focus there is on transactions. Should we expand to somehow mention that large skews introduce a risk of db consistency errors? Can either of you help with a more precise message here?

Contributor

jseldess commented Jul 18, 2016

@tamird, @knz, we do mention clock synchronization in our recommended production settings, but the focus there is on transactions. Should we expand to somehow mention that large skews introduce a risk of db consistency errors? Can either of you help with a more precise message here?

@tamird

This comment has been minimized.

Show comment
Hide comment
@tamird

tamird Jul 25, 2016

Collaborator

Yeah that description is entirely too specific.

Run NTP or other clock synchronization software on each machine. CockroachDB needs moderately accurate time; if the machines’ clocks drift too far apart, bad things will happen: in the worst case, consistency guarantees will be lost; in the best case, nodes will self-terminate to protect data consistency.

Collaborator

tamird commented Jul 25, 2016

Yeah that description is entirely too specific.

Run NTP or other clock synchronization software on each machine. CockroachDB needs moderately accurate time; if the machines’ clocks drift too far apart, bad things will happen: in the worst case, consistency guarantees will be lost; in the best case, nodes will self-terminate to protect data consistency.

@jseldess

This comment has been minimized.

Show comment
Hide comment
@jseldess

jseldess Jul 25, 2016

Contributor

Thanks, @tamird. Can you say more about what's actually happening in the best and worse cases?

Contributor

jseldess commented Jul 25, 2016

Thanks, @tamird. Can you say more about what's actually happening in the best and worse cases?

@tamird

This comment has been minimized.

Show comment
Hide comment
@tamird

tamird Jul 25, 2016

Collaborator

Not sure what you mean.

Collaborator

tamird commented Jul 25, 2016

Not sure what you mean.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment