cluster-ui, db-console: fix bug where store ids are treated as node ids

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: cockroachdb#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.

pkg/ui/workspaces/cluster-ui/src/databases/combiners.ts

@@ -11,7 +11,10 @@
 import { DatabasesListResponse, SqlExecutionErrorMessage } from "../api";
 import { DatabasesPageDataDatabase } from "../databasesPage";
 import {
+  Nodes,
+  Stores,
   buildIndexStatToRecommendationsMap,
+  getNodeIdsFromStoreIds,
   getNodesByRegionString,
   normalizePrivileges,
   normalizeRoles,
@@ -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(
@@ -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 => {
@@ -61,9 +68,9 @@ export const deriveDatabaseDetailsMemoized = createSelector(
         dbName,
         dbDetails,
         spanStatsForDB,
-        dbListResp.error,
         nodeRegions,
         isTenant,
+        nodeStatuses,
       );
     });
   },
@@ -73,12 +80,17 @@ 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,
@@ -99,7 +111,8 @@ const deriveDatabaseDetails = (
     name: database,
     spanStats: spanStats?.data?.results.spanStats,
     tables: dbDetails?.data?.results.tablesResp,
-    nodes: nodes,
+    nodes: nodes.ids,
+    nodeStatuses: nodeStatuses,
     nodesByRegionString,
     numIndexRecommendations,
   };
@@ -147,7 +160,7 @@ const deriveDatabaseTableDetails = (
   const normalizedPrivileges = normalizePrivileges(
     [].concat(...grants.map(grant => grant.privileges)),
   );
-  const nodes = results?.stats.replicaData.nodeIDs || [];
+  const nodes: Nodes = { kind: "node", ids: results?.stats.replicaData.nodeIDs || [] };
   return {
     name: table,
     loading: !!details?.inFlight,
@@ -164,7 +177,7 @@ const deriveDatabaseTableDetails = (
       statsLastUpdated: results?.heuristicsDetails,
       indexStatRecs: results?.stats.indexStats,
       spanStats: results?.stats.spanStats,
-      nodes: nodes,
+      nodes: nodes.ids,
       nodesByRegionString: getNodesByRegionString(nodes, nodeRegions, isTenant),
     },
   };
@@ -188,7 +201,7 @@ export const deriveTablePageDetailsMemoized = createSelector(
         user: grant.user,
         privileges: normalizePrivileges(grant.privileges),
       })) || [];
-    const nodes = results?.stats.replicaData.nodeIDs || [];
+    const nodes: Nodes = { kind: "node", ids: results?.stats.replicaData.nodeIDs || [] };
     return {
       loading: !!details?.inFlight,
       loaded: !!details?.valid,

pkg/ui/workspaces/cluster-ui/src/databases/util.spec.ts

@@ -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",
@@ -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",
@@ -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",
@@ -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",
@@ -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"];

pkg/ui/workspaces/cluster-ui/src/databases/util.tsx

@@ -58,31 +58,71 @@ export class GetDatabaseInfoError extends Error {
   }
 }
 
-// getNodesByRegionString converts a list of node ids and map of
-// node ids to region to a string of node ids by region, ordered
-// by region name, e.g.
-// regionA(n1, n2), regionB(n2,n3), ...
+/** Store ids and node ids are both of type `number[]`. To disambiguate, a
+ * `kind` field is included in the type. */
+export type Stores = { kind: "store", ids: number[] };
+
+/** getNodeIdsFromStoreIds converts a list of store IDs to a list of node IDs.
+ *
+ * @param stores - Store ids for the cluster.
+ * @param nodeStatuses - A list of nodes that includes store information.
+ * @returns A list of node ids for the cluster.
+ */
+export function getNodeIdsFromStoreIds(
+  stores: Stores,
+  nodeStatuses: cockroach.server.status.statuspb.INodeStatus[],
+): number[] {
+  /** Associates stores with their node. Nodes can have multiple stores:
+   * `{ store1: node1, store2: node1 }` */
+  const nodeByStoreMap: Record<number, number> = {};
+  nodeStatuses?.map(
+    node => node.store_statuses?.map(
+      store => {
+        nodeByStoreMap[store.desc.store_id] = node.desc.node_id;
+      }
+    )
+  );
+
+  /** A unique list of node IDs derived from the nodeByStoreMap. */
+  const nodeIds = Array.from(new Set(stores.ids.map(id => nodeByStoreMap[id])));
+
+  return nodeIds;
+}
+
+/** Node ids and store IDs are both of type `number[]`. To disambiguate, a
+ * `kind` field is included in the type. */
+export type Nodes = { kind: "node", ids: number[] };
+
+/** getNodesByRegionString converts a list of node IDs to a user-facing string.
+ *
+ * @param nodes - Node ids for the cluster.
+ * @param nodeRegions - A map of node IDs to region IDs.
+ * @param isTenant - Whether the cluster is a tenant cluster.
+ * @returns A string of node IDs by region, ordered by region name, e.g.
+ * `regionA(n1, n2), regionB(n2,n3), ...`
+ */
 export function getNodesByRegionString(
-  nodes: number[],
+  nodes: Nodes,
   nodeRegions: Record<string, string>,
   isTenant: boolean,
 ): string {
   return nodesByRegionMapToString(
-    createNodesByRegionMap(nodes, nodeRegions),
+    createNodesByRegionMap(nodes.ids, nodeRegions),
     isTenant,
   );
 }
 
-// nodesByRegionMapToString converts a map of regions to node ids,
-// ordered by region name, e.g. converts:
-// { regionA: [1, 2], regionB: [2, 3] }
-// to:
-// regionA(n1, n2), regionB(n2,n3), ...
-// If the cluster is a tenant cluster, then we redact node info
-// and only display the region name, e.g.
-// regionA(n1, n2), regionB(n2,n3), ... becomes:
-// regionA, regionB, ...
-export function nodesByRegionMapToString(
+/** nodesByRegionMapToString converts a map of regions to node ids,
+ * ordered by region name, e.g. converts:
+ * `{ regionA: [1, 2], regionB: [2, 3] }`
+ * to:
+ * `regionA(n1, n2), regionB(n2,n3), ...`
+ *
+ * If the cluster is a tenant cluster, then we redact node info
+ * and only display the region name, e.g.
+ * `regionA(n1, n2), regionB(n2,n3), ...` becomes:
+ * `regionA, regionB, ...` */
+function nodesByRegionMapToString(
   nodesByRegion: Record<string, number[]>,
   isTenant: boolean,
 ): string {
@@ -102,7 +142,7 @@ export function nodesByRegionMapToString(
     .join(", ");
 }
 
-export function createNodesByRegionMap(
+function createNodesByRegionMap(
   nodes: number[],
   nodeRegions: Record<string, string>,
 ): Record<string, number[]> {

pkg/ui/workspaces/cluster-ui/src/databasesPage/databasesPage.stories.tsx

@@ -118,6 +118,7 @@ const withData: DatabasesPageProps = {
       rangeCount: _.random(50, 500),
       nodesByRegionString:
         "gcp-europe-west1(n8), gcp-us-east1(n1), gcp-us-west1(n6)",
+      nodeStatuses: [],
       numIndexRecommendations: 0,
     };
   }),

pkg/ui/workspaces/cluster-ui/src/databasesPage/databasesPage.tsx

@@ -59,6 +59,7 @@ import {
 } from "../api";
 import { InlineAlert } from "@cockroachlabs/ui-components";
 import { checkInfoAvailable } from "../databases";
+import { cockroach } from "../../../../../../_bazel/bin/pkg/ui/workspaces/db-console/src/js/protos";
 
 const cx = classNames.bind(styles);
 const sortableTableCx = classNames.bind(sortableTableStyles);
@@ -107,6 +108,8 @@ export interface DatabasesPageDataDatabase {
   tables?: SqlApiQueryResponse<DatabaseTablesResponse>;
   // Array of node IDs used to unambiguously filter by node and region.
   nodes?: number[];
+  /** A list of node statuses so that store ids can be mapped to nodes. */
+  nodeStatuses: cockroach.server.status.statuspb.INodeStatus[]
   // String of nodes grouped by region in alphabetical order, e.g.
   // regionA(n1,n2), regionB(n3). Used for display in the table's
   // "Regions/Nodes" column.

pkg/ui/workspaces/cluster-ui/src/databasesPage/databasesPageConnected.ts

@@ -65,6 +65,7 @@ const mapStateToProps = (state: AppState): DatabasesPageData => {
       spanStats: state.adminUI?.databaseDetailsSpanStats,
       nodeRegions,
       isTenant,
+      nodeStatuses: state.adminUI.nodes.data,
     }),
     sortSetting: selectDatabasesSortSetting(state),
     search: selectDatabasesSearch(state),

pkg/ui/workspaces/db-console/src/views/databases/databasesPage/redux.ts

@@ -87,6 +87,7 @@ export const mapStateToProps = (state: AdminUIState): DatabasesPageData => {
       spanStats,
       nodeRegions,
       isTenant,
+      nodeStatuses: state.cachedData.nodes.data,
     }),
     sortSetting: sortSettingLocalSetting.selector(state),
     filters: filtersLocalSetting.selector(state),