Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
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]>
- Loading branch information
