roachtest: assert on CPU in rebalance/by-load #102824

kvoli · 2023-05-05T22:25:36Z

The rebalance/by-load/replicas roachtests periodically flake due to a
known limitation, where a cluster with a small number of ranges may not
be properly balanced due to heterogeneous localities (including
multi-store) #88829.

This PR updates the total number of ranges from 1 per-store, to 5
per-store for rebalance/by-load/* roachtests.

The roachtest asserted on the lease count, as a proxy for load, assuming
that the load evenly hits each lease for the created ranges. However,
the principal indicator of load in a read heavy workload is CPU.

This PR updates the test assertion to require that every store's CPU
is within 10% of the cluster mean. The test assertion previously
required that the max-min lease count delta was 0, when no outside
splits occurred; or 1 when the number of ranges was greater than the
number of stores.

The logging format is updated for easier debugging:

cpu outside bounds mean=23.9 tolerance=10.0% (±2.4) bounds=[21.5, 26.3]
   below  = []
   within = [s1: 22 (-6.7%), s2: 22 (-6.3%)]
   above  = [s3: 26 (+13.0%)]
...
cpu within bounds mean=25.7 tolerance=10.0% (±2.6) bounds=[23.2, 28.3]
   stores=[s1: 24 (-3.0%), s2: 24 (-2.9%), s3: 27 (+5.9%)]

resolves: #102801
resolves: #102823

Release note: None

cockroach-teamcity · 2023-05-05T22:25:47Z

This change is

aliher1911

Looks good. Wouldn't it be still susceptible to some random variations in the cluster like backup running?

I put some minor nits.

aliher1911 · 2023-05-09T12:40:21Z

pkg/cmd/roachtest/tests/ts_util.go

@@ -38,9 +38,13 @@ const (
 	rate
 )

+// defaultSampleNanos is the default sampling period for getMetrics.
+const defaultSampleNanos = int64(time.Minute)


I think it would be more readable to have it as

const defaultSamplePeriod = time.Minute

and then use defaultSamplePeriod.Nanoseconds() only when using it in TimeSeriesQueryRequest.

aliher1911 · 2023-05-09T12:40:26Z

pkg/cmd/roachtest/tests/rebalance_load.go

+// isLoadEvenlyDistributed checks whether the load for the stores given are
+// within tolerance of the mean. If the store loads are, true is returned as
+// well as reason, otherwise false. The function expects the loads to be
+// indexed to store IDs, see getLeaseCounts for example format.


see getLeaseCounts for example format

I don't see getLeaseCounts anywhere in the code. Is it stale?

aliher1911 · 2023-05-09T12:40:30Z

pkg/cmd/roachtest/tests/rebalance_load.go

 			}

-			return fmt.Errorf("timed out before leases were evenly spread")
+			return fmt.Errorf("timed out before CPU was evenly balanced")


I think it would be nicer for the reader to have error that would include reason. E.g. CPU is not evenly balanced after timeout: . Then it should be part of error message when test fails and GH issue is created.

Previously, the utility functions for retrieving stats from the timeseries database in roachtest wouldn't allow specifying a sample period nor sources. This commit enhances ts_util.go to enable specifying a sample period via `getMetricsWithSamplePeriod` and updates the `tsQuery` to have a source field - which is applied on queries. Release note: None

The `rebalance/by-load/replicas` roachtests periodically flake due to a known limitation, where a cluster with a small number of ranges may not be properly balanced due to heterogeneous localities (including multi-store) cockroachdb#88829. This commit updates the total number of ranges from 1 per-store, to 5 per-store for `rebalance/by-load/*` roachtests. The roachtest asserted on the lease count, as a proxy for load, assuming that the load evenly hits each lease for the created ranges. However, the principal indicator of load in a read heavy workload is CPU. This commit updates the test assertion to require that every store's CPU is within 10% of the cluster mean. The test assertion previously required that the max-min lease count delta was 0, when no outside splits occurred; or 1 when the number of ranges was greater than the number of stores. The logging format is updated for easier debugging: ``` cpu outside bounds mean=23.9 tolerance=10.0% (±2.4) bounds=[21.5, 26.3] below = [] within = [s1: 22 (-6.7%), s2: 22 (-6.3%)] above = [s3: 26 (+13.0%)] ... cpu within bounds mean=25.7 tolerance=10.0% (±2.6) bounds=[23.2, 28.3] stores=[s1: 24 (-3.0%), s2: 24 (-2.9%), s3: 27 (+5.9%)] ``` resolves: cockroachdb#102801 resolves: cockroachdb#102823 Release note: None

The `rebalance/by-load` roachtests assert on the distribution of CPU utilization between nodes in the cluster, having backups also running could lead to unrelated flakes. This commit updates cluster creation in `rebalance/by-load/` roachtests to opt out of a backup schedule. Epic: none Release note: None

kvoli

It could still be susceptible. I pushed another commit to disable scheduled backups - I'd rather avoid any flakes on this test, since it has been high maintenance recently.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aliher1911, @smg260, and @srosenberg)

pkg/cmd/roachtest/tests/rebalance_load.go line 176 at r2 (raw file):

Previously, aliher1911 (Oleg) wrote…

I think it would be nicer for the reader to have error that would include reason. E.g. CPU is not evenly balanced after timeout: . Then it should be part of error message when test fails and GH issue is created.

Done.

pkg/cmd/roachtest/tests/rebalance_load.go line 322 at r2 (raw file):

Previously, aliher1911 (Oleg) wrote…

see getLeaseCounts for example format

I don't see getLeaseCounts anywhere in the code. Is it stale?

Stale - updated to getStoreCPUFn.

pkg/cmd/roachtest/tests/ts_util.go line 42 at r2 (raw file):

Previously, aliher1911 (Oleg) wrote…

I think it would be more readable to have it as
const defaultSamplePeriod = time.Minute
and then use defaultSamplePeriod.Nanoseconds() only when using it in TimeSeriesQueryRequest.

Good idea, updated to this.

kvoli · 2023-05-09T14:52:55Z

TYFTR

bors r=aliher1911

kvoli · 2023-05-09T16:03:26Z

bors r=aliher1911

craig · 2023-05-09T16:31:27Z

Build succeeded:

Bazel Essential CI (Cockroach)

blathers-crl · 2023-05-09T16:31:45Z

Encountered an error creating backports. Some common things that can go wrong:

The backport branch might have already existed.
There was a merge conflict.
The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

error creating merge commit from d9b5463 to blathers/backport-release-22.2-102824: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.2.x failed. See errors above.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

kvoli added the backport-23.1.x Flags PRs that need to be backported to 23.1 label May 5, 2023

kvoli self-assigned this May 5, 2023

kvoli changed the title ~~storepool: limit storelist string to 2 decimals~~ roachtest: assert on CPU in rebalance/by-load May 5, 2023

kvoli force-pushed the 230505.roachtest-rebalance-load branch from 7ac3143 to aa9f4f0 Compare May 5, 2023 22:28

kvoli mentioned this pull request May 5, 2023

roachtest: rebalance/by-load/replicas/ssds=2 failed #82991

Closed

kvoli added the backport-22.2.x label May 5, 2023

kvoli marked this pull request as ready for review May 5, 2023 23:05

kvoli requested a review from a team as a code owner May 5, 2023 23:05

kvoli requested review from srosenberg, smg260 and aliher1911 and removed request for a team May 5, 2023 23:05

aliher1911 approved these changes May 9, 2023

View reviewed changes

kvoli mentioned this pull request May 9, 2023

roachtest: rebalance/by-load/leases/mixed-version failed #102795

Closed

kvoli added 3 commits May 9, 2023 13:36

kvoli force-pushed the 230505.roachtest-rebalance-load branch from aa9f4f0 to 4d21913 Compare May 9, 2023 13:41

kvoli requested a review from aliher1911 May 9, 2023 13:47

kvoli commented May 9, 2023

View reviewed changes

This was referenced May 9, 2023

roachtest: rebalance/by-load/replicas/ssds=2 failed #100743

Closed

roachtest: rebalance/by-load/replicas/ssds=2 failed #100816

Closed

craig bot merged commit 03e4c91 into cockroachdb:master May 9, 2023

blathers-crl bot mentioned this pull request May 9, 2023

release-23.1: roachtest: assert on CPU in rebalance/by-load #102963

Merged

erikgrinaker mentioned this pull request May 22, 2023

roachtest: rebalance/by-load/replicas/ssds=2 failed #103704

Closed

kvoli mentioned this pull request Jun 27, 2023

release-22.2: assert on CPU in rebalance/by-load and reduce noise #105616

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

roachtest: assert on CPU in rebalance/by-load #102824

roachtest: assert on CPU in rebalance/by-load #102824

kvoli commented May 5, 2023

cockroach-teamcity commented May 5, 2023

aliher1911 left a comment

aliher1911 May 9, 2023

aliher1911 May 9, 2023

aliher1911 May 9, 2023

kvoli left a comment

kvoli commented May 9, 2023 •

edited

Loading

kvoli commented May 9, 2023

craig bot commented May 9, 2023

blathers-crl bot commented May 9, 2023

roachtest: assert on CPU in rebalance/by-load #102824

roachtest: assert on CPU in rebalance/by-load #102824

Conversation

kvoli commented May 5, 2023

cockroach-teamcity commented May 5, 2023

aliher1911 left a comment

Choose a reason for hiding this comment

aliher1911 May 9, 2023

Choose a reason for hiding this comment

aliher1911 May 9, 2023

Choose a reason for hiding this comment

aliher1911 May 9, 2023

Choose a reason for hiding this comment

kvoli left a comment

Choose a reason for hiding this comment

kvoli commented May 9, 2023 • edited Loading

kvoli commented May 9, 2023

craig bot commented May 9, 2023

blathers-crl bot commented May 9, 2023

kvoli commented May 9, 2023 •

edited

Loading