Skip to content
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.

Sign up for GitHub

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

Jump to bottom

kv: test leader leases #132762

Open
2 of 3 tasks
nvanbenschoten opened this issue Oct 16, 2024 · 0 comments
Open
2 of 3 tasks

kv: test leader leases #132762

nvanbenschoten opened this issue Oct 16, 2024 · 0 comments
Assignees
@nvanbenschoten
Labels
A-kv-replication Relating to Raft, consensus, and coordination. A-leader-leases Related to the introduction of leader leases C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-kv KV Team

Comments

@nvanbenschoten
Copy link
Member

nvanbenschoten commented Oct 16, 2024 •
edited
Loading

Part of #123847.

Now that we support leader leases, we should throw the full test suite at them. Specifically, we should use leader leases in:

Jira issue: CRDB-43269

Epic CRDB-37522
@nvanbenschoten nvanbenschoten added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) A-kv-replication Relating to Raft, consensus, and coordination. T-kv KV Team A-leader-leases Related to the introduction of leader leases labels Oct 16, 2024
@nvanbenschoten nvanbenschoten mentioned this issue Oct 16, 2024
Closed
2 tasks
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue Oct 23, 2024
@nvanbenschoten
roachtest: add failover variants with leader leases
961c233 
Part of cockroachdb#132762.

Leader leases have different availability properties than epoch leases under
most failure modes. This patch adds failover test variants that use leader
leases where possible.

Initial test results:

| test                                         | lease=epoch (ms) | lease=expiration (ms) | lease=leader (ms) | parity with expiration |
|:---------------------------------------------|-----------------:|----------------------:|------------------:|:----------------------:|
| failover/chaos/read-only                     | 60,129           | 18,253                | 60,129            | ✔                      |
| failover/chaos/read-write                    | 60,129           | 20,401                | 60,129            | XXX                    |
| failover/liveness/blackhole                  | 9,663            | 369                   | 335               | ✔                      |
| failover/liveness/blackhole-recv             | 11,274           | 402                   | 369               | ✔                      |
| failover/liveness/blackhole-send             | 9,663            | 385                   | 469               | ✔                      |
| failover/liveness/crash                      | 8,053            | 352                   | 318               | ✔                      |
| failover/liveness/deadlock                   | 24,696           | 385                   | 369               | ✔                      |
| failover/liveness/disk-stall                 | 26,843           | 369                   | 419               | ✔                      |
| failover/liveness/pause                      | 10,200           | 385                   | 436               | ✔                      |
| failover/non-system/blackhole                | 7,247            | 7,516                 | 15,032            | XX                     |
| failover/non-system/blackhole-recv           | 12,348           | 10,737                | 18,253            | XX                     |
| failover/non-system/blackhole-send           | 6,979            | 6,979                 | 8,053             | X                      |
| failover/non-system/crash                    | 7,247            | 6,979                 | 9,126             | X                      |
| failover/non-system/deadlock                 | 60,129           | 60,129                | 60,129            | ✔                      |
| failover/non-system/disk-stall               | 22,548           | 22,548                | 25,769            | X                      |
| failover/non-system/pause                    | 7,247            | 7,247                 | 9,126             | X                      |
| failover/partial/lease-gateway               | 8,589            | 19,327 [^1]           | 60,129            | XXX                    |
| failover/partial/lease-leader                | 60,129           | 22,549 [^2]           | 31,139 [^2]       | XX                     |
| failover/partial/lease-liveness              | 8,589            | 301                   | 318               | ✔                      |
| failover/system-non-liveness/blackhole       | 369              | 402                   | 352               | ✔                      |
| failover/system-non-liveness/blackhole-recv  | 335              | 285                   | 318               | ✔                      |
| failover/system-non-liveness/blackhole-send  | 402              | 419                   | 335               | ✔                      |
| failover/system-non-liveness/crash           | 419              | 301                   | 453               | ✔                      |
| failover/system-non-liveness/deadlock        | 369              | 352                   | 402               | ✔                      |
| failover/system-non-liveness/disk-stall      | 402              | 318                   | 453               | ✔                      |
| failover/system-non-liveness/pause           | 369              | 385                   | 335               | ✔                      |

_note: because of the way the test measures pMax, anything under 1,000ms is essentially "no impact"_

**Key _(comparing leader vs. expiration)_**:
✔ = parity
X = minor regression
XX = major regression
XXX = unavailability

[^1]: I don't understand why expiration-based lease perform worse than epoch-based leases on this test.
[^2]: With cockroachdb#133214.

Epic: none
Release note: None
@nvanbenschoten nvanbenschoten mentioned this issue Oct 23, 2024
Merged
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue Oct 23, 2024
@nvanbenschoten
roachtest: add failover variants with leader leases
772f567 
Part of cockroachdb#132762.

Leader leases have different availability properties than epoch leases under
most failure modes. This patch adds failover test variants that use leader
leases where possible.

Initial test results:

| test                                         | lease=epoch (ms) | lease=expiration (ms) | lease=leader (ms) | parity with expiration |
|:---------------------------------------------|-----------------:|----------------------:|------------------:|:----------------------:|
| failover/chaos/read-only                     | 60,129           | 18,253                | 60,129            | ✔                      |
| failover/chaos/read-write                    | 60,129           | 20,401                | 60,129            | XXX                    |
| failover/liveness/blackhole                  | 9,663            | 369                   | 335               | ✔                      |
| failover/liveness/blackhole-recv             | 11,274           | 402                   | 369               | ✔                      |
| failover/liveness/blackhole-send             | 9,663            | 385                   | 469               | ✔                      |
| failover/liveness/crash                      | 8,053            | 352                   | 318               | ✔                      |
| failover/liveness/deadlock                   | 24,696           | 385                   | 369               | ✔                      |
| failover/liveness/disk-stall                 | 26,843           | 369                   | 419               | ✔                      |
| failover/liveness/pause                      | 10,200           | 385                   | 436               | ✔                      |
| failover/non-system/blackhole                | 7,247            | 7,516                 | 15,032            | XX                     |
| failover/non-system/blackhole-recv           | 12,348           | 10,737                | 18,253            | XX                     |
| failover/non-system/blackhole-send           | 6,979            | 6,979                 | 8,053             | X                      |
| failover/non-system/crash                    | 7,247            | 6,979                 | 9,126             | X                      |
| failover/non-system/deadlock                 | 60,129           | 60,129                | 60,129            | ✔                      |
| failover/non-system/disk-stall               | 22,548           | 22,548                | 25,769            | X                      |
| failover/non-system/pause                    | 7,247            | 7,247                 | 9,126             | X                      |
| failover/partial/lease-gateway               | 8,589            | 19,327 [^1]           | 60,129            | XXX                    |
| failover/partial/lease-leader                | 60,129           | 22,549 [^2]           | 31,139 [^2]       | XX                     |
| failover/partial/lease-liveness              | 8,589            | 301                   | 318               | ✔                      |
| failover/system-non-liveness/blackhole       | 369              | 402                   | 352               | ✔                      |
| failover/system-non-liveness/blackhole-recv  | 335              | 285                   | 318               | ✔                      |
| failover/system-non-liveness/blackhole-send  | 402              | 419                   | 335               | ✔                      |
| failover/system-non-liveness/crash           | 419              | 301                   | 453               | ✔                      |
| failover/system-non-liveness/deadlock        | 369              | 352                   | 402               | ✔                      |
| failover/system-non-liveness/disk-stall      | 402              | 318                   | 453               | ✔                      |
| failover/system-non-liveness/pause           | 369              | 385                   | 335               | ✔                      |

_note: because of the way the test measures pMax, anything under 1,000ms is essentially "no impact"_

**Key _(comparing leader vs. expiration)_**:
✔ = parity
X = minor regression
XX = major regression
XXX = unavailability

[^1]: I don't understand why expiration-based lease perform worse than epoch-based leases on this test.
[^2]: With cockroachdb#133214.

Epic: none
Release note: None
@nvanbenschoten nvanbenschoten mentioned this issue Oct 23, 2024
Merged
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue Oct 23, 2024
@nvanbenschoten
roachtest: metamorphically enable leader leases
31cbe20 
Part of cockroachdb#132762.

To increase test coverage of leader leases, this patch adds leader leases
to the set of possible lease types when tests opt-in to metamorphic leases.
As of fc68b0f, most roachtests do enable metamorphic leases, so this will
provide good coverage of leader leases.

Release note: None
@nvanbenschoten nvanbenschoten mentioned this issue Oct 23, 2024
Merged
craig bot pushed a commit that referenced this issue Oct 24, 2024
@nvanbenschoten @michae2
Merge #131624 #133214 #133281
4efd2b4 
131624: roachtest: add `failover` variants with leader leases r=nvanbenschoten a=nvanbenschoten

Part of #132762.

Leader leases have different availability properties than epoch leases under most failure modes. This patch adds failover test variants that use leader leases where possible.

Initial test results:

| test                                         | lease=epoch (ms) | lease=expiration (ms) | lease=leader (ms) | parity with expiration |
|:---------------------------------------------|-----------------:|----------------------:|------------------:|:----------------------:|
| failover/chaos/read-only                     | 60,129           | 18,253                | 60,129            | ✔                      |
| failover/chaos/read-write                    | 60,129           | 20,401                | 60,129            | ❌❌❌                    |
| failover/liveness/blackhole                  | 9,663            | 369                   | 335               | ✔                      |
| failover/liveness/blackhole-recv             | 11,274           | 402                   | 369               | ✔                      |
| failover/liveness/blackhole-send             | 9,663            | 385                   | 469               | ✔                      |
| failover/liveness/crash                      | 8,053            | 352                   | 318               | ✔                      |
| failover/liveness/deadlock                   | 24,696           | 385                   | 369               | ✔                      |
| failover/liveness/disk-stall                 | 26,843           | 369                   | 419               | ✔                      |
| failover/liveness/pause                      | 10,200           | 385                   | 436               | ✔                      |
| failover/non-system/blackhole                | 7,247            | 7,516                 | 15,032            | ❌❌                     |
| failover/non-system/blackhole-recv           | 12,348           | 10,737                | 18,253            | ❌❌                     |
| failover/non-system/blackhole-send           | 6,979            | 6,979                 | 8,053             | ❌                      |
| failover/non-system/crash                    | 7,247            | 6,979                 | 9,126             | ❌                      |
| failover/non-system/deadlock                 | 60,129           | 60,129                | 60,129            | ✔                      |
| failover/non-system/disk-stall               | 22,548           | 22,548                | 25,769            | ❌                      |
| failover/non-system/pause                    | 7,247            | 7,247                 | 9,126             | ❌                      |
| failover/partial/lease-gateway               | 8,589            | 19,327 [^1]           | 60,129            | ❌❌❌                    |
| failover/partial/lease-leader                | 60,129           | 22,549 [^2]           | 31,139 [^2]       | ❌❌                     |
| failover/partial/lease-liveness              | 8,589            | 301                   | 318               | ✔                      |
| failover/system-non-liveness/blackhole       | 369              | 402                   | 352               | ✔                      |
| failover/system-non-liveness/blackhole-recv  | 335              | 285                   | 318               | ✔                      |
| failover/system-non-liveness/blackhole-send  | 402              | 419                   | 335               | ✔                      |
| failover/system-non-liveness/crash           | 419              | 301                   | 453               | ✔                      |
| failover/system-non-liveness/deadlock        | 369              | 352                   | 402               | ✔                      |
| failover/system-non-liveness/disk-stall      | 402              | 318                   | 453               | ✔                      |
| failover/system-non-liveness/pause           | 369              | 385                   | 335               | ✔                      |

_note: because of the way the test measures pMax, anything under 1,000ms is essentially "no impact"_

**Key _(comparing leader vs. expiration)_**:
✔ = parity
❌ = minor regression
❌❌ = major regression
❌❌❌ = unavailability

[^1]: I don't understand why expiration-based lease perform worse than epoch-based leases on this test.
[^2]: With #133214.

Epic: none
Release note: None

133214: roachtest: enable DistSender circuit breakers in failover/partial/lease-leader r=nvanbenschoten a=nvanbenschoten

DistSender circuit breakers are useful in this test to avoid artificially inflated latencies due to the way the test measures failover time (pMax, no timeouts). Without circuit breakers, a request stuck on the partitioned leaseholder will get blocked indefinitely, despite the range recovering on the other side of the partition and becoming available to all new traffic. As a result, the test won't differentiate between temporary and permanent range unavailability. We have other tests which demonstrate the benefit of DistSender circuit breakers (especially when applications do not use statement timeouts), so we don't need to test them here.

With this change, the test's meaured failover time drops from:

| lease=epoch (ms) | lease=expiration (ms) | lease=leader (ms) |
|-----------------:|----------------------:|------------------:|
| 60,129           | 60,129                | 60,129            |

down to:

| lease=epoch (ms) | lease=expiration (ms) | lease=leader (ms) |
|-----------------:|----------------------:|------------------:|
| 60,129           | 22,549                | 31,139            |

This is because the circuit breakers place a 10s timeout on all KV requests, so no request gets stuck indefinitely. Notice that expiration and leader leases now recover, while epoch leases remain unavailable indefinitely.

Epic: None
Release note: None

133281: sql: allow old enum value '1' for sql.defaults.vectorize r=mw5h a=michae2

Fixes: #133278

Release note (bug fix): This commit fixes a bug which causes new connections to fail with the following error after upgrading to v24.2:

```
ERROR: invalid value for parameter "vectorize": "unknown(1)"
SQLSTATE: 22023
HINT: Available values: off,on,experimental_always
```

In order to hit this bug, the cluster must have:
1. been on version v21.1 at some point in the past
2. run `SET CLUSTER SETTING sql.defaults.vectorize = 'on';` on v21.1
3. not set sql.defaults.vectorize after upgrading past v21.1
4. upgraded all the way to v24.2

The conditions required for this bug can be detected using:

```
SELECT * FROM system.settings WHERE name = 'sql.defaults.vectorize';
```

If the value is '1', the following statement should be run to fix it before upgrading to v24.2:

```
RESET CLUSTER SETTING sql.defaults.vectorize;
```

This commit fixes the bug by making '1' a legal value for sql.defaults.vectorize again (mapping to 'on').

Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Co-authored-by: Michael Erickson <michae2@cockroachlabs.com>
blathers-crl bot pushed a commit that referenced this issue Oct 24, 2024
@nvanbenschoten
roachtest: add failover variants with leader leases
dee8143 
Part of #132762.

Leader leases have different availability properties than epoch leases under
most failure modes. This patch adds failover test variants that use leader
leases where possible.

Initial test results:

| test                                         | lease=epoch (ms) | lease=expiration (ms) | lease=leader (ms) | parity with expiration |
|:---------------------------------------------|-----------------:|----------------------:|------------------:|:----------------------:|
| failover/chaos/read-only                     | 60,129           | 18,253                | 60,129            | ✔                      |
| failover/chaos/read-write                    | 60,129           | 20,401                | 60,129            | XXX                    |
| failover/liveness/blackhole                  | 9,663            | 369                   | 335               | ✔                      |
| failover/liveness/blackhole-recv             | 11,274           | 402                   | 369               | ✔                      |
| failover/liveness/blackhole-send             | 9,663            | 385                   | 469               | ✔                      |
| failover/liveness/crash                      | 8,053            | 352                   | 318               | ✔                      |
| failover/liveness/deadlock                   | 24,696           | 385                   | 369               | ✔                      |
| failover/liveness/disk-stall                 | 26,843           | 369                   | 419               | ✔                      |
| failover/liveness/pause                      | 10,200           | 385                   | 436               | ✔                      |
| failover/non-system/blackhole                | 7,247            | 7,516                 | 15,032            | XX                     |
| failover/non-system/blackhole-recv           | 12,348           | 10,737                | 18,253            | XX                     |
| failover/non-system/blackhole-send           | 6,979            | 6,979                 | 8,053             | X                      |
| failover/non-system/crash                    | 7,247            | 6,979                 | 9,126             | X                      |
| failover/non-system/deadlock                 | 60,129           | 60,129                | 60,129            | ✔                      |
| failover/non-system/disk-stall               | 22,548           | 22,548                | 25,769            | X                      |
| failover/non-system/pause                    | 7,247            | 7,247                 | 9,126             | X                      |
| failover/partial/lease-gateway               | 8,589            | 19,327 [^1]           | 60,129            | XXX                    |
| failover/partial/lease-leader                | 60,129           | 22,549 [^2]           | 31,139 [^2]       | XX                     |
| failover/partial/lease-liveness              | 8,589            | 301                   | 318               | ✔                      |
| failover/system-non-liveness/blackhole       | 369              | 402                   | 352               | ✔                      |
| failover/system-non-liveness/blackhole-recv  | 335              | 285                   | 318               | ✔                      |
| failover/system-non-liveness/blackhole-send  | 402              | 419                   | 335               | ✔                      |
| failover/system-non-liveness/crash           | 419              | 301                   | 453               | ✔                      |
| failover/system-non-liveness/deadlock        | 369              | 352                   | 402               | ✔                      |
| failover/system-non-liveness/disk-stall      | 402              | 318                   | 453               | ✔                      |
| failover/system-non-liveness/pause           | 369              | 385                   | 335               | ✔                      |

