Skip to content

Commit

Permalink
Merge #72364 #72380
Browse files Browse the repository at this point in the history 
72364: sql: destroy_tenant queues gc job r=JeffSwenson a=JeffSwenson

Previously, destory_tenant marked the tenant as inactive. A seperate
call to gc_tenant was required to trigger the deletion. Now, the
gc is scheduled by the destroy_tenant command. The gc_tenant command
will be deleted once this change is live in the Cockroach Cloud
Serverless clusters and all inactive tenants are deleted.

Release note: None

72380: Makefile: add proto & log dependencies to roachvet r=knz a=stevendanna

`bin/roachvet` depends on `pkg/testutils/lint/passes/fmtsafe` which
depends on `pkg/util/log/logpb` which requires that the protobufs are
compiled.

This could result in the following failure if you happened to run
`make lint` after pulling in a protobuf update (but before any other
target that would recompile the protos):

```
pkg/util/log/logpb/severity.go:19:10: undefined: Severity
pkg/util/log/logpb/severity.go:20:16: undefined: Severity
pkg/util/log/logpb/severity.go:29:15: undefined: Severity
pkg/util/log/logpb/severity.go:36:9: undefined: Severity
pkg/util/log/logpb/severity.go:36:46: undefined: Severity_UNKNOWN
pkg/util/log/logpb/severity.go:39:9: undefined: Severity
pkg/util/log/logpb/severity.go:42:10: undefined: Severity
pkg/util/log/logpb/severity.go:48:32: undefined: Severity
pkg/util/log/logpb/severity.go:63:10: undefined: Severity
pkg/util/log/logpb/severity.go:72:9: undefined: Severity
pkg/util/log/logpb/severity.go:36:46: too many errors
make: *** [Makefile:1788: bin/roachvet] Error 2
make: *** Waiting for unfinished jobs....
```

Release note: None

Co-authored-by: Jeff <[email protected]>
Co-authored-by: Steven Danna <[email protected]>
  • Loading branch information
@jeffswenson @stevendanna
3 people committed Nov 3, 2021
3 parents f339331 + 8a0066b + 6a7a878 commit dc120d6
Show file tree
Hide file tree
Showing 6 changed files with 19 additions and 33 deletions.
2 changes: 1 addition & 1 deletion Makefile
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1764,7 +1764,7 @@ logictest-bins := bin/logictest bin/logictestopt bin/logictestccl
# Additional dependencies for binaries that depend on generated code.
#
# TODO(benesch): Derive this automatically. This is getting out of hand.
bin/workload bin/docgen bin/execgen bin/roachtest $(logictest-bins): $(SQLPARSER_TARGETS) $(LOG_TARGETS) $(PROTOBUF_TARGETS)
bin/workload bin/docgen bin/execgen bin/roachtest bin/roachvet $(logictest-bins): $(SQLPARSER_TARGETS) $(LOG_TARGETS) $(PROTOBUF_TARGETS)
bin/workload bin/docgen bin/roachtest $(logictest-bins): $(LIBPROJ) $(CGO_FLAGS_FILES)
bin/roachtest $(logictest-bins): $(C_LIBS_CCL) $(CGO_FLAGS_FILES) $(OPTGEN_TARGETS) | $(C_LIBS_DYNAMIC)

Expand Down
5 changes: 1 addition & 4 deletions pkg/ccl/backupccl/backup_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7123,11 +7123,9 @@ func TestBackupRestoreTenant(t *testing.T) {
[][]string{{`10`, `false`, `{"id": "10", "state": "DROP"}`}},
)

