From 415dc1a4b90397f712a76dca6bbd01fd360a1afe Mon Sep 17 00:00:00 2001 From: David Taylor Date: Wed, 16 Feb 2022 13:55:01 +0000 Subject: [PATCH 1/4] jobs: fix missing creation info in resumed jobs Release note: none. --- pkg/jobs/adopt.go | 12 +++++++++--- pkg/jobs/jobs_test.go | 20 ++++++++++++++++++-- 2 files changed, 27 insertions(+), 5 deletions(-) diff --git a/pkg/jobs/adopt.go b/pkg/jobs/adopt.go index 6adf59ce3d1c..6172791e09ff 100644 --- a/pkg/jobs/adopt.go +++ b/pkg/jobs/adopt.go @@ -141,8 +141,8 @@ COALESCE(last_run, created) + least( resumeQueryBaseCols = "status, payload, progress, crdb_internal.sql_liveness_is_alive(claim_session_id)" resumeQueryWhereBase = `id = $1 AND claim_session_id = $2` - resumeQueryWithBackoff = `SELECT ` + resumeQueryBaseCols + `, ` + canRunClause + ` AS can_run` + - ` FROM system.jobs, ` + canRunArgs + " WHERE " + resumeQueryWhereBase + resumeQueryWithBackoff = `SELECT ` + resumeQueryBaseCols + `, ` + canRunClause + ` AS can_run,` + + ` created_by_type, created_by_id FROM system.jobs, ` + canRunArgs + " WHERE " + resumeQueryWhereBase ) // getProcessQuery returns the query that selects the jobs that are claimed @@ -301,7 +301,13 @@ func (r *Registry) resumeJob(ctx context.Context, jobID jobspb.JobID, s sqlliven if err != nil { return err } - job := &Job{id: jobID, registry: r} + + createdBy, err := unmarshalCreatedBy(row[5], row[6]) + if err != nil { + return err + } + + job := &Job{id: jobID, registry: r, createdBy: createdBy} job.mu.payload = *payload job.mu.progress = *progress job.sessionID = s.ID() diff --git a/pkg/jobs/jobs_test.go b/pkg/jobs/jobs_test.go index ffbfd03c8ba0..8ab32bfba35d 100644 --- a/pkg/jobs/jobs_test.go +++ b/pkg/jobs/jobs_test.go @@ -1751,10 +1751,21 @@ func TestJobLifecycle(t *testing.T) { t.Run("job with created by fields", func(t *testing.T) { createdByType := "internal_test" + resumerJob := make(chan *jobs.Job, 1) + jobs.RegisterConstructor( + jobspb.TypeBackup, func(j *jobs.Job, _ *cluster.Settings) jobs.Resumer { + return jobs.FakeResumer{ + OnResume: func(ctx context.Context) error { + resumerJob <- j + return nil + }, + } + }) + jobID := registry.MakeJobID() record := jobs.Record{ - Details: jobspb.RestoreDetails{}, - Progress: jobspb.RestoreProgress{}, + Details: jobspb.BackupDetails{}, + Progress: jobspb.BackupProgress{}, CreatedBy: &jobs.CreatedByInfo{Name: createdByType, ID: 123}, } job, err := registry.CreateAdoptableJobWithTxn(ctx, record, jobID, nil /* txn */) @@ -1764,6 +1775,11 @@ func TestJobLifecycle(t *testing.T) { require.NoError(t, err) require.NotNil(t, loadedJob.CreatedBy()) require.Equal(t, job.CreatedBy(), loadedJob.CreatedBy()) + registry.TestingNudgeAdoptionQueue() + resumedJob := <-resumerJob + require.NotNil(t, resumedJob.CreatedBy()) + require.Equal(t, job.CreatedBy(), resumedJob.CreatedBy()) + }) } From c017ccbfd56c700c0260a6fe0ba66d93d7908b7e Mon Sep 17 00:00:00 2001 From: David Taylor Date: Wed, 16 Feb 2022 13:57:41 +0000 Subject: [PATCH 2/4] backup: move resolution from planning to exec Release note (ops change): the cluster setting bulkio.backup.resolve_destination_in_job.enabled can be used to delay resolution of backup's destination until the job starts running. --- .../settings/settings-for-tenants.txt | 1 + docs/generated/settings/settings.html | 1 + pkg/ccl/backupccl/backup_job.go | 87 +++++++++- pkg/ccl/backupccl/backup_planning.go | 159 ++++++++++++++---- 4 files changed, 208 insertions(+), 40 deletions(-) diff --git a/docs/generated/settings/settings-for-tenants.txt b/docs/generated/settings/settings-for-tenants.txt index c665a1c7ccb4..efdbd7efacb0 100644 --- a/docs/generated/settings/settings-for-tenants.txt +++ b/docs/generated/settings/settings-for-tenants.txt @@ -5,6 +5,7 @@ admission.sql_sql_response.enabled boolean true when true, work performed by the bulkio.backup.file_size byte size 128 MiB target size for individual data files produced during BACKUP bulkio.backup.read_timeout duration 5m0s amount of time after which a read attempt is considered timed out, which causes the backup to fail bulkio.backup.read_with_priority_after duration 1m0s amount of time since the read-as-of time above which a BACKUP should use priority when retrying reads +bulkio.backup.resolve_destination_in_job.enabled boolean false defer the interaction with the external storage used to resolve backup destination until the job starts bulkio.stream_ingestion.minimum_flush_interval duration 5s the minimum timestamp between flushes; flushes may still occur if internal buffers fill up changefeed.node_throttle_config string specifies node level throttling configuration for all changefeeeds cloudstorage.http.custom_ca string custom root CA (appended to system's default CAs) for verifying certificates when interacting with HTTPS storage diff --git a/docs/generated/settings/settings.html b/docs/generated/settings/settings.html index 4f1e1b6f9ad7..f9e4d287249c 100644 --- a/docs/generated/settings/settings.html +++ b/docs/generated/settings/settings.html @@ -7,6 +7,7 @@ bulkio.backup.file_sizebyte size128 MiBtarget size for individual data files produced during BACKUP bulkio.backup.read_timeoutduration5m0samount of time after which a read attempt is considered timed out, which causes the backup to fail bulkio.backup.read_with_priority_afterduration1m0samount of time since the read-as-of time above which a BACKUP should use priority when retrying reads +bulkio.backup.resolve_destination_in_job.enabledbooleanfalsedefer the interaction with the external storage used to resolve backup destination until the job starts bulkio.stream_ingestion.minimum_flush_intervalduration5sthe minimum timestamp between flushes; flushes may still occur if internal buffers fill up changefeed.node_throttle_configstringspecifies node level throttling configuration for all changefeeeds cloudstorage.http.custom_castringcustom root CA (appended to system's default CAs) for verifying certificates when interacting with HTTPS storage diff --git a/pkg/ccl/backupccl/backup_job.go b/pkg/ccl/backupccl/backup_job.go index bf2e457d4104..452d20baf2e1 100644 --- a/pkg/ccl/backupccl/backup_job.go +++ b/pkg/ccl/backupccl/backup_job.go @@ -36,6 +36,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" "github.com/cockroachdb/cockroach/pkg/sql/stats" + "github.com/cockroachdb/cockroach/pkg/util" "github.com/cockroachdb/cockroach/pkg/util/ctxgroup" "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/cockroach/pkg/util/mon" @@ -376,6 +377,80 @@ func (b *backupResumer) Resume(ctx context.Context, execCtx interface{}) error { details := b.job.Details().(jobspb.BackupDetails) p := execCtx.(sql.JobExecContext) + var backupManifest *BackupManifest + + // If planning didn't resolve the external destination, then we need to now. + if details.URI == "" { + initialDetails := details + backupDetails, m, err := getBackupDetailAndManifest( + ctx, p.ExecCfg(), p.ExtendedEvalContext().Txn, details, p.User(), + ) + if err != nil { + return err + } + details = backupDetails + backupManifest = &m + + if len(backupManifest.Spans) > 0 && p.ExecCfg().Codec.ForSystemTenant() { + protectedtsID := uuid.MakeV4() + details.ProtectedTimestampRecord = &protectedtsID + + if err := p.ExecCfg().DB.Txn(ctx, func(ctx context.Context, txn *kv.Txn) error { + return protectTimestampForBackup( + ctx, p.ExecCfg(), txn, b.job.ID(), m, details, + ) + }); err != nil { + return err + } + } + + if err := writeBackupManifestCheckpoint( + ctx, b.job.ID(), backupDetails, backupManifest, p.ExecCfg(), p.User(), + ); err != nil { + return err + } + + if err := p.ExecCfg().DB.Txn(ctx, func(ctx context.Context, txn *kv.Txn) error { + return planSchedulePTSChaining(ctx, p.ExecCfg(), txn, &details, b.job.CreatedBy()) + }); err != nil { + return err + } + + // The description picked during original planning might still say "LATEST", + // if resolving that to the actual directory only just happened above here. + // Ideally we'd re-render the description now that we know the subdir, but + // we don't have backup AST node anymore to easily call the rendering func. + // Instead we can just do a bit of dirty string replacement iff there is one + // "INTO 'LATEST' IN" (if there's >1, somenoe has a weird table/db names and + // we should just leave the description as-is, since it is just for humans). + description := b.job.Payload().Description + const unresolvedText = "INTO 'LATEST' IN" + if initialDetails.Destination.Subdir == "LATEST" && strings.Count(description, unresolvedText) == 1 { + description = strings.ReplaceAll(description, unresolvedText, fmt.Sprintf("INTO '%s' IN", details.Destination.Subdir)) + } + + // Update the job payload (non-volatile job definition) once, with the now + // resolved destination, updated description, etc. If we resume again we'll + // skip this whole block so this isn't an excessive update of payload. + if err := b.job.Update(ctx, nil, func(txn *kv.Txn, md jobs.JobMetadata, ju *jobs.JobUpdater) error { + if err := md.CheckRunningOrReverting(); err != nil { + return err + } + md.Payload.Details = jobspb.WrapPayloadDetails(details) + md.Payload.Description = description + ju.UpdatePayload(md.Payload) + return nil + }); err != nil { + return err + } + + // Collect telemetry, once per backup after resolving its destination. + lic := utilccl.CheckEnterpriseEnabled( + p.ExecCfg().Settings, p.ExecCfg().ClusterID(), p.ExecCfg().Organization(), "", + ) != nil + collectTelemetry(m, details, details, lic) + } + // For all backups, partitioned or not, the main BACKUP manifest is stored at // details.URI. defaultConf, err := cloud.ExternalStorageConfFromURI(details.URI, p.User()) @@ -424,16 +499,20 @@ func (b *backupResumer) Resume(ctx context.Context, execCtx interface{}) error { mem := p.ExecCfg().RootMemoryMonitor.MakeBoundAccount() defer mem.Close(ctx) - backupManifest, memSize, err := b.readManifestOnResume(ctx, &mem, p.ExecCfg(), defaultStore, details) - if err != nil { - return err - } + var memSize int64 defer func() { if memSize != 0 { mem.Shrink(ctx, memSize) } }() + if backupManifest == nil || util.ConstantWithMetamorphicTestBool("backup-read-manifest", false) { + backupManifest, memSize, err = b.readManifestOnResume(ctx, &mem, p.ExecCfg(), defaultStore, details) + if err != nil { + return err + } + } + statsCache := p.ExecCfg().TableStatsCache // We retry on pretty generic failures -- any rpc error. If a worker node were // to restart, it would produce this kind of error, but there may be other diff --git a/pkg/ccl/backupccl/backup_planning.go b/pkg/ccl/backupccl/backup_planning.go index aa7afa05cbfe..ee433efaba5b 100644 --- a/pkg/ccl/backupccl/backup_planning.go +++ b/pkg/ccl/backupccl/backup_planning.go @@ -47,6 +47,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/sql/privilege" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" + "github.com/cockroachdb/cockroach/pkg/util" "github.com/cockroachdb/cockroach/pkg/util/hlc" "github.com/cockroachdb/cockroach/pkg/util/interval" "github.com/cockroachdb/cockroach/pkg/util/log" @@ -88,6 +89,13 @@ var featureBackupEnabled = settings.RegisterBoolSetting( featureflag.FeatureFlagEnabledDefault, ).WithPublic() +var resolveDuringExec = settings.RegisterBoolSetting( + settings.TenantWritable, + "bulkio.backup.resolve_destination_in_job.enabled", + "defer the interaction with the external storage used to resolve backup destination until the job starts", + util.ConstantWithMetamorphicTestBool("backup-resolve-exec", false), +).WithPublic() + func (p *backupKMSEnv) ClusterSettings() *cluster.Settings { return p.settings } @@ -785,6 +793,75 @@ func backupPlanHook( initialDetails.SpecificTenantIds = []roachpb.TenantID{backupStmt.Targets.Tenant} } + jobID := p.ExecCfg().JobRegistry.MakeJobID() + + // TODO(dt): replace setting with cluster version gate. + if resolveDuringExec.Get(p.ExecCfg().SV()) { + description, err := backupJobDescription(p, + backupStmt.Backup, to, incrementalFrom, + encryptionParams.RawKmsUris, + initialDetails.Destination.Subdir, + initialDetails.Destination.IncrementalStorage, + ) + if err != nil { + return err + } + jr := jobs.Record{ + Description: description, + Details: initialDetails, + Progress: jobspb.BackupProgress{}, + CreatedBy: backupStmt.CreatedByInfo, + Username: p.User(), + DescriptorIDs: func() (sqlDescIDs []descpb.ID) { + for i := range targetDescs { + sqlDescIDs = append(sqlDescIDs, targetDescs[i].GetID()) + } + return sqlDescIDs + }(), + } + plannerTxn := p.ExtendedEvalContext().Txn + + if backupStmt.Options.Detached { + // When running inside an explicit transaction, we simply create the job + // record. We do not wait for the job to finish. + _, err := p.ExecCfg().JobRegistry.CreateAdoptableJobWithTxn( + ctx, jr, jobID, plannerTxn) + if err != nil { + return err + } + resultsCh <- tree.Datums{tree.NewDInt(tree.DInt(jobID))} + return nil + } + var sj *jobs.StartableJob + if err := func() (err error) { + defer func() { + if err == nil || sj == nil { + return + } + if cleanupErr := sj.CleanupOnRollback(ctx); cleanupErr != nil { + log.Errorf(ctx, "failed to cleanup job: %v", cleanupErr) + } + }() + if err := p.ExecCfg().JobRegistry.CreateStartableJobWithTxn(ctx, &sj, jobID, plannerTxn, jr); err != nil { + return err + } + // We commit the transaction here so that the job can be started. This + // is safe because we're in an implicit transaction. If we were in an + // explicit transaction the job would have to be run with the detached + // option and would have been handled above. + return plannerTxn.Commit(ctx) + }(); err != nil { + return err + } + if err := sj.Start(ctx); err != nil { + return err + } + if err := sj.AwaitCompletion(ctx); err != nil { + return err + } + return sj.ReportExecutionResults(ctx, resultsCh) + } + // TODO(dt): move this to job execution phase. backupDetails, backupManifest, err := getBackupDetailAndManifest( ctx, p.ExecCfg(), p.ExtendedEvalContext().Txn, initialDetails, p.User(), @@ -798,6 +875,10 @@ func backupPlanHook( return err } + // We create the job record in the planner's transaction to ensure that + // the job record creation happens transactionally. + plannerTxn := p.ExtendedEvalContext().Txn + // Write backup manifest into a temporary checkpoint file. // This accomplishes 2 purposes: // 1. Persists large state needed for backup job completion. @@ -807,23 +888,7 @@ func backupPlanHook( // // TODO (pbardea): For partitioned backups, also add verification for other // stores we are writing to in addition to the default. - doWriteBackupManifestCheckpoint := func(ctx context.Context, jobID jobspb.JobID) error { - defaultStore, err := p.ExecCfg().DistSQLSrv.ExternalStorageFromURI(ctx, backupDetails.URI, p.User()) - if err != nil { - return err - } - defer defaultStore.Close() - - if err := writeBackupManifest( - ctx, p.ExecCfg().Settings, defaultStore, tempCheckpointFileNameForJob(jobID), - backupDetails.EncryptionOptions, &backupManifest, - ); err != nil { - return errors.Wrapf(err, "writing checkpoint file") - } - return nil - } - - if err := planSchedulePTSChaining(ctx, p, &backupDetails, backupStmt); err != nil { + if err := planSchedulePTSChaining(ctx, p.ExecCfg(), plannerTxn, &backupDetails, backupStmt.CreatedByInfo); err != nil { return err } @@ -852,14 +917,8 @@ func backupPlanHook( p.ExecCfg().Settings, p.ExecCfg().ClusterID(), p.ExecCfg().Organization(), "", ) != nil - // We create the job record in the planner's transaction to ensure that - // the job record creation happens transactionally. - plannerTxn := p.ExtendedEvalContext().Txn - - jobID := p.ExecCfg().JobRegistry.MakeJobID() - if err := protectTimestampForBackup( - ctx, p, plannerTxn, jobID, backupManifest, backupDetails, + ctx, p.ExecCfg(), plannerTxn, jobID, backupManifest, backupDetails, ); err != nil { return err } @@ -873,7 +932,9 @@ func backupPlanHook( return err } - if err := doWriteBackupManifestCheckpoint(ctx, jobID); err != nil { + if err := writeBackupManifestCheckpoint( + ctx, jobID, backupDetails, &backupManifest, p.ExecCfg(), p.User(), + ); err != nil { return err } @@ -897,7 +958,9 @@ func backupPlanHook( if err := p.ExecCfg().JobRegistry.CreateStartableJobWithTxn(ctx, &sj, jobID, plannerTxn, jr); err != nil { return err } - if err := doWriteBackupManifestCheckpoint(ctx, jobID); err != nil { + if err := writeBackupManifestCheckpoint( + ctx, jobID, backupDetails, &backupManifest, p.ExecCfg(), p.User(), + ); err != nil { return err } @@ -1007,6 +1070,29 @@ func getScheduledBackupExecutionArgsFromSchedule( return sj, args, nil } +func writeBackupManifestCheckpoint( + ctx context.Context, + jobID jobspb.JobID, + backupDetails jobspb.BackupDetails, + backupManifest *BackupManifest, + execCfg *sql.ExecutorConfig, + user security.SQLUsername, +) error { + defaultStore, err := execCfg.DistSQLSrv.ExternalStorageFromURI(ctx, backupDetails.URI, user) + if err != nil { + return err + } + defer defaultStore.Close() + + if err := writeBackupManifest( + ctx, execCfg.Settings, defaultStore, tempCheckpointFileNameForJob(jobID), + backupDetails.EncryptionOptions, backupManifest, + ); err != nil { + return errors.Wrapf(err, "writing checkpoint file") + } + return nil +} + // planSchedulePTSChaining populates backupDetails with information relevant to // the chaining of protected timestamp records between scheduled backups. // Depending on whether backupStmt is a full or incremental backup, we populate @@ -1014,23 +1100,24 @@ func getScheduledBackupExecutionArgsFromSchedule( // completion of the backup job. func planSchedulePTSChaining( ctx context.Context, - p sql.PlanHookState, + execCfg *sql.ExecutorConfig, + txn *kv.Txn, backupDetails *jobspb.BackupDetails, - backupStmt *annotatedBackupStatement, + createdBy *jobs.CreatedByInfo, ) error { env := scheduledjobs.ProdJobSchedulerEnv - if knobs, ok := p.ExecCfg().DistSQLSrv.TestingKnobs.JobsTestingKnobs.(*jobs.TestingKnobs); ok { + if knobs, ok := execCfg.DistSQLSrv.TestingKnobs.JobsTestingKnobs.(*jobs.TestingKnobs); ok { if knobs.JobSchedulerEnv != nil { env = knobs.JobSchedulerEnv } } // If this is not a scheduled backup, we do not chain pts records. - if backupStmt.CreatedByInfo == nil || backupStmt.CreatedByInfo.Name != jobs.CreatedByScheduledJobs { + if createdBy == nil || createdBy.Name != jobs.CreatedByScheduledJobs { return nil } - _, args, err := getScheduledBackupExecutionArgsFromSchedule(ctx, env, - p.ExtendedEvalContext().Txn, p.ExecCfg().InternalExecutor, backupStmt.CreatedByInfo.ID) + _, args, err := getScheduledBackupExecutionArgsFromSchedule( + ctx, env, txn, execCfg.InternalExecutor, createdBy.ID) if err != nil { return err } @@ -1048,8 +1135,8 @@ func planSchedulePTSChaining( return nil } - _, incArgs, err := getScheduledBackupExecutionArgsFromSchedule(ctx, env, - p.ExtendedEvalContext().Txn, p.ExecCfg().InternalExecutor, args.DependentScheduleID) + _, incArgs, err := getScheduledBackupExecutionArgsFromSchedule( + ctx, env, txn, execCfg.InternalExecutor, args.DependentScheduleID) if err != nil { // If we are unable to resolve the dependent incremental schedule (it // could have been dropped) we do not need to perform any chaining. @@ -1240,7 +1327,7 @@ func getProtectedTimestampTargetForBackup(backupManifest BackupManifest) *ptpb.T func protectTimestampForBackup( ctx context.Context, - p sql.PlanHookState, + execCfg *sql.ExecutorConfig, txn *kv.Txn, jobID jobspb.JobID, backupManifest BackupManifest, @@ -1260,7 +1347,7 @@ func protectTimestampForBackup( target := getProtectedTimestampTargetForBackup(backupManifest) rec := jobsprotectedts.MakeRecord(*backupDetails.ProtectedTimestampRecord, int64(jobID), tsToProtect, backupManifest.Spans, jobsprotectedts.Jobs, target) - err := p.ExecCfg().ProtectedTimestampProvider.Protect(ctx, txn, rec) + err := execCfg.ProtectedTimestampProvider.Protect(ctx, txn, rec) if err != nil { return err } From e0de4756e41de4b73b92d526a316813bb7402d69 Mon Sep 17 00:00:00 2001 From: Santamaura Date: Thu, 17 Feb 2022 16:00:40 -0500 Subject: [PATCH 3/4] ui: change invalid lease to expired lease on problem ranges page When browsing the problem ranges page and invalid leases appears customers regularly are concerned with this status as they interpret it as something with the cluster is wrong which is often not the case. In order to imrpove this, the invalid lease section has been changed to expired lease and there is a description added below which explains that this status is often not a cause for concern. Release note (ui change): change invalid lease to expired lease on problem ranges page --- .../reports/containers/problemRanges/connectionsTable.tsx | 2 +- .../src/views/reports/containers/problemRanges/index.tsx | 7 ++++++- pkg/ui/workspaces/db-console/styl/pages/reports.styl | 3 +++ 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/pkg/ui/workspaces/db-console/src/views/reports/containers/problemRanges/connectionsTable.tsx b/pkg/ui/workspaces/db-console/src/views/reports/containers/problemRanges/connectionsTable.tsx index bdd7fccd8496..b9fa02ab2697 100644 --- a/pkg/ui/workspaces/db-console/src/views/reports/containers/problemRanges/connectionsTable.tsx +++ b/pkg/ui/workspaces/db-console/src/views/reports/containers/problemRanges/connectionsTable.tsx @@ -48,7 +48,7 @@ const connectionTableColumns: ConnectionTableColumn[] = [ extract: problem => problem.no_raft_leader_range_ids.length, }, { - title: "Invalid Lease", + title: "Expired Lease", extract: problem => problem.no_lease_range_ids.length, }, { diff --git a/pkg/ui/workspaces/db-console/src/views/reports/containers/problemRanges/index.tsx b/pkg/ui/workspaces/db-console/src/views/reports/containers/problemRanges/index.tsx index e8e733486802..5924137679ea 100644 --- a/pkg/ui/workspaces/db-console/src/views/reports/containers/problemRanges/index.tsx +++ b/pkg/ui/workspaces/db-console/src/views/reports/containers/problemRanges/index.tsx @@ -46,6 +46,7 @@ function ProblemRangeList(props: { name: string; problems: NodeProblems$Properties[]; extract: (p: NodeProblems$Properties) => Long[]; + description?: string; }) { const ids = _.chain(props.problems) .filter(problem => _.isEmpty(problem.error_message)) @@ -61,6 +62,9 @@ function ProblemRangeList(props: { return (