_note: because of the way the test measures pMax, anything under 1,000ms is essentially "no impact"_

**Key _(comparing leader vs. expiration)_**:
✔ = parity
X = minor regression
XX = major regression
XXX = unavailability

[^1]: I don't understand why expiration-based lease perform worse than epoch-based leases on this test.
[^2]: With #133214.

Epic: none
Release note: None
@blathers-crl blathers-crl bot mentioned this issue Oct 24, 2024
Merged
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue Oct 28, 2024
@nvanbenschoten
kv: redirect requests to raft leader who holds leader lease
861daf0 
Informs cockroachdb#132762.

This commit updates the lease status logic to redirect requests to the raft
leader who holds a leader lease when evaluating a request on a follower.
Previously, the follower would return a NotLeaseHolderError, but it would not
include the lease in the response, so the client would not be immediately
redirected to the leader. Instead, it would continue trying each replica in
order, eventually throwing a `sending to all replicas failed` error.

Furthermore, since no lease was included, request proxying would never be used.
This explains why the `failover/partial/lease-gateway` test was failing to
recover. With this change, the test now observes recovery in in TODOs.

Release note: None
@nvanbenschoten nvanbenschoten mentioned this issue Oct 28, 2024
Merged
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue Oct 28, 2024
@nvanbenschoten
kv: redirect requests to raft leader who holds leader lease
ac77412 
Informs cockroachdb#132762.

This commit updates the lease status logic to redirect requests to the raft
leader who holds a leader lease when evaluating a request on a follower.
Previously, the follower would return a NotLeaseHolderError, but it would not
include the lease in the response, so the client would not be immediately
redirected to the leader. Instead, it would continue trying each replica in
order, eventually throwing a `sending to all replicas failed` error.

Furthermore, since no lease was included, request proxying would never be used.
This explains why the `failover/partial/lease-gateway` test was failing to
recover. With this change, the test now observes recovery in in 7.650s.

Release note: None
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue Oct 28, 2024
@nvanbenschoten
roachtest: disable "deadlock" failure in chaos tests
4c52ba1 
Informs cockroachdb#132762.

This commit disables the replica deadlock failure mode in chaos tests.
We don't want to run this form of failure in the chaos tests because we
do not expect cockroach to recover from it and do not want to lose
signal for the chaos tests. The deadlock failure mode continues to be
used in its own individual test.

None of the three forms of leases (expiration, epoch, and leader) and
survive a replica deadlock under default configuration. Epoch and leader
leases cannot survive it by design without the introduction of a replica
watchdog. Expiration leases can survive it if combined with DistSender
circuit breakers, but those are not yet enabled by default, even though
we do enable them in the chaos tests.

Release note: None
@nvanbenschoten nvanbenschoten mentioned this issue Oct 28, 2024
Merged
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue Oct 28, 2024
@nvanbenschoten
kv: redirect requests to raft leader who holds leader lease
30c6b1b 
Informs cockroachdb#132762.

This commit updates the lease status logic to redirect requests to the raft
leader who holds a leader lease when evaluating a request on a follower.
Previously, the follower would return a NotLeaseHolderError, but it would not
include the lease in the response, so the client would not be immediately
redirected to the leader. Instead, it would continue trying each replica in
order, eventually throwing a `sending to all replicas failed` error.

Furthermore, since no lease was included, request proxying would never be used.
This explains why the `failover/partial/lease-gateway` test was failing to
recover. With this change, the test now observes recovery in in 7.650s.

To demonstrate that this change works as expected, the commit also ports over
two unit test that exercise request proxying to run with leader leases. Without
this change, they fail. With it, they pass.

Release note: None
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue Oct 28, 2024
@nvanbenschoten
roachtest: disable "deadlock" failure in chaos tests
9288116 
Informs cockroachdb#132762.

This commit disables the replica deadlock failure mode in chaos tests.
We don't want to run this form of failure in the chaos tests because we
do not expect cockroach to recover from it and do not want to lose
signal for the chaos tests. The deadlock failure mode continues to be
used in its own individual test.

None of the three forms of leases (expiration, epoch, and leader) can
survive a replica deadlock under default configuration. Epoch and leader
leases cannot survive it by design without the introduction of a replica
watchdog. Expiration leases can survive it if combined with DistSender
circuit breakers, but those are not yet enabled by default, even though
we do enable them in the chaos tests.

Release note: None
craig bot pushed a commit that referenced this issue Oct 28, 2024
@nvanbenschoten
Merge #133594
66a4a76 
133594: roachtest: disable "deadlock" failure in chaos tests r=nvanbenschoten a=nvanbenschoten

Informs #132762.

This commit disables the replica deadlock failure mode in chaos tests. We don't want to run this form of failure in the chaos tests because we do not expect cockroach to recover from it and do not want to lose signal for the chaos tests. The deadlock failure mode continues to be used in its own individual test.

None of the three forms of leases (expiration, epoch, and leader) can survive a replica deadlock under default configuration. Epoch and leader leases cannot survive it by design without the introduction of a replica watchdog. Expiration leases can survive it if combined with DistSender circuit breakers, but those are not yet enabled by default, even though we do enable them in the chaos tests.

Release note: None

Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
blathers-crl bot pushed a commit that referenced this issue Oct 28, 2024
@nvanbenschoten
roachtest: disable "deadlock" failure in chaos tests
53a9e5d 
Informs #132762.

This commit disables the replica deadlock failure mode in chaos tests.
We don't want to run this form of failure in the chaos tests because we
do not expect cockroach to recover from it and do not want to lose
signal for the chaos tests. The deadlock failure mode continues to be
used in its own individual test.

None of the three forms of leases (expiration, epoch, and leader) can
survive a replica deadlock under default configuration. Epoch and leader
leases cannot survive it by design without the introduction of a replica
watchdog. Expiration leases can survive it if combined with DistSender
circuit breakers, but those are not yet enabled by default, even though
we do enable them in the chaos tests.

Release note: None
@blathers-crl blathers-crl bot mentioned this issue Oct 28, 2024
Merged
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue Oct 29, 2024
@nvanbenschoten
kv: redirect requests to raft leader who holds leader lease
85a5001 
Informs cockroachdb#132762.

This commit updates the lease status logic to redirect requests to the raft
leader who holds a leader lease when evaluating a request on a follower.
Previously, the follower would return a NotLeaseHolderError, but it would not
include the lease in the response, so the client would not be immediately
redirected to the leader. Instead, it would continue trying each replica in
order, eventually throwing a `sending to all replicas failed` error.

Furthermore, since no lease was included, request proxying would never be used.
This explains why the `failover/partial/lease-gateway` test was failing to
recover. With this change, the test now observes recovery in in 7.650s.

To demonstrate that this change works as expected, the commit also ports over
two unit test that exercise request proxying to run with leader leases. Without
this change, they fail. With it, they pass.

Release note: None
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue Oct 29, 2024
@nvanbenschoten
roachtest: metamorphically enable leader leases
21a6e6b 
Part of cockroachdb#132762.

To increase test coverage of leader leases, this patch adds leader leases
to the set of possible lease types when tests opt-in to metamorphic leases.
As of fc68b0f, most roachtests do enable metamorphic leases, so this will
provide good coverage of leader leases.

Release note: None
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue Oct 29, 2024
@nvanbenschoten
kv: redirect requests to raft leader who holds leader lease
27faf02 
Informs cockroachdb#132762.

This commit updates the lease status logic to redirect requests to the raft
leader who holds a leader lease when evaluating a request on a follower.
Previously, the follower would return a NotLeaseHolderError, but it would not
include the lease in the response, so the client would not be immediately
redirected to the leader. Instead, it would continue trying each replica in
order, eventually throwing a `sending to all replicas failed` error.

Furthermore, since no lease was included, request proxying would never be used.
This explains why the `failover/partial/lease-gateway` test was failing to
recover. With this change, the test now observes recovery in in 7.650s.

To demonstrate that this change works as expected, the commit also ports over
two unit test that exercise request proxying to run with leader leases. Without
this change, they fail. With it, they pass.

Release note: None
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue Oct 29, 2024
@nvanbenschoten
kv: redirect requests to raft leader who holds leader lease
c22dc13 
Informs cockroachdb#132762.

This commit updates the lease status logic to redirect requests to the raft
leader who holds a leader lease when evaluating a request on a follower.
Previously, the follower would return a NotLeaseHolderError, but it would not
include the lease in the response, so the client would not be immediately
redirected to the leader. Instead, it would continue trying each replica in
order, eventually throwing a `sending to all replicas failed` error.

Furthermore, since no lease was included, request proxying would never be used.
This explains why the `failover/partial/lease-gateway` test was failing to
recover. With this change, the test now observes recovery in in 7.650s.

To demonstrate that this change works as expected, the commit also ports over
two unit test that exercise request proxying to run with leader leases. Without
this change, they fail. With it, they pass.

Release note: None
craig bot pushed a commit that referenced this issue Oct 29, 2024
@DrewKimball @andrewbaptist
@nvanbenschoten @spilchen
Merge #132147 #133365 #133573 #133691
ba71133 
132147: sql: use memo metadata to add routines and UDTs to statement bundles r=DrewKimball a=DrewKimball

#### opt: replace "udf" with "routine" in the metadata

This commit changes the naming of some methods and data structures used
in query plan metadata to refer to "routines" instead of "udfs". This
clarifies that stored procedures are also valid targets for metadata
tracking.

Epic: None

Release note: None

#### sql: use memo metadata to add routines to statement bundles

This commit updates the statement bundle logic to take advantage of the
information stored in the query plan metadata so that only relevant routines
are shown in `schema.sql` for a statement bundle. In addition, stored
procedures are now tracked in the metadata in addition to UDFs. This has
no impact on query-plan caching, since we currently do not cache plans
that invoke stored procedures.

Fixes #132142
Fixes #104976

Release note (bug fix): Fixed a bug that prevented the create statement
for a routine from being shown in a statement bundle. This happened when
the routine was created on a schema other than `public`. The bug has
existed since v23.1.

#### sql: use memo metadata to add UDTs to statement bundle

This commit modifies the statement bundle collection logic to use the
query plan metadata to determine which types to display in `schema.sql`.
This ensures that only types which are referenced by the query are shown.

Informs #104976

Release note: None

133365: roachtest: various minor perturbation cleanups r=kvoli a=andrewbaptist

Various minor cleanups to the perturbation tests. These changes don't impact anything functional but make it slightly easier to write new perturbations.

133573: kv: redirect requests to raft leader who holds leader lease r=nvanbenschoten a=nvanbenschoten

Informs #132762.

