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. Also, a log message is skipped during tests so that the output isn't polluted with expected warnings. 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.
cockroachdb · Feb 15, 2024 · 9c9dd35 · 9c9dd35
1 parent b6b08d7
commit 9c9dd35
Show file tree

Hide file tree

Showing 14 changed files with 265 additions and 49 deletions.
diff --git a/pkg/ui/workspaces/cluster-ui/src/databaseDetailsPage/databaseDetailsConnected.ts b/pkg/ui/workspaces/cluster-ui/src/databaseDetailsPage/databaseDetailsConnected.ts
@@ -43,6 +43,7 @@ import {
   selectDropUnusedIndexDuration,
   selectIndexRecommendationsEnabled,
 } from "../store/clusterSettings/clusterSettings.selectors";
+import { actions as nodesActions } from "../store/nodes/nodes.reducer";
 
 const mapStateToProps = (
   state: AppState,
@@ -54,6 +55,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,
@@ -74,6 +76,7 @@ const mapStateToProps = (
       tableDetails: state.adminUI?.tableDetails,
       nodeRegions,
       isTenant,
+      nodeStatuses,
     }),
     showIndexRecommendations: selectIndexRecommendationsEnabled(state),
     csIndexUnusedDuration: selectDropUnusedIndexDuration(state),
@@ -174,6 +177,9 @@ const mapDispatchToProps = (
       }),
     );
   },
+  refreshNodes: () => {
+    dispatch(nodesActions.refresh());
+  },
 });
 
 export const ConnectedDatabaseDetailsPage = withRouter(

diff --git a/pkg/ui/workspaces/cluster-ui/src/databaseDetailsPage/databaseDetailsPage.stories.tsx b/pkg/ui/workspaces/cluster-ui/src/databaseDetailsPage/databaseDetailsPage.stories.tsx
@@ -53,6 +53,7 @@ const withLoadingIndicator: DatabaseDetailsPageProps = {
   onSortingGrantsChange: () => {},
   refreshDatabaseDetails: () => {},
   refreshTableDetails: () => {},
+  refreshNodes: () => {},
   search: null,
   filters: defaultFilters,
   nodeRegions: {},
@@ -88,6 +89,7 @@ const withoutData: DatabaseDetailsPageProps = {
   onSortingGrantsChange: () => {},
   refreshDatabaseDetails: () => {},
   refreshTableDetails: () => {},
+  refreshNodes: () => {},
   search: null,
   filters: defaultFilters,
   nodeRegions: {},
@@ -163,6 +165,7 @@ const withData: DatabaseDetailsPageProps = {
   onSortingGrantsChange: () => {},
   refreshDatabaseDetails: () => {},
   refreshTableDetails: () => {},
+  refreshNodes: () => {},
   search: null,
   filters: defaultFilters,
   nodeRegions: {},

diff --git a/pkg/ui/workspaces/cluster-ui/src/databaseDetailsPage/databaseDetailsPage.tsx b/pkg/ui/workspaces/cluster-ui/src/databaseDetailsPage/databaseDetailsPage.tsx
@@ -170,6 +170,7 @@ export interface DatabaseDetailsPageActions {
   onSortingTablesChange?: (columnTitle: string, ascending: boolean) => void;
   onSortingGrantsChange?: (columnTitle: string, ascending: boolean) => void;
   onViewModeChange?: (viewMode: ViewMode) => void;
+  refreshNodes: () => void;
 }
 
 export type DatabaseDetailsPageProps = DatabaseDetailsPageData &
@@ -264,6 +265,7 @@ export class DatabaseDetailsPage extends React.Component<
   }
 
   componentDidMount(): void {
+    this.props.refreshNodes();
     if (!this.props.loaded && !this.props.loading && !this.props.requestError) {
       this.props.refreshDatabaseDetails(
         this.props.name,

diff --git a/pkg/ui/workspaces/cluster-ui/src/databaseTablePage/databaseTablePageConnected.ts b/pkg/ui/workspaces/cluster-ui/src/databaseTablePage/databaseTablePageConnected.ts
@@ -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),

diff --git a/pkg/ui/workspaces/cluster-ui/src/databases/combiners.ts b/pkg/ui/workspaces/cluster-ui/src/databases/combiners.ts
@@ -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,
@@ -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,15 +80,22 @@ 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;
-  // TODO #118957 (xinhaoz) Use store id to regions mapping.
-  const stores = dbStats?.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: dbStats?.replicaData.storeIDs || [],
+  };
+  /** List of node IDs for the current cluster. */
+  const nodes = getNodeIdsFromStoreIds(stores, nodeStatuses);
+
   const nodesByRegionString = getNodesByRegionString(
-    stores,
+    nodes,
     nodeRegionsByID,
     isTenant,
   );
@@ -100,7 +114,7 @@ const deriveDatabaseDetails = (
     name: database,
     spanStats: spanStats?.data?.results.spanStats,
     tables: dbDetails?.data?.results.tablesResp,
-    nodes: stores,
+    nodes: nodes.ids,
     nodesByRegionString,
     numIndexRecommendations,
   };
@@ -112,6 +126,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(
@@ -120,18 +136,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,
+      );
     });
   },
 );
@@ -141,14 +165,19 @@ 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 || [];
+  const stores: Stores = {
+    kind: "store",
+    ids: results?.stats.replicaData.storeIDs || [],
+  };
+  const nodes: Nodes = getNodeIdsFromStoreIds(stores, nodeStatuses);
   return {
     name: table,
     loading: !!details?.inFlight,
@@ -165,13 +194,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),
     },
   };
 };
@@ -180,21 +204,35 @@ 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 || [];
+
+    const stores: Stores = {
+      kind: "store",
+      ids: results?.stats.replicaData.storeIDs || [],
+    };
+    const nodes = getNodeIdsFromStoreIds(stores, nodeStatuses);
+
     return {
       loading: !!details?.inFlight,
       loaded: !!details?.valid,