roachtest: enable DistSender circuit breakers in failover/partial/lease-leader

[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

[cockroach-teamcity]

This change is 

[arulajmani]

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @DarrylWong, @nvanbenschoten, and @srosenberg)

---

-- commits line 6 at r1:
I'm a bit conflicted on this change. On one hand, without DistSender circuit breakers and no timeouts, I'm not entirely sure what this test is trying to test is doing to begin with. Especially considering we're measuring pMax, which means we don't differentiate between a temporary outage or a permanent one.

On the other hand, given it's not in our medium term plans to productionize DistSender circuit breakers, I'm wondering if it's fair to turn them on here. You could say they're a proxy for a statement timeout[1].

IMO, we should re-think these tests to not use pMax at all. Instead, we should switch these to track the duration of the outage (like we've spoken about a few times at this point) and not use DistSender circuit breakers at all here (maybe we add statement timeouts). Until then though, turning on DistSender circuit breakers sounds reasonable.

[1] I asked myself why don't we set a statement timeout here instead -- and I think I know why -- it's because adding a statement timeout will put a cap on pMax, which means we'll be back to square one where we can't differentiate between the three lease types.

---

-- commits line 25 at r1:
Mostly for my edification, do you have a high level idea of how this 20s breaks down for expiration based leases? Or maybe even where you're putting your money on the 10s difference between leader leases / expiration based leases?

---

pkg/cmd/roachtest/tests/failover.go line 551 at r1 (raw file):

	// use statement timeouts), so we don't need to test them here.
	// TODO(arul): this can be removed if/when we turn on DistSender circuit
	// breakers for all ranges by default.

Or if we rework this test to track the duration of the outage instead of pMax as well?

[nvanbenschoten]

TFTR!

bors r+

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @arulajmani, @DarrylWong, and @srosenberg)

---

-- commits line 6 at r1:

I asked myself why don't we set a statement timeout here instead -- and I think I know why -- it's because adding a statement timeout will put a cap on pMax, which means we'll be back to square one where we can't differentiate between the three lease types.

Yes, that's exactly right. If we put a statement timeout here and then keep measuring pMax as our proxy for outage duration, then we will consider all outages to last for only statement_timeout.

I'm in agreement with everything you've said. I'll track this in #133361.

---

-- commits line 25 at r1:

Previously, arulajmani (Arul Ajmani) wrote…

Mostly for my edification, do you have a high level idea of how this 20s breaks down for expiration based leases? Or maybe even where you're putting your money on the 10s difference between leader leases / expiration based leases?

For both of them, we are seeing the 10s DistSender circuit breaker timeout, so the outage duration is 10s less than what's measured here. I don't understand the 10s difference between them, but I'd like to.

---

pkg/cmd/roachtest/tests/failover.go line 551 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Or if we rework this test to track the duration of the outage instead of pMax as well?

Agree 100%.

[craig]

Build succeeded:

• macos_amd64_build
• acceptance
• local_roachtest
• linux_amd64_fips_build
• local_roachtest_fips
• windows_build
• examples_orms
• lint
• linux_amd64_build
• linux_arm64_build
• check_generated_code
• macos_arm64_build
• docker_image_amd64
• unit_tests