This commit updates the lease status logic to redirect requests to the raft leader who holds a leader lease when evaluating a request on a follower. Previously, the follower would return a NotLeaseHolderError, but it would not include the lease in the response, so the client would not be immediately redirected to the leader. Instead, it would continue trying each replica in order, eventually throwing a `sending to all replicas failed` error.

Furthermore, since no lease was included, request proxying would never be used. This explains why the `failover/partial/lease-gateway` test was failing to recover. With this change, the test now observes recovery in in 7.650s.

| test                                         | lease=epoch (ms) | lease=expiration (ms) | lease=leader (ms) | parity with expiration |
|:---------------------------------------------|-----------------:|----------------------:|------------------:|:----------------------:|
| failover/partial/lease-gateway               | 8,589            | 19,327           | 7,650            | ✔                      |

**Key _(comparing leader vs. expiration)_**:
✔ = parity
❌ = minor regression
❌❌ = major regression
❌❌❌ = unavailability

To further demonstrate that this change works as expected, the commit also ports over two unit test that exercise request proxying to run with leader leases. Without this change, they fail. With it, they pass.

----

Release note: None

133691: sql/schemachanger: Support enum types for DSC impl of ALTER COLUMN … ALTER TYPE r=spilchen a=spilchen

Previously, attempts to alter a column's type to an enum were blocked due to failures during the backfill process, resulting in the error: "cannot resolve types in DistSQL by name." This change addresses that issue by ensuring all type names are fully resolved when populating the USING expression for the alteration.

