From 903bd5e7c4e6cf787c90163b24f4815d22b02d59 Mon Sep 17 00:00:00 2001 From: Michael Butler Date: Mon, 13 Mar 2023 12:55:32 -0400 Subject: [PATCH 1/5] backupccl: update restore/nodeshutdown tests to use new roachtest framework The restore/nodeshutdown tests have been using a very old workload that will not be restorable when #93804 lands. This patch changes the restore/nodeshutdown workload to a 80GB tpce restore and moves the tests to run on aws instead of gcp. Release note: None Epic: None --- pkg/cmd/roachtest/tests/restore.go | 102 ++++++++++------------------- 1 file changed, 34 insertions(+), 68 deletions(-) diff --git a/pkg/cmd/roachtest/tests/restore.go b/pkg/cmd/roachtest/tests/restore.go index 9b318ba4ddd0..13bfe8b012bf 100644 --- a/pkg/cmd/roachtest/tests/restore.go +++ b/pkg/cmd/roachtest/tests/restore.go @@ -39,7 +39,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/util/httputil" "github.com/cockroachdb/cockroach/pkg/util/humanizeutil" "github.com/cockroachdb/cockroach/pkg/util/log" - "github.com/cockroachdb/cockroach/pkg/util/retry" "github.com/cockroachdb/cockroach/pkg/util/timeutil" "github.com/cockroachdb/cockroach/pkg/workload/histogram" "github.com/cockroachdb/errors" @@ -235,80 +234,37 @@ func (dul *DiskUsageLogger) Runner(ctx context.Context) error { } } func registerRestoreNodeShutdown(r registry.Registry) { + sp := restoreSpecs{ + hardware: makeHardwareSpecs(hardwareSpecs{}), + backup: makeBackupSpecs( + backupSpecs{workload: tpceRestore{customers: 5000}, + version: "v22.2.1"}), + timeout: 1 * time.Hour, + } + makeRestoreStarter := func(ctx context.Context, t test.Test, c cluster.Cluster, gatewayNode int) jobStarter { return func(c cluster.Cluster, t test.Test) (string, error) { - t.L().Printf("connecting to gateway") - gatewayDB := c.Conn(ctx, t.L(), gatewayNode) - defer gatewayDB.Close() - - t.L().Printf("creating bank database") - if _, err := gatewayDB.Exec("CREATE DATABASE bank"); err != nil { - return "", err - } - - errCh := make(chan error, 1) - go func() { - defer close(errCh) - - // 10 GiB restore. - restoreQuery := `RESTORE bank.bank FROM - 'gs://cockroach-fixtures/workload/bank/version=1.0.0,payload-bytes=100,ranges=10,rows=10000000,seed=1/bank?AUTH=implicit'` - - t.L().Printf("starting to run the restore job") - if _, err := gatewayDB.Exec(restoreQuery); err != nil { - errCh <- err - } - t.L().Printf("done running restore job") - }() - - // Wait for the job. - retryOpts := retry.Options{ - MaxRetries: 50, - InitialBackoff: 1 * time.Second, - MaxBackoff: 5 * time.Second, - } - for r := retry.StartWithCtx(ctx, retryOpts); r.Next(); { - var jobCount int - if err := gatewayDB.QueryRowContext(ctx, "SELECT count(*) FROM [SHOW JOBS] WHERE job_type = 'RESTORE'").Scan(&jobCount); err != nil { - return "", err - } - - select { - case err := <-errCh: - // We got an error when starting the job. - return "", err - default: - } - - if jobCount == 0 { - t.L().Printf("waiting for restore job") - } else if jobCount == 1 { - t.L().Printf("found restore job") - break - } else { - t.L().Printf("found multiple restore jobs -- erroring") - return "", errors.New("unexpectedly found multiple restore jobs") - } - } - - var jobID string - if err := gatewayDB.QueryRowContext(ctx, "SELECT job_id FROM [SHOW JOBS] WHERE job_type = 'RESTORE'").Scan(&jobID); err != nil { - return "", errors.Wrap(err, "querying the job ID") - } - return jobID, nil + sp.getRuntimeSpecs(ctx, t, c) + jobID, err := sp.runDetached(ctx, "DATABASE tpce", gatewayNode) + return fmt.Sprintf("%d", jobID), err } } r.Add(registry.TestSpec{ Name: "restore/nodeShutdown/worker", Owner: registry.OwnerDisasterRecovery, - Cluster: r.MakeClusterSpec(4), + Cluster: sp.hardware.makeClusterSpecs(r), + Timeout: sp.timeout, Run: func(ctx context.Context, t test.Test, c cluster.Cluster) { gatewayNode := 2 nodeToShutdown := 3 + + if c.Spec().Cloud != sp.backup.cloud { + // For now, only run the test on the cloud provider that also stores the backup. + t.Skip("test configured to run on %s", sp.backup.cloud) + } c.Put(ctx, t.Cockroach(), "./cockroach") c.Start(ctx, t.L(), option.DefaultStartOptsNoBackups(), install.MakeClusterSettings()) - jobSurvivesNodeShutdown(ctx, t, c, nodeToShutdown, makeRestoreStarter(ctx, t, c, gatewayNode)) }, }) @@ -316,10 +272,18 @@ func registerRestoreNodeShutdown(r registry.Registry) { r.Add(registry.TestSpec{ Name: "restore/nodeShutdown/coordinator", Owner: registry.OwnerDisasterRecovery, - Cluster: r.MakeClusterSpec(4), + Cluster: sp.hardware.makeClusterSpecs(r), + Timeout: sp.timeout, Run: func(ctx context.Context, t test.Test, c cluster.Cluster) { + gatewayNode := 2 nodeToShutdown := 2 + + if c.Spec().Cloud != sp.backup.cloud { + // For now, only run the test on the cloud provider that also stores the backup. + t.Skip("test configured to run on %s", sp.backup.cloud) + } + c.Put(ctx, t.Cockroach(), "./cockroach") c.Start(ctx, t.L(), option.DefaultStartOptsNoBackups(), install.MakeClusterSettings()) @@ -446,7 +410,7 @@ func registerRestore(r registry.Registry) { defer close(jobIDCh) t.Status(`running restore`) metricCollector := withPauseSpecs.initRestorePerfMetrics(ctx, durationGauge) - jobID, err := withPauseSpecs.runDetached(ctx, "DATABASE tpce") + jobID, err := withPauseSpecs.runDetached(ctx, "DATABASE tpce", 1) require.NoError(t, err) jobIDCh <- jobID @@ -840,14 +804,16 @@ func (sp *restoreSpecs) run(ctx context.Context, target string) error { return sp.c.RunE(ctx, sp.c.Node(1), sp.restoreCmd(target, "")) } -func (sp *restoreSpecs) runDetached(ctx context.Context, target string) (jobspb.JobID, error) { - if err := sp.c.RunE(ctx, sp.c.Node(1), sp.restoreCmd(target, "WITH DETACHED")); err != nil { +func (sp *restoreSpecs) runDetached( + ctx context.Context, target string, node int, +) (jobspb.JobID, error) { + if err := sp.c.RunE(ctx, sp.c.Node(node), sp.restoreCmd(target, "WITH DETACHED")); err != nil { return 0, err } - db, err := sp.c.ConnE(ctx, sp.t.L(), sp.c.Node(1)[0]) + db, err := sp.c.ConnE(ctx, sp.t.L(), sp.c.Node(node)[0]) if err != nil { - return 0, errors.Wrap(err, "failed to connect to node 1; running restore detached") + return 0, errors.Wrapf(err, "failed to connect to node %d; running restore detached", node) } var jobID jobspb.JobID if err := db.QueryRow(`SELECT job_id FROM [SHOW JOBS] WHERE job_type = 'RESTORE'`).Scan(&jobID); err != nil { From 7ea327a11df08f66b65a67e35e38d8d10ae0afaf Mon Sep 17 00:00:00 2001 From: Matthew Todd Date: Sat, 11 Mar 2023 06:34:27 -0500 Subject: [PATCH 2/5] cluster-ui: tenants use sqlstats-supplied regions Fixes #98056. As of #95449, the SQL Activity pages in the DB Console can draw regions information directly from the sqlstats tables, rather than having to translate node IDs to regions on page load. Here, we make that switch, but for non-system tenants only, because: 1. The ephemeral nature of serverless nodes made this view-time mapping especially problematic in that context. (See further notes in #95449.) 2. The system-tenant views also include KV node IDs in a special Regions/Nodes column, which we are unable to recreate given the backend storage structure. (Future design work might suggest removing these node IDs altogether, for a unified UI.) Release note: None --- .../src/indexDetailsPage/indexDetailsPage.tsx | 5 +- .../src/sessions/sessionDetails.tsx | 2 +- .../cluster-ui/src/sqlActivity/util.tsx | 9 +-- .../src/statementDetails/statementDetails.tsx | 10 +-- .../src/statementsPage/statementsPage.tsx | 23 ++++--- .../statementsTable/statementsTable.spec.tsx | 59 ------------------ .../src/statementsTable/statementsTable.tsx | 6 +- .../transactionDetails/transactionDetails.tsx | 8 ++- .../transactionsPage/transactions.fixture.ts | 10 +++ .../src/transactionsPage/transactionsPage.tsx | 17 +++-- .../src/transactionsPage/utils.spec.ts | 37 +++-------- .../cluster-ui/src/transactionsPage/utils.ts | 62 ++++++++----------- 12 files changed, 91 insertions(+), 157 deletions(-) delete mode 100644 pkg/ui/workspaces/cluster-ui/src/statementsTable/statementsTable.spec.tsx diff --git a/pkg/ui/workspaces/cluster-ui/src/indexDetailsPage/indexDetailsPage.tsx b/pkg/ui/workspaces/cluster-ui/src/indexDetailsPage/indexDetailsPage.tsx index 5b2ab2c28fc9..c365a129397a 100644 --- a/pkg/ui/workspaces/cluster-ui/src/indexDetailsPage/indexDetailsPage.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/indexDetailsPage/indexDetailsPage.tsx @@ -10,6 +10,7 @@ import React from "react"; import classNames from "classnames/bind"; +import { flatMap } from "lodash"; import { ISortedTablePagination, SortedTable, @@ -462,7 +463,9 @@ export class IndexDetailsPage extends React.Component< .map(n => Number(n)) .sort(); const regions = unique( - nodes.map(node => nodeRegions[node.toString()]), + isTenant + ? flatMap(statements, statement => statement.stats.regions) + : nodes.map(node => nodeRegions[node.toString()]), ).sort(); const filteredStmts = this.filteredStatements(); diff --git a/pkg/ui/workspaces/cluster-ui/src/sessions/sessionDetails.tsx b/pkg/ui/workspaces/cluster-ui/src/sessions/sessionDetails.tsx index 750d97df64e4..a4b82c830437 100644 --- a/pkg/ui/workspaces/cluster-ui/src/sessions/sessionDetails.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/sessions/sessionDetails.tsx @@ -102,8 +102,8 @@ export class SessionDetails extends React.Component { terminateQueryRef: React.RefObject; componentDidMount(): void { - this.props.refreshNodes(); if (!this.props.isTenant) { + this.props.refreshNodes(); this.props.refreshNodesLiveness(); } this.props.refreshSessions(); diff --git a/pkg/ui/workspaces/cluster-ui/src/sqlActivity/util.tsx b/pkg/ui/workspaces/cluster-ui/src/sqlActivity/util.tsx index 80c132182394..5f0bb539cc4e 100644 --- a/pkg/ui/workspaces/cluster-ui/src/sqlActivity/util.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/sqlActivity/util.tsx @@ -61,14 +61,7 @@ export function filteredStatementsData( // list if the list is not empty. statement => regions.length == 0 || - (statement.stats.nodes && - containAny( - statement.stats.nodes.map( - node => nodeRegions[node.toString()], - regions, - ), - regions, - )), + statement.stats.regions?.some(region => regions.includes(region)), ) .filter( // The statement must contain at least one value from the selected nodes diff --git a/pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.tsx b/pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.tsx index 29413afb5570..0688b4d5cf13 100644 --- a/pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.tsx @@ -316,8 +316,8 @@ export class StatementDetails extends React.Component< ); } this.props.refreshUserSQLRoles(); - this.props.refreshNodes(); if (!this.props.isTenant) { + this.props.refreshNodes(); this.props.refreshNodesLiveness(); if (!this.props.hasViewActivityRedactedRole) { this.props.refreshStatementDiagnosticsRequests(); @@ -335,8 +335,8 @@ export class StatementDetails extends React.Component< this.refreshStatementDetails(); } - this.props.refreshNodes(); if (!this.props.isTenant) { + this.props.refreshNodes(); this.props.refreshNodesLiveness(); if (!this.props.hasViewActivityRedactedRole) { this.props.refreshStatementDiagnosticsRequests(); @@ -571,9 +571,9 @@ export class StatementDetails extends React.Component< (stats.nodes || []).map(node => node.toString()), ).sort(); const regions = unique( - (stats.nodes || []) - .map(node => nodeRegions[node.toString()]) - .filter(r => r), // Remove undefined / unknown regions. + isTenant + ? stats.regions || [] + : nodes.map(node => nodeRegions[node]).filter(r => r), // Remove undefined / unknown regions. ).sort(); const lastExec = diff --git a/pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.tsx b/pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.tsx index 10576973d9d5..17c5c8dc081f 100644 --- a/pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.tsx @@ -10,7 +10,7 @@ import React from "react"; import { RouteComponentProps } from "react-router-dom"; -import { isNil, merge } from "lodash"; +import { flatMap, isNil, merge } from "lodash"; import classNames from "classnames/bind"; import { getValidErrorsList, Loading } from "src/loading"; import { Delayed } from "src/delayed"; @@ -352,9 +352,11 @@ export class StatementsPage extends React.Component< this.refreshDatabases(); this.props.refreshUserSQLRoles(); - this.props.refreshNodes(); - if (!this.props.isTenant && !this.props.hasViewActivityRedactedRole) { - this.props.refreshStatementDiagnosticsRequests(); + if (!this.props.isTenant) { + this.props.refreshNodes(); + if (!this.props.hasViewActivityRedactedRole) { + this.props.refreshStatementDiagnosticsRequests(); + } } } @@ -392,9 +394,11 @@ export class StatementsPage extends React.Component< componentDidUpdate = (): void => { this.updateQueryParams(); - this.props.refreshNodes(); - if (!this.props.isTenant && !this.props.hasViewActivityRedactedRole) { - this.props.refreshStatementDiagnosticsRequests(); + if (!this.props.isTenant) { + this.props.refreshNodes(); + if (!this.props.hasViewActivityRedactedRole) { + this.props.refreshStatementDiagnosticsRequests(); + } } }; @@ -603,6 +607,7 @@ export class StatementsPage extends React.Component< onDiagnosticsModalOpen, apps, databases, + statements, search, isTenant, nodeRegions, @@ -614,7 +619,9 @@ export class StatementsPage extends React.Component< .sort(); const regions = unique( - nodes.map(node => nodeRegions[node.toString()]), + isTenant + ? flatMap(statements, statement => statement.stats.regions) + : nodes.map(node => nodeRegions[node.toString()]), ).sort(); const { filters, activeFilters } = this.state; diff --git a/pkg/ui/workspaces/cluster-ui/src/statementsTable/statementsTable.spec.tsx b/pkg/ui/workspaces/cluster-ui/src/statementsTable/statementsTable.spec.tsx deleted file mode 100644 index e4e9de24803c..000000000000 --- a/pkg/ui/workspaces/cluster-ui/src/statementsTable/statementsTable.spec.tsx +++ /dev/null @@ -1,59 +0,0 @@ -// Copyright 2022 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. - -import { assert } from "chai"; -import Long from "long"; -import { - AggregateStatistics, - populateRegionNodeForStatements, -} from "./statementsTable"; - -describe("populateRegionNodeForStatements", () => { - function statementWithNodeIDs(...nodeIDs: number[]): AggregateStatistics { - return { - aggregatedFingerprintID: "", - aggregatedFingerprintHexID: "", - label: "", - summary: "", - aggregatedTs: 0, - implicitTxn: false, - fullScan: false, - database: "", - applicationName: "", - stats: { nodes: nodeIDs.map(id => Long.fromInt(id)) }, - }; - } - - it("maps nodes to regions, sorted", () => { - const statement = statementWithNodeIDs(1, 2); - populateRegionNodeForStatements([statement], { - "1": "gcp-us-west1", - "2": "gcp-us-east1", - }); - assert.deepEqual(["gcp-us-east1", "gcp-us-west1"], statement.regions); - }); - - it("handles statements without nodes", () => { - const statement = statementWithNodeIDs(); - populateRegionNodeForStatements([statement], { - "1": "gcp-us-west1", - "2": "gcp-us-east1", - }); - assert.deepEqual(statement.regions, []); - }); - - it("excludes nodes whose region we don't know", () => { - const statement = statementWithNodeIDs(1, 2); - populateRegionNodeForStatements([statement], { - "1": "gcp-us-west1", - }); - assert.deepEqual(statement.regions, ["gcp-us-west1"]); - }); -}); diff --git a/pkg/ui/workspaces/cluster-ui/src/statementsTable/statementsTable.tsx b/pkg/ui/workspaces/cluster-ui/src/statementsTable/statementsTable.tsx index adb6adbf5e0c..8964473ce5c2 100644 --- a/pkg/ui/workspaces/cluster-ui/src/statementsTable/statementsTable.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/statementsTable/statementsTable.tsx @@ -73,7 +73,6 @@ export interface AggregateStatistics { diagnosticsReports?: StatementDiagnosticsReport[]; // totalWorkload is the sum of service latency of all statements listed on the table. totalWorkload?: Long; - regions?: string[]; regionNodes?: string[]; } @@ -356,9 +355,9 @@ function makeRegionsColumn( title: statisticsTableTitles.regions(statType), className: cx("statements-table__col-regions"), cell: (stmt: AggregateStatistics) => { - return longListWithTooltip(stmt.regions.sort().join(", "), 50); + return longListWithTooltip(stmt.stats.regions.sort().join(", "), 50); }, - sort: (stmt: AggregateStatistics) => stmt.regions.sort().join(", "), + sort: (stmt: AggregateStatistics) => stmt.stats.regions.sort().join(", "), }; } else { return { @@ -417,7 +416,6 @@ export function populateRegionNodeForStatements( ")", ); }); - stmt.regions = Object.keys(regions).sort(); stmt.regionNodes = regionNodes; }); } diff --git a/pkg/ui/workspaces/cluster-ui/src/transactionDetails/transactionDetails.tsx b/pkg/ui/workspaces/cluster-ui/src/transactionDetails/transactionDetails.tsx index 5c2982033a45..3385d03c8e27 100644 --- a/pkg/ui/workspaces/cluster-ui/src/transactionDetails/transactionDetails.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/transactionDetails/transactionDetails.tsx @@ -260,7 +260,9 @@ export class TransactionDetails extends React.Component< ); } this.props.refreshUserSQLRoles(); - this.props.refreshNodes(); + if (!this.props.isTenant) { + this.props.refreshNodes(); + } } componentWillUnmount(): void { @@ -269,7 +271,9 @@ export class TransactionDetails extends React.Component< componentDidUpdate(prevProps: TransactionDetailsProps): void { this.getTransactionStateInfo(prevProps.transactionFingerprintId); - this.props.refreshNodes(); + if (!this.props.isTenant) { + this.props.refreshNodes(); + } } onChangeSortSetting = (ss: SortSetting): void => { diff --git a/pkg/ui/workspaces/cluster-ui/src/transactionsPage/transactions.fixture.ts b/pkg/ui/workspaces/cluster-ui/src/transactionsPage/transactions.fixture.ts index ec88fe77a466..96f096e82e43 100644 --- a/pkg/ui/workspaces/cluster-ui/src/transactionsPage/transactions.fixture.ts +++ b/pkg/ui/workspaces/cluster-ui/src/transactionsPage/transactions.fixture.ts @@ -88,6 +88,7 @@ export const data: cockroach.server.serverpb.IStatementsResponse = { stats: { count: Long.fromInt(557), nodes: [Long.fromNumber(1), Long.fromNumber(2)], + regions: ["gcp-us-east1"], first_attempt_count: Long.fromInt(557), max_retries: Long.fromInt(0), legacy_last_err: "", @@ -167,6 +168,7 @@ export const data: cockroach.server.serverpb.IStatementsResponse = { stats: { count: Long.fromInt(70), nodes: [Long.fromNumber(1), Long.fromNumber(3)], + regions: ["gcp-us-east1", "gcp-us-west1"], first_attempt_count: Long.fromInt(70), max_retries: Long.fromInt(0), legacy_last_err: "", @@ -233,6 +235,7 @@ export const data: cockroach.server.serverpb.IStatementsResponse = { stats: { count: Long.fromInt(1), nodes: [Long.fromNumber(1), Long.fromNumber(3)], + regions: ["gcp-us-east1", "gcp-us-west1"], first_attempt_count: Long.fromInt(1), max_retries: Long.fromInt(0), legacy_last_err: "", @@ -290,6 +293,7 @@ export const data: cockroach.server.serverpb.IStatementsResponse = { stats: { count: Long.fromInt(280), nodes: [Long.fromNumber(3), Long.fromNumber(4)], + regions: ["gcp-us-west1", "gcp-europe-west1"], first_attempt_count: Long.fromInt(280), max_retries: Long.fromInt(0), legacy_last_err: "", @@ -391,6 +395,7 @@ export const data: cockroach.server.serverpb.IStatementsResponse = { stats: { count: Long.fromInt(1), nodes: [Long.fromNumber(2), Long.fromNumber(4)], + regions: ["gcp-us-east1", "gcp-europe-west1"], first_attempt_count: Long.fromInt(1), max_retries: Long.fromInt(0), legacy_last_err: "", @@ -442,6 +447,7 @@ export const data: cockroach.server.serverpb.IStatementsResponse = { stats: { count: Long.fromInt(1), nodes: [Long.fromNumber(1)], + regions: ["gcp-us-east1"], first_attempt_count: Long.fromInt(1), max_retries: Long.fromInt(0), legacy_last_err: "", @@ -482,6 +488,7 @@ export const data: cockroach.server.serverpb.IStatementsResponse = { stats: { count: Long.fromInt(1), nodes: [Long.fromNumber(3), Long.fromNumber(4)], + regions: ["gcp-us-west1", "gcp-europe-west1"], first_attempt_count: Long.fromInt(1), max_retries: Long.fromInt(0), legacy_last_err: "", @@ -533,6 +540,7 @@ export const data: cockroach.server.serverpb.IStatementsResponse = { stats: { count: Long.fromInt(24), nodes: [Long.fromNumber(2), Long.fromNumber(3)], + regions: ["gcp-us-east1", "gcp-us-west1"], first_attempt_count: Long.fromInt(24), max_retries: Long.fromInt(0), legacy_last_err: "", @@ -622,6 +630,7 @@ export const data: cockroach.server.serverpb.IStatementsResponse = { stats: { count: Long.fromInt(141), nodes: [Long.fromNumber(1), Long.fromNumber(2), Long.fromNumber(3)], + regions: ["gcp-us-east1", "gcp-us-west1"], first_attempt_count: Long.fromInt(141), max_retries: Long.fromInt(0), legacy_last_err: "", @@ -740,6 +749,7 @@ export const data: cockroach.server.serverpb.IStatementsResponse = { Long.fromNumber(3), Long.fromNumber(4), ], + regions: ["gcp-us-east1", "gcp-us-west1", "gcp-europe-west1"], first_attempt_count: Long.fromInt(1), max_retries: Long.fromInt(0), legacy_last_err: "", diff --git a/pkg/ui/workspaces/cluster-ui/src/transactionsPage/transactionsPage.tsx b/pkg/ui/workspaces/cluster-ui/src/transactionsPage/transactionsPage.tsx index d5102531cea4..f1495ca0ffdc 100644 --- a/pkg/ui/workspaces/cluster-ui/src/transactionsPage/transactionsPage.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/transactionsPage/transactionsPage.tsx @@ -36,7 +36,7 @@ import { filterTransactions, } from "./utils"; import Long from "long"; -import { merge } from "lodash"; +import { flatMap, merge } from "lodash"; import { unique, syncHistory } from "src/util"; import { EmptyTransactionsPlaceholder } from "./emptyTransactionsPlaceholder"; import { Loading } from "../loading"; @@ -251,7 +251,10 @@ export class TransactionsPage extends React.Component< ); } - this.props.refreshNodes(); + if (!this.props.isTenant) { + this.props.refreshNodes(); + } + this.props.refreshUserSQLRoles(); } @@ -293,7 +296,9 @@ export class TransactionsPage extends React.Component< componentDidUpdate(): void { this.updateQueryParams(); - this.props.refreshNodes(); + if (!this.props.isTenant) { + this.props.refreshNodes(); + } } onChangeSortSetting = (ss: SortSetting): void => { @@ -425,7 +430,9 @@ export class TransactionsPage extends React.Component< .sort(); const regions = unique( - nodes.map(node => nodeRegions[node.toString()]), + isTenant + ? flatMap(statements, statement => statement.stats.regions) + : nodes.map(node => nodeRegions[node.toString()]), ).sort(); // We apply the search filters and app name filters prior to aggregating across Node IDs @@ -508,7 +515,7 @@ export class TransactionsPage extends React.Component< t => ({ stats_data: t.stats_data, node_id: t.node_id, - regions: generateRegion(t, statements, nodeRegions), + regions: generateRegion(t, statements), regionNodes: generateRegionNode(t, statements, nodeRegions), }), ); diff --git a/pkg/ui/workspaces/cluster-ui/src/transactionsPage/utils.spec.ts b/pkg/ui/workspaces/cluster-ui/src/transactionsPage/utils.spec.ts index 1056c9275dfd..b7a415707c89 100644 --- a/pkg/ui/workspaces/cluster-ui/src/transactionsPage/utils.spec.ts +++ b/pkg/ui/workspaces/cluster-ui/src/transactionsPage/utils.spec.ts @@ -293,9 +293,7 @@ SELECT _`, }); describe("generateRegion", () => { - function transactionWithStatementFingerprintIDs( - ...ids: number[] - ): Transaction { + function transaction(...ids: number[]): Transaction { return { stats_data: { statement_fingerprint_ids: ids.map(id => Long.fromInt(id)), @@ -303,35 +301,18 @@ describe("generateRegion", () => { }; } - function statementWithFingerprintAndNodeIDs( - id: number, - ...nodeIDs: number[] - ): Statement { - return { - id: Long.fromInt(id), - stats: { nodes: nodeIDs.map(id => Long.fromInt(id)) }, - }; + function statement(id: number, ...regions: string[]): Statement { + return { id: Long.fromInt(id), stats: { regions } }; } it("gathers up the list of regions for the transaction, sorted", () => { assert.deepEqual( - generateRegion( - transactionWithStatementFingerprintIDs(42), - [statementWithFingerprintAndNodeIDs(42, 1, 2)], - { "1": "gcp-us-west1", "2": "gcp-us-east1" }, - ), - ["gcp-us-east1", "gcp-us-west1"], - ); - }); - - it("skips over nodes with unknown regions", () => { - assert.deepEqual( - generateRegion( - transactionWithStatementFingerprintIDs(42), - [statementWithFingerprintAndNodeIDs(42, 1, 2)], - { "1": "gcp-us-west1" }, - ), - ["gcp-us-west1"], + generateRegion(transaction(42, 43, 44), [ + statement(42, "gcp-us-west1", "gcp-us-east1"), + statement(43, "gcp-us-west1"), + statement(44, "gcp-us-central1"), + ]), + ["gcp-us-central1", "gcp-us-east1", "gcp-us-west1"], ); }); }); diff --git a/pkg/ui/workspaces/cluster-ui/src/transactionsPage/utils.ts b/pkg/ui/workspaces/cluster-ui/src/transactionsPage/utils.ts index 01316df3552f..49459d7e099f 100644 --- a/pkg/ui/workspaces/cluster-ui/src/transactionsPage/utils.ts +++ b/pkg/ui/workspaces/cluster-ui/src/transactionsPage/utils.ts @@ -156,6 +156,7 @@ export const searchTransactionsData = ( ); }; +// TODO(todd): Remove unused nodeRegions parameter. export const filterTransactions = ( data: Transaction[], filters: Filters, @@ -210,31 +211,31 @@ export const filterTransactions = ( timeValue === "empty", ) .filter((t: Transaction) => { - // The transaction must contain at least one value of the nodes - // and regions list (if the list is not empty). - if (regions.length == 0 && nodes.length == 0) return true; - // If the cluster is a tenant cluster we don't care - // about nodes. - let foundRegion: boolean = regions.length == 0; - let foundNode: boolean = isTenant || nodes.length == 0; - - getStatementsByFingerprintId( + // The transaction must contain at least one value of the regions list + // (if the list is not empty). + if (regions.length == 0) return true; + + return getStatementsByFingerprintId( t.stats_data.statement_fingerprint_ids, statements, - ).some(stmt => { - stmt.stats.nodes && - stmt.stats.nodes.some(node => { - if (foundRegion || regions.includes(nodeRegions[node.toString()])) { - foundRegion = true; - } - if (foundNode || nodes.includes("n" + node)) { - foundNode = true; - } - if (foundNode && foundRegion) return true; - }); - }); + ).some(stmt => + stmt.stats.regions?.some(region => regions.includes(region)), + ); + }) + .filter((t: Transaction) => { + // The transaction must contain at least one value of the nodes list + // (if the list is not empty). + if (nodes.length == 0) return true; + + // If the cluster is a tenant cluster we don't care about nodes. + if (isTenant) return true; - return foundRegion && foundNode; + return getStatementsByFingerprintId( + t.stats_data.statement_fingerprint_ids, + statements, + ).some(stmt => + stmt.stats.nodes?.some(node => nodes.includes("n" + node)), + ); }); return { @@ -249,32 +250,21 @@ export const filterTransactions = ( * E.g. of one element of the list: `gcp-us-east1` * @param transaction: list of transactions. * @param statements: list of all statements collected. - * @param nodeRegions: object with keys being the node id and the value - * which region it belongs to. */ export const generateRegion = ( transaction: Transaction, statements: Statement[], - nodeRegions: { [p: string]: string }, ): string[] => { const regions: Set = new Set(); - // Get the list of statements that were executed on the transaction. Combine all - // nodes and regions of all the statements to a single list of `region: nodes` - // for the transaction. - // E.g. {"gcp-us-east1" : [1,3,4]} + getStatementsByFingerprintId( transaction.stats_data.statement_fingerprint_ids, statements, ).forEach(stmt => { - stmt.stats.nodes && - stmt.stats.nodes.forEach(n => { - regions.add(nodeRegions[n.toString()]); - }); + stmt.stats.regions?.forEach(region => regions.add(region)); }); - return Array.from(regions) - .filter(r => r) // Remove undefined / unknown regions. - .sort(); + return Array.from(regions).sort(); }; /** From b03f9f1bd37ae5b33b53f29c8268e41ad1ae20d8 Mon Sep 17 00:00:00 2001 From: Miral Gadani Date: Tue, 14 Mar 2023 10:46:58 -0400 Subject: [PATCH 3/5] upgrade/upgrades: skip TestUpgradeSchemaChangerElements Refs: #98062 Reason: flaky test Generated by bin/skip-test. Release justification: non-production code changes Release note: None --- pkg/upgrade/upgrades/schemachanger_elements_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/upgrade/upgrades/schemachanger_elements_test.go b/pkg/upgrade/upgrades/schemachanger_elements_test.go index a6d5b5c8980b..a4ef9502a815 100644 --- a/pkg/upgrade/upgrades/schemachanger_elements_test.go +++ b/pkg/upgrade/upgrades/schemachanger_elements_test.go @@ -26,6 +26,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/tests" "github.com/cockroachdb/cockroach/pkg/testutils" "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" + "github.com/cockroachdb/cockroach/pkg/testutils/skip" "github.com/cockroachdb/cockroach/pkg/util/ctxgroup" "github.com/cockroachdb/cockroach/pkg/util/leaktest" "github.com/cockroachdb/cockroach/pkg/util/log" @@ -34,6 +35,7 @@ import ( func TestUpgradeSchemaChangerElements(t *testing.T) { defer leaktest.AfterTest(t)() + skip.WithIssue(t, 98062, "flaky test") defer log.Scope(t).Close(t) ctx := context.Background() From 08f02882ba3d98c31db03edf5628c55383f73bd5 Mon Sep 17 00:00:00 2001 From: adityamaru Date: Sun, 12 Mar 2023 17:02:57 -0400 Subject: [PATCH 4/5] jobs.upgrades: add migration to backfill job_info table This change adds a migration and corresponding cluster version after which every job entry in the system.jobs table will have its Payload and Progress written to two rows in the system.job_info table. Informs: #97762 Release note: None --- .../settings/settings-for-tenants.txt | 2 +- docs/generated/settings/settings.html | 2 +- pkg/cli/testdata/declarative-rules/deprules | 2 +- pkg/cli/testdata/declarative-rules/oprules | 2 +- pkg/clusterversion/cockroach_versions.go | 9 ++ pkg/jobs/job_info_storage.go | 16 ++- pkg/upgrade/upgrademanager/manager.go | 1 + pkg/upgrade/upgrades/BUILD.bazel | 3 + .../backfill_jobs_info_table_migration.go | 48 ++++++++ ...backfill_jobs_info_table_migration_test.go | 112 ++++++++++++++++++ pkg/upgrade/upgrades/upgrades.go | 6 + 11 files changed, 197 insertions(+), 6 deletions(-) create mode 100644 pkg/upgrade/upgrades/backfill_jobs_info_table_migration.go create mode 100644 pkg/upgrade/upgrades/backfill_jobs_info_table_migration_test.go diff --git a/docs/generated/settings/settings-for-tenants.txt b/docs/generated/settings/settings-for-tenants.txt index be3464124cb1..a72454410acc 100644 --- a/docs/generated/settings/settings-for-tenants.txt +++ b/docs/generated/settings/settings-for-tenants.txt @@ -299,4 +299,4 @@ trace.opentelemetry.collector string address of an OpenTelemetry trace collecto trace.snapshot.rate duration 0s if non-zero, interval at which background trace snapshots are captured trace.span_registry.enabled boolean true if set, ongoing traces can be seen at https:///#/debug/tracez trace.zipkin.collector string the address of a Zipkin instance to receive traces, as :. If no port is specified, 9411 will be used. -version version 1000022.2-78 set the active cluster version in the format '.' +version version 1000022.2-80 set the active cluster version in the format '.' diff --git a/docs/generated/settings/settings.html b/docs/generated/settings/settings.html index 4b2960fc0186..1aa4da7f7e7e 100644 --- a/docs/generated/settings/settings.html +++ b/docs/generated/settings/settings.html @@ -240,6 +240,6 @@
trace.snapshot.rate
duration0sif non-zero, interval at which background trace snapshots are captured
trace.span_registry.enabled
booleantrueif set, ongoing traces can be seen at https://<ui>/#/debug/tracez
trace.zipkin.collector
stringthe address of a Zipkin instance to receive traces, as <host>:<port>. If no port is specified, 9411 will be used. -
version
version1000022.2-78set the active cluster version in the format '<major>.<minor>' +
version
version1000022.2-80set the active cluster version in the format '<major>.<minor>' diff --git a/pkg/cli/testdata/declarative-rules/deprules b/pkg/cli/testdata/declarative-rules/deprules index 474a59c6a24a..c9e4be873845 100644 --- a/pkg/cli/testdata/declarative-rules/deprules +++ b/pkg/cli/testdata/declarative-rules/deprules @@ -1,6 +1,6 @@ dep ---- -debug declarative-print-rules 1000022.2-78 dep +debug declarative-print-rules 1000022.2-80 dep deprules ---- - name: 'CheckConstraint transitions to ABSENT uphold 2-version invariant: PUBLIC->VALIDATED' diff --git a/pkg/cli/testdata/declarative-rules/oprules b/pkg/cli/testdata/declarative-rules/oprules index 7c72140937fd..01262e55df2f 100644 --- a/pkg/cli/testdata/declarative-rules/oprules +++ b/pkg/cli/testdata/declarative-rules/oprules @@ -1,6 +1,6 @@ op ---- -debug declarative-print-rules 1000022.2-78 op +debug declarative-print-rules 1000022.2-80 op rules ---- [] diff --git a/pkg/clusterversion/cockroach_versions.go b/pkg/clusterversion/cockroach_versions.go index ad623e6110ab..a7529e5aa7e7 100644 --- a/pkg/clusterversion/cockroach_versions.go +++ b/pkg/clusterversion/cockroach_versions.go @@ -479,6 +479,11 @@ const ( // since we've made mixed-version clusters tolerate new privileges. V23_1AllowNewSystemPrivileges + // V23_1JobInfoTableIsBackfilled is a version gate after which the + // system.jobs_info table has been backfilled with rows for the payload and + // progress of each job in the system.jobs table. + V23_1JobInfoTableIsBackfilled + // ************************************************* // Step (1): Add new versions here. // Do not add new versions to a patch release. @@ -828,6 +833,10 @@ var rawVersionsSingleton = keyedVersions{ Key: V23_1AllowNewSystemPrivileges, Version: roachpb.Version{Major: 22, Minor: 2, Internal: 78}, }, + { + Key: V23_1JobInfoTableIsBackfilled, + Version: roachpb.Version{Major: 22, Minor: 2, Internal: 80}, + }, // ************************************************* // Step (2): Add new versions here. diff --git a/pkg/jobs/job_info_storage.go b/pkg/jobs/job_info_storage.go index 918d773eef65..d6f6aef58ef0 100644 --- a/pkg/jobs/job_info_storage.go +++ b/pkg/jobs/job_info_storage.go @@ -109,7 +109,7 @@ func (i InfoStorage) write(ctx context.Context, infoKey, value []byte) error { // First clear out any older revisions of this info. _, err := i.txn.ExecEx( - ctx, "job-info-write", i.txn.KV(), + ctx, "write-job-info-delete", i.txn.KV(), sessiondata.NodeUserSessionDataOverride, "DELETE FROM system.job_info WHERE job_id = $1 AND info_key = $2", j.ID(), infoKey, @@ -120,7 +120,7 @@ func (i InfoStorage) write(ctx context.Context, infoKey, value []byte) error { // Write the new info, using the same transaction. _, err = i.txn.ExecEx( - ctx, "job-info-write", i.txn.KV(), + ctx, "write-job-info-insert", i.txn.KV(), sessiondata.NodeUserSessionDataOverride, `INSERT INTO system.job_info (job_id, info_key, written, value) VALUES ($1, $2, now(), $3)`, j.ID(), infoKey, value, @@ -206,6 +206,18 @@ const ( legacyProgressKey = "legacy_progress" ) +// GetLegacyPayloadKey returns the info_key whose value is the jobspb.Payload of +// the job. +func GetLegacyPayloadKey() []byte { + return []byte(legacyPayloadKey) +} + +// GetLegacyProgressKey returns the info_key whose value is the jobspb.Progress +// of the job. +func GetLegacyProgressKey() []byte { + return []byte(legacyProgressKey) +} + // GetLegacyPayload returns the job's Payload from the system.jobs_info table. func (i InfoStorage) GetLegacyPayload(ctx context.Context) ([]byte, bool, error) { return i.Get(ctx, []byte(legacyPayloadKey)) diff --git a/pkg/upgrade/upgrademanager/manager.go b/pkg/upgrade/upgrademanager/manager.go index 2a6d601b3ce1..4c3afba6010a 100644 --- a/pkg/upgrade/upgrademanager/manager.go +++ b/pkg/upgrade/upgrademanager/manager.go @@ -550,6 +550,7 @@ func (m *Manager) runMigration( // The TenantDeps used here are incomplete, but enough for the "permanent // upgrades" that run under this testing knob. if err := upg.Run(ctx, mig.Version(), upgrade.TenantDeps{ + KVDB: m.deps.DB.KV(), DB: m.deps.DB, Codec: m.codec, Settings: m.settings, diff --git a/pkg/upgrade/upgrades/BUILD.bazel b/pkg/upgrade/upgrades/BUILD.bazel index 99b178582be9..cfba95b260aa 100644 --- a/pkg/upgrade/upgrades/BUILD.bazel +++ b/pkg/upgrade/upgrades/BUILD.bazel @@ -7,6 +7,7 @@ go_library( "alter_jobs_add_job_type.go", "alter_sql_instances_sql_addr.go", "alter_table_statistics_partial_predicate_and_id.go", + "backfill_jobs_info_table_migration.go", "create_index_usage_statement_statistics.go", "create_jobs_metrics_polling_job.go", "database_role_settings_table_user_id_migration.go", @@ -86,6 +87,7 @@ go_test( "alter_jobs_add_job_type_test.go", "alter_sql_instances_sql_addr_test.go", "alter_table_statistics_partial_predicate_and_id_test.go", + "backfill_jobs_info_table_migration_test.go", "builtins_test.go", "create_index_usage_statement_statistics_test.go", "create_jobs_metrics_polling_job_test.go", @@ -126,6 +128,7 @@ go_test( "//pkg/jobs/jobspb", "//pkg/jobs/jobstest", "//pkg/keys", + "//pkg/keyvisualizer", "//pkg/kv", "//pkg/roachpb", "//pkg/security/securityassets", diff --git a/pkg/upgrade/upgrades/backfill_jobs_info_table_migration.go b/pkg/upgrade/upgrades/backfill_jobs_info_table_migration.go new file mode 100644 index 000000000000..08baeb468aec --- /dev/null +++ b/pkg/upgrade/upgrades/backfill_jobs_info_table_migration.go @@ -0,0 +1,48 @@ +// 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 upgrades + +import ( + "context" + + "github.com/cockroachdb/cockroach/pkg/clusterversion" + "github.com/cockroachdb/cockroach/pkg/jobs" + "github.com/cockroachdb/cockroach/pkg/kv" + "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" + "github.com/cockroachdb/cockroach/pkg/upgrade" + "github.com/cockroachdb/errors" +) + +const backfillJobInfoTableWithPayloadStmt = ` +INSERT INTO system.job_info (job_id, info_key, value) +SELECT id, $1, payload FROM system.jobs +` + +const backfillJobInfoTableWithProgressStmt = ` +INSERT INTO system.job_info (job_id, info_key, value) +SELECT id, $1, progress FROM system.jobs +` + +func backfillJobInfoTable( + ctx context.Context, cs clusterversion.ClusterVersion, d upgrade.TenantDeps, +) error { + return d.DB.KV().Txn(ctx, func(ctx context.Context, txn *kv.Txn) error { + _, err := d.InternalExecutor.ExecEx(ctx, "backfill-job-info-payload", txn, + sessiondata.NodeUserSessionDataOverride, backfillJobInfoTableWithPayloadStmt, jobs.GetLegacyPayloadKey()) + if err != nil { + return errors.Wrap(err, "failed to backfill legacy payload") + } + + _, err = d.InternalExecutor.ExecEx(ctx, "backfill-job-info-progress", txn, + sessiondata.NodeUserSessionDataOverride, backfillJobInfoTableWithProgressStmt, jobs.GetLegacyProgressKey()) + return errors.Wrap(err, "failed to backfill legacy progress") + }) +} diff --git a/pkg/upgrade/upgrades/backfill_jobs_info_table_migration_test.go b/pkg/upgrade/upgrades/backfill_jobs_info_table_migration_test.go new file mode 100644 index 000000000000..4246ea49e9fd --- /dev/null +++ b/pkg/upgrade/upgrades/backfill_jobs_info_table_migration_test.go @@ -0,0 +1,112 @@ +// 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 upgrades_test + +import ( + "context" + "testing" + + "github.com/cockroachdb/cockroach/pkg/base" + "github.com/cockroachdb/cockroach/pkg/clusterversion" + "github.com/cockroachdb/cockroach/pkg/jobs" + "github.com/cockroachdb/cockroach/pkg/jobs/jobspb" + "github.com/cockroachdb/cockroach/pkg/keyvisualizer" + "github.com/cockroachdb/cockroach/pkg/security/username" + "github.com/cockroachdb/cockroach/pkg/server" + "github.com/cockroachdb/cockroach/pkg/testutils/sqlutils" + "github.com/cockroachdb/cockroach/pkg/testutils/testcluster" + "github.com/cockroachdb/cockroach/pkg/upgrade/upgradebase" + "github.com/cockroachdb/cockroach/pkg/upgrade/upgrades" + "github.com/cockroachdb/cockroach/pkg/util/leaktest" + "github.com/cockroachdb/cockroach/pkg/util/log" + "github.com/stretchr/testify/require" +) + +// TestBackfillJobsInfoTable tests that the `system.job_info` table is +// backfilled with the progress and payload of each job in the `system.jobs` +// table. +func TestBackfillJobsInfoTable(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + + clusterArgs := base.TestClusterArgs{ + // Disable all automatic jobs creation and adoption. + ServerArgs: base.TestServerArgs{ + DisableSpanConfigs: true, + Knobs: base.TestingKnobs{ + // Avoiding jobs to be adopted. + JobsTestingKnobs: &jobs.TestingKnobs{ + DisableAdoptions: true, + }, + // DisableAdoptions needs this. + UpgradeManager: &upgradebase.TestingKnobs{ + DontUseJobs: true, + SkipJobMetricsPollingJobBootstrap: true, + }, + KeyVisualizer: &keyvisualizer.TestingKnobs{ + SkipJobBootstrap: true, + }, + Server: &server.TestingKnobs{ + DisableAutomaticVersionUpgrade: make(chan struct{}), + BinaryVersionOverride: clusterversion.ByKey(clusterversion.V22_2), + }, + }, + }, + } + + ctx := context.Background() + tc := testcluster.StartTestCluster(t, 1, clusterArgs) + r := tc.Server(0).JobRegistry().(*jobs.Registry) + sqlDB := sqlutils.MakeSQLRunner(tc.ServerConn(0)) + defer tc.Stopper().Stop(ctx) + + createJob := func(id jobspb.JobID, details jobspb.Details, progress jobspb.ProgressDetails) *jobs.Job { + defaultRecord := jobs.Record{ + Details: details, + Progress: progress, + Username: username.TestUserName(), + } + + job, err := r.CreateJobWithTxn(ctx, defaultRecord, id, nil /* txn */) + require.NoError(t, err) + return job + } + + // Create a few different types of jobs. + createJob(1, jobspb.BackupDetails{}, jobspb.BackupProgress{}) + createJob(2, jobspb.RestoreDetails{}, jobspb.RestoreProgress{}) + createJob(3, jobspb.ChangefeedDetails{}, jobspb.ChangefeedProgress{}) + + // Validate that there are no rows in the system.job_info table. + sqlDB.CheckQueryResults(t, `SELECT count(*) FROM system.job_info`, [][]string{{"0"}}) + + upgrades.Upgrade(t, tc.ServerConn(0), clusterversion.V23_1CreateSystemJobInfoTable, nil, false) + + // Create two more jobs that we should see written to both system.jobs and + // system.job_info. + createJob(4, jobspb.ImportDetails{}, jobspb.ImportProgress{}) + createJob(5, jobspb.SchemaChangeDetails{}, jobspb.SchemaChangeProgress{}) + + // Validate that we see 2 rows (payload and progress) in the system.job_info + // table for each row written in the system.jobs table since the last upgrade. + sqlDB.CheckQueryResults(t, `SELECT count(*) FROM system.jobs AS j, system.job_info AS i WHERE j.id = i.job_id AND (j.payload = i.value OR j.progress = i.value)`, + [][]string{{"4"}}) + + upgrades.Upgrade(t, tc.ServerConn(0), clusterversion.V23_1JobInfoTableIsBackfilled, nil, false) + + // At this point the entire system.jobs table should be backfilled into the + // system.job_info table. + // We expect to see 14 rows because of: + // - 4 rows from before + // - 10 rows (payload + progress) for the 5 jobs in system.jobs + sqlDB.CheckQueryResults(t, `SELECT count(*) FROM system.jobs AS j, system.job_info AS i WHERE j.id = i.job_id AND (j.payload = i.value OR j.progress = i.value)`, + [][]string{{"14"}}) +} diff --git a/pkg/upgrade/upgrades/upgrades.go b/pkg/upgrade/upgrades/upgrades.go index e52760725278..8a3c7b7d1df2 100644 --- a/pkg/upgrade/upgrades/upgrades.go +++ b/pkg/upgrade/upgrades/upgrades.go @@ -276,6 +276,12 @@ var upgrades = []upgradebase.Upgrade{ upgrade.NoPrecondition, backfillExternalConnectionsTableOwnerIDColumn, ), + upgrade.NewTenantUpgrade( + "backfill the system.jobs_info table with the payload and progress of each job in the system.jobs table", + toCV(clusterversion.V23_1JobInfoTableIsBackfilled), + upgrade.NoPrecondition, + backfillJobInfoTable, + ), } func init() { From 45a1b589ee1c5fd5a5a81d300f5e1505d02d550d Mon Sep 17 00:00:00 2001 From: gtr Date: Tue, 14 Feb 2023 13:59:25 -0500 Subject: [PATCH 5/5] ui: add error code to stmt and txn insights details pages Part of: #87785. Previously, the stmt and txn insights details pages did not show any further information for failed executions. This commit adds an "error code" column to the insights table for a failed execution in the stmt and txn insights details pages. Additionally, a "status" column was added to the stmt and txn workload insights tables which is either "Completed" or "Failed". Future work involves adding the error message string in addition to the error code but it needs to be redacted first. Additionally, the txn status is missing the implementation of a "Cancelled" status. Release note (ui change): Adds error code column to the insights table for a failed execution in the stmt and txn insights details page. Adds status column to the stmt and txn workload insights tables. --- .../cluster-ui/src/api/stmtInsightsApi.ts | 9 ++++++- .../cluster-ui/src/api/txnInsightsApi.ts | 9 ++++++- .../cluster-ui/src/insights/types.ts | 17 ++++++++++++ .../cluster-ui/src/insights/utils.spec.ts | 6 +++++ .../cluster-ui/src/insights/utils.ts | 3 +++ .../statementInsightsTable.tsx | 27 ++++++++++++++++++- .../transactionInsightsTable.tsx | 27 ++++++++++++++++++- .../workloadInsights/util/insightsColumns.tsx | 5 ++++ .../insightsTable/insightsTable.module.scss | 5 ++++ .../src/insightsTable/insightsTable.tsx | 14 +++++++--- 10 files changed, 114 insertions(+), 8 deletions(-) diff --git a/pkg/ui/workspaces/cluster-ui/src/api/stmtInsightsApi.ts b/pkg/ui/workspaces/cluster-ui/src/api/stmtInsightsApi.ts index 2bdf2a885406..e605a1390432 100644 --- a/pkg/ui/workspaces/cluster-ui/src/api/stmtInsightsApi.ts +++ b/pkg/ui/workspaces/cluster-ui/src/api/stmtInsightsApi.ts @@ -22,6 +22,7 @@ import { ContentionDetails, getInsightsFromProblemsAndCauses, InsightExecEnum, + StatementStatus, StmtInsightEvent, } from "src/insights"; import moment from "moment"; @@ -63,6 +64,8 @@ export type StmtInsightsResponseRow = { index_recommendations: string[]; plan_gist: string; cpu_sql_nanos: number; + error_code: string; + status: StatementStatus; }; const stmtColumns = ` @@ -90,7 +93,9 @@ causes, problem, index_recommendations, plan_gist, -cpu_sql_nanos +cpu_sql_nanos, +error_code, +status `; const stmtInsightsOverviewQuery = (filters?: StmtInsightsReq): string => { @@ -232,6 +237,8 @@ export function formatStmtInsights( ), planGist: row.plan_gist, cpuSQLNanos: row.cpu_sql_nanos, + errorCode: row.error_code, + status: row.status, } as StmtInsightEvent; }); } diff --git a/pkg/ui/workspaces/cluster-ui/src/api/txnInsightsApi.ts b/pkg/ui/workspaces/cluster-ui/src/api/txnInsightsApi.ts index 445f1dcbe5c9..93153be881b1 100644 --- a/pkg/ui/workspaces/cluster-ui/src/api/txnInsightsApi.ts +++ b/pkg/ui/workspaces/cluster-ui/src/api/txnInsightsApi.ts @@ -26,6 +26,7 @@ import { getInsightsFromProblemsAndCauses, InsightExecEnum, InsightNameEnum, + TransactionStatus, TxnContentionInsightDetails, TxnInsightDetails, TxnInsightEvent, @@ -293,6 +294,8 @@ type TxnInsightsResponseRow = { causes: string[]; stmt_execution_ids: string[]; cpu_sql_nanos: number; + last_error_code: string; + status: TransactionStatus; }; type TxnQueryFilters = { @@ -326,7 +329,9 @@ last_retry_reason, problems, causes, stmt_execution_ids, -cpu_sql_nanos`; +cpu_sql_nanos, +last_error_code, +status`; if (filters?.execID) { return ` @@ -394,6 +399,8 @@ function formatTxnInsightsRow(row: TxnInsightsResponseRow): TxnInsightEvent { insights, stmtExecutionIDs: row.stmt_execution_ids, cpuSQLNanos: row.cpu_sql_nanos, + errorCode: row.last_error_code, + status: row.status, }; } diff --git a/pkg/ui/workspaces/cluster-ui/src/insights/types.ts b/pkg/ui/workspaces/cluster-ui/src/insights/types.ts index 620db1424abc..64fc4ac6584f 100644 --- a/pkg/ui/workspaces/cluster-ui/src/insights/types.ts +++ b/pkg/ui/workspaces/cluster-ui/src/insights/types.ts @@ -26,6 +26,18 @@ export enum InsightExecEnum { STATEMENT = "statement", } +export enum StatementStatus { + COMPLETED = "Completed", + FAILED = "Failed", +} + +export enum TransactionStatus { + COMPLETED = "Completed", + FAILED = "Failed", + // Unimplemented, see https://github.com/cockroachdb/cockroach/issues/98219/. + CANCELLED = "Cancelled", +} + // Common fields for both txn and stmt insights. export type InsightEventBase = { application: string; @@ -46,9 +58,11 @@ export type InsightEventBase = { transactionExecutionID: string; transactionFingerprintID: string; username: string; + errorCode: string; }; export type TxnInsightEvent = InsightEventBase & { + status: TransactionStatus; stmtExecutionIDs: string[]; }; @@ -99,6 +113,7 @@ export type StmtInsightEvent = InsightEventBase & { planGist: string; databaseName: string; execType?: InsightExecEnum; + status: StatementStatus; }; export type Insight = { @@ -327,6 +342,8 @@ export interface ExecutionDetails { statementExecutionID?: string; transactionExecutionID?: string; execType?: InsightExecEnum; + errorCode?: string; + status?: string; } export interface insightDetails { diff --git a/pkg/ui/workspaces/cluster-ui/src/insights/utils.spec.ts b/pkg/ui/workspaces/cluster-ui/src/insights/utils.spec.ts index bb8c55a19eaf..63d5a20703ba 100644 --- a/pkg/ui/workspaces/cluster-ui/src/insights/utils.spec.ts +++ b/pkg/ui/workspaces/cluster-ui/src/insights/utils.spec.ts @@ -26,8 +26,10 @@ import { InsightNameEnum, planRegressionInsight, slowExecutionInsight, + StatementStatus, StmtInsightEvent, suboptimalPlanInsight, + TransactionStatus, TxnInsightDetails, TxnInsightEvent, } from "./types"; @@ -76,6 +78,8 @@ const statementInsightMock: StmtInsightEvent = { indexRecommendations: [], planGist: "gist", cpuSQLNanos: 50, + errorCode: "", + status: StatementStatus.COMPLETED, }; function mockStmtInsightEvent( @@ -116,6 +120,8 @@ const txnInsightEventMock: TxnInsightEvent = { elapsedTimeMillis: 1, stmtExecutionIDs: [statementInsightMock.statementExecutionID], cpuSQLNanos: 50, + errorCode: "", + status: TransactionStatus.COMPLETED, }; function mockTxnInsightEvent( diff --git a/pkg/ui/workspaces/cluster-ui/src/insights/utils.ts b/pkg/ui/workspaces/cluster-ui/src/insights/utils.ts index e4ee7e3b4aac..ab26aadc11a6 100644 --- a/pkg/ui/workspaces/cluster-ui/src/insights/utils.ts +++ b/pkg/ui/workspaces/cluster-ui/src/insights/utils.ts @@ -392,6 +392,8 @@ export function getStmtInsightRecommendations( statementExecutionID: insightDetails.statementExecutionID, transactionExecutionID: insightDetails.transactionExecutionID, execType: InsightExecEnum.STATEMENT, + errorCode: insightDetails.errorCode, + status: insightDetails.status, }; const recs: InsightRecommendation[] = insightDetails.insights?.map(insight => @@ -412,6 +414,7 @@ export function getTxnInsightRecommendations( contentionTimeMs: insightDetails.contentionTime.asMilliseconds(), elapsedTimeMillis: insightDetails.elapsedTimeMillis, execType: InsightExecEnum.TRANSACTION, + errorCode: insightDetails.errorCode, }; const recs: InsightRecommendation[] = []; diff --git a/pkg/ui/workspaces/cluster-ui/src/insights/workloadInsights/statementInsights/statementInsightsTable.tsx b/pkg/ui/workspaces/cluster-ui/src/insights/workloadInsights/statementInsights/statementInsightsTable.tsx index 35a7d6d206aa..e0b6130788e2 100644 --- a/pkg/ui/workspaces/cluster-ui/src/insights/workloadInsights/statementInsights/statementInsightsTable.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/insights/workloadInsights/statementInsights/statementInsightsTable.tsx @@ -22,7 +22,11 @@ import { limitText, NO_SAMPLES_FOUND, } from "src/util"; -import { InsightExecEnum, StmtInsightEvent } from "src/insights"; +import { + InsightExecEnum, + StatementStatus, + StmtInsightEvent, +} from "src/insights"; import { InsightCell, insightsTableTitles, @@ -33,9 +37,21 @@ import { Link } from "react-router-dom"; import classNames from "classnames/bind"; import styles from "../util/workloadInsights.module.scss"; import { TimeScale } from "../../../timeScaleDropdown"; +import { Badge } from "src/badge"; const cx = classNames.bind(styles); +function stmtStatusToString(status: StatementStatus) { + switch (status) { + case StatementStatus.COMPLETED: + return "success"; + case StatementStatus.FAILED: + return "danger"; + default: + return "info"; + } +} + interface StatementInsightsTable { data: StmtInsightEvent[]; sortSetting: SortSetting; @@ -77,6 +93,15 @@ export function makeStatementInsightsColumns(): ColumnDescriptor item.query, showByDefault: true, }, + { + name: "status", + title: insightsTableTitles.status(execType), + cell: (item: StmtInsightEvent) => ( + + ), + sort: (item: StmtInsightEvent) => item.status, + showByDefault: true, + }, { name: "insights", title: insightsTableTitles.insights(execType), diff --git a/pkg/ui/workspaces/cluster-ui/src/insights/workloadInsights/transactionInsights/transactionInsightsTable.tsx b/pkg/ui/workspaces/cluster-ui/src/insights/workloadInsights/transactionInsights/transactionInsightsTable.tsx index 2d031945a7ca..d939bc07fed0 100644 --- a/pkg/ui/workspaces/cluster-ui/src/insights/workloadInsights/transactionInsights/transactionInsightsTable.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/insights/workloadInsights/transactionInsights/transactionInsightsTable.tsx @@ -16,7 +16,11 @@ import { SortSetting, } from "src/sortedtable"; import { DATE_WITH_SECONDS_AND_MILLISECONDS_FORMAT, Duration } from "src/util"; -import { InsightExecEnum, TxnInsightEvent } from "src/insights"; +import { + InsightExecEnum, + TransactionStatus, + TxnInsightEvent, +} from "src/insights"; import { InsightCell, insightsTableTitles, @@ -25,6 +29,18 @@ import { } from "../util"; import { Link } from "react-router-dom"; import { TimeScale } from "../../../timeScaleDropdown"; +import { Badge } from "src/badge"; + +function txnStatusToString(status: TransactionStatus) { + switch (status) { + case TransactionStatus.COMPLETED: + return "success"; + case TransactionStatus.FAILED: + return "danger"; + case TransactionStatus.CANCELLED: + return "info"; + } +} interface TransactionInsightsTable { data: TxnInsightEvent[]; @@ -60,6 +76,15 @@ export function makeTransactionInsightsColumns(): ColumnDescriptor QueriesCell([item.query], 50), sort: item => item.query, }, + { + name: "status", + title: insightsTableTitles.status(execType), + cell: item => ( + + ), + sort: item => item.status, + showByDefault: true, + }, { name: "insights", title: insightsTableTitles.insights(execType), diff --git a/pkg/ui/workspaces/cluster-ui/src/insights/workloadInsights/util/insightsColumns.tsx b/pkg/ui/workspaces/cluster-ui/src/insights/workloadInsights/util/insightsColumns.tsx index d6bf4f7f3625..a71f009712ee 100644 --- a/pkg/ui/workspaces/cluster-ui/src/insights/workloadInsights/util/insightsColumns.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/insights/workloadInsights/util/insightsColumns.tsx @@ -21,6 +21,7 @@ export const insightsColumnLabels = { waitingID: "Waiting Execution ID", waitingFingerprintID: "Waiting Fingerprint ID", query: "Execution", + status: "Status", insights: "Insights", startTime: "Start Time (UTC)", elapsedTime: "Elapsed Time", @@ -123,6 +124,10 @@ export const insightsTableTitles: InsightsTableTitleType = { } return makeToolTip(

{tooltipText}

, "query", execType); }, + status: (execType: InsightExecEnum) => { + const tooltipText = `The ${execType} status`; + return makeToolTip(

{tooltipText}

, "status"); + }, insights: (execType: InsightExecEnum) => { return makeToolTip(

The category of insight identified for the {execType} execution.

, diff --git a/pkg/ui/workspaces/cluster-ui/src/insightsTable/insightsTable.module.scss b/pkg/ui/workspaces/cluster-ui/src/insightsTable/insightsTable.module.scss index 2e8970417605..14d1d9f5044b 100644 --- a/pkg/ui/workspaces/cluster-ui/src/insightsTable/insightsTable.module.scss +++ b/pkg/ui/workspaces/cluster-ui/src/insightsTable/insightsTable.module.scss @@ -5,6 +5,11 @@ font-weight: $font-weight--bold; } +.insights-type-failed { + color: $colors--functional-red-4; + font-weight: $font-weight--bold; +} + .label-bold { font-family: $font-family--semi-bold; font-size: $font-size--medium; diff --git a/pkg/ui/workspaces/cluster-ui/src/insightsTable/insightsTable.tsx b/pkg/ui/workspaces/cluster-ui/src/insightsTable/insightsTable.tsx index 4b3841c7db82..8df6cd27a9c3 100644 --- a/pkg/ui/workspaces/cluster-ui/src/insightsTable/insightsTable.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/insightsTable/insightsTable.tsx @@ -31,6 +31,7 @@ import { Link } from "react-router-dom"; import { InsightExecEnum, InsightRecommendation, + InsightType, insightType, } from "../insights"; @@ -93,8 +94,12 @@ export const insightsTableTitles: InsightsTableTitleType = { }, }; -function typeCell(value: string): React.ReactElement { - return
{value}
; +function TypeCell(value: InsightType): React.ReactElement { + const className = + insightType(value) === "FailedExecution" + ? "insight-type-failed" + : "insight-type"; + return
{value}
; } const StatementExecution = ({ @@ -276,7 +281,8 @@ function descriptionCell( return ( <>
- This execution has failed. + Error Code: {" "} + {insightRec.execution.errorCode}
); @@ -409,7 +415,7 @@ export function makeInsightsColumns( { name: "insights", title: insightsTableTitles.insights(), - cell: (item: InsightRecommendation) => typeCell(insightType(item.type)), + cell: (item: InsightRecommendation) => TypeCell(item.type), sort: (item: InsightRecommendation) => item.type, }, {