{props.name}

+ {props.description && ( +
{props.description}
+ )}
{_.map(ids, id => { return ( @@ -179,9 +183,10 @@ export class ProblemRanges extends React.Component { extract={problem => problem.no_raft_leader_range_ids} /> problem.no_lease_range_ids} + description="Note that having expired leases is unlikely to be a problem. They can occur after node restarts and will clear on its own in up to 24 hours." /> Date: Wed, 16 Feb 2022 21:38:56 -0500 Subject: [PATCH 4/4] spanconfig: introduce a new read-only system target type This patch is motivated by the host tenant's desire to fetch all system span configurations it has installed over secondary tenants without explicitly constructing targets for all tenant IDs in the system. This is handy for the host tenant's reconciliation job, which populates an in-memory view of all system span configurations to diff against the state implied by the schema. This inturn allows the host tenant to issue targeted updates/deletes. We introduce a new system target type to achieve this which allows a tenant to request all system span configurations it has installed that target tenant keyspaces. This is only ever consequential in the context of the host as only the host tenant can install system span configurations on other tenants. We also make this target read-only, in that, we disallow persisting its implied encoding in `system.span_configurations`. We add validation to the KVAccessor to this effect. While here, we also disallow creating span targets that overlap with the reserved system span configuration keyspace. We also improve the concepts around these system targets to talk about "tenant keyspaces' and "entire keyspace" as opposed to "tenant" and "cluster". Release note: None --- .../spanconfigreconcilerccl/BUILD.bazel | 1 - .../datadriven_test.go | 3 +- .../migrations/migrate_span_configs_test.go | 40 +-- .../migrations/seed_tenant_span_configs.go | 2 +- pkg/roachpb/span_config.go | 52 ++++ pkg/roachpb/span_config.proto | 64 +++- pkg/rpc/auth_tenant.go | 7 +- pkg/rpc/auth_test.go | 26 +- .../spanconfigkvaccessor/kvaccessor.go | 8 +- .../spanconfigkvaccessor/kvaccessor_test.go | 5 +- .../testdata/system_span_configs | 49 ++- .../spanconfigkvaccessor/validation_test.go | 52 +++- pkg/spanconfig/spanconfigtestutils/utils.go | 17 +- pkg/spanconfig/systemtarget.go | 288 ++++++++++++------ pkg/spanconfig/target.go | 48 ++- pkg/spanconfig/target_test.go | 208 +++++++++++-- 16 files changed, 667 insertions(+), 203 deletions(-) diff --git a/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/BUILD.bazel b/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/BUILD.bazel index 59a04956b03c..9923ad6f5f49 100644 --- a/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/BUILD.bazel +++ b/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/BUILD.bazel @@ -13,7 +13,6 @@ go_test( "//pkg/ccl/partitionccl", "//pkg/ccl/utilccl", "//pkg/jobs", - "//pkg/keys", "//pkg/roachpb", "//pkg/security", "//pkg/security/securitytest", diff --git a/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/datadriven_test.go b/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/datadriven_test.go index 48f211dc7519..b728bc31b5cc 100644 --- a/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/datadriven_test.go +++ b/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/datadriven_test.go @@ -19,7 +19,6 @@ import ( _ "github.com/cockroachdb/cockroach/pkg/ccl/kvccl/kvtenantccl" _ "github.com/cockroachdb/cockroach/pkg/ccl/partitionccl" "github.com/cockroachdb/cockroach/pkg/jobs" - "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/spanconfig" "github.com/cockroachdb/cockroach/pkg/spanconfig/spanconfigtestutils" @@ -192,7 +191,7 @@ func TestDataDriven(t *testing.T) { return nil }) records, err := kvAccessor.GetSpanConfigRecords( - ctx, []spanconfig.Target{spanconfig.MakeTargetFromSpan(keys.EverythingSpan)}, + ctx, spanconfig.TestingEntireSpanConfigurationStateTargets(), ) require.NoError(t, err) sort.Slice(records, func(i, j int) bool { diff --git a/pkg/migration/migrations/migrate_span_configs_test.go b/pkg/migration/migrations/migrate_span_configs_test.go index a62a5c8a1a71..956052920fa7 100644 --- a/pkg/migration/migrations/migrate_span_configs_test.go +++ b/pkg/migration/migrations/migrate_span_configs_test.go @@ -17,7 +17,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/base" "github.com/cockroachdb/cockroach/pkg/clusterversion" - "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/kv/kvclient/rangefeed/rangefeedcache" "github.com/cockroachdb/cockroach/pkg/server" "github.com/cockroachdb/cockroach/pkg/spanconfig" @@ -67,10 +66,11 @@ func TestEnsureSpanConfigReconciliation(t *testing.T) { tdb.Exec(t, `SET CLUSTER SETTING spanconfig.reconciliation_job.enabled = true`) tdb.Exec(t, `SET CLUSTER SETTING spanconfig.reconciliation_job.checkpoint_interval = '100ms'`) - { // Ensure that no span config entries are found. - records, err := scKVAccessor.GetSpanConfigRecords(ctx, []spanconfig.Target{ - spanconfig.MakeTargetFromSpan(keys.EverythingSpan), - }) + { // Ensure that no span config records are found. + records, err := scKVAccessor.GetSpanConfigRecords( + ctx, + spanconfig.TestingEntireSpanConfigurationStateTargets(), + ) require.NoError(t, err) require.Empty(t, records) } @@ -90,9 +90,10 @@ func TestEnsureSpanConfigReconciliation(t *testing.T) { require.False(t, scReconciler.Checkpoint().IsEmpty()) { // Ensure that the host tenant's span configs are installed. - records, err := scKVAccessor.GetSpanConfigRecords(ctx, []spanconfig.Target{ - spanconfig.MakeTargetFromSpan(keys.EverythingSpan), - }) + records, err := scKVAccessor.GetSpanConfigRecords( + ctx, + spanconfig.TestingEntireSpanConfigurationStateTargets(), + ) require.NoError(t, err) require.NotEmpty(t, records) } @@ -152,10 +153,11 @@ func TestEnsureSpanConfigReconciliationMultiNode(t *testing.T) { tdb.Exec(t, `SET CLUSTER SETTING spanconfig.reconciliation_job.enabled = true`) tdb.Exec(t, `SET CLUSTER SETTING spanconfig.reconciliation_job.checkpoint_interval = '100ms'`) - { // Ensure that no span config entries are to be found. - records, err := scKVAccessor.GetSpanConfigRecords(ctx, []spanconfig.Target{ - spanconfig.MakeTargetFromSpan(keys.EverythingSpan), - }) + { // Ensure that no span config records are to be found. + records, err := scKVAccessor.GetSpanConfigRecords( + ctx, + spanconfig.TestingEntireSpanConfigurationStateTargets(), + ) require.NoError(t, err) require.Empty(t, records) } @@ -175,9 +177,10 @@ func TestEnsureSpanConfigReconciliationMultiNode(t *testing.T) { require.False(t, scReconciler.Checkpoint().IsEmpty()) { // Ensure that the host tenant's span configs are installed. - records, err := scKVAccessor.GetSpanConfigRecords(ctx, []spanconfig.Target{ - spanconfig.MakeTargetFromSpan(keys.EverythingSpan), - }) + records, err := scKVAccessor.GetSpanConfigRecords( + ctx, + spanconfig.TestingEntireSpanConfigurationStateTargets(), + ) require.NoError(t, err) require.NotEmpty(t, records) } @@ -219,9 +222,10 @@ func TestEnsureSpanConfigSubscription(t *testing.T) { tdb.Exec(t, `SET CLUSTER SETTING spanconfig.reconciliation_job.enabled = true`) testutils.SucceedsSoon(t, func() error { - records, err := scKVAccessor.GetSpanConfigRecords(ctx, []spanconfig.Target{ - spanconfig.MakeTargetFromSpan(keys.EverythingSpan), - }) + records, err := scKVAccessor.GetSpanConfigRecords( + ctx, + spanconfig.TestingEntireSpanConfigurationStateTargets(), + ) require.NoError(t, err) if len(records) == 0 { return fmt.Errorf("empty global span configuration state") diff --git a/pkg/migration/migrations/seed_tenant_span_configs.go b/pkg/migration/migrations/seed_tenant_span_configs.go index c0b903c7cd9e..3010ac30bacf 100644 --- a/pkg/migration/migrations/seed_tenant_span_configs.go +++ b/pkg/migration/migrations/seed_tenant_span_configs.go @@ -78,7 +78,7 @@ func seedTenantSpanConfigsMigration( return err } if len(scRecords) != 0 { - // This tenant already has span config entries. It was either + // This tenant already has span config records. It was either // already migrated (migrations need to be idempotent) or it was // created after PreSeedTenantSpanConfigs was activated. There's // nothing left to do here. diff --git a/pkg/roachpb/span_config.go b/pkg/roachpb/span_config.go index ad3ebc198a27..9c3cc253da90 100644 --- a/pkg/roachpb/span_config.go +++ b/pkg/roachpb/span_config.go @@ -127,3 +127,55 @@ func TestingDatabaseSystemSpanConfig(host bool) SpanConfig { config.GCPolicy.IgnoreStrictEnforcement = true return config } + +// IsEntireKeyspaceTarget returns true if the receiver targets the entire +// keyspace. +func (st SystemSpanConfigTarget) IsEntireKeyspaceTarget() bool { + return st.Type.GetEntireKeyspace() != nil +} + +// IsSpecificTenantKeyspaceTarget returns true if the receiver targets a +// specific tenant's keyspace. +func (st SystemSpanConfigTarget) IsSpecificTenantKeyspaceTarget() bool { + return st.Type.GetSpecificTenantKeyspace() != nil +} + +// IsAllTenantKeyspaceTargetsSetTarget returns true if the receiver target +// encompasses all targets that have been set on specific tenant keyspaces +// by the system target source. +func (st SystemSpanConfigTarget) IsAllTenantKeyspaceTargetsSetTarget() bool { + return st.Type.GetAllTenantKeyspaceTargetsSet() != nil +} + +// NewEntireKeyspaceTargetType returns a system span config target type that +// targets the entire keyspace. +func NewEntireKeyspaceTargetType() *SystemSpanConfigTarget_Type { + return &SystemSpanConfigTarget_Type{ + Type: &SystemSpanConfigTarget_Type_EntireKeyspace{ + EntireKeyspace: &SystemSpanConfigTarget_EntireKeyspace{}, + }, + } +} + +// NewSpecificTenantKeyspaceTargetType returns a system span config target type +// that the given tenant ID's keyspace. +func NewSpecificTenantKeyspaceTargetType(tenantID TenantID) *SystemSpanConfigTarget_Type { + return &SystemSpanConfigTarget_Type{ + Type: &SystemSpanConfigTarget_Type_SpecificTenantKeyspace{ + SpecificTenantKeyspace: &SystemSpanConfigTarget_TenantKeyspace{ + TenantID: tenantID, + }, + }, + } +} + +// NewAllTenantKeyspaceTargetsSetTargetType returns a read-only system span +// config target type that encompasses all targets that have been set on +// specific tenant keyspaces. +func NewAllTenantKeyspaceTargetsSetTargetType() *SystemSpanConfigTarget_Type { + return &SystemSpanConfigTarget_Type{ + Type: &SystemSpanConfigTarget_Type_AllTenantKeyspaceTargetsSet{ + AllTenantKeyspaceTargetsSet: &SystemSpanConfigTarget_AllTenantKeyspaceTargetsSet{}, + }, + } +} diff --git a/pkg/roachpb/span_config.proto b/pkg/roachpb/span_config.proto index b326da31cc2f..5f15ff040904 100644 --- a/pkg/roachpb/span_config.proto +++ b/pkg/roachpb/span_config.proto @@ -175,6 +175,19 @@ message SpanConfig { } // SystemSpanConfigTarget specifies the target of system span configurations. +// System targets are designed for a few different kinds of interactions. We +// want the ability to: +// 1. Allow the host tenant to set a system span configuration on the entire +// keyspace. +// 2. Allow the host tenant to set a system span configuration on a particular +// tenant's keyspace. +// 3. Allow secondary tenants to set system span configurations on their +// keyspace. +// +// Additionally, we also want each tenant to be able to fetch all system span +// configurations that it has installed. Ideally, we want to be able to do this +// without knowing the tenantID of all other tenants that exist. We provide a +// read-only system span config target type to achieve exactly this. message SystemSpanConfigTarget { option (gogoproto.equal) = true; @@ -182,17 +195,46 @@ message SystemSpanConfigTarget { // configuration. TenantID source_tenant_id = 1 [(gogoproto.nullable) = false, (gogoproto.customname) = "SourceTenantID"]; - // TargetTenantID is the ID of the tenant that the associated system span - // configuration applies to. - // - // If the host tenant is the source and the target is unspecified then the - // associated system span configuration applies over all ranges in the system - // (including those belonging to secondary tenants). - // - // Secondary tenants are only allowed to target themselves and must fill in - // this field. The host tenant may use this field to target a specific - // secondary tenant. - TenantID target_tenant_id = 2 [(gogoproto.customname) = "TargetTenantID"]; + // TenantKeyspace is a target type that targets the keyspace of a specific + // tenant. + message TenantKeyspace { + option (gogoproto.equal) = true; + + // TenantID is the ID of the tenant whose keyspace the the associated + // system span configuration applies to. + // + // Secondary tenants are only allowed to target their keyspace. The host + // tenant may use this field to target a specific secondary tenant's + // keyspace. + TenantID tenant_id = 1 [(gogoproto.nullable) = false, (gogoproto.customname) = "TenantID"]; + }; + + // EntireKeyspace is a target type that targets the entire keyspace (all + // ranges, including those belonging to secondary tenants). Only the host + // tenant is allowed to target the entire keyspace. + message EntireKeyspace{ + option (gogoproto.equal) = true; + }; + + // AllTenantKeyspacesTargetsSet is is a read-only system target type that + // encompasses all system targets that have been set by the source tenant on + // specific tenant's keyspaces. + message AllTenantKeyspaceTargetsSet{ + option (gogoproto.equal) = true; + }; + + message Type { + option (gogoproto.equal) = true; + + oneof type { + TenantKeyspace specific_tenant_keyspace = 1; + EntireKeyspace entire_keyspace = 2; + AllTenantKeyspaceTargetsSet all_tenant_keyspace_targets_set = 3; + } + } + + // Type of the system target. + Type type = 2; } // SpanConfigTarget specifies the target of an associated span configuration. diff --git a/pkg/rpc/auth_tenant.go b/pkg/rpc/auth_tenant.go index 819f20841a3e..201bfaf95fd8 100644 --- a/pkg/rpc/auth_tenant.go +++ b/pkg/rpc/auth_tenant.go @@ -315,11 +315,12 @@ func validateSpanConfigTarget( return nil } - if target.TargetTenantID == nil { - return authErrorf("secondary tenants must explicitly target themselves") + if target.IsEntireKeyspaceTarget() { + return authErrorf("secondary tenants cannot target the entire keyspace") } - if target.SourceTenantID != *target.TargetTenantID { + if target.IsSpecificTenantKeyspaceTarget() && + target.Type.GetSpecificTenantKeyspace().TenantID != target.SourceTenantID { return authErrorf( "secondary tenants cannot interact with system span configurations of other tenants", ) diff --git a/pkg/rpc/auth_test.go b/pkg/rpc/auth_test.go index 613e741d41b2..9bb313bdaaa9 100644 --- a/pkg/rpc/auth_test.go +++ b/pkg/rpc/auth_test.go @@ -187,12 +187,11 @@ func TestTenantAuthRequest(t *testing.T) { return ru } makeSystemSpanConfigTarget := func(source, target uint64) roachpb.SpanConfigTarget { - targetID := roachpb.MakeTenantID(target) return roachpb.SpanConfigTarget{ Union: &roachpb.SpanConfigTarget_SystemSpanConfigTarget{ SystemSpanConfigTarget: &roachpb.SystemSpanConfigTarget{ SourceTenantID: roachpb.MakeTenantID(source), - TargetTenantID: &targetID, + Type: roachpb.NewSpecificTenantKeyspaceTargetType(roachpb.MakeTenantID(target)), }, }, } @@ -536,11 +535,22 @@ func TestTenantAuthRequest(t *testing.T) { Union: &roachpb.SpanConfigTarget_SystemSpanConfigTarget{ SystemSpanConfigTarget: &roachpb.SystemSpanConfigTarget{ SourceTenantID: roachpb.MakeTenantID(10), - TargetTenantID: nil, + Type: roachpb.NewEntireKeyspaceTargetType(), }, }, }), - expErr: `secondary tenants must explicitly target themselves`, + expErr: `secondary tenants cannot target the entire keyspace`, + }, + { + req: makeGetSpanConfigsReq(roachpb.SpanConfigTarget{ + Union: &roachpb.SpanConfigTarget_SystemSpanConfigTarget{ + SystemSpanConfigTarget: &roachpb.SystemSpanConfigTarget{ + SourceTenantID: roachpb.MakeTenantID(20), + Type: roachpb.NewEntireKeyspaceTargetType(), + }, + }, + }), + expErr: `malformed source tenant field`, }, }, "/cockroach.roachpb.Internal/UpdateSpanConfigs": { @@ -651,11 +661,11 @@ func TestTenantAuthRequest(t *testing.T) { Union: &roachpb.SpanConfigTarget_SystemSpanConfigTarget{ SystemSpanConfigTarget: &roachpb.SystemSpanConfigTarget{ SourceTenantID: roachpb.MakeTenantID(10), - TargetTenantID: nil, + Type: roachpb.NewEntireKeyspaceTargetType(), }, }, }, false), - expErr: `secondary tenants must explicitly target themselves`, + expErr: `secondary tenants cannot target the entire keyspace`, }, { req: makeUpdateSpanConfigsReq(makeSystemSpanConfigTarget(10, 10), true), @@ -676,11 +686,11 @@ func TestTenantAuthRequest(t *testing.T) { Union: &roachpb.SpanConfigTarget_SystemSpanConfigTarget{ SystemSpanConfigTarget: &roachpb.SystemSpanConfigTarget{ SourceTenantID: roachpb.MakeTenantID(10), - TargetTenantID: nil, + Type: roachpb.NewEntireKeyspaceTargetType(), }, }, }, true), - expErr: `secondary tenants must explicitly target themselves`, + expErr: `secondary tenants cannot target the entire keyspace`, }, }, diff --git a/pkg/spanconfig/spanconfigkvaccessor/kvaccessor.go b/pkg/spanconfig/spanconfigkvaccessor/kvaccessor.go index 581499c9f823..a1f725bc8bb2 100644 --- a/pkg/spanconfig/spanconfigkvaccessor/kvaccessor.go +++ b/pkg/spanconfig/spanconfigkvaccessor/kvaccessor.go @@ -309,7 +309,7 @@ func (k *KVAccessor) constructDeleteStmtAndArgs( } // constructUpsertStmtAndArgs constructs the statement and query arguments -// needed to upsert the given span config entries. +// needed to upsert the given span config records. func (k *KVAccessor) constructUpsertStmtAndArgs( toUpsert []spanconfig.Record, ) (string, []interface{}, error) { @@ -429,6 +429,12 @@ func validateUpdateArgs(toDelete []spanconfig.Target, toUpsert []spanconfig.Reco copy(targets, list) sort.Sort(spanconfig.Targets(targets)) for i := range targets { + if targets[i].IsSystemTarget() && targets[i].GetSystemTarget().IsReadOnly() { + return errors.AssertionFailedf( + "cannot use read only system target %s as an update argument", targets[i], + ) + } + if i == 0 { continue } diff --git a/pkg/spanconfig/spanconfigkvaccessor/kvaccessor_test.go b/pkg/spanconfig/spanconfigkvaccessor/kvaccessor_test.go index 55d8dc610ad4..83721614f8d4 100644 --- a/pkg/spanconfig/spanconfigkvaccessor/kvaccessor_test.go +++ b/pkg/spanconfig/spanconfigkvaccessor/kvaccessor_test.go @@ -34,9 +34,11 @@ import ( // span [a,e) // span [a,b) // span [b,c) +// system-target {cluster} // system-target {source=1,target=20} // system-target {source=1,target=1} // system-target {source=20,target=20} +// system-target {source=1, all-tenant-keyspace-targets-set} // ---- // // kvaccessor-update @@ -45,13 +47,14 @@ import ( // upsert [d,e):D // delete {source=1,target=1} // upsert {source=1,target=1}:A +// upsert {cluster}:F // ---- // // They tie into GetSpanConfigRecords and UpdateSpanConfigRecords // respectively. For kvaccessor-get, each listed target is added to the set of // targets being read. For kvaccessor-update, the lines prefixed with "delete" // count towards the targets being deleted, and for "upsert" they correspond to -// the span config entries being upserted. See +// the span config records being upserted. See // spanconfigtestutils.Parse{Span,Config,SpanConfigRecord} for more details. func TestDataDriven(t *testing.T) { defer leaktest.AfterTest(t)() diff --git a/pkg/spanconfig/spanconfigkvaccessor/testdata/system_span_configs b/pkg/spanconfig/spanconfigkvaccessor/testdata/system_span_configs index 42a6e592e75f..4e18e76149f2 100644 --- a/pkg/spanconfig/spanconfigkvaccessor/testdata/system_span_configs +++ b/pkg/spanconfig/spanconfigkvaccessor/testdata/system_span_configs @@ -2,7 +2,7 @@ # Test with an empty slate. kvaccessor-get -system-target {cluster} +system-target {entire-keyspace} system-target {source=1,target=1} system-target {source=1,target=10} system-target {source=20,target=20} @@ -10,13 +10,13 @@ system-target {source=20,target=20} # Try deleting a system span configuration that doesn't exist. kvaccessor-update -delete {cluster} +delete {entire-keyspace} ---- err: expected to delete 1 row(s), deleted 0 # Basic tests that set all possible combinations of system targets. kvaccessor-update -upsert {cluster}:A +upsert {entire-keyspace}:A upsert {source=1,target=1}:B upsert {source=1,target=10}:C upsert {source=20,target=20}:D @@ -24,19 +24,19 @@ upsert {source=20,target=20}:D ok kvaccessor-get -system-target {cluster} +system-target {entire-keyspace} system-target {source=1,target=1} system-target {source=1,target=10} system-target {source=20,target=20} ---- -{cluster}:A +{entire-keyspace}:A {source=1,target=1}:B {source=1,target=10}:C {source=20,target=20}:D # Update some of the span configurations that we added and ensure that works. kvaccessor-update -upsert {cluster}:F +upsert {entire-keyspace}:F upsert {source=1,target=1}:G upsert {source=1,target=10}:H upsert {source=20,target=20}:I @@ -44,20 +44,27 @@ upsert {source=20,target=20}:I ok kvaccessor-get -system-target {cluster} +system-target {entire-keyspace} system-target {source=1,target=1} system-target {source=1,target=10} system-target {source=20,target=20} ---- -{cluster}:F +{entire-keyspace}:F {source=1,target=1}:G {source=1,target=10}:H {source=20,target=20}:I +kvaccessor-get +system-target {source=1,all-tenant-keyspace-targets-set} +---- +{source=1,target=1}:G +{source=1,target=10}:H + + # Delete all the system span configurations that we just added and ensure # they take effect. kvaccessor-update -delete {cluster} +delete {entire-keyspace} delete {source=1,target=1} delete {source=1,target=10} delete {source=20,target=20} @@ -65,17 +72,21 @@ delete {source=20,target=20} ok kvaccessor-get -system-target {cluster} +system-target {entire-keyspace} system-target {source=1,target=1} system-target {source=1,target=10} system-target {source=20,target=20} ---- +kvaccessor-get +system-target {source=1,all-tenant-keyspace-targets-set} +---- + # Lastly, try adding multiple system targets set by the host tenant that apply # to distinct secondary tenants. We also add a system span configuration set by # one of these secondary tenant's on itself for kicks. kvaccessor-update -upsert {cluster}:Z +upsert {entire-keyspace}:Z upsert {source=1,target=10}:A upsert {source=1,target=20}:B upsert {source=1,target=30}:C @@ -84,14 +95,26 @@ upsert {source=10,target=10}:G ok kvaccessor-get -system-target {cluster} +system-target {entire-keyspace} system-target {source=1,target=10} system-target {source=1,target=20} system-target {source=1,target=30} system-target {source=10,target=10} ---- -{cluster}:Z +{entire-keyspace}:Z {source=1,target=10}:A {source=1,target=20}:B {source=1,target=30}:C {source=10,target=10}:G + +kvaccessor-get +system-target {source=1,all-tenant-keyspace-targets-set} +---- +{source=1,target=10}:A +{source=1,target=20}:B +{source=1,target=30}:C + +kvaccessor-get +system-target {source=10,all-tenant-keyspace-targets-set} +---- +{source=10,target=10}:G diff --git a/pkg/spanconfig/spanconfigkvaccessor/validation_test.go b/pkg/spanconfig/spanconfigkvaccessor/validation_test.go index a8215bbdda53..040f899aa404 100644 --- a/pkg/spanconfig/spanconfigkvaccessor/validation_test.go +++ b/pkg/spanconfig/spanconfigkvaccessor/validation_test.go @@ -25,13 +25,12 @@ import ( func TestValidateUpdateArgs(t *testing.T) { defer leaktest.AfterTest(t)() - clusterTarget := spanconfig.MakeTargetFromSystemTarget(spanconfig.SystemTarget{ - SourceTenantID: roachpb.SystemTenantID, - TargetTenantID: nil, - }) + entireKeyspaceTarget := spanconfig.MakeTargetFromSystemTarget( + spanconfig.MakeEntireKeyspaceTarget(), + ) makeTenantTarget := func(id uint64) spanconfig.Target { - target, err := spanconfig.MakeTenantTarget(roachpb.MakeTenantID(id), roachpb.MakeTenantID(id)) + target, err := spanconfig.MakeTenantKeyspaceTarget(roachpb.MakeTenantID(id), roachpb.MakeTenantID(id)) require.NoError(t, err) return spanconfig.MakeTargetFromSystemTarget(target) } @@ -145,9 +144,9 @@ func TestValidateUpdateArgs(t *testing.T) { // Duplicate in toDelete with some span targets. toDelete: []spanconfig.Target{ spanconfig.MakeTargetFromSpan(roachpb.Span{Key: roachpb.Key("a"), EndKey: roachpb.Key("c")}), - clusterTarget, + entireKeyspaceTarget, spanconfig.MakeTargetFromSpan(roachpb.Span{Key: roachpb.Key("e"), EndKey: roachpb.Key("f")}), - clusterTarget, + entireKeyspaceTarget, spanconfig.MakeTargetFromSpan(roachpb.Span{Key: roachpb.Key("g"), EndKey: roachpb.Key("h")}), }, expErr: "duplicate system targets .* in the same list", @@ -156,9 +155,9 @@ func TestValidateUpdateArgs(t *testing.T) { // Duplicate in toDelete with some span targets. toDelete: []spanconfig.Target{ spanconfig.MakeTargetFromSpan(roachpb.Span{Key: roachpb.Key("a"), EndKey: roachpb.Key("c")}), - clusterTarget, + entireKeyspaceTarget, spanconfig.MakeTargetFromSpan(roachpb.Span{Key: roachpb.Key("e"), EndKey: roachpb.Key("f")}), - clusterTarget, + entireKeyspaceTarget, spanconfig.MakeTargetFromSpan(roachpb.Span{Key: roachpb.Key("g"), EndKey: roachpb.Key("h")}), }, expErr: "duplicate system targets .* in the same list", @@ -170,7 +169,7 @@ func TestValidateUpdateArgs(t *testing.T) { spanconfig.MakeTargetFromSpan(roachpb.Span{Key: roachpb.Key("a"), EndKey: roachpb.Key("c")}), makeTenantTarget(20), spanconfig.MakeTargetFromSpan(roachpb.Span{Key: roachpb.Key("e"), EndKey: roachpb.Key("f")}), - clusterTarget, + entireKeyspaceTarget, spanconfig.MakeTargetFromSpan(roachpb.Span{Key: roachpb.Key("g"), EndKey: roachpb.Key("h")}), }, toUpsert: []spanconfig.Record{ @@ -183,7 +182,38 @@ func TestValidateUpdateArgs(t *testing.T) { }, expErr: "", }, + { + // Read only targets are not valid delete args. + toDelete: []spanconfig.Target{ + spanconfig.MakeTargetFromSystemTarget( + spanconfig.MakeAllTenantKeyspaceTargetsSet(roachpb.SystemTenantID), + ), + }, + expErr: "cannot use read only system target .* as an update argument", + }, + { + // Read only targets are not valid upsert args. + toUpsert: []spanconfig.Record{ + { + Target: spanconfig.MakeTargetFromSystemTarget( + spanconfig.MakeAllTenantKeyspaceTargetsSet(roachpb.SystemTenantID), + ), + }, + }, + expErr: "cannot use read only system target .* as an update argument", + }, + { + // Read only target validation also applies when the source is a secondary + // tenant. + toDelete: []spanconfig.Target{ + spanconfig.MakeTargetFromSystemTarget( + spanconfig.MakeAllTenantKeyspaceTargetsSet(roachpb.MakeTenantID(10)), + ), + }, + expErr: "cannot use read only system target .* as an update argument", + }, } { - require.True(t, testutils.IsError(validateUpdateArgs(tc.toDelete, tc.toUpsert), tc.expErr)) + err := validateUpdateArgs(tc.toDelete, tc.toUpsert) + require.True(t, testutils.IsError(err, tc.expErr), "exp %s; got %s", tc.expErr, err) } } diff --git a/pkg/spanconfig/spanconfigtestutils/utils.go b/pkg/spanconfig/spanconfigtestutils/utils.go index 5ae42e7a398e..a8f485baf75d 100644 --- a/pkg/spanconfig/spanconfigtestutils/utils.go +++ b/pkg/spanconfig/spanconfigtestutils/utils.go @@ -33,8 +33,10 @@ import ( var spanRe = regexp.MustCompile(`^\[(\w+),\s??(\w+)\)$`) // systemTargetRe matches strings of the form -// "{source=(|system),target=(|system)}". -var systemTargetRe = regexp.MustCompile(`^{(cluster)|(source=(\d*),\s??target=(\d*))}$`) +// "{entire-keyspace|source=,(target=|all-tenant-keyspace-targets-set)}". +var systemTargetRe = regexp.MustCompile( + `^{(entire-keyspace)|(source=(\d*),\s??((target=(\d*))|all-tenant-keyspace-targets-set))}$`, +) // configRe matches a single word. It's a shorthand for declaring a unique // config. @@ -63,15 +65,18 @@ func parseSystemTarget(t *testing.T, systemTarget string) spanconfig.SystemTarge } matches := systemTargetRe.FindStringSubmatch(systemTarget) - if matches[1] == "cluster" { - return spanconfig.MakeClusterTarget() + if matches[1] == "entire-keyspace" { + return spanconfig.MakeEntireKeyspaceTarget() } sourceID, err := strconv.Atoi(matches[3]) require.NoError(t, err) - targetID, err := strconv.Atoi(matches[4]) + if matches[4] == "all-tenant-keyspace-targets-set" { + return spanconfig.MakeAllTenantKeyspaceTargetsSet(roachpb.MakeTenantID(uint64(sourceID))) + } + targetID, err := strconv.Atoi(matches[6]) require.NoError(t, err) - target, err := spanconfig.MakeTenantTarget( + target, err := spanconfig.MakeTenantKeyspaceTarget( roachpb.MakeTenantID(uint64(sourceID)), roachpb.MakeTenantID(uint64(targetID)), ) require.NoError(t, err) diff --git a/pkg/spanconfig/systemtarget.go b/pkg/spanconfig/systemtarget.go index be77cfcd808e..e8d8bb0df364 100644 --- a/pkg/spanconfig/systemtarget.go +++ b/pkg/spanconfig/systemtarget.go @@ -22,159 +22,265 @@ import ( // SystemTarget specifies the target of a system span configuration. type SystemTarget struct { - // SourceTenantID is the ID of the tenant that specified the system span + // sourceTenantID is the ID of the tenant that specified the system span // configuration. - // SourceTenantID is the ID of the tenant that specified the system span - // configuration. - SourceTenantID roachpb.TenantID + sourceTenantID roachpb.TenantID - // TargetTenantID is the ID of the tenant that the associated system span - // configuration applies to. - // - // If the host tenant is the source and the TargetTenantID is unspecified then - // the associated system span configuration applies over all ranges in the - // system (including those belonging to secondary tenants). + // targetTenantID is the ID of the tenant whose kesypace the associated system + // span configuration applies. This field can only be set in conjunction with + // the type being SystemTargetTypeSpecificTenantKeyspace; it must be left + // unset for all other system target types. // - // Secondary tenants are only allowed to target themselves. The host tenant - // may use this field to target a specific secondary tenant. We validate this - // when constructing new system targets. - TargetTenantID *roachpb.TenantID + // Secondary tenants are only allowed to target their own keyspace. The host + // tenant may use this field to target a specific secondary tenant. + targetTenantID *roachpb.TenantID + + // systemTargetType indicates the type of the system target. targetTenantID + // can only be set if the system target is specific. + systemTargetType systemTargetType } -// MakeTenantTarget constructs, validates, and returns a new SystemTarget that -// targets the physical keyspace of the targetTenantID. -func MakeTenantTarget( +// systemTargetType indicates the type of SystemTarget. +type systemTargetType int + +const ( + _ systemTargetType = iota + // SystemTargetTypeSpecificTenantKeyspace indicates that the system target is + // targeting a specific tenant's keyspace. + SystemTargetTypeSpecificTenantKeyspace + // SystemTargetTypeEntireKeyspace indicates that the system target is + // targeting the entire keyspace. Only the host tenant is allowed to do so. + SystemTargetTypeEntireKeyspace + // SystemTargetTypeAllTenantKeyspaceTargetsSet represents a system target that + // encompasses all system targets that have been set by the source tenant over + // specific tenant's keyspace. + // + // This is a read-only system target type as it may translate to more than one + // system targets that may have been persisted. This target type is useful in + // fetching all system span configurations a tenant may have set on tenant + // keyspaces without knowing the tenant ID of all other tenants in the system. + // This is only ever significant for the host tenant as it can set system span + // configurations that target other tenant's keyspaces. + SystemTargetTypeAllTenantKeyspaceTargetsSet +) + +// MakeTenantKeyspaceTarget constructs, validates, and returns a new +// SystemTarget that targets the keyspace of the target tenant. +func MakeTenantKeyspaceTarget( sourceTenantID roachpb.TenantID, targetTenantID roachpb.TenantID, ) (SystemTarget, error) { t := SystemTarget{ - SourceTenantID: sourceTenantID, - TargetTenantID: &targetTenantID, + sourceTenantID: sourceTenantID, + targetTenantID: &targetTenantID, + systemTargetType: SystemTargetTypeSpecificTenantKeyspace, } return t, t.validate() } -// MakeSystemTargetFromProto constructs a SystemTarget from a +// makeSystemTargetFromProto constructs a SystemTarget from a // roachpb.SystemSpanConfigTarget and validates it. -func MakeSystemTargetFromProto(proto *roachpb.SystemSpanConfigTarget) (SystemTarget, error) { - if proto.SourceTenantID == roachpb.SystemTenantID && proto.TargetTenantID == nil { - return MakeClusterTarget(), nil - } else if proto.TargetTenantID == nil { - return SystemTarget{}, - errors.Newf("secondary tenant %s cannot target the entire cluster", proto.SourceTenantID) +func makeSystemTargetFromProto(proto *roachpb.SystemSpanConfigTarget) (SystemTarget, error) { + var t SystemTarget + switch { + case proto.IsSpecificTenantKeyspaceTarget(): + t = SystemTarget{ + sourceTenantID: proto.SourceTenantID, + targetTenantID: &proto.Type.GetSpecificTenantKeyspace().TenantID, + systemTargetType: SystemTargetTypeSpecificTenantKeyspace, + } + case proto.IsEntireKeyspaceTarget(): + t = SystemTarget{ + sourceTenantID: proto.SourceTenantID, + targetTenantID: nil, + systemTargetType: SystemTargetTypeEntireKeyspace, + } + case proto.IsAllTenantKeyspaceTargetsSetTarget(): + t = SystemTarget{ + sourceTenantID: proto.SourceTenantID, + targetTenantID: nil, + systemTargetType: SystemTargetTypeAllTenantKeyspaceTargetsSet, + } + default: + return SystemTarget{}, errors.AssertionFailedf("unknown system target type") } + return t, t.validate() +} - return MakeTenantTarget(proto.SourceTenantID, *proto.TargetTenantID) +func (st SystemTarget) toProto() *roachpb.SystemSpanConfigTarget { + var systemTargetType *roachpb.SystemSpanConfigTarget_Type + switch st.systemTargetType { + case SystemTargetTypeEntireKeyspace: + systemTargetType = roachpb.NewEntireKeyspaceTargetType() + case SystemTargetTypeAllTenantKeyspaceTargetsSet: + systemTargetType = roachpb.NewAllTenantKeyspaceTargetsSetTargetType() + case SystemTargetTypeSpecificTenantKeyspace: + systemTargetType = roachpb.NewSpecificTenantKeyspaceTargetType(*st.targetTenantID) + default: + panic("unknown system target type") + } + return &roachpb.SystemSpanConfigTarget{ + SourceTenantID: st.sourceTenantID, + Type: systemTargetType, + } } -// MakeClusterTarget returns a new system target that targets the entire cluster. -// Only the host tenant is allowed to target the entire cluster. -func MakeClusterTarget() SystemTarget { +// MakeEntireKeyspaceTarget returns a new system target that targets the entire +// keyspace. Only the host tenant is allowed to target the entire keyspace. +func MakeEntireKeyspaceTarget() SystemTarget { return SystemTarget{ - SourceTenantID: roachpb.SystemTenantID, + sourceTenantID: roachpb.SystemTenantID, + systemTargetType: SystemTargetTypeEntireKeyspace, } } -// targetsEntireCluster returns true if the target applies to all ranges in the +// MakeAllTenantKeyspaceTargetsSet returns a new SystemTarget that +// represents all system span configurations installed by the given tenant ID +// on specific tenant's keyspace (including itself and other tenants). +func MakeAllTenantKeyspaceTargetsSet(sourceID roachpb.TenantID) SystemTarget { + return SystemTarget{ + sourceTenantID: sourceID, + systemTargetType: SystemTargetTypeAllTenantKeyspaceTargetsSet, + } +} + +// targetsEntireKeyspace returns true if the target applies to all ranges in the // system (including those belonging to secondary tenants). -func (st SystemTarget) targetsEntireCluster() bool { - return st.SourceTenantID == roachpb.SystemTenantID && st.TargetTenantID == nil +func (st SystemTarget) targetsEntireKeyspace() bool { + return st.systemTargetType == SystemTargetTypeEntireKeyspace +} + +// IsReadOnly returns true if the system target is read-only. Read only targets +// should not be persisted. +func (st SystemTarget) IsReadOnly() bool { + return st.systemTargetType == SystemTargetTypeAllTenantKeyspaceTargetsSet } // encode returns an encoded span associated with the receiver which is suitable -// for persistence in system.span_configurations. +// for interaction with system.span_configurations table. func (st SystemTarget) encode() roachpb.Span { var k roachpb.Key - if st.SourceTenantID == roachpb.SystemTenantID && - st.TargetTenantID == nil { + switch st.systemTargetType { + case SystemTargetTypeEntireKeyspace: k = keys.SystemSpanConfigEntireKeyspace - } else if st.SourceTenantID == roachpb.SystemTenantID { - k = encoding.EncodeUvarintAscending( - keys.SystemSpanConfigHostOnTenantKeyspace, st.TargetTenantID.ToUint64(), - ) - } else { - k = encoding.EncodeUvarintAscending( - keys.SystemSpanConfigSecondaryTenantOnEntireKeyspace, st.SourceTenantID.ToUint64(), - ) + case SystemTargetTypeSpecificTenantKeyspace: + if st.sourceTenantID == roachpb.SystemTenantID { + k = encoding.EncodeUvarintAscending( + keys.SystemSpanConfigHostOnTenantKeyspace, st.targetTenantID.ToUint64(), + ) + } else { + k = encoding.EncodeUvarintAscending( + keys.SystemSpanConfigSecondaryTenantOnEntireKeyspace, st.sourceTenantID.ToUint64(), + ) + } + case SystemTargetTypeAllTenantKeyspaceTargetsSet: + if st.sourceTenantID == roachpb.SystemTenantID { + k = keys.SystemSpanConfigHostOnTenantKeyspace + } else { + k = encoding.EncodeUvarintAscending( + keys.SystemSpanConfigSecondaryTenantOnEntireKeyspace, st.sourceTenantID.ToUint64(), + ) + } } return roachpb.Span{Key: k, EndKey: k.PrefixEnd()} } // validate ensures that the receiver is well-formed. func (st SystemTarget) validate() error { - if st.SourceTenantID == roachpb.SystemTenantID { - // The system tenant can target itself, other secondary tenants, and the - // entire cluster (including secondary tenant ranges). - return nil - } - if st.TargetTenantID == nil { - return errors.AssertionFailedf( - "secondary tenant %s cannot have unspecified target tenant ID", st.SourceTenantID, - ) - } - if st.SourceTenantID != *st.TargetTenantID { - return errors.AssertionFailedf( - "secondary tenant %s cannot target another tenant with ID %s", - st.SourceTenantID, - st.TargetTenantID, - ) + switch st.systemTargetType { + case SystemTargetTypeAllTenantKeyspaceTargetsSet: + if st.targetTenantID != nil { + return errors.AssertionFailedf( + "targetTenantID must be unset when targeting everything installed on tenants", + ) + } + case SystemTargetTypeEntireKeyspace: + if st.sourceTenantID != roachpb.SystemTenantID { + return errors.AssertionFailedf("only the host tenant is allowed to target the entire keyspace") + } + if st.targetTenantID != nil { + return errors.AssertionFailedf("malformed system target for entire keyspace; targetTenantID set") + } + case SystemTargetTypeSpecificTenantKeyspace: + if st.targetTenantID == nil { + return errors.AssertionFailedf( + "malformed system target for specific tenant keyspace; targetTenantID unset", + ) + } + if st.sourceTenantID != roachpb.SystemTenantID && st.sourceTenantID != *st.targetTenantID { + return errors.AssertionFailedf( + "secondary tenant %s cannot target another tenant with ID %s", + st.sourceTenantID, + st.targetTenantID, + ) + } + default: + return errors.AssertionFailedf("invalid system target type") } return nil } // isEmpty returns true if the receiver is empty. func (st SystemTarget) isEmpty() bool { - return st.SourceTenantID.Equal(roachpb.TenantID{}) && st.TargetTenantID.Equal(nil) + return st.sourceTenantID.Equal(roachpb.TenantID{}) && st.targetTenantID.Equal(nil) } // less returns true if the receiver is considered less than the supplied // target. The semantics are defined as follows: -// - host installed targets that target the entire cluster come first. -// - host installed targets that target a tenant come next (ordered by target -// tenant ID). -// - secondary tenant installed targets come next, ordered by secondary tenant -// ID. +// - read only targets come first, ordered by tenant ID. +// - targets that target the entire keyspace come next. +// - targets that target a specific tenant's keyspace come last, sorted by +// source tenant ID; target tenant ID is used as a tiebreaker if two targets +// have the same source. func (st SystemTarget) less(ot SystemTarget) bool { - if st.SourceTenantID == roachpb.SystemTenantID && - ot.SourceTenantID == roachpb.SystemTenantID { - if st.targetsEntireCluster() { - return true - } else if ot.targetsEntireCluster() { - return false - } + if st.IsReadOnly() && ot.IsReadOnly() { + return st.sourceTenantID.ToUint64() < ot.sourceTenantID.ToUint64() + } - return st.TargetTenantID.ToUint64() < ot.TargetTenantID.ToUint64() + if st.IsReadOnly() { + return true + } else if ot.IsReadOnly() { + return false } - if st.SourceTenantID == roachpb.SystemTenantID { + if st.targetsEntireKeyspace() { return true - } else if ot.SourceTenantID == roachpb.SystemTenantID { + } else if ot.targetsEntireKeyspace() { return false } - return st.SourceTenantID.ToUint64() < ot.SourceTenantID.ToUint64() + if st.sourceTenantID.ToUint64() == ot.sourceTenantID.ToUint64() { + return st.targetTenantID.ToUint64() < ot.targetTenantID.ToUint64() + } + + return st.sourceTenantID.ToUint64() < ot.sourceTenantID.ToUint64() } // equal returns true iff the receiver is equal to the supplied system target. func (st SystemTarget) equal(ot SystemTarget) bool { - return st.SourceTenantID.Equal(ot.SourceTenantID) && st.TargetTenantID.Equal(ot.TargetTenantID) + return st.sourceTenantID.Equal(ot.sourceTenantID) && st.targetTenantID.Equal(ot.targetTenantID) } // String returns a pretty printed version of a system target. func (st SystemTarget) String() string { - if st.targetsEntireCluster() { - return "{cluster}" + switch st.systemTargetType { + case SystemTargetTypeEntireKeyspace: + return "{entire-keyspace}" + case SystemTargetTypeAllTenantKeyspaceTargetsSet: + return fmt.Sprintf("{source=%d, all-tenant-keyspace-targets-set}", st.sourceTenantID) + case SystemTargetTypeSpecificTenantKeyspace: + return fmt.Sprintf( + "{source=%d,target=%d}", + st.sourceTenantID.ToUint64(), + st.targetTenantID.ToUint64(), + ) + default: + panic("unreachable") } - return fmt.Sprintf( - "{source=%d,target=%d}", - st.SourceTenantID.ToUint64(), - st.TargetTenantID.ToUint64(), - ) } // decodeSystemTarget converts the given span into a SystemTarget. An error is -// returned if the supplied span does not conform to a system target's -// encoding. +// returned if the supplied span does not conform to a system target's encoding. func decodeSystemTarget(span roachpb.Span) (SystemTarget, error) { // Validate the end key is well-formed. if !span.EndKey.Equal(span.Key.PrefixEnd()) { @@ -182,7 +288,7 @@ func decodeSystemTarget(span roachpb.Span) (SystemTarget, error) { } switch { case bytes.Equal(span.Key, keys.SystemSpanConfigEntireKeyspace): - return MakeClusterTarget(), nil + return MakeEntireKeyspaceTarget(), nil case bytes.HasPrefix(span.Key, keys.SystemSpanConfigHostOnTenantKeyspace): // System span config was applied by the host tenant over a secondary // tenant's entire keyspace. @@ -192,7 +298,7 @@ func decodeSystemTarget(span roachpb.Span) (SystemTarget, error) { return SystemTarget{}, err } tenID := roachpb.MakeTenantID(tenIDRaw) - return MakeTenantTarget(roachpb.SystemTenantID, tenID) + return MakeTenantKeyspaceTarget(roachpb.SystemTenantID, tenID) case bytes.HasPrefix(span.Key, keys.SystemSpanConfigSecondaryTenantOnEntireKeyspace): // System span config was applied by a secondary tenant over its entire // keyspace. @@ -202,7 +308,7 @@ func decodeSystemTarget(span roachpb.Span) (SystemTarget, error) { return SystemTarget{}, err } tenID := roachpb.MakeTenantID(tenIDRaw) - return MakeTenantTarget(tenID, tenID) + return MakeTenantKeyspaceTarget(tenID, tenID) default: return SystemTarget{}, errors.AssertionFailedf("span %s did not conform to SystemTarget encoding", span) diff --git a/pkg/spanconfig/target.go b/pkg/spanconfig/target.go index cd69dd604f6d..d0c1703826d0 100644 --- a/pkg/spanconfig/target.go +++ b/pkg/spanconfig/target.go @@ -11,6 +11,7 @@ package spanconfig import ( + "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/errors" ) @@ -26,9 +27,9 @@ type Target struct { func MakeTarget(t roachpb.SpanConfigTarget) (Target, error) { switch t.Union.(type) { case *roachpb.SpanConfigTarget_Span: - return MakeTargetFromSpan(*t.GetSpan()), nil + return MakeSpanTargetFromProto(t) case *roachpb.SpanConfigTarget_SystemSpanConfigTarget: - systemTarget, err := MakeSystemTargetFromProto(t.GetSystemSpanConfigTarget()) + systemTarget, err := makeSystemTargetFromProto(t.GetSystemSpanConfigTarget()) if err != nil { return Target{}, err } @@ -38,8 +39,28 @@ func MakeTarget(t roachpb.SpanConfigTarget) (Target, error) { } } -// MakeTargetFromSpan constructs and returns a span target. +// MakeSpanTargetFromProto returns a new Target backed by an underlying span. +// An error is returned if the proto does not contain a span or if the span +// overlaps with the reserved system span config keyspace. +func MakeSpanTargetFromProto(spanTarget roachpb.SpanConfigTarget) (Target, error) { + if spanTarget.GetSpan() == nil { + return Target{}, errors.AssertionFailedf("span config target did not contain a span") + } + if keys.SystemSpanConfigSpan.Overlaps(*spanTarget.GetSpan()) { + return Target{}, errors.AssertionFailedf( + "cannot target spans in reserved system span config keyspace", + ) + } + return MakeTargetFromSpan(*spanTarget.GetSpan()), nil +} + +// MakeTargetFromSpan constructs and returns a span target. Callers are not +// allowed to target the reserved system span config keyspace (or part of it) +// directly; system targets should be used instead. func MakeTargetFromSpan(span roachpb.Span) Target { + if keys.SystemSpanConfigSpan.Overlaps(span) { + panic("cannot target spans in reserved system span config keyspace") + } return Target{span: span} } @@ -76,8 +97,8 @@ func (t Target) GetSystemTarget() SystemTarget { return t.systemTarget } -// Encode returns an encoded span suitable for persistence in -// system.span_configurations. +// Encode returns an encoded span suitable for interaction with the +// system.span_configurations table. func (t Target) Encode() roachpb.Span { switch { case t.IsSpanTarget(): @@ -155,10 +176,7 @@ func (t Target) ToProto() roachpb.SpanConfigTarget { case t.IsSystemTarget(): return roachpb.SpanConfigTarget{ Union: &roachpb.SpanConfigTarget_SystemSpanConfigTarget{ - SystemSpanConfigTarget: &roachpb.SystemSpanConfigTarget{ - SourceTenantID: t.GetSystemTarget().SourceTenantID, - TargetTenantID: t.GetSystemTarget().TargetTenantID, - }, + SystemSpanConfigTarget: t.GetSystemTarget().toProto(), }, } default: @@ -248,3 +266,15 @@ func TargetsFromProtos(protoTargets []roachpb.SpanConfigTarget) ([]Target, error } return targets, nil } + +// TestingEntireSpanConfigurationStateTargets returns a list of targets which +// can be used to read the entire span configuration state. This includes all +// span configurations installed by all tenants and all system span +// configurations, including those installed by secondary tenants. +func TestingEntireSpanConfigurationStateTargets() []Target { + return Targets{ + Target{ + span: keys.EverythingSpan, + }, + } +} diff --git a/pkg/spanconfig/target_test.go b/pkg/spanconfig/target_test.go index c31e8c25ff00..7d5a63598e71 100644 --- a/pkg/spanconfig/target_test.go +++ b/pkg/spanconfig/target_test.go @@ -26,13 +26,13 @@ import ( func TestEncodeDecodeSystemTarget(t *testing.T) { for _, testTarget := range []SystemTarget{ // Tenant targeting its logical cluster. - makeTenantTargetOrFatal(t, roachpb.MakeTenantID(10), roachpb.MakeTenantID(10)), + makeTenantKeyspaceTargetOrFatal(t, roachpb.MakeTenantID(10), roachpb.MakeTenantID(10)), // System tenant targeting its logical cluster. - makeTenantTargetOrFatal(t, roachpb.SystemTenantID, roachpb.SystemTenantID), + makeTenantKeyspaceTargetOrFatal(t, roachpb.SystemTenantID, roachpb.SystemTenantID), // System tenant targeting a secondary tenant. - makeTenantTargetOrFatal(t, roachpb.SystemTenantID, roachpb.MakeTenantID(10)), - // System tenant targeting the entire cluster. - MakeClusterTarget(), + makeTenantKeyspaceTargetOrFatal(t, roachpb.SystemTenantID, roachpb.MakeTenantID(10)), + // System tenant targeting the entire keyspace. + MakeEntireKeyspaceTarget(), } { systemTarget, err := decodeSystemTarget(testTarget.encode()) require.NoError(t, err) @@ -45,6 +45,37 @@ func TestEncodeDecodeSystemTarget(t *testing.T) { } } +// TestTargetToFromProto ensures that converting system targets to protos and +// from protos is roundtripable. +func TestTargetToFromProto(t *testing.T) { + for _, testSystemTarget := range []SystemTarget{ + // Tenant targeting its logical cluster. + makeTenantKeyspaceTargetOrFatal(t, roachpb.MakeTenantID(10), roachpb.MakeTenantID(10)), + // System tenant targeting its logical cluster. + makeTenantKeyspaceTargetOrFatal(t, roachpb.SystemTenantID, roachpb.SystemTenantID), + // System tenant targeting a secondary tenant. + makeTenantKeyspaceTargetOrFatal(t, roachpb.SystemTenantID, roachpb.MakeTenantID(10)), + // System tenant targeting the entire keyspace. + MakeEntireKeyspaceTarget(), + // System tenant's read-only target to fetch all system span configurations + // it has set over secondary tenant keyspaces. + MakeAllTenantKeyspaceTargetsSet(roachpb.SystemTenantID), + // A secondary tenant's read-only target to fetch all system span + // configurations it has set over secondary tenant keyspaces. + MakeAllTenantKeyspaceTargetsSet(roachpb.MakeTenantID(10)), + } { + systemTarget, err := makeSystemTargetFromProto(testSystemTarget.toProto()) + require.NoError(t, err) + require.Equal(t, testSystemTarget, systemTarget) + + // For good measure, let's also test at the level of a spanconfig.Target. + testTarget := MakeTargetFromSystemTarget(testSystemTarget) + target, err := MakeTarget(testTarget.ToProto()) + require.NoError(t, err) + require.Equal(t, target, testTarget) + } +} + // TestDecodeInvalidSpanAsSystemTarget ensures that decoding an invalid span // as a system target fails. func TestDecodeInvalidSpanAsSystemTarget(t *testing.T) { @@ -87,45 +118,128 @@ func TestDecodeInvalidSpanAsSystemTarget(t *testing.T) { // TestSystemTargetValidation ensures target.validate() works as expected. func TestSystemTargetValidation(t *testing.T) { + tenant10 := roachpb.MakeTenantID(10) + tenant20 := roachpb.MakeTenantID(20) for _, tc := range []struct { sourceTenantID roachpb.TenantID - targetTenantID roachpb.TenantID + targetTenantID *roachpb.TenantID + targetType systemTargetType expErr string }{ { // Secondary tenants cannot target the system tenant. - sourceTenantID: roachpb.MakeTenantID(10), - targetTenantID: roachpb.SystemTenantID, - expErr: "secondary tenant 10 cannot target another tenant with ID", + sourceTenantID: tenant10, + targetTenantID: &roachpb.SystemTenantID, + targetType: SystemTargetTypeSpecificTenantKeyspace, + expErr: "secondary tenant 10 cannot target another tenant with ID system", }, { // Secondary tenants cannot target other secondary tenants. - sourceTenantID: roachpb.MakeTenantID(10), - targetTenantID: roachpb.MakeTenantID(20), - expErr: "secondary tenant 10 cannot target another tenant with ID", + sourceTenantID: tenant10, + targetTenantID: &tenant20, + targetType: SystemTargetTypeSpecificTenantKeyspace, + expErr: "secondary tenant 10 cannot target another tenant with ID 20", + }, + { + // Secondary tenants cannot target the entire keyspace. + sourceTenantID: tenant10, + targetTenantID: nil, + targetType: SystemTargetTypeEntireKeyspace, + expErr: "only the host tenant is allowed to target the entire keyspace", + }, + { + // Ensure secondary tenants can't target the entire keyspace even if they + // set targetTenantID to themselves. + sourceTenantID: tenant10, + targetTenantID: &tenant10, + targetType: SystemTargetTypeEntireKeyspace, + expErr: "only the host tenant is allowed to target the entire keyspace", + }, + { + // System tenant can't set both targetTenantID and target everything + // installed on tenants. + sourceTenantID: roachpb.SystemTenantID, + targetTenantID: &tenant10, + targetType: SystemTargetTypeAllTenantKeyspaceTargetsSet, + expErr: "targetTenantID must be unset when targeting everything installed", + }, + { + // System tenant must fill in a targetTenantID when targeting a specific + // tenant. + sourceTenantID: roachpb.SystemTenantID, + targetTenantID: nil, + targetType: SystemTargetTypeSpecificTenantKeyspace, + expErr: "malformed system target for specific tenant keyspace; targetTenantID unset", + }, + { + // System tenant can't set both targetTenantID and target the entire + // keyspace. + sourceTenantID: roachpb.SystemTenantID, + targetTenantID: &tenant10, + targetType: SystemTargetTypeEntireKeyspace, + expErr: "malformed system target for entire keyspace; targetTenantID set", + }, + { + // secondary tenant can't set both targetTenantID and target everything + // installed on tenants. + sourceTenantID: tenant10, + targetTenantID: &tenant10, + targetType: SystemTargetTypeAllTenantKeyspaceTargetsSet, + expErr: "targetTenantID must be unset when targeting everything installed", }, // Test some valid targets. { // System tenant targeting secondary tenant is allowed. sourceTenantID: roachpb.SystemTenantID, - targetTenantID: roachpb.MakeTenantID(20), + targetTenantID: &tenant20, + targetType: SystemTargetTypeSpecificTenantKeyspace, + }, + { + // System tenant targeting the entire keyspace is allowed. + sourceTenantID: roachpb.SystemTenantID, + targetTenantID: nil, + targetType: SystemTargetTypeEntireKeyspace, }, { // System tenant targeting itself is allowed. sourceTenantID: roachpb.SystemTenantID, - targetTenantID: roachpb.SystemTenantID, + targetTenantID: &roachpb.SystemTenantID, + targetType: SystemTargetTypeSpecificTenantKeyspace, }, { // Secondary tenant targeting itself is allowed. - sourceTenantID: roachpb.MakeTenantID(10), - targetTenantID: roachpb.MakeTenantID(10), + sourceTenantID: tenant10, + targetTenantID: &tenant10, + targetType: SystemTargetTypeSpecificTenantKeyspace, + }, + { + // Secondary tenant targeting everything installed on tenants by it is + // allowed. + sourceTenantID: tenant10, + targetTenantID: nil, + targetType: SystemTargetTypeAllTenantKeyspaceTargetsSet, + }, + { + // System tenant targeting everything installed on tenants by it is + // allowed. + sourceTenantID: roachpb.SystemTenantID, + targetTenantID: nil, + targetType: SystemTargetTypeAllTenantKeyspaceTargetsSet, }, } { target := SystemTarget{ - SourceTenantID: tc.sourceTenantID, - TargetTenantID: &tc.targetTenantID, + sourceTenantID: tc.sourceTenantID, + targetTenantID: tc.targetTenantID, + systemTargetType: tc.targetType, } - require.True(t, testutils.IsError(target.validate(), tc.expErr)) + err := target.validate() + require.True( + t, + testutils.IsError(err, tc.expErr), + "expected: %s got: %s ", + tc.expErr, + err, + ) } } @@ -133,12 +247,24 @@ func TestSystemTargetValidation(t *testing.T) { func TestTargetSortingRandomized(t *testing.T) { // Construct a set of sorted targets. sortedTargets := Targets{ - MakeTargetFromSystemTarget(MakeClusterTarget()), - MakeTargetFromSystemTarget(makeTenantTargetOrFatal(t, roachpb.SystemTenantID, roachpb.SystemTenantID)), - MakeTargetFromSystemTarget(makeTenantTargetOrFatal(t, roachpb.SystemTenantID, roachpb.MakeTenantID(10))), - MakeTargetFromSystemTarget(makeTenantTargetOrFatal(t, roachpb.SystemTenantID, roachpb.MakeTenantID(20))), - MakeTargetFromSystemTarget(makeTenantTargetOrFatal(t, roachpb.MakeTenantID(5), roachpb.MakeTenantID(5))), - MakeTargetFromSystemTarget(makeTenantTargetOrFatal(t, roachpb.MakeTenantID(10), roachpb.MakeTenantID(10))), + MakeTargetFromSystemTarget(MakeAllTenantKeyspaceTargetsSet(roachpb.SystemTenantID)), + MakeTargetFromSystemTarget(MakeAllTenantKeyspaceTargetsSet(roachpb.MakeTenantID(10))), + MakeTargetFromSystemTarget(MakeEntireKeyspaceTarget()), + MakeTargetFromSystemTarget( + makeTenantKeyspaceTargetOrFatal(t, roachpb.SystemTenantID, roachpb.SystemTenantID), + ), + MakeTargetFromSystemTarget( + makeTenantKeyspaceTargetOrFatal(t, roachpb.SystemTenantID, roachpb.MakeTenantID(10)), + ), + MakeTargetFromSystemTarget( + makeTenantKeyspaceTargetOrFatal(t, roachpb.SystemTenantID, roachpb.MakeTenantID(20)), + ), + MakeTargetFromSystemTarget( + makeTenantKeyspaceTargetOrFatal(t, roachpb.MakeTenantID(5), roachpb.MakeTenantID(5)), + ), + MakeTargetFromSystemTarget( + makeTenantKeyspaceTargetOrFatal(t, roachpb.MakeTenantID(10), roachpb.MakeTenantID(10)), + ), MakeTargetFromSpan(roachpb.Span{Key: roachpb.Key("a"), EndKey: roachpb.Key("b")}), MakeTargetFromSpan(roachpb.Span{Key: roachpb.Key("a"), EndKey: roachpb.Key("d")}), MakeTargetFromSpan(roachpb.Span{Key: roachpb.Key("y"), EndKey: roachpb.Key("z")}), @@ -158,10 +284,38 @@ func TestTargetSortingRandomized(t *testing.T) { } } -func makeTenantTargetOrFatal( +// TestSpanTargetsConstructedInSystemSpanConfigKeyspace ensures that +// constructing span targets +func TestSpanTargetsConstructedInSystemSpanConfigKeyspace(t *testing.T) { + for _, tc := range []roachpb.Span{ + MakeEntireKeyspaceTarget().encode(), + makeTenantKeyspaceTargetOrFatal(t, roachpb.MakeTenantID(10), roachpb.MakeTenantID(10)).encode(), + makeTenantKeyspaceTargetOrFatal(t, roachpb.SystemTenantID, roachpb.SystemTenantID).encode(), + makeTenantKeyspaceTargetOrFatal(t, roachpb.SystemTenantID, roachpb.MakeTenantID(10)).encode(), + { + // Extends into from the left + Key: keys.TimeseriesKeyMax, + EndKey: keys.SystemSpanConfigPrefix.Next(), // End Key isn't inclusive. + }, + { + // Entirely contained. + Key: keys.SystemSpanConfigPrefix.Next(), + EndKey: keys.SystemSpanConfigPrefix.Next().PrefixEnd(), + }, + { + // Extends beyond on the right. + Key: keys.SystemSpanConfigPrefix.Next().PrefixEnd(), + EndKey: keys.SystemSpanConfigKeyMax.Next().Next(), + }, + } { + require.Panics(t, func() { MakeTargetFromSpan(tc) }) + } +} + +func makeTenantKeyspaceTargetOrFatal( t *testing.T, sourceID roachpb.TenantID, targetID roachpb.TenantID, ) SystemTarget { - target, err := MakeTenantTarget(sourceID, targetID) + target, err := MakeTenantKeyspaceTarget(sourceID, targetID) require.NoError(t, err) return target }