Additionally, I've corrected an error message for column type conversions involving an enum type without a USING clause. The previous message included a hint with an unhydrated type name, which was confusing for users. The hint used to look like this:
```
HINT: You might need to specify "USING: x::`@100117".`
```

This fix now provides a more meaningful hint by displaying the correct type name.

Epic: CRDB-25314
Closes #132936
Release note: none

Co-authored-by: Drew Kimball <drewk@cockroachlabs.com>
Co-authored-by: Andrew Baptist <baptist@cockroachlabs.com>
Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Co-authored-by: Matt Spilchen <matt.spilchen@cockroachlabs.com>
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue Oct 30, 2024
@nvanbenschoten
raft: don't panic on defortifying snapshot from new term
8105a38 
Informs cockroachdb#132762.

This commit fixes a bug introduced by 58a9f53. Now that we no longer assume
that snapshots are coming from the leader, we were not defortifying the raft
leadership when receiving a snapshot. This meant that if a snapshot came from
a new term, we would hit an assertion failure, which the new test reproduces.

This commit addresses this bug by distinguishing between messages that are
always sent from the leader of the message's term and messages that indicate
that there is a new leader of the message's term, even if the message is not
from the leader itself.

This is temporary, and can be removed when we address cockroachdb#127349.

Release note: None
@nvanbenschoten nvanbenschoten mentioned this issue Oct 30, 2024
Merged
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue Oct 30, 2024
@nvanbenschoten
kv: redirect requests to raft leader who holds leader lease
45f3aaa 
Informs cockroachdb#132762.

This commit updates the lease status logic to redirect requests to the raft
leader who holds a leader lease when evaluating a request on a follower.
Previously, the follower would return a NotLeaseHolderError, but it would not
include the lease in the response, so the client would not be immediately
redirected to the leader. Instead, it would continue trying each replica in
order, eventually throwing a `sending to all replicas failed` error.

Furthermore, since no lease was included, request proxying would never be used.
This explains why the `failover/partial/lease-gateway` test was failing to
recover. With this change, the test now observes recovery in in 7.650s.

To demonstrate that this change works as expected, the commit also ports over
two unit test that exercise request proxying to run with leader leases. Without
this change, they fail. With it, they pass.

Release note: None
@nvanbenschoten nvanbenschoten mentioned this issue Oct 30, 2024
Merged
craig bot pushed a commit that referenced this issue Oct 30, 2024
@annrpom @stevendanna
@rickystewart @nvanbenschoten @yuzefovich
Merge #132967 #133725 #133848 #133874 #133878
ef26d15 
132967: schemachanger: add support discarding database/table zc r=rafiss a=annrpom

### scbuild: version gate SetZoneConfig with isV243Active
New support was added in 24.3 in `SetZoneConfig` for subzone
configs. Since this work was a significant change, we should
gate `SetZoneConfig` to minimize the risk of compatibility
issues with earlier versions.

Epic: none

Release note: None

---

### schemachanger: re-enable dsc zone config
Since 24.3 has been cut, we can turn zone configs
on in the declarative schema changer by default.

Epic: none

Release note: None

---

### scexec: extend UpdateZoneConfig to allow for deletes
This patch extends UpdateZoneConfig to allow for deletes in
the system.zones table.

Epic: None

Release note: None

---

### scbuildstmt: refactor zoneConfigObjBuilder method
This patch refactors the `addZoneConfigToBuildCtx` method
of our `zoneConfigObjBuilder` to be less specific; we pull
the `b.add` out so that we can decide in `SetZoneConfig`
whether to add or drop this element.

Epic: none

Release note: None

---

### schemachanger: add support discarding database/table zc
This patch supports discarding a database/table zone config
in the declarative schema changer.

Informs: #133157

Release note: None

133725: kvserver/rangefeed: add metric for processor-level send timeout r=wenyihu6a a=stevendanna

Epic: none
Release note: None

133848: bazel-github-helper: sort list of failed tests in summary r=rail a=rickystewart

Epic: CRDB-17171
Release note: None
Release justification: Non-production code changes

133874: raft: don't panic on defortifying snapshot from new term r=nvanbenschoten a=nvanbenschoten

Informs #132762.

This commit fixes a bug introduced by 58a9f53. Now that we no longer assume that snapshots are coming from the leader, we were not defortifying the raft leadership when receiving a snapshot. This meant that if a snapshot came from a new term, we would hit an assertion failure, which the new test reproduces.

This commit addresses this bug by distinguishing between messages that are always sent from the leader of the message's term and messages that indicate that there is a new leader of the message's term, even if the message is not from the leader itself.

This is temporary, and can be removed when we address #127349.

Release note: None

133878: revert "logictest: deflake TestLogic_union" r=yuzefovich a=yuzefovich

This reverts commit 6218f23.

I don't think this commit did anything to remove or reduce flakiness in the union logic test because the error occurs on the SELECT query _after_ the DDL and DML statements modified by this commit. Reverting it to allow for cleaner backports.

Epic: None
Release note: None

Co-authored-by: Annie Pompa <annie@cockroachlabs.com>
Co-authored-by: Steven Danna <danna@cockroachlabs.com>
Co-authored-by: Ricky Stewart <ricky@cockroachlabs.com>
Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>
blathers-crl bot pushed a commit that referenced this issue Oct 30, 2024
@nvanbenschoten
raft: don't panic on defortifying snapshot from new term
1b212ac 
Informs #132762.

This commit fixes a bug introduced by 58a9f53. Now that we no longer assume
that snapshots are coming from the leader, we were not defortifying the raft
leadership when receiving a snapshot. This meant that if a snapshot came from
a new term, we would hit an assertion failure, which the new test reproduces.

This commit addresses this bug by distinguishing between messages that are
always sent from the leader of the message's term and messages that indicate
that there is a new leader of the message's term, even if the message is not
from the leader itself.

This is temporary, and can be removed when we address #127349.

Release note: None
@blathers-crl blathers-crl bot mentioned this issue Oct 30, 2024
Merged
blathers-crl bot pushed a commit that referenced this issue Nov 8, 2024
@nvanbenschoten
roachtest: disable "deadlock" failure in chaos tests
d3f3d37 
Informs #132762.

This commit disables the replica deadlock failure mode in chaos tests.
We don't want to run this form of failure in the chaos tests because we
do not expect cockroach to recover from it and do not want to lose
signal for the chaos tests. The deadlock failure mode continues to be
used in its own individual test.

None of the three forms of leases (expiration, epoch, and leader) can
survive a replica deadlock under default configuration. Epoch and leader
leases cannot survive it by design without the introduction of a replica
watchdog. Expiration leases can survive it if combined with DistSender
circuit breakers, but those are not yet enabled by default, even though
we do enable them in the chaos tests.

Release note: None
@blathers-crl blathers-crl bot mentioned this issue Nov 8, 2024
Merged
blathers-crl bot pushed a commit that referenced this issue Nov 8, 2024
@nvanbenschoten
roachtest: disable "deadlock" failure in chaos tests
8258020 
Informs #132762.

This commit disables the replica deadlock failure mode in chaos tests.
We don't want to run this form of failure in the chaos tests because we
do not expect cockroach to recover from it and do not want to lose
signal for the chaos tests. The deadlock failure mode continues to be
used in its own individual test.

None of the three forms of leases (expiration, epoch, and leader) can
survive a replica deadlock under default configuration. Epoch and leader
leases cannot survive it by design without the introduction of a replica
watchdog. Expiration leases can survive it if combined with DistSender
circuit breakers, but those are not yet enabled by default, even though
we do enable them in the chaos tests.

Release note: None
@blathers-crl blathers-crl bot mentioned this issue Nov 8, 2024
Merged
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue Nov 12, 2024
@nvanbenschoten
raft: don't step down to newer term while fortifying
e1c081a 
Informs cockroachdb#132762.

This commit fixes a bug where a peer would step down to a newer term while
fortifying a leader. This would break the leader fortification promise, which
is critical for the leader lease disjointness property.

Instead, we now ignore the message as a follower and let the leader handle the
process of stepping down, safely defortifying, and eventually stepping back up
leader in the new term.

A subsequent commit will combine this logic with the other leader lease check
for votes and pre-votes.

Release note: None
@nvanbenschoten nvanbenschoten mentioned this issue Nov 12, 2024
Merged
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue Nov 12, 2024
@nvanbenschoten
raft: don't step down to newer term while fortifying
61226e5 
Informs cockroachdb#132762.

This commit fixes a bug where a peer would step down to a newer term while
fortifying a leader. This would break the leader fortification promise, which
is critical for the leader lease disjointness property.

Instead, we now ignore the message as a follower and let the leader handle the
process of stepping down, safely defortifying, and eventually stepping back up
leader in the new term.

A subsequent commit will combine this logic with the other leader lease check
for votes and pre-votes.

Release note: None
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue Nov 12, 2024
@nvanbenschoten
raft: don't step down to newer term while fortifying
52a1003 
Informs cockroachdb#132762.

This commit fixes a bug where a peer would step down to a newer term while
fortifying a leader. This would break the leader fortification promise, which
is critical for the leader lease disjointness property.

Instead, we now ignore the message as a follower and let the leader handle the
process of stepping down, safely defortifying, and eventually stepping back up
leader in the new term.

A subsequent commit will combine this logic with the other leader lease check
for votes and pre-votes.

Release note: None
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue Nov 12, 2024
@nvanbenschoten
roachtest: metamorphically enable leader leases
09a23c7 
Part of cockroachdb#132762.

To increase test coverage of leader leases, this patch adds leader leases
to the set of possible lease types when tests opt-in to metamorphic leases.
As of fc68b0f, most roachtests do enable metamorphic leases, so this will
provide good coverage of leader leases.

Release note: None
craig bot pushed a commit that referenced this issue Nov 12, 2024
@nvanbenschoten @tbg @wenyihu6
Merge #134922 #134943 #134954
096316f 
134922: raft: don't step down to newer term while fortifying r=nvanbenschoten a=nvanbenschoten

Informs #132762.

This commit fixes a bug where a peer would step down to a newer term while fortifying a leader. This would break the leader fortification promise, which is critical for the leader lease disjointness property.

Instead, we now ignore the message as a follower and let the leader handle the process of stepping down, safely defortifying, and eventually stepping back up leader in the new term.

A subsequent commit will combine this logic with the other leader lease check for votes and pre-votes.

Release note: None

134943: go.mod: do not lie about googleproto version r=tbg a=tbg

It seems to "just work" after also upgrading `github.com/golang/protobuf`. The third commit optimistically upgrades `google.golang.org/protobuf` to the latest version v1.35.1.

Fixes #134941.

Epic: none

134954: kvserver/rangefeed: make r.disconnect a public method r=tbg,stevendanna a=wenyihu6

This patch renames `r.disconnect` to `r.Disconnect`, making it a public method. This
change allows other packages to call `r.Disconnect` at the node-level `MuxRangefeed`
in future commits.

Part of: #110432
Release note: none

Co-authored-by: Steven Danna danna@cockroachlabs.com

Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Co-authored-by: Tobias Grieger <tobias.b.grieger@gmail.com>
Co-authored-by: Wenyi Hu <wenyi@cockroachlabs.com>
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue Nov 12, 2024
@nvanbenschoten
roachtest: metamorphically enable leader leases
9513109 
Part of cockroachdb#132762.

To increase test coverage of leader leases, this patch adds leader leases
to the set of possible lease types when tests opt-in to metamorphic leases.
As of fc68b0f, most roachtests do enable metamorphic leases, so this will
provide good coverage of leader leases.

Release note: None
craig bot pushed a commit that referenced this issue Nov 12, 2024
@nvanbenschoten @spilchen @rafiss
Merge #133216 #133217 #134411 #134875
c1a452d 
133216: roachtest: add kv/splits/nodes=3/quiesce=false/lease=leader r=nvanbenschoten a=nvanbenschoten

Part of #132762.

This commit adds a `kv/splits/nodes=3/quiesce=false/lease=leader` test. It should be able to support 80k ranges even without quiescence, because leader leases use store liveness for failure detection and lease extension.

To push higher than this, we will likely need to address #133885.

Release note: None

133217: roachtest: metamorphically enable leader leases r=nvanbenschoten a=nvanbenschoten

Part of #132762.

To increase test coverage of leader leases, this patch adds leader leases to the set of possible lease types when tests opt-in to metamorphic leases. As of fc68b0f, most roachtests do enable metamorphic leases, so this will provide good coverage of leader leases.

Before merging this, we should manually run all of these tests with leader leases to verify that they pass.

Release note: None

134411: sql/schemachanger: Prevent Temporary Disappearance of Altered Columns r=spilchen a=spilchen

Previously, during an ALTER COLUMN ... TYPE operation, the column being modified could briefly disappear, leading to "column does not exist" errors for any queries attempting to access it. This issue arose from the incorrect ordering of dependency rules, which has now been addressed.

The changes to the dependency rules during the column type alteration are as follows:
- The name of the new column is not set until we are ready to make it public. Previously, the column name was assigned too early in the pre-commit phase, which necessitated additional shadow transient column name logic. This new approach simplifies the process and eliminates the need for that extra rename logic.
- The ColumnName and Column elements for both the old and new columns are swapped within the same stage.

I have also maintained the existing rules for cases where the column type is not being altered by utilizing the IsAlterColumnTypeOp and IsNotAlterColumnTypeOp predicates.

I updated existing tests to query the column during each stage of the alter. This allowed me to reproduce the issue.

Epic: CRDB-25314
Closes #133996
Release note: none

134875: license: unredact logs written by license enforcer r=rafiss a=rafiss

This makes it so log messages are not redacted unnecessarily.

- Use redact.StringBuilder instead of strings.Builder.
- Avoid using `.String()` arguments for log.Infof, since strings are always redacted.
- Mark license type as a redact.SafeValue.

Epic: None
Release note: None

Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Co-authored-by: Matt Spilchen <matt.spilchen@cockroachlabs.com>
Co-authored-by: Rafi Shamim <rafi@cockroachlabs.com>
blathers-crl bot pushed a commit that referenced this issue Nov 12, 2024
@nvanbenschoten
roachtest: metamorphically enable leader leases
ba2383f 
Part of #132762.

To increase test coverage of leader leases, this patch adds leader leases
to the set of possible lease types when tests opt-in to metamorphic leases.
As of fc68b0f, most roachtests do enable metamorphic leases, so this will
provide good coverage of leader leases.

Release note: None
This was referenced Nov 12, 2024
Open
Open
iskettaneh pushed a commit to iskettaneh/cockroach that referenced this issue Nov 13, 2024
@nvanbenschoten @iskettaneh
raft: don't step down to newer term while fortifying
12c4b20 
Informs cockroachdb#132762.

This commit fixes a bug where a peer would step down to a newer term while
fortifying a leader. This would break the leader fortification promise, which
is critical for the leader lease disjointness property.

Instead, we now ignore the message as a follower and let the leader handle the
process of stepping down, safely defortifying, and eventually stepping back up
leader in the new term.

A subsequent commit will combine this logic with the other leader lease check
for votes and pre-votes.

Release note: None
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@nvanbenschoten nvanbenschoten

Labels
A-kv-replication Relating to Raft, consensus, and coordination. A-leader-leases Related to the introduction of leader leases C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-kv KV Team
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@nvanbenschoten @miraradeva