diff --git a/pkg/ccl/backupccl/backup_planning_tenant.go b/pkg/ccl/backupccl/backup_planning_tenant.go index 4b4d3524556e..d8e51dcfec6b 100644 --- a/pkg/ccl/backupccl/backup_planning_tenant.go +++ b/pkg/ccl/backupccl/backup_planning_tenant.go @@ -156,21 +156,22 @@ func retrieveAllTenantsMetadata( ) ([]mtinfopb.TenantInfoWithUsage, error) { rows, err := txn.QueryBuffered( ctx, "backupccl.retrieveAllTenantsMetadata", txn.KV(), - // TODO(?): Should we add a `WHERE active`? We require the tenant to be active - // when it is specified. - // See: https://github.com/cockroachdb/cockroach/issues/89997 tenantMetadataQuery(ctx, settings)+` WHERE id != $1`, roachpb.SystemTenantID.ToUint64(), ) if err != nil { return nil, err } - res := make([]mtinfopb.TenantInfoWithUsage, len(rows)) + res := make([]mtinfopb.TenantInfoWithUsage, 0, len(rows)) for i := range rows { - res[i], err = tenantMetadataFromRow(rows[i]) + r, err := tenantMetadataFromRow(rows[i]) if err != nil { return nil, err } + if r.DataState != mtinfopb.DataStateReady { + continue + } + res = append(res, r) } return res, nil } diff --git a/pkg/ccl/backupccl/testdata/backup-restore/restore-tenants b/pkg/ccl/backupccl/testdata/backup-restore/restore-tenants index f3dda12d2577..2624353591c2 100644 --- a/pkg/ccl/backupccl/testdata/backup-restore/restore-tenants +++ b/pkg/ccl/backupccl/testdata/backup-restore/restore-tenants @@ -60,7 +60,6 @@ query-sql SELECT id,active,crdb_internal.pb_to_json('cockroach.multitenant.ProtoInfo', info, true) FROM system.tenants; ---- 1 true {"capabilities": {"canAdminSplit": false}, "deprecatedDataState": "READY", "deprecatedId": "1", "droppedName": "", "tenantReplicationJobId": "0"} -5 false {"capabilities": {"canAdminSplit": false}, "deprecatedDataState": "DROP", "deprecatedId": "5", "droppedName": "tenant-5", "tenantReplicationJobId": "0"} 6 false {"capabilities": {"canAdminSplit": false}, "deprecatedDataState": "ADD", "deprecatedId": "6", "droppedName": "", "tenantReplicationJobId": "0"} exec-sql @@ -82,7 +81,6 @@ query-sql SELECT id,name,data_state,service_mode,active,crdb_internal.pb_to_json('cockroach.multitenant.ProtoInfo', info, true) FROM system.tenants; ---- 1 system 1 2 true {"capabilities": {"canAdminSplit": false}, "deprecatedDataState": "READY", "deprecatedId": "1", "droppedName": "", "tenantReplicationJobId": "0"} -5 tenant-5 2 0 false {"capabilities": {"canAdminSplit": false}, "deprecatedDataState": "DROP", "deprecatedId": "5", "droppedName": "tenant-5", "tenantReplicationJobId": "0"} 6 tenant-6 1 1 true {"capabilities": {"canAdminSplit": false}, "deprecatedDataState": "READY", "deprecatedId": "6", "droppedName": "", "tenantReplicationJobId": "0"} exec-sql expect-error-regex=(tenant 6 already exists) @@ -109,7 +107,6 @@ SELECT id,name,data_state,service_mode,active,crdb_internal.pb_to_json('cockroac ---- 1 system 1 2 true {"capabilities": {"canAdminSplit": false}, "deprecatedDataState": "READY", "deprecatedId": "1", "droppedName": "", "tenantReplicationJobId": "0"} 2 newname 1 1 true {"capabilities": {"canAdminSplit": false}, "deprecatedDataState": "READY", "deprecatedId": "2", "droppedName": "", "tenantReplicationJobId": "0"} -5 tenant-5 2 0 false {"capabilities": {"canAdminSplit": false}, "deprecatedDataState": "DROP", "deprecatedId": "5", "droppedName": "tenant-5", "tenantReplicationJobId": "0"} 6 tenant-6 1 1 true {"capabilities": {"canAdminSplit": false}, "deprecatedDataState": "READY", "deprecatedId": "6", "droppedName": "", "tenantReplicationJobId": "0"} # Check that another service mode is also preserved. diff --git a/pkg/cli/clierrorplus/decorate_error.go b/pkg/cli/clierrorplus/decorate_error.go index 8b2f380b56db..9f57f2fdadb7 100644 --- a/pkg/cli/clierrorplus/decorate_error.go +++ b/pkg/cli/clierrorplus/decorate_error.go @@ -120,7 +120,7 @@ func MaybeDecorateError( return connInsecureHint() } - if wErr := (*security.Error)(nil); errors.As(err, &wErr) { + if errors.Is(err, security.ErrCertManagement) { // Avoid errors.Wrapf here so that we have more control over the // formatting of the message with error text. const format = "cannot load certificates.\n" + diff --git a/pkg/cli/democluster/demo_cluster.go b/pkg/cli/democluster/demo_cluster.go index e5c1b7560824..9887d35ecec4 100644 --- a/pkg/cli/democluster/demo_cluster.go +++ b/pkg/cli/democluster/demo_cluster.go @@ -1303,8 +1303,8 @@ func (c *transientCluster) generateCerts(ctx context.Context, certsDir string) ( nodeKeyExists && nodeCertExists && rootClientKeyExists && rootClientCertExists && demoClientKeyExists && demoClientCertExists && - tenantSigningKeyExists && tenantSigningCertExists && - tenantKeyExists && tenantCertExists { + (!c.demoCtx.Multitenant || (tenantSigningKeyExists && tenantSigningCertExists && + (c.demoCtx.DisableServerController || (tenantKeyExists && tenantCertExists)))) { // All good. return nil } @@ -1430,33 +1430,37 @@ func (c *transientCluster) generateCerts(ctx context.Context, certsDir string) ( } } - if !(tenantKeyExists && tenantCertExists) { - c.infoLog(ctx, "generating tenant server key/cert pair in %q", certsDir) - pair, err := security.CreateTenantPair( - certsDir, - caKeyPath, - c.demoCtx.DefaultKeySize, - c.demoCtx.DefaultCertLifetime, - 2, - tlsServerNames, - ) - if err != nil { - return err - } - if err := security.WriteTenantPair(certsDir, pair, true /* overwrite */); err != nil { - return err + if c.demoCtx.Multitenant { + if !(tenantSigningKeyExists && tenantSigningCertExists) { + c.infoLog(ctx, "generating tenant signing key/cert pair in %q", certsDir) + if err := security.CreateTenantSigningPair( + certsDir, + c.demoCtx.DefaultCertLifetime, + true, /* overwrite */ + 2, + ); err != nil { + return err + } } - } - if !(tenantSigningKeyExists && tenantSigningCertExists) { - c.infoLog(ctx, "generating tenant signing key/cert pair in %q", certsDir) - if err := security.CreateTenantSigningPair( - certsDir, - c.demoCtx.DefaultCertLifetime, - true, /* overwrite */ - 2, - ); err != nil { - return err + if c.demoCtx.DisableServerController { + if !(tenantKeyExists && tenantCertExists) { + c.infoLog(ctx, "generating tenant server key/cert pair in %q", certsDir) + pair, err := security.CreateTenantPair( + certsDir, + caKeyPath, + c.demoCtx.DefaultKeySize, + c.demoCtx.DefaultCertLifetime, + 2, + tlsServerNames, + ) + if err != nil { + return err + } + if err := security.WriteTenantPair(certsDir, pair, true /* overwrite */); err != nil { + return err + } + } } } diff --git a/pkg/cli/interactive_tests/test_cert_advisory_validation.tcl b/pkg/cli/interactive_tests/test_cert_advisory_validation.tcl index 50dd331071c2..f91d388af8b2 100644 --- a/pkg/cli/interactive_tests/test_cert_advisory_validation.tcl +++ b/pkg/cli/interactive_tests/test_cert_advisory_validation.tcl @@ -12,7 +12,7 @@ eexpect $prompt # create some cert without an IP address in there. set db_dir "logs/db" -set certs_dir "logs/my-safe-directory" +set certs_dir "my-safe-directory" send "mkdir -p $certs_dir\r" eexpect $prompt diff --git a/pkg/cli/interactive_tests/test_error_hints.tcl b/pkg/cli/interactive_tests/test_error_hints.tcl index 0db35cd579ea..80439e40c955 100644 --- a/pkg/cli/interactive_tests/test_error_hints.tcl +++ b/pkg/cli/interactive_tests/test_error_hints.tcl @@ -43,7 +43,7 @@ end_test # Check what happens when attempting to connect securely to an # insecure server. -send "$argv start-single-node --insecure\r" +send "$argv start-single-node --insecure --store=logs/db\r" eexpect "initialized new cluster" spawn /bin/bash @@ -73,9 +73,24 @@ interrupt interrupt eexpect ":/# " -send "$argv start-single-node --listen-addr=localhost --certs-dir=$certs_dir\r" +send "$argv start-single-node --listen-addr=localhost --certs-dir=$certs_dir --store=logs/db\r" eexpect "restarted pre-existing node" +set spawn_id $client_spawn_id +start_test "Connecting an insecure RPC client to a secure server" +send "$argv node drain 1 --insecure\r" +eexpect "ERROR" +eexpect "failed to connect to the node" +eexpect ":/# " +end_test + +start_test "Connecting an insecure SQL client to a secure server" +send "$argv sql -e 'select 1' --insecure\r" +eexpect "ERROR: node is running secure mode, SSL connection required" +eexpect ":/# " +end_test + + # Check what happens when attempting to connect to something # that is not a CockroachDB server. set spawn_id $shell_spawn_id diff --git a/pkg/cli/testdata/zip/testzip b/pkg/cli/testdata/zip/testzip index 4d5ced4a563c..4446c9df1853 100644 --- a/pkg/cli/testdata/zip/testzip +++ b/pkg/cli/testdata/zip/testzip @@ -108,7 +108,7 @@ debug zip --concurrency=1 --cpu-profile-duration=1s /dev/null [node 1] requesting heap file list... received response... done [node ?] ? heap profiles found [node 1] requesting goroutine dump list... received response... done -[node 1] 0 goroutine dumps found +[node ?] ? goroutine dumps found [node 1] requesting log file ... [node 1] 0 log file ... [node 1] requesting ranges... received response... done diff --git a/pkg/cli/testdata/zip/unavailable b/pkg/cli/testdata/zip/unavailable index a25c97f0c941..65de4298e0c7 100644 --- a/pkg/cli/testdata/zip/unavailable +++ b/pkg/cli/testdata/zip/unavailable @@ -6,87 +6,203 @@ debug zip --concurrency=1 --cpu-profile-duration=0 /dev/null --timeout=.5s [cluster] using SQL address: ... [cluster] creating output file /dev/null... done [cluster] requesting data for debug/events... received response... -[cluster] requesting data for debug/events: last request failed: operation "[cluster] requesting data for debug/events" timed out after 500ms: rpc error: ... +[cluster] requesting data for debug/events: last request failed: operation "[cluster] requesting data for debug/events" timed out after... [cluster] requesting data for debug/events: creating error output: debug/events.json.err.txt... done [cluster] requesting data for debug/rangelog... received response... -[cluster] requesting data for debug/rangelog: last request failed: operation "[cluster] requesting data for debug/rangelog" timed out after 500ms: rpc error: ... +[cluster] requesting data for debug/rangelog: last request failed: operation "[cluster] requesting data for debug/rangelog" timed out after... [cluster] requesting data for debug/rangelog: creating error output: debug/rangelog.json.err.txt... done -[cluster] requesting data for debug/settings... received response... converting to JSON... writing binary output: debug/settings.json... done +[cluster] requesting data for debug/settings... received response... +[cluster] requesting data for debug/settings: last request failed: operation "[cluster] requesting data for debug/settings" timed out after... +[cluster] requesting data for debug/settings: creating error output: debug/settings.json.err.txt... done [cluster] requesting data for debug/reports/problemranges... received response... converting to JSON... writing binary output: debug/reports/problemranges.json... done +[cluster] retrieving SQL data for "".crdb_internal.create_function_statements... writing output: debug/crdb_internal.create_function_statements.txt... +[cluster] retrieving SQL data for "".crdb_internal.create_function_statements: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) +[cluster] retrieving SQL data for "".crdb_internal.create_function_statements: creating error output: debug/crdb_internal.create_function_statements.txt.err.txt... done +[cluster] retrieving SQL data for "".crdb_internal.create_schema_statements... writing output: debug/crdb_internal.create_schema_statements.txt... +[cluster] retrieving SQL data for "".crdb_internal.create_schema_statements: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) +[cluster] retrieving SQL data for "".crdb_internal.create_schema_statements: creating error output: debug/crdb_internal.create_schema_statements.txt.err.txt... done +[cluster] retrieving SQL data for "".crdb_internal.create_statements... writing output: debug/crdb_internal.create_statements.txt... +[cluster] retrieving SQL data for "".crdb_internal.create_statements: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) +[cluster] retrieving SQL data for "".crdb_internal.create_statements: creating error output: debug/crdb_internal.create_statements.txt.err.txt... done +[cluster] retrieving SQL data for "".crdb_internal.create_type_statements... writing output: debug/crdb_internal.create_type_statements.txt... +[cluster] retrieving SQL data for "".crdb_internal.create_type_statements: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) +[cluster] retrieving SQL data for "".crdb_internal.create_type_statements: creating error output: debug/crdb_internal.create_type_statements.txt.err.txt... done [cluster] retrieving SQL data for crdb_internal.cluster_contention_events... writing output: debug/crdb_internal.cluster_contention_events.txt... -[cluster] retrieving SQL data for crdb_internal.cluster_contention_events: last request failed: pq: query execution canceled due to statement timeout +[cluster] retrieving SQL data for crdb_internal.cluster_contention_events: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) [cluster] retrieving SQL data for crdb_internal.cluster_contention_events: creating error output: debug/crdb_internal.cluster_contention_events.txt.err.txt... done +[cluster] retrieving SQL data for crdb_internal.cluster_database_privileges... writing output: debug/crdb_internal.cluster_database_privileges.txt... done [cluster] retrieving SQL data for crdb_internal.cluster_distsql_flows... writing output: debug/crdb_internal.cluster_distsql_flows.txt... -[cluster] retrieving SQL data for crdb_internal.cluster_distsql_flows: last request failed: pq: query execution canceled due to statement timeout +[cluster] retrieving SQL data for crdb_internal.cluster_distsql_flows: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) [cluster] retrieving SQL data for crdb_internal.cluster_distsql_flows: creating error output: debug/crdb_internal.cluster_distsql_flows.txt.err.txt... done -[cluster] retrieving SQL data for crdb_internal.cluster_database_privileges... writing output: debug/crdb_internal.cluster_database_privileges.txt... done +[cluster] retrieving SQL data for crdb_internal.cluster_execution_insights... writing output: debug/crdb_internal.cluster_execution_insights.txt... +[cluster] retrieving SQL data for crdb_internal.cluster_execution_insights: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) +[cluster] retrieving SQL data for crdb_internal.cluster_execution_insights: creating error output: debug/crdb_internal.cluster_execution_insights.txt.err.txt... done +[cluster] retrieving SQL data for crdb_internal.cluster_locks... writing output: debug/crdb_internal.cluster_locks.txt... +[cluster] retrieving SQL data for crdb_internal.cluster_locks: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) +[cluster] retrieving SQL data for crdb_internal.cluster_locks: creating error output: debug/crdb_internal.cluster_locks.txt.err.txt... done [cluster] retrieving SQL data for crdb_internal.cluster_queries... writing output: debug/crdb_internal.cluster_queries.txt... -[cluster] retrieving SQL data for crdb_internal.cluster_queries: last request failed: pq: query execution canceled due to statement timeout +[cluster] retrieving SQL data for crdb_internal.cluster_queries: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) [cluster] retrieving SQL data for crdb_internal.cluster_queries: creating error output: debug/crdb_internal.cluster_queries.txt.err.txt... done [cluster] retrieving SQL data for crdb_internal.cluster_sessions... writing output: debug/crdb_internal.cluster_sessions.txt... -[cluster] retrieving SQL data for crdb_internal.cluster_sessions: last request failed: pq: query execution canceled due to statement timeout +[cluster] retrieving SQL data for crdb_internal.cluster_sessions: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) [cluster] retrieving SQL data for crdb_internal.cluster_sessions: creating error output: debug/crdb_internal.cluster_sessions.txt.err.txt... done [cluster] retrieving SQL data for crdb_internal.cluster_settings... writing output: debug/crdb_internal.cluster_settings.txt... done [cluster] retrieving SQL data for crdb_internal.cluster_transactions... writing output: debug/crdb_internal.cluster_transactions.txt... -[cluster] retrieving SQL data for crdb_internal.cluster_transactions: last request failed: pq: query execution canceled due to statement timeout +[cluster] retrieving SQL data for crdb_internal.cluster_transactions: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) [cluster] retrieving SQL data for crdb_internal.cluster_transactions: creating error output: debug/crdb_internal.cluster_transactions.txt.err.txt... done +[cluster] retrieving SQL data for crdb_internal.cluster_txn_execution_insights... writing output: debug/crdb_internal.cluster_txn_execution_insights.txt... +[cluster] retrieving SQL data for crdb_internal.cluster_txn_execution_insights: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) +[cluster] retrieving SQL data for crdb_internal.cluster_txn_execution_insights: creating error output: debug/crdb_internal.cluster_txn_execution_insights.txt.err.txt... done [cluster] retrieving SQL data for crdb_internal.default_privileges... writing output: debug/crdb_internal.default_privileges.txt... -[cluster] retrieving SQL data for crdb_internal.default_privileges: last request failed: pq: query execution canceled due to statement timeout +[cluster] retrieving SQL data for crdb_internal.default_privileges: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) [cluster] retrieving SQL data for crdb_internal.default_privileges: creating error output: debug/crdb_internal.default_privileges.txt.err.txt... done +[cluster] retrieving SQL data for crdb_internal.index_usage_statistics... writing output: debug/crdb_internal.index_usage_statistics.txt... +[cluster] retrieving SQL data for crdb_internal.index_usage_statistics: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) +[cluster] retrieving SQL data for crdb_internal.index_usage_statistics: creating error output: debug/crdb_internal.index_usage_statistics.txt.err.txt... done +[cluster] retrieving SQL data for crdb_internal.invalid_objects... writing output: debug/crdb_internal.invalid_objects.txt... +[cluster] retrieving SQL data for crdb_internal.invalid_objects: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) +[cluster] retrieving SQL data for crdb_internal.invalid_objects: creating error output: debug/crdb_internal.invalid_objects.txt.err.txt... done [cluster] retrieving SQL data for crdb_internal.jobs... writing output: debug/crdb_internal.jobs.txt... -[cluster] retrieving SQL data for crdb_internal.jobs: last request failed: pq: query execution canceled due to statement timeout +[cluster] retrieving SQL data for crdb_internal.jobs: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) [cluster] retrieving SQL data for crdb_internal.jobs: creating error output: debug/crdb_internal.jobs.txt.err.txt... done -[cluster] retrieving SQL data for system.jobs... writing output: debug/system.jobs.txt... -[cluster] retrieving SQL data for system.jobs: last request failed: pq: query execution canceled due to statement timeout -[cluster] retrieving SQL data for system.jobs: creating error output: debug/system.jobs.txt.err.txt... done -[cluster] retrieving SQL data for system.descriptor... writing output: debug/system.descriptor.txt... -[cluster] retrieving SQL data for system.descriptor: last request failed: pq: query execution canceled due to statement timeout -[cluster] retrieving SQL data for system.descriptor: creating error output: debug/system.descriptor.txt.err.txt... done -[cluster] retrieving SQL data for system.namespace... writing output: debug/system.namespace.txt... -[cluster] retrieving SQL data for system.namespace: last request failed: pq: query execution canceled due to statement timeout -[cluster] retrieving SQL data for system.namespace: creating error output: debug/system.namespace.txt.err.txt... done -[cluster] retrieving SQL data for system.scheduled_jobs... writing output: debug/system.scheduled_jobs.txt... -[cluster] retrieving SQL data for system.scheduled_jobs: last request failed: pq: query execution canceled due to statement timeout -[cluster] retrieving SQL data for system.scheduled_jobs: creating error output: debug/system.scheduled_jobs.txt.err.txt... done -[cluster] retrieving SQL data for "".crdb_internal.create_statements... writing output: debug/crdb_internal.create_statements.txt... -[cluster] retrieving SQL data for "".crdb_internal.create_statements: last request failed: pq: query execution canceled due to statement timeout -[cluster] retrieving SQL data for "".crdb_internal.create_statements: creating error output: debug/crdb_internal.create_statements.txt.err.txt... done -[cluster] retrieving SQL data for "".crdb_internal.create_type_statements... writing output: debug/crdb_internal.create_type_statements.txt... -[cluster] retrieving SQL data for "".crdb_internal.create_type_statements: last request failed: pq: query execution canceled due to statement timeout -[cluster] retrieving SQL data for "".crdb_internal.create_type_statements: creating error output: debug/crdb_internal.create_type_statements.txt.err.txt... done [cluster] retrieving SQL data for crdb_internal.kv_node_liveness... writing output: debug/crdb_internal.kv_node_liveness.txt... -[cluster] retrieving SQL data for crdb_internal.kv_node_liveness: last request failed: pq: query execution canceled due to statement timeout +[cluster] retrieving SQL data for crdb_internal.kv_node_liveness: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) [cluster] retrieving SQL data for crdb_internal.kv_node_liveness: creating error output: debug/crdb_internal.kv_node_liveness.txt.err.txt... done [cluster] retrieving SQL data for crdb_internal.kv_node_status... writing output: debug/crdb_internal.kv_node_status.txt... -[cluster] retrieving SQL data for crdb_internal.kv_node_status: last request failed: pq: query execution canceled due to statement timeout +[cluster] retrieving SQL data for crdb_internal.kv_node_status: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) [cluster] retrieving SQL data for crdb_internal.kv_node_status: creating error output: debug/crdb_internal.kv_node_status.txt.err.txt... done [cluster] retrieving SQL data for crdb_internal.kv_store_status... writing output: debug/crdb_internal.kv_store_status.txt... -[cluster] retrieving SQL data for crdb_internal.kv_store_status: last request failed: pq: query execution canceled due to statement timeout +[cluster] retrieving SQL data for crdb_internal.kv_store_status: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) [cluster] retrieving SQL data for crdb_internal.kv_store_status: creating error output: debug/crdb_internal.kv_store_status.txt.err.txt... done +[cluster] retrieving SQL data for crdb_internal.partitions... writing output: debug/crdb_internal.partitions.txt... +[cluster] retrieving SQL data for crdb_internal.partitions: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) +[cluster] retrieving SQL data for crdb_internal.partitions: creating error output: debug/crdb_internal.partitions.txt.err.txt... done [cluster] retrieving SQL data for crdb_internal.regions... writing output: debug/crdb_internal.regions.txt... -[cluster] retrieving SQL data for crdb_internal.regions: last request failed: pq: query execution canceled due to statement timeout +[cluster] retrieving SQL data for crdb_internal.regions: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) [cluster] retrieving SQL data for crdb_internal.regions: creating error output: debug/crdb_internal.regions.txt.err.txt... done [cluster] retrieving SQL data for crdb_internal.schema_changes... writing output: debug/crdb_internal.schema_changes.txt... -[cluster] retrieving SQL data for crdb_internal.schema_changes: last request failed: pq: query execution canceled due to statement timeout +[cluster] retrieving SQL data for crdb_internal.schema_changes: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) [cluster] retrieving SQL data for crdb_internal.schema_changes: creating error output: debug/crdb_internal.schema_changes.txt.err.txt... done -[cluster] retrieving SQL data for crdb_internal.partitions... writing output: debug/crdb_internal.partitions.txt... -[cluster] retrieving SQL data for crdb_internal.partitions: last request failed: pq: query execution canceled due to statement timeout -[cluster] retrieving SQL data for crdb_internal.partitions: creating error output: debug/crdb_internal.partitions.txt.err.txt... done +[cluster] retrieving SQL data for crdb_internal.super_regions... writing output: debug/crdb_internal.super_regions.txt... +[cluster] retrieving SQL data for crdb_internal.super_regions: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) +[cluster] retrieving SQL data for crdb_internal.super_regions: creating error output: debug/crdb_internal.super_regions.txt.err.txt... done +[cluster] retrieving SQL data for crdb_internal.system_jobs... writing output: debug/crdb_internal.system_jobs.txt... +[cluster] retrieving SQL data for crdb_internal.system_jobs: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) +[cluster] retrieving SQL data for crdb_internal.system_jobs: creating error output: debug/crdb_internal.system_jobs.txt.err.txt... done +[cluster] retrieving SQL data for crdb_internal.table_indexes... writing output: debug/crdb_internal.table_indexes.txt... +[cluster] retrieving SQL data for crdb_internal.table_indexes: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) +[cluster] retrieving SQL data for crdb_internal.table_indexes: creating error output: debug/crdb_internal.table_indexes.txt.err.txt... done +[cluster] retrieving SQL data for crdb_internal.transaction_contention_events... writing output: debug/crdb_internal.transaction_contention_events.txt... +[cluster] retrieving SQL data for crdb_internal.transaction_contention_events: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) +[cluster] retrieving SQL data for crdb_internal.transaction_contention_events: creating error output: debug/crdb_internal.transaction_contention_events.txt.err.txt... done [cluster] retrieving SQL data for crdb_internal.zones... writing output: debug/crdb_internal.zones.txt... -[cluster] retrieving SQL data for crdb_internal.zones: last request failed: pq: query execution canceled due to statement timeout +[cluster] retrieving SQL data for crdb_internal.zones: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) [cluster] retrieving SQL data for crdb_internal.zones: creating error output: debug/crdb_internal.zones.txt.err.txt... done -[cluster] retrieving SQL data for crdb_internal.invalid_objects... writing output: debug/crdb_internal.invalid_objects.txt... -[cluster] retrieving SQL data for crdb_internal.invalid_objects: last request failed: pq: query execution canceled due to statement timeout -[cluster] retrieving SQL data for crdb_internal.invalid_objects: creating error output: debug/crdb_internal.invalid_objects.txt.err.txt... done -[cluster] retrieving SQL data for crdb_internal.index_usage_statistics... writing output: debug/crdb_internal.index_usage_statistics.txt... -[cluster] retrieving SQL data for crdb_internal.index_usage_statistics: last request failed: pq: query execution canceled due to statement timeout -[cluster] retrieving SQL data for crdb_internal.index_usage_statistics: creating error output: debug/crdb_internal.index_usage_statistics.txt.err.txt... done +[cluster] retrieving SQL data for system.database_role_settings... writing output: debug/system.database_role_settings.txt... +[cluster] retrieving SQL data for system.database_role_settings: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) +[cluster] retrieving SQL data for system.database_role_settings: creating error output: debug/system.database_role_settings.txt.err.txt... done +[cluster] retrieving SQL data for system.descriptor... writing output: debug/system.descriptor.txt... +[cluster] retrieving SQL data for system.descriptor: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) +[cluster] retrieving SQL data for system.descriptor: creating error output: debug/system.descriptor.txt.err.txt... done +[cluster] retrieving SQL data for system.eventlog... writing output: debug/system.eventlog.txt... +[cluster] retrieving SQL data for system.eventlog: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) +[cluster] retrieving SQL data for system.eventlog: creating error output: debug/system.eventlog.txt.err.txt... done +[cluster] retrieving SQL data for system.external_connections... writing output: debug/system.external_connections.txt... +[cluster] retrieving SQL data for system.external_connections: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) +[cluster] retrieving SQL data for system.external_connections: creating error output: debug/system.external_connections.txt.err.txt... done +[cluster] retrieving SQL data for system.jobs... writing output: debug/system.jobs.txt... +[cluster] retrieving SQL data for system.jobs: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) +[cluster] retrieving SQL data for system.jobs: creating error output: debug/system.jobs.txt.err.txt... done +[cluster] retrieving SQL data for system.lease... writing output: debug/system.lease.txt... +[cluster] retrieving SQL data for system.lease: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) +[cluster] retrieving SQL data for system.lease: creating error output: debug/system.lease.txt.err.txt... done +[cluster] retrieving SQL data for system.locations... writing output: debug/system.locations.txt... +[cluster] retrieving SQL data for system.locations: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) +[cluster] retrieving SQL data for system.locations: creating error output: debug/system.locations.txt.err.txt... done +[cluster] retrieving SQL data for system.migrations... writing output: debug/system.migrations.txt... +[cluster] retrieving SQL data for system.migrations: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) +[cluster] retrieving SQL data for system.migrations: creating error output: debug/system.migrations.txt.err.txt... done +[cluster] retrieving SQL data for system.namespace... writing output: debug/system.namespace.txt... +[cluster] retrieving SQL data for system.namespace: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) +[cluster] retrieving SQL data for system.namespace: creating error output: debug/system.namespace.txt.err.txt... done +[cluster] retrieving SQL data for system.privileges... writing output: debug/system.privileges.txt... +[cluster] retrieving SQL data for system.privileges: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) +[cluster] retrieving SQL data for system.privileges: creating error output: debug/system.privileges.txt.err.txt... done +[cluster] retrieving SQL data for system.protected_ts_meta... writing output: debug/system.protected_ts_meta.txt... +[cluster] retrieving SQL data for system.protected_ts_meta: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) +[cluster] retrieving SQL data for system.protected_ts_meta: creating error output: debug/system.protected_ts_meta.txt.err.txt... done +[cluster] retrieving SQL data for system.protected_ts_records... writing output: debug/system.protected_ts_records.txt... +[cluster] retrieving SQL data for system.protected_ts_records: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) +[cluster] retrieving SQL data for system.protected_ts_records: creating error output: debug/system.protected_ts_records.txt.err.txt... done +[cluster] retrieving SQL data for system.rangelog... writing output: debug/system.rangelog.txt... +[cluster] retrieving SQL data for system.rangelog: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) +[cluster] retrieving SQL data for system.rangelog: creating error output: debug/system.rangelog.txt.err.txt... done +[cluster] retrieving SQL data for system.replication_constraint_stats... writing output: debug/system.replication_constraint_stats.txt... +[cluster] retrieving SQL data for system.replication_constraint_stats: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) +[cluster] retrieving SQL data for system.replication_constraint_stats: creating error output: debug/system.replication_constraint_stats.txt.err.txt... done +[cluster] retrieving SQL data for system.replication_critical_localities... writing output: debug/system.replication_critical_localities.txt... +[cluster] retrieving SQL data for system.replication_critical_localities: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) +[cluster] retrieving SQL data for system.replication_critical_localities: creating error output: debug/system.replication_critical_localities.txt.err.txt... done +[cluster] retrieving SQL data for system.replication_stats... writing output: debug/system.replication_stats.txt... +[cluster] retrieving SQL data for system.replication_stats: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) +[cluster] retrieving SQL data for system.replication_stats: creating error output: debug/system.replication_stats.txt.err.txt... done +[cluster] retrieving SQL data for system.reports_meta... writing output: debug/system.reports_meta.txt... +[cluster] retrieving SQL data for system.reports_meta: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) +[cluster] retrieving SQL data for system.reports_meta: creating error output: debug/system.reports_meta.txt.err.txt... done +[cluster] retrieving SQL data for system.role_id_seq... writing output: debug/system.role_id_seq.txt... +[cluster] retrieving SQL data for system.role_id_seq: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) +[cluster] retrieving SQL data for system.role_id_seq: creating error output: debug/system.role_id_seq.txt.err.txt... done +[cluster] retrieving SQL data for system.role_members... writing output: debug/system.role_members.txt... +[cluster] retrieving SQL data for system.role_members: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) +[cluster] retrieving SQL data for system.role_members: creating error output: debug/system.role_members.txt.err.txt... done +[cluster] retrieving SQL data for system.role_options... writing output: debug/system.role_options.txt... +[cluster] retrieving SQL data for system.role_options: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) +[cluster] retrieving SQL data for system.role_options: creating error output: debug/system.role_options.txt.err.txt... done +[cluster] retrieving SQL data for system.scheduled_jobs... writing output: debug/system.scheduled_jobs.txt... +[cluster] retrieving SQL data for system.scheduled_jobs: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) +[cluster] retrieving SQL data for system.scheduled_jobs: creating error output: debug/system.scheduled_jobs.txt.err.txt... done +[cluster] retrieving SQL data for system.settings... writing output: debug/system.settings.txt... +[cluster] retrieving SQL data for system.settings: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) +[cluster] retrieving SQL data for system.settings: creating error output: debug/system.settings.txt.err.txt... done +[cluster] retrieving SQL data for system.span_configurations... writing output: debug/system.span_configurations.txt... +[cluster] retrieving SQL data for system.span_configurations: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) +[cluster] retrieving SQL data for system.span_configurations: creating error output: debug/system.span_configurations.txt.err.txt... done +[cluster] retrieving SQL data for system.sql_instances... writing output: debug/system.sql_instances.txt... +[cluster] retrieving SQL data for system.sql_instances: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) +[cluster] retrieving SQL data for system.sql_instances: creating error output: debug/system.sql_instances.txt.err.txt... done +[cluster] retrieving SQL data for system.sqlliveness... writing output: debug/system.sqlliveness.txt... +[cluster] retrieving SQL data for system.sqlliveness: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) +[cluster] retrieving SQL data for system.sqlliveness: creating error output: debug/system.sqlliveness.txt.err.txt... done +[cluster] retrieving SQL data for system.statement_diagnostics... writing output: debug/system.statement_diagnostics.txt... +[cluster] retrieving SQL data for system.statement_diagnostics: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) +[cluster] retrieving SQL data for system.statement_diagnostics: creating error output: debug/system.statement_diagnostics.txt.err.txt... done +[cluster] retrieving SQL data for system.statement_diagnostics_requests... writing output: debug/system.statement_diagnostics_requests.txt... +[cluster] retrieving SQL data for system.statement_diagnostics_requests: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) +[cluster] retrieving SQL data for system.statement_diagnostics_requests: creating error output: debug/system.statement_diagnostics_requests.txt.err.txt... done +[cluster] retrieving SQL data for system.table_statistics... writing output: debug/system.table_statistics.txt... +[cluster] retrieving SQL data for system.table_statistics: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) +[cluster] retrieving SQL data for system.table_statistics: creating error output: debug/system.table_statistics.txt.err.txt... done +[cluster] retrieving SQL data for system.tenant_settings... writing output: debug/system.tenant_settings.txt... +[cluster] retrieving SQL data for system.tenant_settings: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) +[cluster] retrieving SQL data for system.tenant_settings: creating error output: debug/system.tenant_settings.txt.err.txt... done +[cluster] retrieving SQL data for system.tenant_usage... writing output: debug/system.tenant_usage.txt... +[cluster] retrieving SQL data for system.tenant_usage: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) +[cluster] retrieving SQL data for system.tenant_usage: creating error output: debug/system.tenant_usage.txt.err.txt... done +[cluster] retrieving SQL data for system.tenants... writing output: debug/system.tenants.txt... +[cluster] retrieving SQL data for system.tenants: last request failed: ERROR: query execution canceled due to statement timeout (SQLSTATE 57014) +[cluster] retrieving SQL data for system.tenants: creating error output: debug/system.tenants.txt.err.txt... done [cluster] requesting nodes... received response... -[cluster] requesting nodes: last request failed: operation "[cluster] requesting nodes" timed out after 500ms: rpc error: ... +[cluster] requesting nodes: last request failed: operation "[cluster] requesting nodes" timed out after... [cluster] requesting nodes: creating error output: debug/nodes.json.err.txt... done -[cluster] requesting liveness... received response... converting to JSON... writing binary output: debug/liveness.json... done +[cluster] requesting liveness... received response... +[cluster] requesting liveness: last request failed: operation "[cluster] requesting liveness" timed out after... +[cluster] requesting liveness: creating error output: debug/liveness.json.err.txt... done +[cluster] requesting tenant ranges... received response... +[cluster] requesting tenant ranges: last request failed: rpc error: ... +[cluster] requesting tenant ranges: creating error output: debug/tenant_ranges.err.txt... done [node 1] node status... converting to JSON... writing binary output: debug/nodes/1/status.json... done [node 1] using SQL connection URL: postgresql://... +[node 1] retrieving SQL data for crdb_internal.active_range_feeds... writing output: debug/nodes/1/crdb_internal.active_range_feeds.txt... done [node 1] retrieving SQL data for crdb_internal.feature_usage... writing output: debug/nodes/1/crdb_internal.feature_usage.txt... done [node 1] retrieving SQL data for crdb_internal.gossip_alerts... writing output: debug/nodes/1/crdb_internal.gossip_alerts.txt... done [node 1] retrieving SQL data for crdb_internal.gossip_liveness... writing output: debug/nodes/1/crdb_internal.gossip_liveness.txt... done @@ -95,6 +211,7 @@ debug zip --concurrency=1 --cpu-profile-duration=0 /dev/null --timeout=.5s [node 1] retrieving SQL data for crdb_internal.node_build_info... writing output: debug/nodes/1/crdb_internal.node_build_info.txt... done [node 1] retrieving SQL data for crdb_internal.node_contention_events... writing output: debug/nodes/1/crdb_internal.node_contention_events.txt... done [node 1] retrieving SQL data for crdb_internal.node_distsql_flows... writing output: debug/nodes/1/crdb_internal.node_distsql_flows.txt... done +[node 1] retrieving SQL data for crdb_internal.node_execution_insights... writing output: debug/nodes/1/crdb_internal.node_execution_insights.txt... done [node 1] retrieving SQL data for crdb_internal.node_inflight_trace_spans... writing output: debug/nodes/1/crdb_internal.node_inflight_trace_spans.txt... done [node 1] retrieving SQL data for crdb_internal.node_metrics... writing output: debug/nodes/1/crdb_internal.node_metrics.txt... done [node 1] retrieving SQL data for crdb_internal.node_queries... writing output: debug/nodes/1/crdb_internal.node_queries.txt... done @@ -103,11 +220,13 @@ debug zip --concurrency=1 --cpu-profile-duration=0 /dev/null --timeout=.5s [node 1] retrieving SQL data for crdb_internal.node_statement_statistics... writing output: debug/nodes/1/crdb_internal.node_statement_statistics.txt... done [node 1] retrieving SQL data for crdb_internal.node_transaction_statistics... writing output: debug/nodes/1/crdb_internal.node_transaction_statistics.txt... done [node 1] retrieving SQL data for crdb_internal.node_transactions... writing output: debug/nodes/1/crdb_internal.node_transactions.txt... done +[node 1] retrieving SQL data for crdb_internal.node_txn_execution_insights... writing output: debug/nodes/1/crdb_internal.node_txn_execution_insights.txt... done [node 1] retrieving SQL data for crdb_internal.node_txn_stats... writing output: debug/nodes/1/crdb_internal.node_txn_stats.txt... done [node 1] requesting data for debug/nodes/1/details... received response... converting to JSON... writing binary output: debug/nodes/1/details.json... done [node 1] requesting data for debug/nodes/1/gossip... received response... converting to JSON... writing binary output: debug/nodes/1/gossip.json... done [node 1] requesting data for debug/nodes/1/enginestats... received response... converting to JSON... writing binary output: debug/nodes/1/enginestats.json... done [node 1] requesting stacks... received response... writing binary output: debug/nodes/1/stacks.txt... done +[node 1] requesting stacks with labels... received response... writing binary output: debug/nodes/1/stacks_with_labels.txt... done [node 1] requesting heap profile... received response... writing binary output: debug/nodes/1/heap.pprof... done [node 1] requesting heap file list... received response... [node 1] requesting heap file list: last request failed: rpc error: ... @@ -119,7 +238,7 @@ debug zip --concurrency=1 --cpu-profile-duration=0 /dev/null --timeout=.5s [node 1] 1 log file ... [node 1] [log file ... [node 1] requesting ranges... received response... done -[node 1] 42 ranges found +[node 1] 58 ranges found [node 1] writing range 1... converting to JSON... writing binary output: debug/nodes/1/ranges/1.json... done [node 1] writing range 2... converting to JSON... writing binary output: debug/nodes/1/ranges/2.json... done [node 1] writing range 3... converting to JSON... writing binary output: debug/nodes/1/ranges/3.json... done @@ -162,5 +281,22 @@ debug zip --concurrency=1 --cpu-profile-duration=0 /dev/null --timeout=.5s [node 1] writing range 40... converting to JSON... writing binary output: debug/nodes/1/ranges/40.json... done [node 1] writing range 41... converting to JSON... writing binary output: debug/nodes/1/ranges/41.json... done [node 1] writing range 42... converting to JSON... writing binary output: debug/nodes/1/ranges/42.json... done +[node 1] writing range 43... converting to JSON... writing binary output: debug/nodes/1/ranges/43.json... done +[node 1] writing range 44... converting to JSON... writing binary output: debug/nodes/1/ranges/44.json... done +[node 1] writing range 45... converting to JSON... writing binary output: debug/nodes/1/ranges/45.json... done +[node 1] writing range 46... converting to JSON... writing binary output: debug/nodes/1/ranges/46.json... done +[node 1] writing range 47... converting to JSON... writing binary output: debug/nodes/1/ranges/47.json... done +[node 1] writing range 48... converting to JSON... writing binary output: debug/nodes/1/ranges/48.json... done +[node 1] writing range 49... converting to JSON... writing binary output: debug/nodes/1/ranges/49.json... done +[node 1] writing range 50... converting to JSON... writing binary output: debug/nodes/1/ranges/50.json... done +[node 1] writing range 51... converting to JSON... writing binary output: debug/nodes/1/ranges/51.json... done +[node 1] writing range 52... converting to JSON... writing binary output: debug/nodes/1/ranges/52.json... done +[node 1] writing range 53... converting to JSON... writing binary output: debug/nodes/1/ranges/53.json... done +[node 1] writing range 54... converting to JSON... writing binary output: debug/nodes/1/ranges/54.json... done +[node 1] writing range 55... converting to JSON... writing binary output: debug/nodes/1/ranges/55.json... done +[node 1] writing range 56... converting to JSON... writing binary output: debug/nodes/1/ranges/56.json... done +[node 1] writing range 57... converting to JSON... writing binary output: debug/nodes/1/ranges/57.json... done +[node 1] writing range 58... converting to JSON... writing binary output: debug/nodes/1/ranges/58.json... done [cluster] pprof summary script... writing binary output: debug/pprof-summary.sh... done [cluster] hot range summary script... writing binary output: debug/hot-ranges.sh... done +[cluster] tenant hot range summary script... writing binary output: debug/hot-ranges-tenant.sh... done diff --git a/pkg/cli/zip_test.go b/pkg/cli/zip_test.go index 70a05e5bfdff..7daf8baa1a09 100644 --- a/pkg/cli/zip_test.go +++ b/pkg/cli/zip_test.go @@ -345,16 +345,22 @@ func eraseNonDeterministicZipOutput(out string) string { out = re.ReplaceAllString(out, `dial tcp ...`) re = regexp.MustCompile(`(?m)rpc error: .*$`) out = re.ReplaceAllString(out, `rpc error: ...`) + re = regexp.MustCompile(`(?m)timed out after.*$`) + out = re.ReplaceAllString(out, `timed out after...`) re = regexp.MustCompile(`(?m)failed to connect to .*$`) out = re.ReplaceAllString(out, `failed to connect to ...`) // The number of memory profiles previously collected is not deterministic. re = regexp.MustCompile(`(?m)^\[node \d+\] \d+ heap profiles found$`) out = re.ReplaceAllString(out, `[node ?] ? heap profiles found`) + re = regexp.MustCompile(`(?m)^\[node \d+\] \d+ goroutine dumps found$`) + out = re.ReplaceAllString(out, `[node ?] ? goroutine dumps found`) re = regexp.MustCompile(`(?m)^\[node \d+\] retrieving (memprof|memstats).*$` + "\n") out = re.ReplaceAllString(out, ``) re = regexp.MustCompile(`(?m)^\[node \d+\] writing profile.*$` + "\n") out = re.ReplaceAllString(out, ``) + re = regexp.MustCompile(`(?m)^\[node \d+\] writing dump.*$` + "\n") + out = re.ReplaceAllString(out, ``) //out = strings.ReplaceAll(out, "\n\n", "\n") return out diff --git a/pkg/kv/kvserver/kvstorage/BUILD.bazel b/pkg/kv/kvserver/kvstorage/BUILD.bazel index 1974aafe25b1..5887d87140f9 100644 --- a/pkg/kv/kvserver/kvstorage/BUILD.bazel +++ b/pkg/kv/kvserver/kvstorage/BUILD.bazel @@ -7,13 +7,16 @@ go_library( "cluster_version.go", "doc.go", "init.go", + "replica_state.go", ], importpath = "github.com/cockroachdb/cockroach/pkg/kv/kvserver/kvstorage", visibility = ["//visibility:public"], deps = [ "//pkg/clusterversion", "//pkg/keys", + "//pkg/kv/kvserver/kvserverpb", "//pkg/kv/kvserver/logstore", + "//pkg/kv/kvserver/stateloader", "//pkg/roachpb", "//pkg/storage", "//pkg/util/hlc", diff --git a/pkg/kv/kvserver/kvstorage/init.go b/pkg/kv/kvserver/kvstorage/init.go index 00d76d452460..f791d8d99217 100644 --- a/pkg/kv/kvserver/kvstorage/init.go +++ b/pkg/kv/kvserver/kvstorage/init.go @@ -17,6 +17,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/kv/kvserver/logstore" + "github.com/cockroachdb/cockroach/pkg/kv/kvserver/stateloader" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/storage" "github.com/cockroachdb/cockroach/pkg/util/hlc" @@ -312,6 +313,29 @@ func (r Replica) ID() storage.FullReplicaID { } } +// Load loads the state necessary to instantiate a replica in memory. +func (r Replica) Load( + ctx context.Context, eng storage.Reader, storeID roachpb.StoreID, +) (LoadedReplicaState, error) { + ls := LoadedReplicaState{ + ReplicaID: r.ReplicaID, + hardState: r.hardState, + } + sl := stateloader.Make(r.Desc.RangeID) + var err error + if ls.LastIndex, err = sl.LoadLastIndex(ctx, eng); err != nil { + return LoadedReplicaState{}, err + } + if ls.ReplState, err = sl.Load(ctx, eng, r.Desc); err != nil { + return LoadedReplicaState{}, err + } + + if err := ls.check(storeID); err != nil { + return LoadedReplicaState{}, err + } + return ls, nil +} + // A replicaMap organizes a set of Replicas with unique RangeIDs. type replicaMap map[roachpb.RangeID]Replica diff --git a/pkg/kv/kvserver/kvstorage/replica_state.go b/pkg/kv/kvserver/kvstorage/replica_state.go new file mode 100644 index 000000000000..245edb789edb --- /dev/null +++ b/pkg/kv/kvserver/kvstorage/replica_state.go @@ -0,0 +1,103 @@ +// Copyright 2023 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package kvstorage + +import ( + "context" + + "github.com/cockroachdb/cockroach/pkg/kv/kvserver/kvserverpb" + "github.com/cockroachdb/cockroach/pkg/kv/kvserver/stateloader" + "github.com/cockroachdb/cockroach/pkg/roachpb" + "github.com/cockroachdb/cockroach/pkg/storage" + "github.com/cockroachdb/errors" + "go.etcd.io/raft/v3/raftpb" +) + +// LoadedReplicaState represents the state of a Replica loaded from storage, and +// is used to initialize the in-memory Replica instance. +// TODO(pavelkalinnikov): integrate with kvstorage.Replica. +type LoadedReplicaState struct { + ReplicaID roachpb.ReplicaID + LastIndex uint64 + ReplState kvserverpb.ReplicaState + + hardState raftpb.HardState +} + +// LoadReplicaState loads the state necessary to create a Replica with the +// specified range descriptor, which can be either initialized or uninitialized. +// It also verifies replica state invariants. +// TODO(pavelkalinnikov): integrate with stateloader. +func LoadReplicaState( + ctx context.Context, + eng storage.Reader, + storeID roachpb.StoreID, + desc *roachpb.RangeDescriptor, + replicaID roachpb.ReplicaID, +) (LoadedReplicaState, error) { + sl := stateloader.Make(desc.RangeID) + id, found, err := sl.LoadRaftReplicaID(ctx, eng) + if err != nil { + return LoadedReplicaState{}, err + } else if !found { + return LoadedReplicaState{}, errors.AssertionFailedf( + "r%d: RaftReplicaID not found", desc.RangeID) + } else if loaded := id.ReplicaID; loaded != replicaID { + return LoadedReplicaState{}, errors.AssertionFailedf( + "r%d: loaded RaftReplicaID %d does not match %d", desc.RangeID, loaded, replicaID) + } + + ls := LoadedReplicaState{ReplicaID: replicaID} + if ls.hardState, err = sl.LoadHardState(ctx, eng); err != nil { + return LoadedReplicaState{}, err + } + if ls.LastIndex, err = sl.LoadLastIndex(ctx, eng); err != nil { + return LoadedReplicaState{}, err + } + if ls.ReplState, err = sl.Load(ctx, eng, desc); err != nil { + return LoadedReplicaState{}, err + } + + if err := ls.check(storeID); err != nil { + return LoadedReplicaState{}, err + } + return ls, nil +} + +// check makes sure that the replica invariants hold for the loaded state. +func (r LoadedReplicaState) check(storeID roachpb.StoreID) error { + desc := r.ReplState.Desc + if r.ReplicaID == 0 { + return errors.AssertionFailedf("r%d: replicaID is 0", desc.RangeID) + } + + if !desc.IsInitialized() { + // An uninitialized replica must have an empty HardState.Commit at all + // times. Failure to maintain this invariant indicates corruption. And yet, + // we have observed this in the wild. See #40213. + if hs := r.hardState; hs.Commit != 0 { + return errors.AssertionFailedf( + "r%d/%d: non-zero HardState.Commit on uninitialized replica: %+v", desc.RangeID, r.ReplicaID, hs) + } + // TODO(pavelkalinnikov): assert r.lastIndex == 0? + return nil + } + // desc.IsInitialized() == true + + // INVARIANT: a replica's RangeDescriptor always contains the local Store. + if replDesc, ok := desc.GetReplicaDescriptor(storeID); !ok { + return errors.AssertionFailedf("%+v does not contain local store s%d", desc, storeID) + } else if replDesc.ReplicaID != r.ReplicaID { + return errors.AssertionFailedf( + "%+v does not contain replicaID %d for local store s%d", desc, r.ReplicaID, storeID) + } + return nil +} diff --git a/pkg/kv/kvserver/logstore/logstore.go b/pkg/kv/kvserver/logstore/logstore.go index 88c785695dd5..e6243bd236ff 100644 --- a/pkg/kv/kvserver/logstore/logstore.go +++ b/pkg/kv/kvserver/logstore/logstore.go @@ -154,7 +154,7 @@ func (s *LogStore) storeEntriesAndCommitBatch( // it once the in-progress disk writes complete. defer func() { if batch != nil { - defer batch.Close() + batch.Close() } }() @@ -243,19 +243,23 @@ func (s *LogStore) storeEntriesAndCommitBatch( if nonBlockingSync { // If non-blocking synchronization is enabled, apply the batched updates to // the engine and initiate a synchronous disk write, but don't wait for the - // write to complete. Instead, enqueue that waiting on the SyncWaiterLoop, - // who will signal the callback when the write completes. + // write to complete. if err := batch.CommitNoSyncWait(); err != nil { const expl = "while committing batch without sync wait" return RaftState{}, errors.Wrap(err, expl) } stats.PebbleEnd = timeutil.Now() - s.SyncWaiter.enqueue(ctx, batch, func() { - // NOTE: run on the SyncWaiterLoop goroutine. - logCommitEnd := timeutil.Now() - s.Metrics.RaftLogCommitLatency.RecordValue(logCommitEnd.Sub(stats.PebbleBegin).Nanoseconds()) - cb.OnLogSync(ctx, m.Responses) - }) + // Instead, enqueue that waiting on the SyncWaiterLoop, who will signal the + // callback when the write completes. + waiterCallback := nonBlockingSyncWaiterCallbackPool.Get().(*nonBlockingSyncWaiterCallback) + *waiterCallback = nonBlockingSyncWaiterCallback{ + ctx: ctx, + cb: cb, + msgs: m.Responses, + metrics: s.Metrics, + logCommitBegin: stats.PebbleBegin, + } + s.SyncWaiter.enqueue(ctx, batch, waiterCallback) // Do not Close batch on return. Will be Closed by SyncWaiterLoop. batch = nil } else { @@ -310,6 +314,38 @@ func (s *LogStore) storeEntriesAndCommitBatch( return state, nil } +// nonBlockingSyncWaiterCallback packages up the callback that is handed to the +// SyncWaiterLoop during a non-blocking Raft log sync. Structuring the callback +// as a struct with a method instead of an anonymous function avoids individual +// fields escaping to the heap. It also provides the opportunity to pool the +// callback. +type nonBlockingSyncWaiterCallback struct { + // Used to run SyncCallback. + ctx context.Context + cb SyncCallback + msgs []raftpb.Message + // Used to record Metrics. + metrics Metrics + logCommitBegin time.Time +} + +// run is the callback's logic. It is executed on the SyncWaiterLoop goroutine. +func (cb *nonBlockingSyncWaiterCallback) run() { + dur := timeutil.Since(cb.logCommitBegin).Nanoseconds() + cb.metrics.RaftLogCommitLatency.RecordValue(dur) + cb.cb.OnLogSync(cb.ctx, cb.msgs) + cb.release() +} + +func (cb *nonBlockingSyncWaiterCallback) release() { + *cb = nonBlockingSyncWaiterCallback{} + nonBlockingSyncWaiterCallbackPool.Put(cb) +} + +var nonBlockingSyncWaiterCallbackPool = sync.Pool{ + New: func() interface{} { return new(nonBlockingSyncWaiterCallback) }, +} + var valPool = sync.Pool{ New: func() interface{} { return &roachpb.Value{} }, } diff --git a/pkg/kv/kvserver/logstore/sync_waiter.go b/pkg/kv/kvserver/logstore/sync_waiter.go index e62f482b8579..31880796dfc0 100644 --- a/pkg/kv/kvserver/logstore/sync_waiter.go +++ b/pkg/kv/kvserver/logstore/sync_waiter.go @@ -31,6 +31,15 @@ type syncWaiter interface { var _ syncWaiter = storage.Batch(nil) +// syncWaiterCallback is a callback provided to a SyncWaiterLoop. +// The callback is structured as an interface instead of a closure to allow +// users to batch the callback and its inputs into a single heap object, and +// then pool the allocation of that object. +type syncWaiterCallback interface { + // run executes the callback. + run() +} + // SyncWaiterLoop waits on a sequence of in-progress disk writes, notifying // callbacks when their corresponding disk writes have completed. // Invariant: The callbacks are notified in the order that they were enqueued @@ -44,16 +53,19 @@ type SyncWaiterLoop struct { type syncBatch struct { wg syncWaiter - cb func() + cb syncWaiterCallback } // NewSyncWaiterLoop constructs a SyncWaiterLoop. It must be Started before use. func NewSyncWaiterLoop() *SyncWaiterLoop { return &SyncWaiterLoop{ - // We size the waiter loop's queue to the same size as Pebble's sync - // concurrency. This is the maximum number of pending syncWaiter's that - // pebble allows. - q: make(chan syncBatch, record.SyncConcurrency), + // We size the waiter loop's queue to twice the size of Pebble's sync + // concurrency, which is the maximum number of pending syncWaiter's that + // pebble allows. Doubling the size gives us headroom to prevent the sync + // waiter loop from blocking on calls to enqueue, even if consumption from + // the queue is delayed. If the pipeline is going to block, we'd prefer for + // it to do so during the call to batch.CommitNoSyncWait. + q: make(chan syncBatch, 2*record.SyncConcurrency), stopped: make(chan struct{}), logEveryEnqueueBlocked: log.Every(1 * time.Second), } @@ -84,7 +96,7 @@ func (w *SyncWaiterLoop) waitLoop(ctx context.Context, stopper *stop.Stopper) { log.Fatalf(ctx, "SyncWait error: %+v", err) } w.wg.Close() - w.cb() + w.cb.run() case <-stopper.ShouldQuiesce(): return } @@ -100,7 +112,7 @@ func (w *SyncWaiterLoop) waitLoop(ctx context.Context, stopper *stop.Stopper) { // // If the SyncWaiterLoop has already been stopped, the callback will never be // called. -func (w *SyncWaiterLoop) enqueue(ctx context.Context, wg syncWaiter, cb func()) { +func (w *SyncWaiterLoop) enqueue(ctx context.Context, wg syncWaiter, cb syncWaiterCallback) { b := syncBatch{wg, cb} select { case w.q <- b: @@ -108,9 +120,10 @@ func (w *SyncWaiterLoop) enqueue(ctx context.Context, wg syncWaiter, cb func()) default: if w.logEveryEnqueueBlocked.ShouldLog() { // NOTE: we don't expect to hit this because we size the enqueue channel - // with enough capacity to hold as many in-progress sync operations as - // Pebble allows (pebble/record.SyncConcurrency). - log.Warningf(ctx, "SyncWaiterLoop.enqueue blocking due to insufficient channel capacity") + // with enough capacity to hold more in-progress sync operations than + // Pebble allows (pebble/record.SyncConcurrency). However, we can still + // see this in cases where consumption from the queue is delayed. + log.VWarningf(ctx, 1, "SyncWaiterLoop.enqueue blocking due to insufficient channel capacity") } select { case w.q <- b: diff --git a/pkg/kv/kvserver/logstore/sync_waiter_test.go b/pkg/kv/kvserver/logstore/sync_waiter_test.go index ffc12492933f..ff050ff3b3ad 100644 --- a/pkg/kv/kvserver/logstore/sync_waiter_test.go +++ b/pkg/kv/kvserver/logstore/sync_waiter_test.go @@ -32,7 +32,8 @@ func TestSyncWaiterLoop(t *testing.T) { // Enqueue a waiter while the loop is running. c := make(chan struct{}) wg1 := make(chanSyncWaiter) - w.enqueue(ctx, wg1, func() { close(c) }) + cb1 := funcSyncWaiterCallback(func() { close(c) }) + w.enqueue(ctx, wg1, cb1) // Callback is not called before SyncWait completes. select { @@ -49,8 +50,9 @@ func TestSyncWaiterLoop(t *testing.T) { // regardless of how many times it is called. stopper.Stop(ctx) wg2 := make(chanSyncWaiter) + cb2 := funcSyncWaiterCallback(func() { t.Fatalf("callback unexpectedly called") }) for i := 0; i < 2*cap(w.q); i++ { - w.enqueue(ctx, wg2, func() { t.Fatalf("callback unexpectedly called") }) + w.enqueue(ctx, wg2, cb2) } // Callback should not be called, even after SyncWait completes. @@ -72,7 +74,7 @@ func BenchmarkSyncWaiterLoop(b *testing.B) { // performance of operations inside the SyncWaiterLoop. wg := make(chanSyncWaiter) c := make(chan struct{}) - cb := func() { c <- struct{}{} } + cb := funcSyncWaiterCallback(func() { c <- struct{}{} }) b.ResetTimer() for i := 0; i < b.N; i++ { @@ -91,3 +93,8 @@ func (c chanSyncWaiter) SyncWait() error { } func (c chanSyncWaiter) Close() {} + +// funcSyncWaiterCallback implements the syncWaiterCallback interface. +type funcSyncWaiterCallback func() + +func (f funcSyncWaiterCallback) run() { f() } diff --git a/pkg/kv/kvserver/replica_init.go b/pkg/kv/kvserver/replica_init.go index c2494e35bba7..4b8caf4aa77c 100644 --- a/pkg/kv/kvserver/replica_init.go +++ b/pkg/kv/kvserver/replica_init.go @@ -20,6 +20,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/kv/kvserver/closedts/tracker" "github.com/cockroachdb/cockroach/pkg/kv/kvserver/concurrency" "github.com/cockroachdb/cockroach/pkg/kv/kvserver/kvserverbase" + "github.com/cockroachdb/cockroach/pkg/kv/kvserver/kvstorage" "github.com/cockroachdb/cockroach/pkg/kv/kvserver/load" "github.com/cockroachdb/cockroach/pkg/kv/kvserver/logstore" "github.com/cockroachdb/cockroach/pkg/kv/kvserver/split" @@ -41,45 +42,43 @@ const ( mergeQueueThrottleDuration = 5 * time.Second ) -// newReplica constructs a new Replica. If the desc is initialized, the store -// must be present in it and the corresponding replica descriptor must have -// replicaID as its ReplicaID. -func newReplica( - ctx context.Context, desc *roachpb.RangeDescriptor, store *Store, replicaID roachpb.ReplicaID, +// loadInitializedReplicaForTesting loads and constructs an initialized Replica, +// after checking its invariants. +func loadInitializedReplicaForTesting( + ctx context.Context, store *Store, desc *roachpb.RangeDescriptor, replicaID roachpb.ReplicaID, ) (*Replica, error) { - repl := newUnloadedReplica(ctx, desc.RangeID, store, replicaID) - repl.raftMu.Lock() - defer repl.raftMu.Unlock() - repl.mu.Lock() - defer repl.mu.Unlock() - - // TODO(pavelkalinnikov): this path is taken only in tests. Remove it and - // assert desc.IsInitialized(). if !desc.IsInitialized() { - repl.assertStateRaftMuLockedReplicaMuRLocked(ctx, store.Engine()) - return repl, nil + return nil, errors.AssertionFailedf("can not load with uninitialized descriptor: %s", desc) } + state, err := kvstorage.LoadReplicaState(ctx, store.engine, store.StoreID(), desc, replicaID) + if err != nil { + return nil, err + } + return newInitializedReplica(store, state) +} - if err := repl.loadRaftMuLockedReplicaMuLocked(desc); err != nil { +// newInitializedReplica creates an initialized Replica from its loaded state. +func newInitializedReplica(store *Store, loaded kvstorage.LoadedReplicaState) (*Replica, error) { + r := newUninitializedReplica(store, loaded.ReplState.Desc.RangeID, loaded.ReplicaID) + r.raftMu.Lock() + defer r.raftMu.Unlock() + r.mu.Lock() + defer r.mu.Unlock() + if err := r.initRaftMuLockedReplicaMuLocked(loaded); err != nil { return nil, err } - return repl, nil + return r, nil } -// newUnloadedReplica partially constructs a Replica. The returned replica is -// assumed to be uninitialized, until Replica.loadRaftMuLockedReplicaMuLocked() -// is called with the correct descriptor. The primary reason this function -// exists separately from Replica.loadRaftMuLockedReplicaMuLocked() is to avoid -// attempting to fully construct a Replica and load it from storage prior to -// proving that it can exist during the delicate synchronization dance in -// Store.tryGetOrCreateReplica(). A Replica returned from this function must not -// be used in any way until the load method has been called. -func newUnloadedReplica( - ctx context.Context, rangeID roachpb.RangeID, store *Store, replicaID roachpb.ReplicaID, +// newUninitializedReplica constructs an uninitialized Replica with the given +// range/replica ID. The returned replica remains uninitialized until +// Replica.loadRaftMuLockedReplicaMuLocked() is called. +// +// TODO(#94912): we actually have another initialization path which should be +// refactored: Store.maybeMarkReplicaInitializedLockedReplLocked(). +func newUninitializedReplica( + store *Store, rangeID roachpb.RangeID, replicaID roachpb.ReplicaID, ) *Replica { - if replicaID == 0 { - log.Fatalf(ctx, "cannot construct a replica for range %d with a 0 replica ID", rangeID) - } uninitState := stateloader.UninitializedReplicaState(rangeID) r := &Replica{ AmbientContext: store.cfg.AmbientCtx, @@ -177,26 +176,26 @@ func (r *Replica) setStartKeyLocked(startKey roachpb.RKey) { r.startKey = startKey } -// loadRaftMuLockedReplicaMuLocked loads the state of the initialized replica -// from storage. After this method returns, Replica is initialized, and can not -// be loaded again. -// -// This method is called in two places: +// initRaftMuLockedReplicaMuLocked initializes the Replica using the state +// loaded from storage. Must not be called more than once on a Replica. // -// 1. newReplica - used when the store is initializing and during testing -// 2. splitPostApply - this call initializes a previously uninitialized Replica. -func (r *Replica) loadRaftMuLockedReplicaMuLocked(desc *roachpb.RangeDescriptor) error { - ctx := r.AnnotateCtx(context.TODO()) - if !desc.IsInitialized() { - return errors.AssertionFailedf("r%d: cannot load an uninitialized replica", desc.RangeID) +// This method is called in: +// - loadInitializedReplicaForTesting, to finalize creating an initialized replica; +// - splitPostApply, to initialize a previously uninitialized replica. +func (r *Replica) initRaftMuLockedReplicaMuLocked(s kvstorage.LoadedReplicaState) error { + desc := s.ReplState.Desc + // Ensure that the loaded state corresponds to the same replica. + if desc.RangeID != r.RangeID || s.ReplicaID != r.replicaID { + return errors.AssertionFailedf( + "%s: trying to init with other replica's state r%d/%d", r, desc.RangeID, s.ReplicaID) } - if r.IsInitialized() { - return errors.AssertionFailedf("r%d: cannot reinitialize an initialized replica", desc.RangeID) - } else if r.replicaID == 0 { - // NB: This is just a defensive check as r.mu.replicaID should never be 0. - return errors.AssertionFailedf("r%d: cannot initialize replica without a replicaID", - desc.RangeID) + // Ensure that we transition to initialized replica, and do it only once. + if !desc.IsInitialized() { + return errors.AssertionFailedf("%s: cannot init replica with uninitialized descriptor", r) + } else if r.IsInitialized() { + return errors.AssertionFailedf("%s: cannot reinitialize an initialized replica", r) } + r.setStartKeyLocked(desc.StartKey) // Clear the internal raft group in case we're being reset. Since we're @@ -204,31 +203,11 @@ func (r *Replica) loadRaftMuLockedReplicaMuLocked(desc *roachpb.RangeDescriptor) // group. r.mu.internalRaftGroup = nil - var err error - if r.mu.state, err = r.mu.stateLoader.Load(ctx, r.Engine(), desc); err != nil { - return err - } - r.mu.lastIndexNotDurable, err = r.mu.stateLoader.LoadLastIndex(ctx, r.Engine()) - if err != nil { - return err - } + r.mu.state = s.ReplState + r.mu.lastIndexNotDurable = s.LastIndex r.mu.lastTermNotDurable = invalidLastTerm - // Ensure that we're not trying to load a replica with a different ID than - // was used to construct this Replica. - var replicaID roachpb.ReplicaID - if replicaDesc, found := r.mu.state.Desc.GetReplicaDescriptor(r.StoreID()); found { - replicaID = replicaDesc.ReplicaID - } else { - return errors.AssertionFailedf("r%d: cannot initialize replica which is not in descriptor %v", - desc.RangeID, desc) - } - if r.replicaID != replicaID { - return errors.AssertionFailedf("attempting to initialize a replica which has ID %d with ID %d", - r.replicaID, replicaID) - } - - r.setDescLockedRaftMuLocked(ctx, desc) + r.setDescLockedRaftMuLocked(r.AnnotateCtx(context.TODO()), desc) // Only do this if there was a previous lease. This shouldn't be important // to do but consider that the first lease which is obtained is back-dated @@ -242,8 +221,6 @@ func (r *Replica) loadRaftMuLockedReplicaMuLocked(desc *roachpb.RangeDescriptor) r.mu.minLeaseProposedTS = r.Clock().NowAsClockTimestamp() } - r.assertStateRaftMuLockedReplicaMuRLocked(ctx, r.store.Engine()) - return nil } diff --git a/pkg/kv/kvserver/split_queue_test.go b/pkg/kv/kvserver/split_queue_test.go index 9b7ac669ca7a..9f3fe38cdf92 100644 --- a/pkg/kv/kvserver/split_queue_test.go +++ b/pkg/kv/kvserver/split_queue_test.go @@ -100,7 +100,7 @@ func TestSplitQueueShouldQueue(t *testing.T) { replicaID := cpy.Replicas().VoterDescriptors()[0].ReplicaID require.NoError(t, logstore.NewStateLoader(cpy.RangeID).SetRaftReplicaID(ctx, tc.store.engine, replicaID)) - repl, err := newReplica(ctx, &cpy, tc.store, replicaID) + repl, err := loadInitializedReplicaForTesting(ctx, tc.store, &cpy, replicaID) if err != nil { t.Fatal(err) } diff --git a/pkg/kv/kvserver/store.go b/pkg/kv/kvserver/store.go index 06dd59d52255..72d8b09ebf6b 100644 --- a/pkg/kv/kvserver/store.go +++ b/pkg/kv/kvserver/store.go @@ -1862,7 +1862,12 @@ func (s *Store) Start(ctx context.Context, stopper *stop.Stopper) error { // Uninitialized Replicas are not currently instantiated at store start. continue } - rep, err := newReplica(ctx, repl.Desc, s, repl.ReplicaID) + // TODO(pavelkalinnikov): integrate into kvstorage.LoadAndReconcileReplicas. + state, err := repl.Load(ctx, s.Engine(), s.StoreID()) + if err != nil { + return err + } + rep, err := newInitializedReplica(s, state) if err != nil { return err } diff --git a/pkg/kv/kvserver/store_create_replica.go b/pkg/kv/kvserver/store_create_replica.go index 947b000c39a0..edd8d4de8313 100644 --- a/pkg/kv/kvserver/store_create_replica.go +++ b/pkg/kv/kvserver/store_create_replica.go @@ -15,6 +15,7 @@ import ( "time" "github.com/cockroachdb/cockroach/pkg/keys" + "github.com/cockroachdb/cockroach/pkg/kv/kvserver/kvstorage" "github.com/cockroachdb/cockroach/pkg/kv/kvserver/stateloader" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/storage" @@ -222,16 +223,6 @@ func (s *Store) tryGetOrCreateReplica( return nil, false, &roachpb.RaftGroupDeletedError{} } - // An uninitialized replica must have an empty HardState.Commit at all times. - // Failure to maintain this invariant indicates corruption. And yet, we have - // observed this in the wild. See #40213. - sl := stateloader.Make(rangeID) - if hs, err := sl.LoadHardState(ctx, s.Engine()); err != nil { - return nil, false, err - } else if hs.Commit != 0 { - log.Fatalf(ctx, "found non-zero HardState.Commit on uninitialized replica r%d/%d. HS=%+v", - rangeID, replicaID, hs) - } // Write the RaftReplicaID for this replica. This is the only place in the // CockroachDB code that we are creating a new *uninitialized* replica. // Note that it is possible that we have already created the HardState for @@ -258,20 +249,21 @@ func (s *Store) tryGetOrCreateReplica( // HardState but no RaftReplicaID, see kvstorage.LoadAndReconcileReplicas. // So after first call to this method we have the invariant that all replicas // have a RaftReplicaID persisted. + sl := stateloader.Make(rangeID) if err := sl.SetRaftReplicaID(ctx, s.Engine(), replicaID); err != nil { return nil, false, err } + // Make sure that storage invariants for this uninitialized replica hold. + uninitDesc := roachpb.RangeDescriptor{RangeID: rangeID} + _, err := kvstorage.LoadReplicaState(ctx, s.Engine(), s.StoreID(), &uninitDesc, replicaID) + if err != nil { + return nil, false, err + } + // Create a new uninitialized replica and lock it for raft processing. - repl := newUnloadedReplica(ctx, rangeID, s, replicaID) + repl := newUninitializedReplica(s, rangeID, replicaID) repl.raftMu.Lock() // not unlocked - repl.mu.Lock() - // TODO(pavelkalinnikov): there is little benefit in this check, since loading - // ReplicaID is a no-op after the above write, and the ReplicaState load is - // only for making sure it's empty. Distill the useful IO and make its result - // the direct input into Replica creation, then this check won't be needed. - repl.assertStateRaftMuLockedReplicaMuRLocked(ctx, s.Engine()) - repl.mu.Unlock() // Install the replica in the store's replica map. s.mu.Lock() diff --git a/pkg/kv/kvserver/store_pool_test.go b/pkg/kv/kvserver/store_pool_test.go index c19bb7d981b8..8a05320d9d54 100644 --- a/pkg/kv/kvserver/store_pool_test.go +++ b/pkg/kv/kvserver/store_pool_test.go @@ -223,7 +223,7 @@ func TestStorePoolUpdateLocalStoreBeforeGossip(t *testing.T) { const replicaID = 1 require.NoError(t, logstore.NewStateLoader(rg.RangeID).SetRaftReplicaID(ctx, store.engine, replicaID)) - replica, err := newReplica(ctx, &rg, store, replicaID) + replica, err := loadInitializedReplicaForTesting(ctx, store, &rg, replicaID) if err != nil { t.Fatalf("make replica error : %+v", err) } diff --git a/pkg/kv/kvserver/store_split.go b/pkg/kv/kvserver/store_split.go index d787397674b0..7f71f16f277a 100644 --- a/pkg/kv/kvserver/store_split.go +++ b/pkg/kv/kvserver/store_split.go @@ -14,6 +14,7 @@ import ( "bytes" "context" + "github.com/cockroachdb/cockroach/pkg/kv/kvserver/kvstorage" "github.com/cockroachdb/cockroach/pkg/kv/kvserver/load" "github.com/cockroachdb/cockroach/pkg/kv/kvserver/stateloader" "github.com/cockroachdb/cockroach/pkg/roachpb" @@ -261,8 +262,11 @@ func prepareRightReplicaForSplit( } // Finish initialization of the RHS. - err := rightRepl.loadRaftMuLockedReplicaMuLocked(&split.RightDesc) - if err != nil { + if state, err := kvstorage.LoadReplicaState( + ctx, r.Engine(), r.StoreID(), &split.RightDesc, rightRepl.replicaID, + ); err != nil { + log.Fatalf(ctx, "%v", err) + } else if err := rightRepl.initRaftMuLockedReplicaMuLocked(state); err != nil { log.Fatalf(ctx, "%v", err) } @@ -291,10 +295,9 @@ func prepareRightReplicaForSplit( // until it receives a Raft message addressed to the right-hand range. But // since new replicas start out quiesced, unless we explicitly awaken the // Raft group, there might not be any Raft traffic for quite a while. - err = rightRepl.withRaftGroupLocked(true, func(r *raft.RawNode) (unquiesceAndWakeLeader bool, _ error) { + if err := rightRepl.withRaftGroupLocked(true, func(r *raft.RawNode) (unquiesceAndWakeLeader bool, _ error) { return true, nil - }) - if err != nil { + }); err != nil { log.Fatalf(ctx, "unable to create raft group for right-hand range in split: %+v", err) } diff --git a/pkg/kv/kvserver/store_test.go b/pkg/kv/kvserver/store_test.go index bebeebef138f..9cd597593ac4 100644 --- a/pkg/kv/kvserver/store_test.go +++ b/pkg/kv/kvserver/store_test.go @@ -546,7 +546,7 @@ func createReplica(s *Store, rangeID roachpb.RangeID, start, end roachpb.RKey) * ); err != nil { panic(err) } - r, err := newReplica(ctx, desc, s, replicaID) + r, err := loadInitializedReplicaForTesting(ctx, s, desc, replicaID) if err != nil { panic(err) } @@ -835,17 +835,12 @@ func TestMaybeMarkReplicaInitialized(t *testing.T) { } newRangeID := roachpb.RangeID(3) - desc := &roachpb.RangeDescriptor{ - RangeID: newRangeID, - } - const replicaID = 1 require.NoError(t, - logstore.NewStateLoader(desc.RangeID).SetRaftReplicaID(ctx, store.engine, replicaID)) - r, err := newReplica(ctx, desc, store, replicaID) - if err != nil { - t.Fatal(err) - } + logstore.NewStateLoader(newRangeID).SetRaftReplicaID(ctx, store.engine, replicaID)) + + r := newUninitializedReplica(store, newRangeID, replicaID) + require.NoError(t, err) store.mu.Lock() defer store.mu.Unlock() @@ -861,7 +856,7 @@ func TestMaybeMarkReplicaInitialized(t *testing.T) { }() // Initialize the range with start and end keys. - desc = protoutil.Clone(desc).(*roachpb.RangeDescriptor) + desc := protoutil.Clone(r.Desc()).(*roachpb.RangeDescriptor) desc.StartKey = roachpb.RKey("b") desc.EndKey = roachpb.RKey("d") desc.InternalReplicas = []roachpb.ReplicaDescriptor{{ diff --git a/pkg/kv/kvserver/stores_test.go b/pkg/kv/kvserver/stores_test.go index c7c90ad16e2a..429b0fbc83cd 100644 --- a/pkg/kv/kvserver/stores_test.go +++ b/pkg/kv/kvserver/stores_test.go @@ -157,7 +157,7 @@ func TestStoresGetReplicaForRangeID(t *testing.T) { require.NoError(t, logstore.NewStateLoader(desc.RangeID).SetRaftReplicaID(ctx, store.engine, replicaID)) - replica, err := newReplica(ctx, desc, store, replicaID) + replica, err := loadInitializedReplicaForTesting(ctx, store, desc, replicaID) if err != nil { t.Fatalf("unexpected error when creating replica: %+v", err) } diff --git a/pkg/rpc/context.go b/pkg/rpc/context.go index 61928d9aebd6..b3ddf4368b2f 100644 --- a/pkg/rpc/context.go +++ b/pkg/rpc/context.go @@ -362,7 +362,7 @@ func (c *Connection) Health() error { // thing. type Context struct { ContextOptions - SecurityContext + *SecurityContext breakerClock breakerClock RemoteClocks *RemoteClockMonitor @@ -499,6 +499,12 @@ type ContextOptions struct { // utility, not a server, and thus misses server configuration, a // cluster version, a node ID, etc. ClientOnly bool + + // UseNodeAuth is only used when ClientOnly is not set. + // When set, it indicates that this rpc.Context is running inside + // the same process as a KV layer and thus should feel empowered + // to use its node cert to perform outgoing RPC dials. + UseNodeAuth bool } func (c ContextOptions) validate() error { @@ -604,9 +610,16 @@ func NewContext(ctx context.Context, opts ContextOptions) *Context { masterCtx, _ := opts.Stopper.WithCancelOnQuiesce(ctx) + secCtx := NewSecurityContext( + opts.Config, + security.ClusterTLSSettings(opts.Settings), + opts.TenantID, + ) + secCtx.useNodeAuth = opts.UseNodeAuth + rpcCtx := &Context{ ContextOptions: opts, - SecurityContext: MakeSecurityContext(opts.Config, security.ClusterTLSSettings(opts.Settings), opts.TenantID), + SecurityContext: secCtx, breakerClock: breakerClock{ clock: opts.Clock, }, @@ -618,6 +631,14 @@ func NewContext(ctx context.Context, opts ContextOptions) *Context { logClosingConnEvery: log.Every(time.Second), } + if !opts.TenantID.IsSet() { + panic("tenant ID not set") + } + + if opts.ClientOnly && opts.Config.User.Undefined() { + panic("client username not set") + } + if !opts.TenantID.IsSystem() { rpcCtx.clientCreds = newTenantClientCreds(opts.TenantID) } @@ -1484,7 +1505,7 @@ func (rpcCtx *Context) SetLocalInternalServer( ) { clientTenantID := rpcCtx.TenantID separateTracers := false - if rpcCtx.TenantID.IsSet() && !rpcCtx.TenantID.IsSystem() { + if !clientTenantID.IsSystem() { // This is a secondary tenant server in the same process as the KV // layer (shared-process multitenancy). In this case, the caller // and the callee use separate tracers, so we can't mix and match @@ -1589,6 +1610,42 @@ func (rpcCtx *Context) dialOptsLocal() ([]grpc.DialOption, error) { return dialOpts, err } +// GetClientTLSConfig decides which TLS client configuration (& +// certificates) to use to reach the remote node. +func (rpcCtx *Context) GetClientTLSConfig() (*tls.Config, error) { + if rpcCtx.config.Insecure { + return nil, nil + } + + cm, err := rpcCtx.GetCertificateManager() + if err != nil { + return nil, wrapError(err) + } + + switch { + case rpcCtx.ClientOnly: + // A CLI command is performing a remote RPC. + tlsCfg, err := cm.GetClientTLSConfig(rpcCtx.config.User) + return tlsCfg, wrapError(err) + + case rpcCtx.UseNodeAuth || rpcCtx.tenID.IsSystem(): + tlsCfg, err := cm.GetNodeClientTLSConfig() + return tlsCfg, wrapError(err) + + case !rpcCtx.tenID.IsSystem(): + // A SQL server running in a standalone server doesn't have access + // to the node certs, and thus must use the standalone tenant + // client cert. + tlsCfg, err := cm.GetTenantTLSConfig() + return tlsCfg, wrapError(err) + + default: + // We don't currently support any other way to use the rpc context. + // go away. + return nil, errors.AssertionFailedf("programming error: rpc context not initialized correctly") + } +} + // dialOptsNetworkCredentials computes options that determines how the // RPC client authenticates itself to the remote server. func (rpcCtx *Context) dialOptsNetworkCredentials() ([]grpc.DialOption, error) { @@ -1596,13 +1653,7 @@ func (rpcCtx *Context) dialOptsNetworkCredentials() ([]grpc.DialOption, error) { if rpcCtx.Config.Insecure { dialOpts = append(dialOpts, grpc.WithTransportCredentials(insecure.NewCredentials())) } else { - var tlsConfig *tls.Config - var err error - if rpcCtx.tenID == roachpb.SystemTenantID { - tlsConfig, err = rpcCtx.GetClientTLSConfig() - } else { - tlsConfig, err = rpcCtx.GetTenantTLSConfig() - } + tlsConfig, err := rpcCtx.GetClientTLSConfig() if err != nil { return nil, err } diff --git a/pkg/rpc/tls.go b/pkg/rpc/tls.go index 7469be778e21..e3fd28dd0fc9 100644 --- a/pkg/rpc/tls.go +++ b/pkg/rpc/tls.go @@ -44,11 +44,11 @@ type lazyCertificateManager struct { } func wrapError(err error) error { - if !errors.HasType(err, (*security.Error)(nil)) { - return &security.Error{ - Message: "problem using security settings", - Err: err, - } + if err == nil { + return nil + } + if errors.Is(err, security.ErrCertManagement) { + err = errors.Wrap(err, "problem using security settings") } return err } @@ -66,18 +66,19 @@ type SecurityContext struct { // httpClient uses the client TLS config. It is initialized lazily. httpClient lazyHTTPClient } + useNodeAuth bool } -// MakeSecurityContext makes a SecurityContext. +// NewSecurityContext instantiates a SecurityContext. // // TODO(tbg): don't take a whole Config. This can be trimmed down significantly. -func MakeSecurityContext( +func NewSecurityContext( cfg *base.Config, tlsSettings security.TLSSettings, tenID roachpb.TenantID, -) SecurityContext { +) *SecurityContext { if tenID.ToUint64() == 0 { panic(errors.AssertionFailedf("programming error: tenant ID not defined")) } - return SecurityContext{ + return &SecurityContext{ Locator: certnames.MakeLocator(cfg.SSLCertsDir), TLSSettings: tlsSettings, config: cfg, @@ -91,7 +92,7 @@ func MakeSecurityContext( func (ctx *SecurityContext) GetCertificateManager() (*security.CertificateManager, error) { ctx.lazy.certificateManager.Do(func() { var opts []security.Option - if ctx.tenID != roachpb.SystemTenantID { + if !(ctx.useNodeAuth || ctx.tenID == roachpb.SystemTenantID) { opts = append(opts, security.ForTenant(ctx.tenID.ToUint64())) } ctx.lazy.certificateManager.cm, ctx.lazy.certificateManager.err = @@ -112,7 +113,9 @@ func (ctx *SecurityContext) GetCertificateManager() (*security.CertificateManage return ctx.lazy.certificateManager.cm, ctx.lazy.certificateManager.err } -var errNoCertificatesFound = errors.New("no certificates found; does certs dir exist?") +var errNoCertificatesFound = errors.Mark( + errors.New("no certificates found; does certs dir exist?"), + security.ErrCertManagement) // GetServerTLSConfig returns the server TLS config, initializing it if needed. // If Insecure is true, return a nil config, otherwise ask the certificate @@ -135,52 +138,6 @@ func (ctx *SecurityContext) GetServerTLSConfig() (*tls.Config, error) { return tlsCfg, nil } -// GetClientTLSConfig returns the client TLS config, initializing it if needed. -// If Insecure is true, return a nil config, otherwise ask the certificate -// manager for a TLS config using certs for the config.User. -// This TLSConfig might **NOT** be suitable to talk to the Admin UI, use GetUIClientTLSConfig instead. -func (ctx *SecurityContext) GetClientTLSConfig() (*tls.Config, error) { - // Early out. - if ctx.config.Insecure { - return nil, nil - } - - cm, err := ctx.GetCertificateManager() - if err != nil { - return nil, wrapError(err) - } - - tlsCfg, err := cm.GetClientTLSConfig(ctx.config.User) - if err != nil { - return nil, wrapError(err) - } - return tlsCfg, nil -} - -// GetTenantTLSConfig returns the client TLS config for the tenant, provided -// the SecurityContext operates on behalf of a secondary tenant (i.e. not the -// system tenant). -// -// If Insecure is true, return a nil config, otherwise retrieves the client -// certificate for the configured tenant from the cert manager. -func (ctx *SecurityContext) GetTenantTLSConfig() (*tls.Config, error) { - // Early out. - if ctx.config.Insecure { - return nil, nil - } - - cm, err := ctx.GetCertificateManager() - if err != nil { - return nil, wrapError(err) - } - - tlsCfg, err := cm.GetTenantTLSConfig() - if err != nil { - return nil, wrapError(err) - } - return tlsCfg, nil -} - // getUIClientTLSConfig returns the client TLS config for Admin UI clients, initializing it if needed. // If Insecure is true, return a nil config, otherwise ask the certificate // manager for a TLS config configured to talk to the Admin UI. diff --git a/pkg/rpc/tls_test.go b/pkg/rpc/tls_test.go index 361576e2d867..5f9a30f8f7f4 100644 --- a/pkg/rpc/tls_test.go +++ b/pkg/rpc/tls_test.go @@ -67,12 +67,13 @@ func TestClientSSLSettings(t *testing.T) { stopper := stop.NewStopper() defer stopper.Stop(ctx) rpcContext := NewContext(ctx, ContextOptions{ - TenantID: roachpb.SystemTenantID, - Clock: &timeutil.DefaultTimeSource{}, - MaxOffset: time.Nanosecond, - Stopper: stopper, - Settings: cluster.MakeTestingClusterSettings(), - Config: cfg, + TenantID: roachpb.SystemTenantID, + ClientOnly: true, + Clock: &timeutil.DefaultTimeSource{}, + MaxOffset: time.Nanosecond, + Stopper: stopper, + Settings: cluster.MakeTestingClusterSettings(), + Config: cfg, }) if cfg.HTTPRequestScheme() != tc.requestScheme { diff --git a/pkg/security/certificate_manager.go b/pkg/security/certificate_manager.go index 1986f01a2d57..d63cbb9a06de 100644 --- a/pkg/security/certificate_manager.go +++ b/pkg/security/certificate_manager.go @@ -13,7 +13,6 @@ package security import ( "context" "crypto/tls" - "fmt" "strconv" "github.com/cockroachdb/cockroach/pkg/security/certnames" @@ -310,29 +309,19 @@ func (cm *CertificateManager) ClientCerts() map[username.SQLUsername]*CertInfo { return cm.clientCerts } -// Error is the error type for this package. -// TODO(knz): make this an error wrapper. -type Error struct { - Message string - Err error -} +// ErrCertManagement is a marker error for errors produced in this package that +// can be identified with errors.Is(). +var ErrCertManagement error = errCertManagement{} -// Error implements the error interface. -func (e *Error) Error() string { - return fmt.Sprintf("%s: %v", e.Message, e.Err) -} +type errCertManagement struct{} + +func (errCertManagement) Error() string { return "error " } // makeErrorf constructs an Error and returns it. -func makeErrorf(err error, format string, args ...interface{}) *Error { - return &Error{ - Message: fmt.Sprintf(format, args...), - Err: err, - } +func makeErrorf(err error, format string, args ...interface{}) error { + return errors.Mark(errors.Wrapf(err, format, args...), ErrCertManagement) } -// makeError constructs an Error with just a string. -func makeError(err error, s string) *Error { return makeErrorf(err, "%s", s) } - // LoadCertificates creates a CertificateLoader to load all certs and keys. // Upon success, it swaps the existing certificates for the new ones. func (cm *CertificateManager) LoadCertificates() error { @@ -396,29 +385,29 @@ func (cm *CertificateManager) LoadCertificates() error { if cm.initialized { // If we ran before, make sure we don't reload with missing/bad certificates. if err := checkCertIsValid(caCert); checkCertIsValid(cm.caCert) == nil && err != nil { - return makeError(err, "reload would lose valid CA cert") + return makeErrorf(err, "reload would lose valid CA cert") } if err := checkCertIsValid(nodeCert); checkCertIsValid(cm.nodeCert) == nil && err != nil { - return makeError(err, "reload would lose valid node cert") + return makeErrorf(err, "reload would lose valid node cert") } if err := checkCertIsValid(nodeClientCert); checkCertIsValid(cm.nodeClientCert) == nil && err != nil { return makeErrorf(err, "reload would lose valid client cert for '%s'", username.NodeUser) } if err := checkCertIsValid(clientCACert); checkCertIsValid(cm.clientCACert) == nil && err != nil { - return makeError(err, "reload would lose valid CA certificate for client verification") + return makeErrorf(err, "reload would lose valid CA certificate for client verification") } if err := checkCertIsValid(uiCACert); checkCertIsValid(cm.uiCACert) == nil && err != nil { - return makeError(err, "reload would lose valid CA certificate for UI") + return makeErrorf(err, "reload would lose valid CA certificate for UI") } if err := checkCertIsValid(uiCert); checkCertIsValid(cm.uiCert) == nil && err != nil { - return makeError(err, "reload would lose valid UI certificate") + return makeErrorf(err, "reload would lose valid UI certificate") } if err := checkCertIsValid(tenantCACert); checkCertIsValid(cm.tenantCACert) == nil && err != nil { - return makeError(err, "reload would lose valid tenant client CA certificate") + return makeErrorf(err, "reload would lose valid tenant client CA certificate") } if err := checkCertIsValid(tenantCert); checkCertIsValid(cm.tenantCert) == nil && err != nil { - return makeError(err, "reload would lose valid tenant client certificate") + return makeErrorf(err, "reload would lose valid tenant client certificate") } } @@ -430,7 +419,7 @@ func (cm *CertificateManager) LoadCertificates() error { // No client certificate for node, but we have a node certificate. Check that // it contains the required client fields. if err := validateDualPurposeNodeCert(nodeCert); err != nil { - return err + return makeErrorf(err, "validating node cert") } } @@ -579,6 +568,41 @@ func (cm *CertificateManager) getEmbeddedServerTLSConfig( return cfg, nil } +// GetNodeClientTLSConfig returns a client TLS config suitable for +// dialing other KV nodes. +func (cm *CertificateManager) GetNodeClientTLSConfig() (*tls.Config, error) { + cm.mu.Lock() + defer cm.mu.Unlock() + + // Return the cached config if we have one. + if cm.clientConfig != nil { + return cm.clientConfig, nil + } + + ca, err := cm.getCACertLocked() + if err != nil { + return nil, err + } + + clientCert, err := cm.getNodeClientCertLocked() + if err != nil { + return nil, err + } + + cfg, err := newClientTLSConfig( + cm.tlsSettings, + clientCert.FileContents, + clientCert.KeyFileContents, + ca.FileContents) + if err != nil { + return nil, err + } + + // Cache the config. + cm.clientConfig = cfg + return cfg, nil +} + // GetUIServerTLSConfig returns a server TLS config for the Admin UI with a // callback to fetch the latest TLS config. We still attempt to get the config to make sure // the initial call has a valid config loaded. @@ -625,7 +649,7 @@ func (cm *CertificateManager) getEmbeddedUIServerTLSConfig( // cm.mu must be held. func (cm *CertificateManager) getCACertLocked() (*CertInfo, error) { if err := checkCertIsValid(cm.caCert); err != nil { - return nil, makeError(err, "problem with CA certificate") + return nil, makeErrorf(err, "problem with CA certificate") } return cm.caCert, nil } @@ -640,7 +664,7 @@ func (cm *CertificateManager) getClientCACertLocked() (*CertInfo, error) { } if err := checkCertIsValid(cm.clientCACert); err != nil { - return nil, makeError(err, "problem with client CA certificate") + return nil, makeErrorf(err, "problem with client CA certificate") } return cm.clientCACert, nil } @@ -655,7 +679,7 @@ func (cm *CertificateManager) getUICACertLocked() (*CertInfo, error) { } if err := checkCertIsValid(cm.uiCACert); err != nil { - return nil, makeError(err, "problem with UI CA certificate") + return nil, makeErrorf(err, "problem with UI CA certificate") } return cm.uiCACert, nil } @@ -664,7 +688,7 @@ func (cm *CertificateManager) getUICACertLocked() (*CertInfo, error) { // cm.mu must be held. func (cm *CertificateManager) getNodeCertLocked() (*CertInfo, error) { if err := checkCertIsValid(cm.nodeCert); err != nil { - return nil, makeError(err, "problem with node certificate") + return nil, makeErrorf(err, "problem with node certificate") } return cm.nodeCert, nil } @@ -683,7 +707,7 @@ func (cm *CertificateManager) getUICertLocked() (*CertInfo, error) { return cm.getTenantCertLocked() } if err := checkCertIsValid(cm.uiCert); err != nil { - return nil, makeError(err, "problem with UI certificate") + return nil, makeErrorf(err, "problem with UI certificate") } return cm.uiCert, nil } @@ -709,16 +733,11 @@ func (cm *CertificateManager) getClientCertLocked(user username.SQLUsername) (*C // cm.mu must be held. func (cm *CertificateManager) getNodeClientCertLocked() (*CertInfo, error) { if cm.nodeClientCert == nil { - // No specific client cert for 'node': use multi-purpose node cert, - // but only if we are in the host cluster. - if cm.IsForTenant() { - return nil, errors.New("no node client cert for a SQL server") - } return cm.getNodeCertLocked() } if err := checkCertIsValid(cm.nodeClientCert); err != nil { - return nil, makeError(err, "problem with node client certificate") + return nil, makeErrorf(err, "problem with node client certificate") } return cm.nodeClientCert, nil } @@ -732,7 +751,7 @@ func (cm *CertificateManager) getTenantCACertLocked() (*CertInfo, error) { } c := cm.tenantCACert if err := checkCertIsValid(c); err != nil { - return nil, makeError(err, "problem with tenant client CA certificate") + return nil, makeErrorf(err, "problem with tenant client CA certificate") } return c, nil } @@ -742,7 +761,7 @@ func (cm *CertificateManager) getTenantCACertLocked() (*CertInfo, error) { func (cm *CertificateManager) getTenantCertLocked() (*CertInfo, error) { c := cm.tenantCert if err := checkCertIsValid(c); err != nil { - return nil, makeError(err, "problem with tenant client certificate") + return nil, makeErrorf(err, "problem with tenant client certificate") } return c, nil } @@ -799,7 +818,7 @@ func (cm *CertificateManager) GetTenantSigningCert() (*CertInfo, error) { c := cm.tenantSigningCert if err := checkCertIsValid(c); err != nil { - return nil, makeError(err, "problem with tenant signing certificate") + return nil, makeErrorf(err, "problem with tenant signing certificate") } return c, nil } @@ -808,51 +827,37 @@ func (cm *CertificateManager) GetTenantSigningCert() (*CertInfo, error) { // Returns the dual-purpose node certs if user == NodeUser and there is no // separate client cert for 'node'. func (cm *CertificateManager) GetClientTLSConfig(user username.SQLUsername) (*tls.Config, error) { + if user.IsNodeUser() { + return cm.GetNodeClientTLSConfig() + } + cm.mu.Lock() defer cm.mu.Unlock() - // We always need the CA cert. - var ca *CertInfo - var err error - if !cm.IsForTenant() { - // Host cluster. - ca, err = cm.getCACertLocked() - if err != nil { - return nil, err - } - } else { - // Tenant server. - ca, err = cm.getTenantCACertLocked() - if err != nil { - return nil, err - } + // The client could be connecting to a KV node or a tenant server. + // We need at least one of the two CAs. If none is available, we + // won't be able to verify the server's identity. + if cm.caCert == nil && cm.tenantCACert == nil { + return nil, makeErrorf(errors.New("no CA certificate found, cannot authenticate remote server"), + "problem loading CA certificate") } - - if !user.IsNodeUser() { - clientCert, err := cm.getClientCertLocked(user) + var caBlob []byte + if cm.caCert != nil { + ca, err := cm.getCACertLocked() if err != nil { return nil, err } - - cfg, err := newClientTLSConfig( - cm.tlsSettings, - clientCert.FileContents, - clientCert.KeyFileContents, - ca.FileContents) + caBlob = AppendCertificatesToBlob(caBlob, ca.FileContents) + } + if cm.tenantCACert != nil { + ca, err := cm.getTenantCACertLocked() if err != nil { return nil, err } - - return cfg, nil - } - - // We're the node user. - // Return the cached config if we have one. - if cm.clientConfig != nil { - return cm.clientConfig, nil + caBlob = AppendCertificatesToBlob(caBlob, ca.FileContents) } - clientCert, err := cm.getNodeClientCertLocked() + clientCert, err := cm.getClientCertLocked(user) if err != nil { return nil, err } @@ -861,13 +866,11 @@ func (cm *CertificateManager) GetClientTLSConfig(user username.SQLUsername) (*tl cm.tlsSettings, clientCert.FileContents, clientCert.KeyFileContents, - ca.FileContents) + caBlob) if err != nil { return nil, err } - // Cache the config. - cm.clientConfig = cfg return cfg, nil } @@ -907,7 +910,7 @@ func (cm *CertificateManager) ListCertificates() ([]*CertInfo, error) { defer cm.mu.RUnlock() if !cm.initialized { - return nil, errors.New("certificate manager has not been initialized") + return nil, errors.AssertionFailedf("certificate manager has not been initialized") } ret := make([]*CertInfo, 0, 2+len(cm.clientCerts)) diff --git a/pkg/security/certs_rotation_test.go b/pkg/security/certs_rotation_test.go index a1831d99c118..1487313fac90 100644 --- a/pkg/security/certs_rotation_test.go +++ b/pkg/security/certs_rotation_test.go @@ -105,7 +105,7 @@ func TestRotateCerts(t *testing.T) { // Test client with the same certs. clientContext := testutils.NewNodeTestBaseContext() clientContext.SSLCertsDir = certsDir - firstSCtx := rpc.MakeSecurityContext(clientContext, security.CommandTLSSettings{}, roachpb.SystemTenantID) + firstSCtx := rpc.NewSecurityContext(clientContext, security.CommandTLSSettings{}, roachpb.SystemTenantID) firstClient, err := firstSCtx.GetHTTPClient() if err != nil { t.Fatalf("could not create http client: %v", err) @@ -137,7 +137,7 @@ func TestRotateCerts(t *testing.T) { clientContext = testutils.NewNodeTestBaseContext() clientContext.SSLCertsDir = certsDir - secondSCtx := rpc.MakeSecurityContext(clientContext, security.CommandTLSSettings{}, roachpb.SystemTenantID) + secondSCtx := rpc.NewSecurityContext(clientContext, security.CommandTLSSettings{}, roachpb.SystemTenantID) secondClient, err := secondSCtx.GetHTTPClient() if err != nil { t.Fatalf("could not create http client: %v", err) @@ -246,7 +246,7 @@ func TestRotateCerts(t *testing.T) { // This is HTTP and succeeds because we do not ask for or verify client certificates. clientContext = testutils.NewNodeTestBaseContext() clientContext.SSLCertsDir = certsDir - thirdSCtx := rpc.MakeSecurityContext(clientContext, security.CommandTLSSettings{}, roachpb.SystemTenantID) + thirdSCtx := rpc.NewSecurityContext(clientContext, security.CommandTLSSettings{}, roachpb.SystemTenantID) thirdClient, err := thirdSCtx.GetHTTPClient() if err != nil { t.Fatalf("could not create http client: %v", err) diff --git a/pkg/security/certs_test.go b/pkg/security/certs_test.go index 0f358097c9ed..2da9a9390918 100644 --- a/pkg/security/certs_test.go +++ b/pkg/security/certs_test.go @@ -399,7 +399,7 @@ func TestUseCerts(t *testing.T) { // Insecure mode. clientContext := testutils.NewNodeTestBaseContext() clientContext.Insecure = true - sCtx := rpc.MakeSecurityContext(clientContext, security.CommandTLSSettings{}, roachpb.SystemTenantID) + sCtx := rpc.NewSecurityContext(clientContext, security.CommandTLSSettings{}, roachpb.SystemTenantID) httpClient, err := sCtx.GetHTTPClient() if err != nil { t.Fatal(err) @@ -419,7 +419,7 @@ func TestUseCerts(t *testing.T) { clientContext = testutils.NewNodeTestBaseContext() clientContext.SSLCertsDir = certsDir { - secondSCtx := rpc.MakeSecurityContext(clientContext, security.CommandTLSSettings{}, roachpb.SystemTenantID) + secondSCtx := rpc.NewSecurityContext(clientContext, security.CommandTLSSettings{}, roachpb.SystemTenantID) httpClient, err = secondSCtx.GetHTTPClient() } if err != nil { @@ -481,7 +481,7 @@ func TestUseSplitCACerts(t *testing.T) { // Insecure mode. clientContext := testutils.NewNodeTestBaseContext() clientContext.Insecure = true - sCtx := rpc.MakeSecurityContext(clientContext, security.CommandTLSSettings{}, roachpb.SystemTenantID) + sCtx := rpc.NewSecurityContext(clientContext, security.CommandTLSSettings{}, roachpb.SystemTenantID) httpClient, err := sCtx.GetHTTPClient() if err != nil { t.Fatal(err) @@ -501,7 +501,7 @@ func TestUseSplitCACerts(t *testing.T) { clientContext = testutils.NewNodeTestBaseContext() clientContext.SSLCertsDir = certsDir { - secondSCtx := rpc.MakeSecurityContext(clientContext, security.CommandTLSSettings{}, roachpb.SystemTenantID) + secondSCtx := rpc.NewSecurityContext(clientContext, security.CommandTLSSettings{}, roachpb.SystemTenantID) httpClient, err = secondSCtx.GetHTTPClient() } if err != nil { @@ -599,7 +599,7 @@ func TestUseWrongSplitCACerts(t *testing.T) { // Insecure mode. clientContext := testutils.NewNodeTestBaseContext() clientContext.Insecure = true - sCtx := rpc.MakeSecurityContext(clientContext, security.CommandTLSSettings{}, roachpb.SystemTenantID) + sCtx := rpc.NewSecurityContext(clientContext, security.CommandTLSSettings{}, roachpb.SystemTenantID) httpClient, err := sCtx.GetHTTPClient() if err != nil { t.Fatal(err) @@ -619,7 +619,7 @@ func TestUseWrongSplitCACerts(t *testing.T) { clientContext = testutils.NewNodeTestBaseContext() clientContext.SSLCertsDir = certsDir { - secondCtx := rpc.MakeSecurityContext(clientContext, security.CommandTLSSettings{}, roachpb.SystemTenantID) + secondCtx := rpc.NewSecurityContext(clientContext, security.CommandTLSSettings{}, roachpb.SystemTenantID) httpClient, err = secondCtx.GetHTTPClient() } if err != nil { diff --git a/pkg/server/server.go b/pkg/server/server.go index 8db279869848..82180f53363a 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -286,6 +286,7 @@ func NewServer(cfg Config, stopper *stop.Stopper) (*Server, error) { rpcCtxOpts := rpc.ContextOptions{ TenantID: roachpb.SystemTenantID, + UseNodeAuth: true, NodeID: cfg.IDContainer, StorageClusterID: cfg.ClusterIDContainer, Config: cfg.Config, diff --git a/pkg/server/server_controller_new_server.go b/pkg/server/server_controller_new_server.go index 6167f742fb0d..07ee8401374f 100644 --- a/pkg/server/server_controller_new_server.go +++ b/pkg/server/server_controller_new_server.go @@ -354,6 +354,9 @@ func makeSharedProcessTenantServerConfig( sqlCfg.TableStatCacheSize = kvServerCfg.SQLConfig.TableStatCacheSize sqlCfg.QueryCacheSize = kvServerCfg.SQLConfig.QueryCacheSize + // LocalKVServerInfo tells the rpc.Context of the tenant's server + // that it is inside the same process as the KV layer and how to + // reach this KV layer without going through the network. sqlCfg.LocalKVServerInfo = &kvServerInfo return baseCfg, sqlCfg, nil diff --git a/pkg/server/tenant.go b/pkg/server/tenant.go index 749d8a2ffb7b..dbda14740cc2 100644 --- a/pkg/server/tenant.go +++ b/pkg/server/tenant.go @@ -827,6 +827,7 @@ func makeTenantSQLServerArgs( } rpcContext := rpc.NewContext(startupCtx, rpc.ContextOptions{ TenantID: sqlCfg.TenantID, + UseNodeAuth: sqlCfg.LocalKVServerInfo != nil, NodeID: baseCfg.IDContainer, StorageClusterID: baseCfg.ClusterIDContainer, Config: baseCfg.Config, @@ -845,10 +846,8 @@ func makeTenantSQLServerArgs( if _, err := rpcContext.GetServerTLSConfig(); err != nil { return sqlServerArgs{}, err } - // Needed for outgoing connections, until this issue - // is fixed: - // https://github.com/cockroachdb/cockroach/issues/96215 - if _, err := rpcContext.GetTenantTLSConfig(); err != nil { + // Needed for outgoing connections. + if _, err := rpcContext.GetClientTLSConfig(); err != nil { return sqlServerArgs{}, err } cm, err := rpcContext.GetCertificateManager() diff --git a/pkg/testutils/lint/passes/errwrap/functions.go b/pkg/testutils/lint/passes/errwrap/functions.go index 0e859fc35761..fe4597356fb0 100644 --- a/pkg/testutils/lint/passes/errwrap/functions.go +++ b/pkg/testutils/lint/passes/errwrap/functions.go @@ -57,6 +57,8 @@ var ErrorFnFormatStringIndex = map[string]int{ "github.com/cockroachdb/errors.NewAssertionErrorWithWrappedErrf": 1, "github.com/cockroachdb/errors.WithSafeDetails": 1, + "github.com/cockroachdb/cockroach/pkg/security.makeErrorf": 1, + "github.com/cockroachdb/cockroach/pkg/roachpb.NewErrorf": 0, "github.com/cockroachdb/cockroach/pkg/sql/importer.makeRowErr": 3,