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

diagnosticsccl: TestUsageQuantization fails with secondary tenants #106901

Closed
knz opened this issue Jul 17, 2023 · 0 comments · Fixed by #107807
Closed

diagnosticsccl: TestUsageQuantization fails with secondary tenants #106901

knz opened this issue Jul 17, 2023 · 0 comments · Fixed by #107807
Assignees
@zachlite
Labels
A-cluster-observability Related to cluster observability A-multitenancy Related to multi-tenancy C-test-failure Broken test (automatically or manually discovered).

Comments

@knz
Copy link
Contributor

knz commented Jul 17, 2023 •
edited
Loading

Issue extracted from #106899.

Epic: CRDB-26687
Found while investigating #76378.

Describe the problem

The test TestUsageQuantization crashes when pointed to a secondary tenant.

*
* ERROR: a panic has occurred!
* runtime error: invalid memory address or nil pointer dereference
* (1) attached stack trace
*   -- stack trace:
*   | runtime.gopanic
*   |   GOROOT/src/runtime/panic.go:884
*   | github.com/cockroachdb/cockroach/pkg/util/stop.(*Stopper).Stop
*   |   github.com/cockroachdb/cockroach/pkg/util/stop/stopper.go:521
*   | runtime.gopanic
*   |   GOROOT/src/runtime/panic.go:884
*   | runtime.panicmem
*   |   GOROOT/src/runtime/panic.go:260
*   | runtime.sigpanic
*   |   GOROOT/src/runtime/signal_unix.go:839
*   | pkg/ccl/serverccl/diagnosticsccl/diagnosticsccl_test_test.TestUsageQuantization
*   |   pkg/ccl/serverccl/diagnosticsccl/diagnosticsccl_test_test/pkg/ccl/serverccl/diagnosticsccl/reporter_test.go:360

Which points to this call:

last := r.LastRequestData()
for _, s := range last.SqlStats // <- here

The last pointer is nil in that case.

To Reproduce

On top of the #106899 branch, modify the test to replace TestDoesNotWorkWithSecondaryTenantsButWeDontKnowWhyYet by TestTenantAlwaysEnabled.

Expected behavior

The test should work the same whether or not a secondary tenant is used.
In an ideal world the parameter DefaultTestTenant is omitted and the default value Probabilistic can be used.

Jira issue: CRDB-29755
@knz knz added the C-test-failure Broken test (automatically or manually discovered). label Jul 17, 2023
knz added a commit to knz/cockroach that referenced this issue Jul 17, 2023
@knz
diagnosticsccl: remove uses of TODOTestTenantDisabled
1ed2e7f 
Followup issue: cockroachdb#106901

Release note: None
@knz knz mentioned this issue Jul 17, 2023
Merged
@knz knz added A-multitenancy Related to multi-tenancy T-cluster-observability A-cluster-observability Related to cluster observability labels Jul 17, 2023
@knz knz mentioned this issue Jul 17, 2023
Merged
craig bot pushed a commit that referenced this issue Jul 17, 2023
@miraradeva @knz
Merge #106760 #106899
b87dec8 
106760: kvserver: deflake TestMergeQueueSeesNonVoters r=tbg a=miraradeva

TestMergeQueueSeesNonVoters was flaky because of a race between turning the merge queue on and off. The merge queue was first turned on via a cluster config, and then turned off using the store directly. However, it's possible for the cluster config to take effect after the queue was turned off via the store and leave the merge queue on for the rest of the test, causing unexpected range merges. The cluster config is actually unnecessary, so this change just removes it.

Fixes: #105726
Epic: CRDB-29164

Release note: None

106899: diagnosticsccl: remove uses of `TODOTestTenantDisabled` r=dhartunian a=knz

Informs #76378 
Epic: CRDB-18499
First commit from #106900.

Followup issue: #106901


Co-authored-by: Mira Radeva <[email protected]>
Co-authored-by: Raphael 'kena' Poss <[email protected]>
knz added a commit to knz/cockroach that referenced this issue Jul 17, 2023
@knz
serverccl: remove remaining uses of TODOTestTenantDisabled
c701779 
This also simplifies some tests so they don't need to start an entire
cluster.

The remaining mentions of `TODOTestTenantDisabled` are handled in
issues  cockroachdb#106897, cockroachdb#106903 and cockroachdb#106901.

Release note: None
knz added a commit to knz/cockroach that referenced this issue Jul 18, 2023
@knz
serverccl: remove remaining uses of TODOTestTenantDisabled
e5a7c25 
This also simplifies some tests so they don't need to start an entire
cluster.

The remaining mentions of `TODOTestTenantDisabled` are handled in
issues  cockroachdb#106897, cockroachdb#106903 and cockroachdb#106901.

Release note: None
craig bot pushed a commit that referenced this issue Jul 18, 2023
@knz
Merge #106910
1cee475 
106910: serverccl: remove remaining uses of TODOTestTenantDisabled r=yuzefovich a=knz

Epic: CRDB-18499
Informs #76378.

This also simplifies some tests so they don't need to start an entire
cluster.

The remaining mentions of `TODOTestTenantDisabled` are handled in
issues  #106897, #106903 and #106901.



Co-authored-by: Raphael 'kena' Poss <[email protected]>
@zachlite zachlite mentioned this issue Jul 28, 2023
Merged
craig bot pushed a commit that referenced this issue Jul 28, 2023
@xinhaoz @zachlite
Merge #107563 #107798 #107807
7ba8059 
107563: acceptance: make `TestDockerCLI_test_exec_log` wait for unambiguous l… r=abarganier a=xinhaoz

…og line

The `TestDockerCLI_test_exec_log` test had a section where it waits for the last executed stmt to appear in the exec_log file before proceeding to the next section. This is done by grep'ing the query string of the last stmt. This works fine most of the time, however, the 2nd last and last stmt executed happen to have the same query string, so it's possible that we are only seeing the 2nd to last stmt in the log. This can cause the test to fail as in the next section, we expect all query logs to be present in the file.

This commit fixes this by issuing a distinct query as the last query so that we can grep for a distinct query string to verify all stmts should have been written.

A new proc is also added to `common.tcl`: flush_and_sync_logs is a proc that takes 2 arguments, `filename` and `grep_text`. It will trigger a signal hang up to trigger a log flush and then attempt to find the `grep_text` in the specified file.


Epic: none
Fixes: #106367

107798: cluster-ui: delete unused vars r=xinhaoz a=xinhaoz

Delete unused vars in cluster-ui. A following commit will turn unused vars to errors via the eslint config.

Epic: None

Release note: None

107807: diagnosticsccl: fix TestUsageQuantization r=zachlite a=zachlite

TestUsageQuantization failed with secondary tenants because the test's server.TestingKnobs were not being passed through to test tenant creation.

Resolves #106901
Epic: none
Release note: None

Co-authored-by: Xin Hao Zhang <[email protected]>
Co-authored-by: zachlite <[email protected]>
@craig craig bot closed this as completed in 9e5de35 Jul 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@zachlite zachlite

Labels
A-cluster-observability Related to cluster observability A-multitenancy Related to multi-tenancy C-test-failure Broken test (automatically or manually discovered).
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

diagnosticsccl: fix TestUsageQuantization zachlite/cockroach
2 participants
@knz @zachlite