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

server: investigate why TestAdminDebugRedirect does not work with a shared process virtual cluster #112955

Closed
herkolategan opened this issue Oct 24, 2023 · 0 comments · Fixed by #120653
Closed

server: investigate why TestAdminDebugRedirect does not work with a shared process virtual cluster #112955

herkolategan opened this issue Oct 24, 2023 · 0 comments · Fixed by #120653
Labels
C-investigation Further steps needed to qualify. C-label will change. T-multitenant Issues owned by the multi-tenant virtual team

Comments

@herkolategan
Copy link
Collaborator

herkolategan commented Oct 24, 2023 •
edited
Loading

Describe the problem

When running the test with a shared process virtual cluster:

=== RUN   TestAdminDebugRedirect
    test_log_scope.go:170: test logs captured to: /var/folders/sh/9k10t77143q0f1ffxxv99lbh0000gq/T/logTestAdminDebugRedirect3513466581
    test_log_scope.go:81: use -show-logs to present logs inline
    debug_test.go:222: expected location https://127.0.0.1:64036/debug/?cluster=test-tenant; got https://127.0.0.1:64036/debug/
    debug_test.go:225: -- test log scope end --

How to reproduce

Set the following in TestServerArgs to force a shared process tenant and cause the failure to happen.

DefaultTestTenant: base.SharedTestTenantAlwaysEnabled,

Expected behavior
The test should pass when running with a shared process virtual cluster, unless after investigation the test is expected not to work with one.

Ref: #112857
Epic: CRDB-26687

Jira issue: CRDB-32692
@herkolategan herkolategan added C-investigation Further steps needed to qualify. C-label will change. T-multitenant Issues owned by the multi-tenant virtual team labels Oct 24, 2023
@dhartunian dhartunian mentioned this issue Mar 18, 2024
Merged
dhartunian added a commit to dhartunian/cockroach that referenced this issue Mar 18, 2024
@dhartunian
server: refactor TestAdminDebugRedirect test
0926796 
Adjusts test to use more standard redirect ignorig behavior in stdlib,
and removes the test tenant override since this test works with
tenants now after some adjustements to URL handling.

The cockroachdb#120095 issue was a timeout that this change doesn't explicitly
deal with here since that problem isn't reproducible. The hope is
that modified redirect error handling might trigger a less error-prone
branch in the HTTP-client. There's nothing else to really change in
this test since it's quite simple and we haven't seen similar timeouts
persist in other HTTP tests.

Resolves: cockroachdb#120095
Resolves: cockroachdb#112955
Epic: None

Release note: None
craig bot pushed a commit that referenced this issue Mar 19, 2024
@xinhaoz @dhartunian @rickystewart
Merge #120410 #120646 #120653 #120699 #120715
f0116ea 
120410: sql: allow additional fmt flags for stmt fingerprinting r=xinhaoz a=xinhaoz

This commit introduces a way to add additional format flags when
formatting a statement AST into its statement fingerprint representation
for sql stats. This allows us to more aggressively generalize the
statement fingerprint.

`sql.stats.statement_fingerprint.format_mask` will be used to supply
these additional flags to the formatter.  It is currently 0 by default
since no new flags for fingerprints have been added.

Epic: none
Part of: #120409

Release note: None

120646: telemetryccl: use log spy in backup/restore test r=abarganier a=dhartunian

Previously this test used file logging to test the telemetry output, which can result in flakes on CI. This commit modifies the test to use a log spy which is a bit more reliable. Additionally, the deserialization now happens in the `Intercept()` method which makes the test easier to read.

Resolves: #120115
Epic: None
Release note: None

120653: server: refactor TestAdminDebugRedirect test r=abarganier a=dhartunian

Adjusts test to use more standard redirect ignoring behavior in stdlib, and removes the test tenant override since this test works with tenants now after some adjustments to URL handling.

The #120095 issue was a timeout that this change doesn't explicitly deal with here since that problem isn't reproducible. The hope is that modified redirect error handling might trigger a less error-prone branch in the HTTP-client. There's nothing else to really change in this test since it's quite simple and we haven't seen similar timeouts persist in other HTTP tests.

Resolves: #120095
Resolves: #112955
Epic: None

Release note: None

120699: sql: skip TestSqlActivityUpdateTopLimitJob r=abarganier a=dhartunian

Release note: None

120715: workflows: tag `cockroach` builds for integration tests r=rail a=rickystewart

... with the tag `integration-test-artifact-build`. We do this to track how long it takes to build these artifacts specifically.

Epic: CRDB-8308
Release note: None

Co-authored-by: Xin Hao Zhang <[email protected]>
Co-authored-by: David Hartunian <[email protected]>
Co-authored-by: Ricky Stewart <[email protected]>
@craig craig bot closed this as completed in #120653 Mar 20, 2024
@craig craig bot closed this as completed in 1385f55 Mar 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
C-investigation Further steps needed to qualify. C-label will change. T-multitenant Issues owned by the multi-tenant virtual team
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

server: refactor TestAdminDebugRedirect test dhartunian/cockroach
1 participant
@herkolategan