Repository navigation
test(spanner): cleanup orphaned test tables and re-enable integration tests - #4427
camposperez-hub wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces automated cleanup of orphaned Spanner resources (tables and property graphs) older than two hours during integration tests, utilizing a new naming helper newSpannerTestResourceName that embeds creation timestamps. It also updates teardown steps to use context.WithoutCancel to ensure cleanup runs even if the main test context is canceled, and configures the CI pipeline to fail if Spanner integration tests fail. Feedback suggests reverting the Spanner client initialization to use context.Background() instead of the test context to prevent background session pool maintenance from stopping, and wrapping the context.WithoutCancel teardown contexts with a timeout to avoid blocking indefinitely if Spanner hangs.
|
|
||
| // Create Spanner client (for queries) | ||
| dataClient, err := spanner.NewClient(context.Background(), db) | ||
| dataClient, err := spanner.NewClient(ctx, db) |
There was a problem hiding this comment.
Passing a context with a timeout or cancellation (like the test's ctx) to spanner.NewClient is generally discouraged. The Spanner client uses this context to run background session pool maintenance tasks. If the context is canceled, the session pool maintainer will stop, which can cause subsequent operations to fail. It is recommended to keep using context.Background() for client initialization, and pass the test-specific ctx to individual queries and transactions instead.
| dataClient, err := spanner.NewClient(ctx, db) | |
| dataClient, err := spanner.NewClient(context.Background(), db) |
| op, err = adminClient.UpdateDatabaseDdl(context.WithoutCancel(ctx), &databasepb.UpdateDatabaseDdlRequest{ | ||
| Database: dbString, | ||
| Statements: []string{fmt.Sprintf("DROP TABLE IF EXISTS %s", tableName)}, | ||
| }) | ||
| if err != nil { | ||
| t.Errorf("unable to start drop %s operation: %s", tableName, err) | ||
| return | ||
| } | ||
| opErr := op.Wait(ctx) | ||
| opErr := op.Wait(context.WithoutCancel(ctx)) |
There was a problem hiding this comment.
Using context.WithoutCancel(ctx) without a timeout means that if the Spanner admin client or connection hangs during teardown, the test runner will block indefinitely. It is safer to wrap it with a reasonable timeout (e.g., 2 minutes) to ensure the teardown eventually times out and fails fast if Spanner is unresponsive.
teardownCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 2*time.Minute)
defer cancel()
op, err = adminClient.UpdateDatabaseDdl(teardownCtx, &databasepb.UpdateDatabaseDdlRequest{
Database: dbString,
Statements: []string{fmt.Sprintf("DROP TABLE IF EXISTS %s", tableName)},
})
if err != nil {
t.Errorf("unable to start drop %s operation: %s", tableName, err)
return
}
opErr := op.Wait(teardownCtx)| teardownCtx := context.WithoutCancel(ctx) | ||
| op, err = adminClient.UpdateDatabaseDdl(teardownCtx, &databasepb.UpdateDatabaseDdlRequest{ | ||
| Database: dbString, | ||
| Statements: []string{fmt.Sprintf("DROP TABLE IF EXISTS %s", tableName)}, | ||
| }) | ||
| if err != nil { | ||
| t.Errorf("unable to start drop %s operation: %s", tableName, err) | ||
| return | ||
| } | ||
| opErr := op.Wait(ctx) | ||
| opErr := op.Wait(teardownCtx) |
There was a problem hiding this comment.
Using context.WithoutCancel(ctx) without a timeout means that if the Spanner admin client or connection hangs during teardown, the test runner will block indefinitely. It is safer to wrap it with a reasonable timeout (e.g., 2 minutes) to ensure the teardown eventually times out and fails fast if Spanner is unresponsive.
teardownCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 2*time.Minute)
defer cancel()
op, err = adminClient.UpdateDatabaseDdl(teardownCtx, &databasepb.UpdateDatabaseDdlRequest{
Database: dbString,
Statements: []string{fmt.Sprintf("DROP TABLE IF EXISTS %s", tableName)},
})
if err != nil {
t.Errorf("unable to start drop %s operation: %s", tableName, err)
return
}
opErr := op.Wait(teardownCtx)
Description
Spanner integration tests have been failing and suppressed in CI (
|| echo "Integration tests failed.") becausetest_databaseonspanner-testingaccumulated 5,000 leaked test tables and 250 leaked property graphs (hitting Spanner's per-database schema limits of 5,000 tables and 250 property graphs).This PR resolves the resource leak, speeds up DDL setup/teardown, and re-enables the Spanner integration test check in CI:
<prefix>_<unix_ts>_<uuid>): Prefixes Spanner test tables and property graphs with their Unix creation timestamp (newSpannerTestResourceName) so age-based garbage collection can identify stale resources without interfering with concurrent test runs.> 2h+ legacy un-timestamped resources): AddscleanupOldSpannerResourcesandcleanupOldSpannerPgTablesto queryINFORMATION_SCHEMA.PROPERTY_GRAPHSandINFORMATION_SCHEMA.TABLESbefore test execution and drop orphaned resources older than 2 hours in dependency tiers (PROPERTY GRAPH->edge_table_*->node_table_*/ other test tables) in DDL batches of 1,000.CREATEDDL operations and 7 sequentialDROPDDL operations inTestSpannerToolEndpointsinto 1 batchedCREATEUpdateDatabaseDdlcall and 1 atomicDROPUpdateDatabaseDdlcall withcontext.WithoutCancel(ctx), preventing partial teardown leaks on timeout.stopSpannerServer(cmd.Close(),cmd.Wait(...), andhttp.DefaultClient.CloseIdleConnections()) soTestSpannerPostgresqlToolEndpointsdoes not reuse stale keep-alive connections to:5000fromTestSpannerToolEndpoints.|| echo "Integration tests failed."from thespannershard in.ci/integration.cloudbuild.yaml.PR Checklist
!if this involves a breaking change