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/tenantrate: TestCloser is flakey #86822

Closed
ajwerner opened this issue Aug 24, 2022 · 5 comments · Fixed by #88303
Closed

kv/kvserver/tenantrate: TestCloser is flakey #86822

ajwerner opened this issue Aug 24, 2022 · 5 comments · Fixed by #88303
Assignees
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. skipped-test T-kv KV Team

Comments

@ajwerner
Copy link
Contributor

ajwerner commented Aug 24, 2022

@ajwerner ajwerner added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Aug 24, 2022
@irfansharif irfansharif self-assigned this Aug 24, 2022
@irfansharif
Copy link
Contributor

Off of latest master.

$ dev test pkg/kv/kvserver/tenantrate -f TestCloser --stress
...
49356 runs so far, 0 failures, over 5m5s
50165 runs so far, 0 failures, over 5m10s
50986 runs so far, 0 failures, over 5m15s
51801 runs so far, 0 failures, over 5m20s

irfansharif added a commit to irfansharif/cockroach that referenced this issue Aug 24, 2022
Refs: cockroachdb#86822

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 Aug 24, 2022
86825: kv/kvserver/tenantrate: skip TestCloser r=irfansharif a=irfansharif

Refs: #86822

Reason: flaky test

Generated by bin/skip-test.

Release justification: non-production code changes
Release note: None

Co-authored-by: irfan sharif <[email protected]>
@irfansharif
Copy link
Contributor

Huh, ok repros on linux. Bisecting.

@irfansharif
Copy link
Contributor

On my GCE worker I trip things up as far back as 05bc43d using dev test pkg/kv/kvserver/tenantrate --stress. What's happening here?

@blathers-crl blathers-crl bot added the T-kv KV Team label Aug 24, 2022
@irfansharif
Copy link
Contributor

Bisecting took me conclusively to 61674bc (+cc @andy-kimball). Triple checked running the preceding commit and that one. I'll keep this on the KV plate for now to get to during stability. I'm not sure why this only starting firing in CI now but it should be easy to fix.

blathers-crl bot pushed a commit that referenced this issue Aug 26, 2022
Refs: #86822

Reason: flaky test

Generated by bin/skip-test.

Release justification: non-production code changes

Release note: None
@kvoli kvoli self-assigned this Sep 20, 2022
@irfansharif irfansharif removed their assignment Sep 20, 2022
@kvoli
Copy link
Collaborator

kvoli commented Sep 20, 2022

It looks like the update to RU accounting made it such that the request that was meant to hit a try again/retry timer in this test, doesn't.

The configured limit for the test:

tenant 2 rate limiter initialized (rate: 6000 RU/s; burst: 60000 RU)

The request is around 46k 1024Mb * 45. Which slides under the burst limit so the request never waits.

The updated accounting made this req cheaper I believe.

Pushing the req to double the size (1<<31) solves this issue.

craig bot pushed a commit that referenced this issue Sep 21, 2022
88303: tenantrate: deflake testcloser r=irfansharif a=kvoli

This patch increases the request size in `TestCloser` in order to jam the request, to assert on timers closing. An earlier patch lowered the RU cost below the burst threshold, making it so that this request never waited.

The test previously flaked after 1 run.

`53538 runs so far, 0 failures, over 2m5s`

resolves: #86822

Release note: None

Co-authored-by: Austen McClernon <[email protected]>
@craig craig bot closed this as completed in e59cbad Sep 21, 2022
blathers-crl bot pushed a commit that referenced this issue Sep 21, 2022
This patch increases the request size in `TestCloser` in order to jam
the request, to assert on timers closing. An earlier patch lowered the
RU cost below the burst threshold, making it so that this request never
waited.

The test previously flaked after 1 run.

`53538 runs so far, 0 failures, over 2m5s`

resolves: #86822

Release note: None
blathers-crl bot pushed a commit that referenced this issue Sep 21, 2022
This patch increases the request size in `TestCloser` in order to jam
the request, to assert on timers closing. An earlier patch lowered the
RU cost below the burst threshold, making it so that this request never
waited.

The test previously flaked after 1 run.

`53538 runs so far, 0 failures, over 2m5s`

resolves: #86822

Release note: None
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. skipped-test T-kv KV Team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants