roachtest: increase the token return time with disk bandwidth limit#137019
Conversation
andrewbaptist
requested review from
csgourav and
shailendra-patel
and removed request for
a team
|
This change is |
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @andrewbaptist, @csgourav, and @shailendra-patel)
-- commits line 4 at r1:
nit: it is referenced below, but the test [sic] isn't known here yet, consider mentioning it here or updating the commit header.
pkg/cmd/roachtest/tests/admission_control_elastic_mixed_version.go line 136 at r1 (raw file):
// TODO(pav-kv): also validate that the write throughput was kept under // control, and the foreground traffic was not starved. roachtestutil.ValidateTokensReturned(ctx, t, c, c.CRDBNodes(), time.Minute)
The commit header:
roachtest: increase the token return time with disk bandwidth limit
The wait duration is decreasing from 10 minutes, to 1 minute unless the diskBandwidthLimit is enabled.
I suggest either updating the commit message/header, or leaving the token return time unchanged for non-disk-bw-limit variations.
Also, do you have any stats handy that confirm the decrease from 10 minutes, to 1 minute, is sufficient to prevent flakes?
Previously the perturbation/* tests would wait 10m for tokens to be returned. Without the disk bandwidth limit set, they typically return almost immediately but with a limit they can take ~30m to return in some cases even after the workload is stopped and the system is idle. This change fixes some of the perturbation/metamorphic/* tests that are hitting this slow token return. Additionally this change reduces the token wait time for the test admission-control/elastic-workload/mixed-version to 1 minute as this test doesn't typically wait more then a few seconds for token return. Epic: none Fixes: cockroachdb#136982 Fixes: cockroachdb#136553 Informs: cockroachdb#137017 Release note: None
andrewbaptist
force-pushed
the
2024-12-09-slow-token-return
branch
from
039a2d2 to
87b463a
Compare
andrewbaptist
left a comment
andrewbaptist
left a comment
There was a problem hiding this comment.
TFTR!
Changes made, let me know if you have additional comments.
Reviewable status:
Previously, kvoli (Austen) wrote…
nit: it is referenced below, but the test [sic] isn't known here yet, consider mentioning it here or updating the commit header.
Updated to add the test name here.
pkg/cmd/roachtest/tests/admission_control_elastic_mixed_version.go line 136 at r1 (raw file):
Previously, kvoli (Austen) wrote…
The commit header:
roachtest: increase the token return time with disk bandwidth limitThe wait duration is decreasing from 10 minutes, to 1 minute unless the
diiskBandwidthLimitis enabled.I suggest either updating the commit message/header, or leaving the token return time unchanged for non-disk-bw-limit variations.
Also, do you have any stats handy that confirm the decrease from 10 minutes, to 1 minute, is sufficient to prevent flakes?
I left this as 1 minute after looking through the last several runs for this test where it was always in the 1-2s range. It might be easier to catch any regressions in this behavior on this test with a lower bound.
kvoli
left a comment
kvoli
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status:
pkg/cmd/roachtest/tests/admission_control_elastic_mixed_version.go line 136 at r1 (raw file):
Previously, andrewbaptist (Andrew Baptist) wrote…
I left this as 1 minute after looking through the last several runs for this test where it was always in the 1-2s range. It might be easier to catch any regressions in this behavior on this test with a lower bound.
ack
|
TFTR! bors r=kvoli |
3 participants
Previously the test would wait 10m for tokens to be returned. Without the disk bandwidth limit set, they typically return almost immediately but with a limit they can take ~30m to return in some cases even after the workload is stopped and the system is idle. This change fixes some of the perturbation/metamorphic/* tests that are hitting this slow token return.
Epic: none
Fixes: #136982
Fixes: #136553
Informs: #137017
Release note: None