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

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

Merged
merged 3 commits into from
May 10, 2023
Merged

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

merged 3 commits into from
May 10, 2023

Conversation

blathers-crl[bot]
Copy link

@blathers-crl blathers-crl bot commented May 9, 2023 •
edited by kvoli
Loading

Backport 3/3 commits from #102824 on behalf of @kvoli.

/cc @cockroachdb/release

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

Release justification: Test only change to reduce flakes in rebalance/by-load roachtests.

kvoli added 3 commits May 5, 2023 21:09
@kvoli
roachtest: add sample period and source to ts_util
25bc2b8 
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
@kvoli
roachtest: assert on CPU in rebalance/by-load
0f02a9d 
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 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: #102801
resolves: #102823

Release note: None
@kvoli
roachtest: skip backup schedule on rebalance/*
1343265 
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
@blathers-crl blathers-crl bot force-pushed the blathers/backport-release-23.1-102824 branch from 0f02a9d to 4582c2b Compare May 9, 2023 16:31
@blathers-crl blathers-crl bot requested a review from a team as a code owner May 9, 2023 16:31
@blathers-crl blathers-crl bot requested review from herkolategan and srosenberg and removed request for a team May 9, 2023 16:31
@blathers-crl blathers-crl bot force-pushed the blathers/backport-release-23.1-102824 branch from 1ff3672 to 1343265 Compare May 9, 2023 16:31
@blathers-crl blathers-crl bot added blathers-backport This is a backport that Blathers created automatically. O-robot Originated from a bot. labels May 9, 2023
@blathers-crl
Copy link
Author

blathers-crl bot commented May 9, 2023 •
edited by kvoli
Loading

Thanks for opening a backport.

Please check the backport criteria before merging:

  • Patches should only be created for serious issues or test-only changes.
  • Patches should not break backwards-compatibility.
  • Patches should change as little code as possible.
  • Patches should not change on-disk formats or node communication protocols.
  • Patches should not add new functionality.
  • Patches must not add, edit, or otherwise modify cluster versions; or add version gates.
If some of the basic criteria cannot be satisfied, ensure that the exceptional criteria are satisfied within.
  • There is a high priority need for the functionality that cannot wait until the next release and is difficult to address in another way.
  • The new functionality is additive-only and only runs for clusters which have specifically “opted in” to it (e.g. by a cluster setting).
  • New code is protected by a conditional check that is trivial to verify and ensures that it only runs for opt-in clusters.
  • The PM and TL on the team that owns the changed code have signed off that the change obeys the above rules.

Add a brief release justification to the body of your PR to justify this backport.

Some other things to consider:

  • What did we do to ensure that a user that doesn’t know & care about this backport, has no idea that it happened?
  • Will this work in a cluster of mixed patch versions? Did we test that?
  • If a user upgrades a patch version, uses this feature, and then downgrades, what happens?

@blathers-crl blathers-crl bot requested review from aliher1911, kvoli and smg260 May 9, 2023 16:31
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@kvoli
Copy link
Collaborator

kvoli commented May 9, 2023

TYFTR

@kvoli
Copy link
Collaborator

kvoli commented May 9, 2023

I'll hold off merging until the master branch runs the updated roachtests.

@kvoli kvoli merged commit 25b297d into release-23.1 May 10, 2023
@kvoli kvoli deleted the blathers/backport-release-23.1-102824 branch May 10, 2023 12:48
This was referenced May 10, 2023
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aliher1911 aliher1911 aliher1911 approved these changes

@herkolategan herkolategan Awaiting requested review from herkolategan herkolategan is a code owner automatically assigned from cockroachdb/test-eng

@srosenberg srosenberg Awaiting requested review from srosenberg srosenberg is a code owner automatically assigned from cockroachdb/test-eng

@kvoli kvoli Awaiting requested review from kvoli

@smg260 smg260 Awaiting requested review from smg260

Assignees

@kvoli kvoli

Labels
blathers-backport This is a backport that Blathers created automatically. O-robot Originated from a bot.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cockroach-teamcity @kvoli @aliher1911