kv/kvserver: TestTenantRateLimiter failed #129612
Assignees
Labels
A-testing
Testing tools and infrastructure
branch-release-23.2
Used to mark GA and release blockers, technical advisories, and bugs for 23.2
branch-release-24.1
Used to mark GA and release blockers, technical advisories, and bugs for 24.1
branch-release-24.1.4-rc
branch-release-24.2
Used to mark GA and release blockers, technical advisories, and bugs for 24.2
C-bug
Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
C-test-failure
Broken test (automatically or manually discovered).
O-robot
Originated from a bot.
T-kv
KV Team
Milestone
Comments
cockroach-teamcity
added
branch-release-24.1.4-rc
C-test-failure
|
Related to #127697 (comment) and #125728. Looking at the test, it's almost certainly that we're hitting the 120s context cancellation that's set up here: /pkg/kv/kvserver/client_tenant_test.go#L243-L246 // This test shouldn't take forever. If we're going to fail, better to
// do it in minutes than in an hour.
ctx, cancel := context.WithTimeout(ctx, 2*time.Minute)
defer cancel()This cancellation seems to have been put there because this is a test that tends to deadlock instead of fail. The test wants to see that a certain number of tenant requests is not blocked and that excess requests are blocked. But if background writes sneak in, maybe some of the ones that we think shouldn't get blocked would block, and this would block the test until the context cancellation hits. In fact, the error above tells us that we're sitting on a block that verifies that "the first burst writes don't get blocked" here: /pkg/kv/kvserver/client_tenant_test.go#L254-L258 // Now ensure that in the same instant the write QPS limit does affect the
// tenant. First issue requests that can happen without blocking.
for i := 0; i < burstWrites; i++ {
require.NoError(t, ts.DB().Put(ctx, mkKey(), 0))
}but from the looks of it they are certainly getting blocked, perhaps due to an errant request sneaking in. The test has plenty of evidence (various things disabled, etc) that suggests that this kind of thing has been an issue in the past. I'll send an update to the test that fails it gracefully - i.e. with a clear error and without hanging for two minute first - but it will remain flaky. I'll add some debug logging that prints the source of everything that consumes from the limiter, so next failure should hand it to us on a platter so that we can react. |
tbg
added
C-bug
|
kv/kvserver.TestTenantRateLimiter failed on release-24.1.4-rc @ 75d6ac620fb2d223454026e220a6999cedbba41c: Parameters:
Same failure on other branches
|
craig bot
pushed a commit
that referenced
this issue
Sep 6, 2024
129640: kvserver: deflake, fix, speed up TestTenantRateLimiter r=tbg a=tbg This test would occasionally (rarely) hang for 120s and then fail with an opaque AmbiguousResultError in case that some of the requests the test doesn't expect to block end up doing so. I'm fairly sure this test can sometimes deadlock because enough extra writes sneak in to cause the test's requests to block. Seeing how this is a tarpit test (once you start looking your work day is consumed), I addressed the following problems: - it claimed to verify metrics, but the regexp had rotted and so it wasn't verifying anything. - it ran into throttling in the client-side tenant rate limiter, so the test took 10s-15s. Now it runs in ~1s. - it also did >3k requests, so it was quite expensive. Now it does a couple hundred bigger ones, which should be a smidge cheaper. - it was flaky due to trying to make very strong claims about when requests would block, despite dozens of background requests that are hard to avoid sneaking in on every run. - it would be difficult to debug when hanging due to unexpected blocking. Now it's a somewhat weaker but still meaningful test that going forward we are paying a much smaller tax for. Should it block again the test will fail gracefully; it will still be difficult to root cause but it is also now wholly unexpected that this should ever occur. See: #129612 (comment) Fixes #129612. Release note: None Epic: None Co-authored-by: Tobias Grieger <[email protected]>
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=373d9101a12edc3847d2fb6ae78e3adcfb4ca41e&v=4){kind=avatar}
|
Based on the specified backports for linked PR #129640, I applied the following new label(s) to this issue: branch-release-23.2, branch-release-24.1, branch-release-24.2. Please adjust the labels as needed to match the branches actually affected by this issue, including adding any known older branches. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
blathers-crl
bot
added
branch-release-24.1
blathers-crl bot
pushed a commit
that referenced
this issue
Sep 13, 2024
This test would occasionally (rarely) hang for 120s and then fail with an opaque AmbiguousResultError in case that some of the requests the test doesn't expect to block end up doing so. I'm fairly sure this test can sometimes deadlock because enough extra writes sneak in to cause the test's requests to block. Seeing how this is a tarpit test (once you start looking your work day is consumed), I addressed the following problems: - it claimed to verify metrics, but the regexp had rotted and so it wasn't verifying anything. - it ran into throttling in the client-side tenant rate limiter, so the test took 10s-15s. Now it runs in ~1s. - it also did >3k requests, so it was quite expensive. Now it does a couple hundred bigger ones, which should be a smidge cheaper. - it was flaky due to trying to make very strong claims about when requests would block, despite dozens of background requests that are hard to avoid sneaking in on every run. - it would be difficult to debug when hanging due to unexpected blocking. Now it's a somewhat weaker but still meaningful test that going forward we are paying a much smaller tax for. Should it block again the test will fail gracefully; it will still be difficult to root cause but it is also now wholly unexpected that this should ever occur. See: #129612 (comment) Fixes #129612. Release note: None Epic: None
blathers-crl bot
pushed a commit
that referenced
this issue
Sep 13, 2024
This test would occasionally (rarely) hang for 120s and then fail with an opaque AmbiguousResultError in case that some of the requests the test doesn't expect to block end up doing so. I'm fairly sure this test can sometimes deadlock because enough extra writes sneak in to cause the test's requests to block. Seeing how this is a tarpit test (once you start looking your work day is consumed), I addressed the following problems: - it claimed to verify metrics, but the regexp had rotted and so it wasn't verifying anything. - it ran into throttling in the client-side tenant rate limiter, so the test took 10s-15s. Now it runs in ~1s. - it also did >3k requests, so it was quite expensive. Now it does a couple hundred bigger ones, which should be a smidge cheaper. - it was flaky due to trying to make very strong claims about when requests would block, despite dozens of background requests that are hard to avoid sneaking in on every run. - it would be difficult to debug when hanging due to unexpected blocking. Now it's a somewhat weaker but still meaningful test that going forward we are paying a much smaller tax for. Should it block again the test will fail gracefully; it will still be difficult to root cause but it is also now wholly unexpected that this should ever occur. See: #129612 (comment) Fixes #129612. Release note: None Epic: None
blathers-crl bot
pushed a commit
that referenced
this issue
Sep 13, 2024
This test would occasionally (rarely) hang for 120s and then fail with an opaque AmbiguousResultError in case that some of the requests the test doesn't expect to block end up doing so. I'm fairly sure this test can sometimes deadlock because enough extra writes sneak in to cause the test's requests to block. Seeing how this is a tarpit test (once you start looking your work day is consumed), I addressed the following problems: - it claimed to verify metrics, but the regexp had rotted and so it wasn't verifying anything. - it ran into throttling in the client-side tenant rate limiter, so the test took 10s-15s. Now it runs in ~1s. - it also did >3k requests, so it was quite expensive. Now it does a couple hundred bigger ones, which should be a smidge cheaper. - it was flaky due to trying to make very strong claims about when requests would block, despite dozens of background requests that are hard to avoid sneaking in on every run. - it would be difficult to debug when hanging due to unexpected blocking. Now it's a somewhat weaker but still meaningful test that going forward we are paying a much smaller tax for. Should it block again the test will fail gracefully; it will still be difficult to root cause but it is also now wholly unexpected that this should ever occur. See: #129612 (comment) Fixes #129612. Release note: None Epic: None
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
kv/kvserver.TestTenantRateLimiter failed on release-24.1.4-rc @ a1cbd782a33a51f7500444373ccdaa81eb2ed971:
Parameters:
attempt=1run=21shard=24Help
See also: How To Investigate a Go Test Failure (internal)
Same failure on other branches
This test on roachdash | Improve this report!
Jira issue: CRDB-41613
The text was updated successfully, but these errors were encountered: