roachtest: include reads for graceful shutdown test

[andrewbaptist]

Small changes to the test:

1. Increase the cluster size from 3 to 6.
2. Introduce both reads and follower reads.
3. Increase the length of the test run and the time between
   starts/stops.
4. Include both reads and writes when measuring QPS.

Epic: none

Release note: None

[cockroach-teamcity]

This change is 

[andrewbaptist]

This isn't quite ready for review, because I need to figure out if I want to tie it to the change to reordering of nodes based on locality before latency.

Also, I don't really like the tangledness of the injecting of the health function into DistSender. It isn't strictly necessary, but I didn't want to do a bigger refactor since we likely want to backport this change. If there is a cleaner way to do this without splitting up the NodeLiveneess code, I would definitely do that.

[andrewbaptist]

@nvanbenschoten I don't think https://github.com/cockroachdb/cockroach/pull/112926/files#diff-104a11712684e520f0affa95e6a053faeb713306a10f69ee99b9729cc9dd1996 is safe (changing the func pointer after construction). It maybe works, since there wouldn't have been any requests sent when this is constructed, but that is just "lucky".

The alternative of refactoring the code to change the construction order is complex and not something I'd want to try and backport. Are there other good alternatives (other than a mutex around the use of this function every time)?

[andrewbaptist]

Test running:

roachprod run $CLUSTER:7 "./cockroach workload run kv --read-percent 50 --follower-read-percent 50 {pgurl:1-5}"

Some graphics of this with and without the change:
Without the change (drain at ~18:01:30)

With the change (drain at ~17:52:30)

[andrewbaptist]

@nvanbenschoten This should be ready for review. I need to look at the failure in TestBoundedStalenessDataDriven but I think the problem is an incorrect assumption in the test. It had failed independently on the same line in a different PR during race, so I don't think this is a new failure.

[nvanbenschoten]

Reviewed 4 of 11 files at r2, 1 of 4 files at r4, 7 of 7 files at r5.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andrewbaptist, @michae2, @renatolabs, and @srosenberg)

---

pkg/kv/kvserver/liveness/livenesspb/liveness.go line 274 at r5 (raw file):

	case ReplicaGCQueue:
		return nv.isAlive()
	case DistSender:

The way this turned out is quite nice.

---

pkg/server/server.go line 550 at r5 (raw file):

	// struct.
	distSender.SetHealthFunc(func(id roachpb.NodeID) bool {
		return nodeLiveness.GetNodeVitalityFromCache(id).IsLive(livenesspb.DistSender)

Calling GetNodeVitalityFromCache O(repl x log(repl)) times per RPC feels like it could come with a sizable performance penalty. Internally, this is acquiring the liveness cache's read lock twice, grabbing an HLC clock reading, and checking the RPC connection health (which itself acquires a few mutexes in gossip and in the RPC context).

Have we measured how expensive a single one of these calls is in a realistic setting (e.g. on a 32-core machine with 10k qps)?

Do we have any other callers of this API on the data path?

---

pkg/ccl/kvccl/kvfollowerreadsccl/followerreads_test.go line 1268 at r5 (raw file):

		Requests: b.Requests(),
	}
	br.Header.Timestamp = readTime

nit: can't you do b.Header.Timestamp for this, and then still use db.Run?

[andrewbaptist]

Looking at the perf data running:

./cockroach workload run kv --duration "1m" --read-percent 100 --follower-read-percent 100 --concurrency 200

WIth the setting disabled (no health check):
33375.0 ops/sec

With the setting enabled:
33445.9 ops/sec

However OptimizeReplicaOrder is much more expensive with it enabled:
Disabled - 0.33%

Enabled - 2.17%

I'm going to try a few things to reduce this overhead, although I don't think it will go to 0. The biggest cost right now is recomputing it more than once per node (since it is done within sort) and also looking at the connection health.

[andrewbaptist]

Closing this for now - the PR #114205 will temporarily supersede this.

[andrewbaptist]

The only remaining part of this was the change to the roachtest to include follower reads in the testing. I'm changing to @kvoli as he has worked on this test before.

[kvoli]

Reviewed 1 of 1 files at r10, 25 of 25 files at r11, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andrewbaptist)

---

-- commits line 4 at r11:
nit: reference the name of the test, since it is changed here.

---

-- commits line 12 at r11:
nit: rm empty line.

---

pkg/cmd/roachtest/tests/kv.go line 622 at r11 (raw file):

					case <-ctx.Done():
						return
					case <-time.After(2 * time.Minute):

Q: Why the wait increase?

[andrewbaptist]

TFTR!

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @kvoli and @nvanbenschoten)

---

-- commits line 4 at r11:

Previously, kvoli (Austen) wrote…

nit: reference the name of the test, since it is changed here.

Done

---

-- commits line 12 at r11:

Previously, kvoli (Austen) wrote…

nit: rm empty line.

Done

---

pkg/cmd/roachtest/tests/kv.go line 622 at r11 (raw file):

Previously, kvoli (Austen) wrote…

Q: Why the wait increase?

This was to make sure we capture any "latent" impacts of the restart. Previously it was restarting before the system was fully healthy (not all caught up and leases rebalanced). A longer wait here prevents this.

---

pkg/ccl/kvccl/kvfollowerreadsccl/followerreads_test.go line 1268 at r5 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

nit: can't you do b.Header.Timestamp for this, and then still use db.Run?

Thanks - changed.

---

pkg/kv/kvserver/liveness/livenesspb/liveness.go line 274 at r5 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

The way this turned out is quite nice.

👍

---

pkg/server/server.go line 550 at r5 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Calling GetNodeVitalityFromCache O(repl x log(repl)) times per RPC feels like it could come with a sizable performance penalty. Internally, this is acquiring the liveness cache's read lock twice, grabbing an HLC clock reading, and checking the RPC connection health (which itself acquires a few mutexes in gossip and in the RPC context).

Have we measured how expensive a single one of these calls is in a realistic setting (e.g. on a 32-core machine with 10k qps)?

Do we have any other callers of this API on the data path?

This was pulled into the other PR - but now this is cached. It turned out the most expensive part of this was the ConnHealth which is done twice without the other changes.

[andrewbaptist]

Note that this test will pass, but have long latency periods after each restart due to misrouted follower reads:

This is part of the work to fix 24.2, however since this test is currently asserting on throughput not latency we can leave it enabled with follower reads without a high risk of it flaking.

[andrewbaptist]

bors r=kvoli

[craig]

Build succeeded:

• lint
• local_roachtest
• Bazel Essential CI (Cockroach)
• linux_amd64_fips_build
• linux_amd64_build
• check_generated_code
• macos_amd64_build
• docker_image_amd64
• macos_arm64_build
• linux_arm64_build
• examples_orms
• windows_build
• acceptance
• local_roachtest_fips