rpc: enforce max clock offset across nodes in a cluster #9612
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
What's this? Explanation would be good. |
spencerkimball
force-pushed
the
spencerkimball/enforce-max-offset
branch
from
8bb6f0e to
cffa4e6
Compare
spencerkimball
changed the title
|
commit comment got munged as this was targeted to wrong base branch. |
|
LGTM |
tbg
approved these changes
Sep 29, 2016
| // This check is ignored if either offset is set to 0 (for unittests). | ||
| mo, amo := hs.clock.MaxOffset(), time.Duration(args.MaxOffsetNanos) | ||
| if mo != 0 && amo != 0 && mo != amo { | ||
| panic(fmt.Sprintf("clock max offset mismatch node=%s vs %s=%s", mo, args.Addr, amo)) |
There was a problem hiding this comment.
Arguments are out of order. Also, since this might actually be seen by users, I think this would be clearer:
panic(fmt.Sprintf("locally configured maximum clock offset (%s) does not match that of node %d (%s)", mo, args.Addr, amo))Commits suicide if offset is different than any contacted node. In order to change the max clock offset, the cluster must be stopped and restarted.
spencerkimball
force-pushed
the
spencerkimball/enforce-max-offset
branch
from
cffa4e6 to
8a9bda9
Compare
Closed
nvanbenschoten
added a commit
to nvanbenschoten/cockroach
that referenced
this pull request
Aug 12, 2022
…ster Fixes cockroachdb#66868. Before this commit, it was not possible to change the maximum clock offset of a cluster without shutting down all nodes in the cluster and bringing them all back up with a new `--max-offset` flag. This was enforced by protection added in cockroachdb#9612, which would crash a node if it detected different settings on different nodes. A stop-the-world cluster restart is quite disruptive and prevents us from changing the default value of the `--max-offset` flag between releases of CockroachDB or on existing CC clusters. This commit removes this protection. The change itself is trivial. The real work is convincing ourselves that it is safe to run in a mixed max-offset cluster. We do that below. In the following table, we detail each use of `--max-offset`. We then explore the hazards of running in a cluster with mixed `--max-offset` settings. \### Uses of `--max-offset` | Use | Description | | ----------- | ----------- | | `kv.NewTxn` | Used to configure the transaction's global uncertainty limit | | `computeIntervalForNonTxn` | Used to configure the non-transacitonal request's global uncertainty limit | | `TxnCoordSender.maybeCommitWait` | **When in "linearizable" mode**, used to sleep until all clocks beyond local HLC | | `Txn.DeadlineLikelySufficient` | Used to estimate whether the current txn deadline is sufficiently far in the future | | `closedts.TargetForPolicy` | Used to estimate lead time for global tables | | `Replica.leaseStatus` | Used to determine the start of the stasis period for leases | | `startupmigrations.LeaseManager.timeRemaining` | Used to determine the time remaining on a migration lease | | `RemoteClockMonitor.VerifyClockOffset` | Used to detect clock synchronization errors | \### Hazards of mixed `--max-offset` | Use | skew < min(max-offset) | min(max-offset) < skew < max(max-offset) | max(max-offset) < skew | | ----------- | ----------- | ----------- | ----------- | | `kv.NewTxn` | N/A | Stale reads | Stale reads | | `computeIntervalForNonTxn` | N/A | Stale reads | Stale reads | | `TxnCoordSender.maybeCommitWait` | N/A | Causal reverse | Causal reverse | | `Txn.DeadlineLikelySufficient` | N/A | Deadline exceeded retry errors | Deadline exceeded retry errors | | `closedts.TargetForPolicy` | N/A | Global table reads redirect to leaseholder | Global table reads redirect to leaseholder | | `Replica.leaseStatus` | N/A | Stale reads for non-txn requests | Stale reads for non-txn requests | | `startupmigrations.LeaseManager.timeRemaining` | N/A | Lease replaced unexpectedly, [error](https://github.com/cockroachdb/cockroach/blob/8e3ee57f3c8ea734c7221ff4c758c91cd928b6d3/pkg/startupmigrations/leasemanager/lease.go#L167) | Lease replaced unexpectedly, [error](https://github.com/cockroachdb/cockroach/blob/8e3ee57f3c8ea734c7221ff4c758c91cd928b6d3/pkg/startupmigrations/leasemanager/lease.go#L167) | | `RemoteClockMonitor.VerifyClockOffset` | N/A | Nodes in minority self-terminate **if their own max-offset < skew** | Nodes in minority self-terminate | Notice that in all cases except the last, the outcome of `min(max-offset) < skew < max(max-offset)` and `max(max-offset) < skew` are identical. This means that, ignoring the last use, the hazards of `max-offset < skew` in a cluster with a homogeneous setting for `--max-offset` is the same as the hazard of `min(max-offset) < skew` in a cluster with a heterogeneous setting for `--max-offset`. \#### Use in `Replica.leaseStatus` The use in `Replica.leaseStatus` of the max clock offset to determine whether a request falls into a lease's stasis period is a subtle case here, so it deserves a bit more explanation. As a reminder, the lease stasis period is described [here](https://github.com/cockroachdb/cockroach/blob/8e3ee57f3c8ea734c7221ff4c758c91cd928b6d3/pkg/kv/kvserver/kvserverpb/lease_status.proto#L27). Consider the case where `min(max-offset) < skew < max(max-offset)` and a request is sent to an outgoing leaseholder near the end of its lease. The interesting case is where the outgoing leaseholder has a smaller max-offset than the skew. In such cases, it may not consider a non-transaction request to be in its stasis period even though a different replica saw the lease as expired, requested a new lease, and served a write. This could allow the non-transactional request to serve a stale read. Transactional requests are [not subject](https://github.com/cockroachdb/cockroach/blob/8e3ee57f3c8ea734c7221ff4c758c91cd928b6d3/pkg/kv/kvserver/replica_range_lease.go#L611) to the stasis period, so they are not discussed in this example. However, the effect of `min(max-offset) < skew < max(max-offset)` is the same — stale reads. \#### Use in `RemoteClockMonitor.VerifyClockOffset` The use of `RemoteClockMonitor.VerifyClockOffset` also deserves discussion because it's the one case where mixed `--max-offset` values make a difference. If the nodes with skew (i.e. those in the minority) have lower `--max-offset` values, they will self-terminate. However, if the nodes with skew have higher `--max-offset` values, they will not self-terminate. This could leave the cluster open to the other hazards indefinitely, without the skewed nodes ever deciding to self-terminate. For instance, these skewed nodes could continue to perform writes at high timestamps, causing other nodes with lower `--max-offset` values to serve stale reads. One option I explored to address this was to have `RemoteClockMonitor.VerifyClockOffset` use the minimum max offset across the cluster instead of its local max offset when verifying clocks. This would be possible by using the `OriginMaxOffsetNanos` value communicated in each `PingRequest`. I decided against this for now in favor of keeping things simple, as it had some rough edges around ignoring stale max-offset values. It's also not clear how this protection will work in a world with dynamic clock uncertainty error bounds. ---- After this lands, we'll need to update the following text in the docs: ``` Note that this value must be the same on all nodes in the cluster and cannot be changed with a rolling upgrade. In order to change it, first stop every node in the cluster. Then once the entire cluster is offline, restart each node with the new value. ``` Release note (ops change): clusters can now run nodes with different --max-offset settings at the same time. This enables operators to perform a rolling restart to change the value of each node's --max-offset flag.
craig bot
pushed a commit
that referenced
this pull request
Aug 13, 2022
85983: kv: permit different max clock offsets on different nodes in same cluster r=nvanbenschoten a=nvanbenschoten Fixes #66868. Before this commit, it was not possible to change the maximum clock offset of a cluster without shutting down all nodes in the cluster and bringing them all back up with a new `--max-offset` flag. This was enforced by protection added in #9612, which would crash a node if it detected different settings on different nodes. A stop-the-world cluster restart is quite disruptive and prevents us from changing the default value of the `--max-offset` flag between releases of CockroachDB or on existing CC clusters. This commit removes this protection. The change itself is trivial. The real work is convincing ourselves that it is safe to run in a mixed max-offset cluster. We do that below. In the following table, we detail each use of `--max-offset`. We then explore the hazards of running in a cluster with mixed `--max-offset` settings. ### Uses of `--max-offset` | Use | Description | | ----------- | ----------- | | `kv.NewTxn` | Used to configure the transaction's global uncertainty limit | | `computeIntervalForNonTxn` | Used to configure the non-transacitonal request's global uncertainty limit | | `TxnCoordSender.maybeCommitWait` | **When in "linearizable" mode**, used to sleep until all clocks beyond local HLC | | `Txn.DeadlineLikelySufficient` | Used to estimate whether the current txn deadline is sufficiently far in the future | | `closedts.TargetForPolicy` | Used to estimate lead time for global tables | | `Replica.leaseStatus` | Used to determine the start of the stasis period for leases | | `startupmigrations.LeaseManager.timeRemaining` | Used to determine the time remaining on a migration lease | | `RemoteClockMonitor.VerifyClockOffset` | Used to detect clock synchronization errors | ### Hazards of mixed `--max-offset` | Use | skew < min(max-offset) | min(max-offset) < skew < max(max-offset) | max(max-offset) < skew | | ----------- | ----------- | ----------- | ----------- | | `kv.NewTxn` | N/A | Stale reads | Stale reads | | `computeIntervalForNonTxn` | N/A | Stale reads | Stale reads | | `TxnCoordSender.maybeCommitWait` | N/A | Causal reverse | Causal reverse | | `Txn.DeadlineLikelySufficient` | N/A | Deadline exceeded retry errors | Deadline exceeded retry errors | | `closedts.TargetForPolicy` | N/A | Global table reads redirect to leaseholder | Global table reads redirect to leaseholder | | `Replica.leaseStatus` | N/A | Stale reads for non-txn requests | Stale reads for non-txn requests | | `startupmigrations.LeaseManager.timeRemaining` | N/A | Lease replaced unexpectedly, [error](https://github.com/cockroachdb/cockroach/blob/8e3ee57f3c8ea734c7221ff4c758c91cd928b6d3/pkg/startupmigrations/leasemanager/lease.go#L167) | Lease replaced unexpectedly, [error](https://github.com/cockroachdb/cockroach/blob/8e3ee57f3c8ea734c7221ff4c758c91cd928b6d3/pkg/startupmigrations/leasemanager/lease.go#L167) | | `RemoteClockMonitor.VerifyClockOffset` | N/A | Nodes in minority self-terminate **if their own max-offset < skew** | Nodes in minority self-terminate | Notice that in all cases except the last, the outcome of `min(max-offset) < skew < max(max-offset)` and `max(max-offset) < skew` are identical. This means that, ignoring the last use, the hazards of `max-offset < skew` in a cluster with a homogeneous setting for `--max-offset` is the same as the hazard of `min(max-offset) < skew` in a cluster with a heterogeneous setting for `--max-offset`. As a result, we can consider `min(max-offset)` to be the "effective" max-offset of a cluster. #### Use in `Replica.leaseStatus` The use in `Replica.leaseStatus` of the max clock offset to determine whether a request falls into a lease's stasis period is a subtle case here, so it deserves a bit more explanation. As a reminder, the lease stasis period is described [here](https://github.com/cockroachdb/cockroach/blob/8e3ee57f3c8ea734c7221ff4c758c91cd928b6d3/pkg/kv/kvserver/kvserverpb/lease_status.proto#L27). Consider the case where `min(max-offset) < skew < max(max-offset)` and a request is sent to an outgoing leaseholder near the end of its lease. The interesting case is where the outgoing leaseholder has a smaller max-offset than the skew. In such cases, it may not consider a non-transaction request to be in its stasis period even though a different replica saw the lease as expired, requested a new lease, and served a write. This could allow the non-transactional request to serve a stale read. Transactional requests are [not subject](https://github.com/cockroachdb/cockroach/blob/8e3ee57f3c8ea734c7221ff4c758c91cd928b6d3/pkg/kv/kvserver/replica_range_lease.go#L611) to the stasis period, so they are not discussed in this example. However, the effect of `min(max-offset) < skew < max(max-offset)` is the same — stale reads. #### Use in `RemoteClockMonitor.VerifyClockOffset` The use of `RemoteClockMonitor.VerifyClockOffset` also deserves discussion because it's the one case where mixed `--max-offset` values make a difference. If the nodes with skew (i.e. those in the minority) have lower `--max-offset` values, they will self-terminate. However, if the nodes with skew have higher `--max-offset` values, they will not self-terminate. This could leave the cluster open to the other hazards indefinitely, without the skewed nodes ever deciding to self-terminate. For instance, these skewed nodes could continue to perform writes at high timestamps, causing other nodes with lower `--max-offset` values to serve stale reads. One option I explored to address this was to have `RemoteClockMonitor.VerifyClockOffset` use the minimum max offset across the cluster instead of its local max offset when verifying clocks. This would be possible by using the `OriginMaxOffsetNanos` value communicated in each `PingRequest`. I decided against this for now in favor of keeping things simple, as it had some rough edges around ignoring stale max-offset values. It's also not clear how this protection will work in a world with dynamic clock uncertainty error bounds. ---- After this lands, we'll need to remove the following text in the docs, which was added in cockroachdb/docs#10988 and in other PRs: ``` Note that this value must be the same on all nodes in the cluster and cannot be changed with a rolling upgrade. In order to change it, first stop every node in the cluster. Then once the entire cluster is offline, restart each node with the new value. ``` and ``` For existing clusters, changing the setting will require restarting all of the nodes in your cluster at the same time; it cannot be done with a rolling restart. ``` ---- Release note (ops change): clusters can now run nodes with different --max-offset settings at the same time. This enables operators to perform a rolling restart to change the value of each node's --max-offset flag. Co-authored-by: Nathan VanBenschoten <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Commits suicide if offset is different than any contacted node. In order to
change the max clock offset, the cluster must be stopped and restarted.
This change is