// Make GC jobs run in 1 second.
// Make GC job scheduled by destroy_tenant run in 1 second.
restoreDB.Exec(t, "SET CLUSTER SETTING kv.range_merge.queue_enabled = false")
restoreDB.Exec(t, "ALTER RANGE tenants CONFIGURE ZONE USING gc.ttlseconds = 1;")
// Now run the GC job to delete the tenant and its data.
restoreDB.Exec(t, `SELECT crdb_internal.gc_tenant(10)`)
// Wait for tenant GC job to complete.
restoreDB.CheckQueryResultsRetry(
t,
Expand Down Expand Up @@ -7159,7 +7157,6 @@ func TestBackupRestoreTenant(t *testing.T) {
restoreTenant10.CheckQueryResults(t, `select * from foo.bar2`, tenant10.QueryStr(t, `select * from foo.bar2`))

restoreDB.Exec(t, `SELECT crdb_internal.destroy_tenant(10)`)
restoreDB.Exec(t, `SELECT crdb_internal.gc_tenant(10)`)
// Wait for tenant GC job to complete.
restoreDB.CheckQueryResultsRetry(
t,
Expand Down
3 changes: 2 additions & 1 deletion pkg/ccl/logictestccl/testdata/logic_test/tenant_usage
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@ SELECT crdb_internal.update_tenant_resource_limits(5, 1000, 100, 0, now(), 0)

# TODO(radu): inspect internal tenant_usage state.

# Note this just marks the tenant as dropped but does not call GC.
# Note this marks the tenant as dropped. The GC will not delete the tenant
# until after the ttl expires.
query I
SELECT crdb_internal.destroy_tenant(5)
----
Expand Down
15 changes: 3 additions & 12 deletions pkg/sql/logictest/testdata/logic_test/tenant
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,18 +46,9 @@ id active crdb_internal.pb_to_json
5 false {"id": "5", "state": "DROP"}
10 true {"id": "10", "state": "ACTIVE"}

# Try to recreate an existing tenant.

query error pgcode 42710 tenant "5" already exists
SELECT crdb_internal.create_tenant(5)

query I
SELECT crdb_internal.gc_tenant(5)
----
5

# GC job has not run yet.

# Try to recreate an existing tenant.
# GC job for tenant 5 has not run yet.
query error pgcode 42710 tenant "5" already exists
SELECT crdb_internal.create_tenant(5)

Expand Down Expand Up @@ -142,7 +133,7 @@ SELECT status FROM [
succeeded

query error pgcode 42704 tenant "5" does not exist
SELECT crdb_internal.gc_tenant(5)
SELECT crdb_internal.destroy_tenant(5)

query IBT colnames
SELECT id, active, crdb_internal.pb_to_json('cockroach.sql.sqlbase.TenantInfo', info, true)
Expand Down
8 changes: 2 additions & 6 deletions pkg/sql/sem/builtins/builtins.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4566,12 +4566,6 @@ value if you rely on the HLC for accuracy.`,
}
return args[0], nil
},
// TODO(spaskob): this built-in currently does not actually delete the
// data but just marks it as DROP. This is for done for safety in case we
// would like to restore the tenant later. If data in needs to be removed
// use gc_tenant built-in.
// We should just add a new built-in called `drop_tenant` instead and use
// this one to really destroy the tenant.
Info: "Destroys a tenant with the provided ID. Must be run by the System tenant.",
Volatility: tree.VolatilityVolatile,
},
Expand Down Expand Up @@ -5663,6 +5657,8 @@ value if you rely on the HLC for accuracy.`,
),

"crdb_internal.gc_tenant": makeBuiltin(
// TODO(jeffswenson): Delete internal_crdb.gc_tenant after the DestroyTenant
// changes are deployed to all Cockroach Cloud serverless hosts.
tree.FunctionProperties{
Category: categoryMultiTenancy,
Undocumented: true,
Expand Down
19 changes: 10 additions & 9 deletions pkg/sql/tenant.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -330,11 +330,6 @@ func clearTenant(ctx context.Context, execCfg *ExecutorConfig, info *descpb.Tena
}

// DestroyTenant implements the tree.TenantOperator interface.
// TODO(spaskob): this function currently does not actually delete the data but
// just marks it as DROP. This is for done for safety in case we would like to
// restore the tenant later.
// We should just add a new function DropTenant to the interface and convert
// this one to really remove the tenant and its data.
func (p *planner) DestroyTenant(ctx context.Context, tenID uint64) error {
const op = "destroy"
if err := rejectIfCantCoordinateMultiTenancy(p.execCfg.Codec, op); err != nil {
Expand All @@ -356,7 +351,11 @@ func (p *planner) DestroyTenant(ctx context.Context, tenID uint64) error {

// Mark the tenant as dropping.
info.State = descpb.TenantInfo_DROP
return errors.Wrap(updateTenantRecord(ctx, p.execCfg, p.txn, info), "destroying tenant")
if err := updateTenantRecord(ctx, p.execCfg, p.txn, info); err != nil {
return errors.Wrap(err, "destroying tenant")
}

return errors.Wrap(gcTenantJob(ctx, p.execCfg, p.txn, p.User(), tenID), "scheduling gc job")
}

// GCTenantSync clears the tenant's data and removes its record.
Expand Down Expand Up @@ -394,8 +393,8 @@ func GCTenantSync(ctx context.Context, execCfg *ExecutorConfig, info *descpb.Ten
return errors.Wrapf(err, "deleting tenant %d record", info.ID)
}

// GCTenantJob clears the tenant's data and removes its record using a GC job.
func GCTenantJob(
// gcTenantJob clears the tenant's data and removes its record using a GC job.
func gcTenantJob(
ctx context.Context,
execCfg *ExecutorConfig,
txn *kv.Txn,
Expand Down Expand Up @@ -425,6 +424,8 @@ func GCTenantJob(

// GCTenant implements the tree.TenantOperator interface.
func (p *planner) GCTenant(ctx context.Context, tenID uint64) error {
// TODO(jeffswenson): Delete internal_crdb.gc_tenant after the DestroyTenant
// changes are deployed to all Cockroach Cloud serverless hosts.
if !p.ExtendedEvalContext().TxnImplicit {
return errors.Errorf("gc_tenant cannot be used inside a transaction")
}
Expand All @@ -442,7 +443,7 @@ func (p *planner) GCTenant(ctx context.Context, tenID uint64) error {
return errors.Errorf("tenant %d is not in state DROP", info.ID)
}

return GCTenantJob(ctx, p.ExecCfg(), p.Txn(), p.User(), tenID)
return gcTenantJob(ctx, p.ExecCfg(), p.Txn(), p.User(), tenID)
}

// UpdateTenantResourceLimits implements the tree.TenantOperator interface.
Expand Down

0 comments on commit dc120d6

Please sign in to comment.