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.

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

kv/kvserver: TestTenantRateLimiter failed #70456

Closed
cockroach-teamcity opened this issue Sep 20, 2021 · 6 comments · Fixed by #70723
Closed

kv/kvserver: TestTenantRateLimiter failed #70456

cockroach-teamcity opened this issue Sep 20, 2021 · 6 comments · Fixed by #70723
Assignees
Labels
branch-master Failures and bugs on the master branch. C-test-failure Broken test (automatically or manually discovered). O-robot Originated from a bot. skipped-test

Comments

@cockroach-teamcity
Copy link
Member

kv/kvserver.TestTenantRateLimiter failed with artifacts on master @ bfdcfa00836232b910cb0fd109ebedf8913b1466:

=== RUN   TestTenantRateLimiter
    test_log_scope.go:79: test logs captured to: /go/src/github.com/cockroachdb/cockroach/artifacts/logTestTenantRateLimiter069762263
    test_log_scope.go:80: use -show-logs to present logs inline
    client_tenant_test.go:248: 
        	Error Trace:	client_tenant_test.go:248
        	Error:      	
        	Test:       	TestTenantRateLimiter
    panic.go:613: -- test log scope end --
test logs left over in: /go/src/github.com/cockroachdb/cockroach/artifacts/logTestTenantRateLimiter069762263
--- FAIL: TestTenantRateLimiter (23.34s)
Reproduce

To reproduce, try:

make stressrace TESTS=TestTenantRateLimiter PKG=./pkg/kv/kvserver TESTTIMEOUT=5m STRESSFLAGS='-timeout 5m' 2>&1

Parameters in this failure:

  • GOFLAGS=-json

/cc @cockroachdb/kv ajwerner

This test on roachdash | Improve this report!

@cockroach-teamcity cockroach-teamcity added branch-master Failures and bugs on the master branch. C-test-failure Broken test (automatically or manually discovered). O-robot Originated from a bot. labels Sep 20, 2021
@cockroach-teamcity
Copy link
Member Author

kv/kvserver.TestTenantRateLimiter failed with artifacts on master @ 85b1e92455eb2795043d71e5766b1b56dab308f2:

=== RUN   TestTenantRateLimiter
    test_log_scope.go:79: test logs captured to: /go/src/github.com/cockroachdb/cockroach/artifacts/logTestTenantRateLimiter750218330
    test_log_scope.go:80: use -show-logs to present logs inline
    client_tenant_test.go:248: 
        	Error Trace:	client_tenant_test.go:248
        	Error:      	
        	Test:       	TestTenantRateLimiter
    panic.go:613: -- test log scope end --
test logs left over in: /go/src/github.com/cockroachdb/cockroach/artifacts/logTestTenantRateLimiter750218330
--- FAIL: TestTenantRateLimiter (31.25s)
Reproduce

To reproduce, try:

make stressrace TESTS=TestTenantRateLimiter PKG=./pkg/kv/kvserver TESTTIMEOUT=5m STRESSFLAGS='-timeout 5m' 2>&1

Parameters in this failure:

  • GOFLAGS=-json

/cc @cockroachdb/kv ajwerner

This test on roachdash | Improve this report!

@tbg
Copy link
Member

tbg commented Sep 24, 2021

The failure is on this line:

require.Contains(t, getMetrics(), makeMetricStr(int64(tooManyWrites)))

It's curious that it doesn't output anything. There should be a message "X does not contain Y", and the objects we're passing to Contains are both strings (i.e. there's no internal panic trying to do something with whatever we're passing). Also, getMetrics does not call t.Helper() so if somehow the error is from there it should get reported from there as well.

Think we'll need to repro this to see what's going on.

@stevendanna
Copy link
Collaborator

Also, getMetrics does not call t.Helper() so if somehow the error is from there it should get reported from there as well.

Interesting, I was also looking into this and hadn't excluded getMetrics() from what I was looking at. It seems to happen in CI under bazel a bit more often than the non-bazel job.

@cockroach-teamcity
Copy link
Member Author

kv/kvserver.TestTenantRateLimiter failed with artifacts on master @ a0d6d46d3283be18483e3d449adc7d90a02649c1:

=== RUN   TestTenantRateLimiter
    test_log_scope.go:79: test logs captured to: /go/src/github.com/cockroachdb/cockroach/artifacts/logTestTenantRateLimiter129364174
    test_log_scope.go:80: use -show-logs to present logs inline
    client_tenant_test.go:248: 
        	Error Trace:	client_tenant_test.go:248
        	Error:      	
        	Test:       	TestTenantRateLimiter
    panic.go:613: -- test log scope end --
test logs left over in: /go/src/github.com/cockroachdb/cockroach/artifacts/logTestTenantRateLimiter129364174
--- FAIL: TestTenantRateLimiter (31.99s)
Reproduce

To reproduce, try:

make stressrace TESTS=TestTenantRateLimiter PKG=./pkg/kv/kvserver TESTTIMEOUT=5m STRESSFLAGS='-timeout 5m' 2>&1

Parameters in this failure:

  • GOFLAGS=-json

/cc @cockroachdb/kv ajwerner

This test on roachdash | Improve this report!

tbg added a commit to tbg/cockroach that referenced this issue Sep 24, 2021
Refs: cockroachdb#70456

Reason: flaky test

Generated by bin/skip-test.

Release justification: non-production code changes

Release note: None
craig bot pushed a commit that referenced this issue Sep 24, 2021
68740: opt: support BYTES for histogram range calculations r=mgartner a=mgartner

Fixes #68346

Release note (performance improvement): The accuracy of histogram
calculations for BYTES types has been improved. As a result, the
optimizer should generate more efficient query plans in some cases.

70710: kv/kvserver: skip TestTenantRateLimiter r=aayushshah15 a=tbg

Refs: #70456

Reason: flaky test

Generated by bin/skip-test.

Release justification: non-production code changes

Release note: None

Co-authored-by: Marcus Gartner <[email protected]>
Co-authored-by: Tobias Grieger <[email protected]>
@RaduBerinde RaduBerinde self-assigned this Sep 24, 2021
@RaduBerinde
Copy link
Member

I can repro locally. Instead of

kv_tenant_rate_limit_write_requests_admitted{store="1",tenant_id="10"} 24002

we have

kv_tenant_rate_limit_write_requests_admitted{store="1",tenant_id="10"} 24003

Some background write has snuck in and messed with out strict check. Almost surely triggered by my recent change to increase the limits.

@RaduBerinde
Copy link
Member

It's curious that it doesn't output anything. There should be a message "X does not contain Y",

I'm not sure exactly how it happens, but I think it's because the metrics string is very long.

craig bot pushed a commit that referenced this issue Sep 25, 2021
70723: kv: deflake TestTenantRateLimiter r=RaduBerinde a=RaduBerinde

A recent change (#70328) increased the rate limit and that exposed a
fragility in this test. This change restores the test to use the
original (small) rate limit.

Fixes #70456.

Release note: None

Release justification: test-only fix.

Co-authored-by: Radu Berinde <[email protected]>
@craig craig bot closed this as completed in c37f179 Sep 25, 2021
blathers-crl bot pushed a commit that referenced this issue Sep 25, 2021
A recent change (#70328) increased the rate limit and that exposed a
fragility in this test. This change restores the test to use the
original (small) rate limit.

Fixes #70456.

Release note: None

Release justification: test-only fix.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch-master Failures and bugs on the master branch. C-test-failure Broken test (automatically or manually discovered). O-robot Originated from a bot. skipped-test
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants