Skip to content

Commit

Permalink
cluster-ui, db-console: fix bug where store ids are treated as node ids
Browse files Browse the repository at this point in the history 
Previously, store ids were being treated as node ids on the databases
view. This resulted in "Regions/Nodes" column showing inaccurate
information in some cases. Now, store ids are mapped to node ids on the
databases view which resolves the issue.

Fixes: #118957

Release note (ui change): Resolves an issue where clusters with multiple
stores per node may list inaccurate region/node information in the
databases table.
  • Loading branch information
@laurenbarker
laurenbarker committed Feb 21, 2024
1 parent dd69eb9 commit 68fe962
Show file tree
Hide file tree
Showing 12 changed files with 363 additions and 41 deletions.
2 changes: 2 additions & 0 deletions pkg/ui/workspaces/cluster-ui/src/databaseDetailsPage/databaseDetailsConnected.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ const mapStateToProps = (
databaseDetails[database]?.data?.results.tablesResp.tables || [];
const nodeRegions = nodeRegionsByIDSelector(state);
const isTenant = selectIsTenant(state);
const nodeStatuses = state.adminUI?.nodes.data;
return {
loading: !!databaseDetails[database]?.inFlight,
loaded: !!databaseDetails[database]?.valid,
Expand All @@ -74,6 +75,7 @@ const mapStateToProps = (
tableDetails: state.adminUI?.tableDetails,
nodeRegions,
isTenant,
nodeStatuses,
}),
showIndexRecommendations: selectIndexRecommendationsEnabled(state),
csIndexUnusedDuration: selectDropUnusedIndexDuration(state),
Expand Down
8 changes: 7 additions & 1 deletion pkg/ui/workspaces/cluster-ui/src/databaseTablePage/databaseTablePageConnected.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,11 +65,17 @@ export const mapStateToProps = (
);
const nodeRegions = nodeRegionsByIDSelector(state);
const isTenant = selectIsTenant(state);
const nodeStatuses = state.adminUI?.nodes.data;
return {
databaseName: database,
name: table,
schemaName: schema,
details: deriveTablePageDetailsMemoized({ details, nodeRegions, isTenant }),
details: deriveTablePageDetailsMemoized({
details,
nodeRegions,
isTenant,
nodeStatuses,
}),
showNodeRegionsSection: Object.keys(nodeRegions).length > 1 && !isTenant,
automaticStatsCollectionEnabled:
selectAutomaticStatsCollectionEnabled(state),
Expand Down
87 changes: 71 additions & 16 deletions pkg/ui/workspaces/cluster-ui/src/databases/combiners.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,13 @@
// by the Apache License, Version 2.0, included in the file
// licenses/APL.txt.

import { DatabasesListResponse, SqlExecutionErrorMessage } from "../api";
import { DatabasesListResponse } from "../api";
import { DatabasesPageDataDatabase } from "../databasesPage";
import {
Nodes,
Stores,
buildIndexStatToRecommendationsMap,
getNodeIdsFromStoreIds,
getNodesByRegionString,
normalizePrivileges,
normalizeRoles,
Expand All @@ -38,6 +41,8 @@ interface DerivedDatabaseDetailsParams {
spanStats: Record<string, DatabaseDetailsSpanStatsState>;
nodeRegions: Record<string, string>;
isTenant: boolean;
/** A list of node statuses so that store ids can be mapped to nodes. */
nodeStatuses: cockroach.server.status.statuspb.INodeStatus[];
}

export const deriveDatabaseDetailsMemoized = createSelector(
Expand All @@ -46,12 +51,14 @@ export const deriveDatabaseDetailsMemoized = createSelector(
(params: DerivedDatabaseDetailsParams) => params.spanStats,
(params: DerivedDatabaseDetailsParams) => params.nodeRegions,
(params: DerivedDatabaseDetailsParams) => params.isTenant,
(params: DerivedDatabaseDetailsParams) => params.nodeStatuses,
(
dbListResp,
databaseDetails,
spanStats,
nodeRegions,
isTenant,
nodeStatuses,
): DatabasesPageDataDatabase[] => {
const databases = dbListResp?.databases ?? [];
return databases.map(dbName => {
Expand All @@ -61,9 +68,9 @@ export const deriveDatabaseDetailsMemoized = createSelector(
dbName,
dbDetails,
spanStatsForDB,
dbListResp.error,
nodeRegions,
isTenant,
nodeStatuses,
);
});
},
Expand All @@ -73,12 +80,23 @@ const deriveDatabaseDetails = (
database: string,
dbDetails: DatabaseDetailsState,
spanStats: DatabaseDetailsSpanStatsState,
dbListError: SqlExecutionErrorMessage,
nodeRegionsByID: Record<string, string>,
isTenant: boolean,
nodeStatuses: cockroach.server.status.statuspb.INodeStatus[],
): DatabasesPageDataDatabase => {
const dbStats = dbDetails?.data?.results.stats;
const nodes = dbStats?.replicaData.replicas || [];
/** List of store IDs for the current cluster. All of the values in the
* `*replicas` columns correspond to store IDs. */
const stores: Stores = {
kind: "store",
ids: dbStats?.replicaData.replicas || [],
};
/** List of node IDs for the current cluster. */
const nodes: Nodes = {
kind: "node",
ids: getNodeIdsFromStoreIds(stores, nodeStatuses),
};

const nodesByRegionString = getNodesByRegionString(
nodes,
nodeRegionsByID,
Expand All @@ -99,7 +117,8 @@ const deriveDatabaseDetails = (
name: database,
spanStats: spanStats?.data?.results.spanStats,
tables: dbDetails?.data?.results.tablesResp,
nodes: nodes,
nodes: nodes.ids,
nodeStatuses: nodeStatuses,
nodesByRegionString,
numIndexRecommendations,
};
Expand All @@ -111,6 +130,8 @@ interface DerivedTableDetailsParams {
tableDetails: Record<string, TableDetailsState>;
nodeRegions: Record<string, string>;
isTenant: boolean;
/** A list of node statuses so that store ids can be mapped to nodes. */
nodeStatuses: cockroach.server.status.statuspb.INodeStatus[];
}

export const deriveTableDetailsMemoized = createSelector(
Expand All @@ -119,18 +140,26 @@ export const deriveTableDetailsMemoized = createSelector(
(params: DerivedTableDetailsParams) => params.tableDetails,
(params: DerivedTableDetailsParams) => params.nodeRegions,
(params: DerivedTableDetailsParams) => params.isTenant,
(params: DerivedTableDetailsParams) => params.nodeStatuses,
(
dbName,
tables,
tableDetails,
nodeRegions,
isTenant,
nodeStatuses,
): DatabaseDetailsPageDataTable[] => {
tables = tables || [];
return tables.map(table => {
const tableID = generateTableID(dbName, table);
const details = tableDetails[tableID];
return deriveDatabaseTableDetails(table, details, nodeRegions, isTenant);
return deriveDatabaseTableDetails(
table,
details,
nodeRegions,
isTenant,
nodeStatuses,
);
});
},
);
Expand All @@ -140,14 +169,25 @@ const deriveDatabaseTableDetails = (
details: TableDetailsState,
nodeRegions: Record<string, string>,
isTenant: boolean,
nodeStatuses: cockroach.server.status.statuspb.INodeStatus[],
): DatabaseDetailsPageDataTable => {
const results = details?.data?.results;
const grants = results?.grantsResp.grants ?? [];
const normalizedRoles = normalizeRoles(grants.map(grant => grant.user));
const normalizedPrivileges = normalizePrivileges(
[].concat(...grants.map(grant => grant.privileges)),
);
const storeIDs = results?.stats.replicaData.storeIDs || [];
/** List of store IDs for the current cluster. All of the values in the
* `*replicas` columns correspond to store IDs. */
const stores: Stores = {
kind: "store",
ids: results?.stats.replicaData.storeIDs || [],
};
/** List of node IDs for the current cluster. */
const nodes: Nodes = {
kind: "node",
ids: getNodeIdsFromStoreIds(stores, nodeStatuses),
};
return {
name: table,
loading: !!details?.inFlight,
Expand All @@ -164,13 +204,8 @@ const deriveDatabaseTableDetails = (
statsLastUpdated: results?.heuristicsDetails,
indexStatRecs: results?.stats.indexStats,
spanStats: results?.stats.spanStats,
// TODO #118957 (xinhaoz) Store IDs and node IDs cannot be used interchangeably.
nodes: storeIDs,
nodesByRegionString: getNodesByRegionString(
storeIDs,
nodeRegions,
isTenant,
),
nodes: nodes.ids,
nodesByRegionString: getNodesByRegionString(nodes, nodeRegions, isTenant),
},
};
};
Expand All @@ -179,21 +214,41 @@ interface DerivedTablePageDetailsParams {
details: TableDetailsState;
nodeRegions: Record<string, string>;
isTenant: boolean;
/** A list of node statuses so that store ids can be mapped to nodes. */
nodeStatuses: cockroach.server.status.statuspb.INodeStatus[];
}

export const deriveTablePageDetailsMemoized = createSelector(
(params: DerivedTablePageDetailsParams) => params.details,
(params: DerivedTablePageDetailsParams) => params.nodeRegions,
(params: DerivedTablePageDetailsParams) => params.isTenant,
(details, nodeRegions, isTenant): DatabaseTablePageDataDetails => {
(params: DerivedTablePageDetailsParams) => params.nodeStatuses,
(
details,
nodeRegions,
isTenant,
nodeStatuses,
): DatabaseTablePageDataDetails => {
const results = details?.data?.results;
const grants = results?.grantsResp.grants || [];
const normalizedGrants =
grants.map(grant => ({
user: grant.user,
privileges: normalizePrivileges(grant.privileges),
})) || [];
const nodes = results?.stats.replicaData.storeIDs || [];

/** List of store IDs for the current cluster. All of the values in the
* `*replicas` columns correspond to store IDs. */
const stores: Stores = {
kind: "store",
ids: results?.stats.replicaData.storeIDs || [],
};
/** List of node IDs for the current cluster. */
const nodes: Nodes = {
kind: "node",
ids: getNodeIdsFromStoreIds(stores, nodeStatuses),
};

return {
loading: !!details?.inFlight,
loaded: !!details?.valid,
Expand Down
87 changes: 82 additions & 5 deletions pkg/ui/workspaces/cluster-ui/src/databases/util.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,20 @@
// by the Apache License, Version 2.0, included in the file
// licenses/APL.txt.

import { INodeStatus } from "../util";
import {
Nodes,
Stores,
getNodesByRegionString,
getNodeIdsFromStoreIds,
normalizePrivileges,
normalizeRoles,
} from "./util";

describe("Getting nodes by region string", () => {
describe("is not tenant", () => {
it("when all nodes different regions", () => {
const nodes = [1, 2, 3];
const nodes: Nodes = { kind: "node", ids: [1, 2, 3] };
const regions = {
"1": "region1",
"2": "region2",
Expand All @@ -28,7 +32,7 @@ describe("Getting nodes by region string", () => {
});

it("when all nodes same region", () => {
const nodes = [1, 2, 3];
const nodes: Nodes = { kind: "node", ids: [1, 2, 3] };
const regions = {
"1": "region1",
"2": "region1",
Expand All @@ -39,7 +43,7 @@ describe("Getting nodes by region string", () => {
});

it("when some nodes different regions", () => {
const nodes = [1, 2, 3];
const nodes: Nodes = { kind: "node", ids: [1, 2, 3] };
const regions = {
"1": "region1",
"2": "region1",
Expand All @@ -50,14 +54,14 @@ describe("Getting nodes by region string", () => {
});

it("when region map is empty", () => {
const nodes = [1, 2, 3];
const nodes: Nodes = { kind: "node", ids: [1, 2, 3] };
const regions = {};
const result = getNodesByRegionString(nodes, regions, false);
expect(result).toEqual("");
});

it("when nodes are empty", () => {
const nodes: number[] = [];
const nodes: Nodes = { kind: "node", ids: [] };
const regions = {
"1": "region1",
"2": "region1",
Expand All @@ -69,6 +73,79 @@ describe("Getting nodes by region string", () => {
});
});

describe("getNodeIdsFromStoreIds", () => {
it("returns the correct node ids when all nodes have multiple stores", () => {
const stores: Stores = { kind: "store", ids: [1, 3, 6, 2, 4, 5] };
const nodeStatuses: INodeStatus[] = [
{
desc: {
node_id: 1,
},
store_statuses: [{ desc: { store_id: 1 } }, { desc: { store_id: 2 } }],
},
{
desc: {
node_id: 2,
},
store_statuses: [{ desc: { store_id: 3 } }, { desc: { store_id: 5 } }],
},
{
desc: {
node_id: 3,
},
store_statuses: [{ desc: { store_id: 4 } }, { desc: { store_id: 6 } }],
},
];
const result = getNodeIdsFromStoreIds(stores, nodeStatuses);
expect(result).toEqual([1, 2, 3]);
});

it("returns an empty list when no stores ids are provided", () => {
const stores: Stores = { kind: "store", ids: [] };
const result = getNodeIdsFromStoreIds(stores, []);
expect(result).toEqual([]);
});

it("returns the correct node ids when there is one store per node", () => {
const stores: Stores = { kind: "store", ids: [1, 3, 4] };
const nodeStatuses: INodeStatus[] = [
{
desc: {
node_id: 1,
},
store_statuses: [{ desc: { store_id: 1 } }],
},
{
desc: {
node_id: 2,
},
store_statuses: [{ desc: { store_id: 3 } }],
},
{
desc: {
node_id: 3,
},
store_statuses: [{ desc: { store_id: 4 } }],
},
];
const result = getNodeIdsFromStoreIds(stores, nodeStatuses);
expect(result).toEqual([1, 2, 3]);
});
it("returns the correct node ids when there is only one node", () => {
const stores: Stores = { kind: "store", ids: [3] };
const nodeStatuses: INodeStatus[] = [
{
desc: {
node_id: 1,
},
store_statuses: [{ desc: { store_id: 3 } }],
},
];
const result = getNodeIdsFromStoreIds(stores, nodeStatuses);
expect(result).toEqual([1]);
});
});

describe("Normalize privileges", () => {
it("sorts correctly when input is disordered", () => {
const privs = ["CREATE", "DELETE", "UPDATE", "ALL", "GRANT"];
Expand Down

0 comments on commit 68fe962

Please sign in to comment.