Skip to content

fix(Storage): fix disks view for degraded group #1930

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Feb 17, 2025
Merged

fix(Storage): fix disks view for degraded group #1930

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a3d0c74
fix(Storage): fix disks view for degraded group
artemmufazalov Feb 11, 2025
cc4a5c9
fix: add comments
artemmufazalov Feb 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions src/containers/Storage/Disks/Disks.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,10 @@ import React from 'react';
import {Flex, useLayoutContext} from '@gravity-ui/uikit';

import {VDisk} from '../../../components/VDisk/VDisk';
import {valueIsDefined} from '../../../utils';
import type {Erasure} from '../../../types/api/storage';
import {cn} from '../../../utils/cn';
import type {PreparedVDisk} from '../../../utils/disks/types';
import {isNumeric} from '../../../utils/utils';
import {PDisk} from '../PDisk';
import type {StorageViewContext} from '../types';
import {isVdiskActive, useVDisksWithDCMargins} from '../utils';
Expand All @@ -19,12 +20,13 @@ const VDISKS_CONTAINER_WIDTH = 300;
interface DisksProps {
vDisks?: PreparedVDisk[];
viewContext?: StorageViewContext;
erasure?: Erasure;
}

export function Disks({vDisks = [], viewContext}: DisksProps) {
export function Disks({vDisks = [], viewContext, erasure}: DisksProps) {
const [highlightedVDisk, setHighlightedVDisk] = React.useState<string | undefined>();

const vDisksWithDCMargins = useVDisksWithDCMargins(vDisks);
const vDisksWithDCMargins = useVDisksWithDCMargins(vDisks, erasure);

const {
theme: {spaceBaseSize},
Expand All @@ -40,9 +42,9 @@ export function Disks({vDisks = [], viewContext}: DisksProps) {
return (
<div className={b(null)}>
<Flex direction={'row'} gap={1} grow style={{width: VDISKS_CONTAINER_WIDTH}}>
{vDisks?.map((vDisk) => (
{vDisks?.map((vDisk, index) => (
<VDiskItem
key={vDisk.StringifiedId}
key={vDisk.StringifiedId || index}
vDisk={vDisk}
inactive={!isVdiskActive(vDisk, viewContext)}
highlightedVDisk={highlightedVDisk}
Expand All @@ -55,7 +57,7 @@ export function Disks({vDisks = [], viewContext}: DisksProps) {
<div className={b('pdisks-wrapper')}>
{vDisks?.map((vDisk, index) => (
<PDiskItem
key={vDisk?.PDisk?.StringifiedId}
key={vDisk?.PDisk?.StringifiedId || index}
Copy link
Member Author

@artemmufazalov artemmufazalov Feb 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There could be no proper id, use index in such case. Array index will be always different from pdisk and vdisk ids, since ids include several numbers joined by dash (e.g. "1-1001")

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in which cases there may be no StringifiedId and why cant we always use index

I dont mean to rework anything - I just dont understand (:

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. If there is no data about PDisk, then there would be no id, so we use index to prevent undefined ids - with undefined ids there is a bug where new disks added on every rerender
  2. Generally it's not a good practice to use indexes as keys, since they could cause some unexpected behaviour if array is reordered or some elements added or deleted (and it's not recommended in react docs). So we use unique ids everywhere where it's possible. For the case of missing vdisks' pdisks there is no much difference, since they have no data and displayed the same way

vDisk={vDisk}
highlightedVDisk={highlightedVDisk}
setHighlightedVDisk={setHighlightedVDisk}
Expand Down Expand Up @@ -89,7 +91,7 @@ function VDiskItem({
const vDiskId = vDisk.StringifiedId;

// show vdisks without AllocatedSize as having average width (#1433)
const minWidth = valueIsDefined(vDiskToShow.AllocatedSize) ? undefined : unavailableVDiskWidth;
const minWidth = isNumeric(vDiskToShow.AllocatedSize) ? undefined : unavailableVDiskWidth;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AllocatedSize could be numeric or NaN, but not undefined

const flexGrow = Number(vDiskToShow.AllocatedSize) || 1;

return (
Expand Down
12 changes: 10 additions & 2 deletions src/containers/Storage/StorageGroups/columns/columns.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,9 @@ const getVDisksColumn = (data?: GetStorageColumnsData): StorageGroupsColumn => (
name: STORAGE_GROUPS_COLUMNS_IDS.VDisks,
header: STORAGE_GROUPS_COLUMNS_TITLES.VDisks,
className: b('vdisks-column'),
render: ({row}) => <VDisks vDisks={row.VDisks} viewContext={data?.viewContext} />,
render: ({row}) => (
<VDisks vDisks={row.VDisks} viewContext={data?.viewContext} erasure={row.ErasureSpecies} />
),
align: DataTable.CENTER,
width: 780, // usually 8-9 vdisks, this width corresponds to 8 vdisks, column is expanded if more
resizeable: false,
Expand All @@ -248,7 +250,13 @@ const getDisksColumn = (data?: GetStorageColumnsData): StorageGroupsColumn => ({
header: STORAGE_GROUPS_COLUMNS_TITLES.VDisksPDisks,
className: b('disks-column'),
render: ({row}) => {
return <Disks vDisks={row.VDisks} viewContext={data?.viewContext} />;
return (
<Disks
vDisks={row.VDisks}
viewContext={data?.viewContext}
erasure={row.ErasureSpecies}
/>
);
},
align: DataTable.CENTER,
width: 900,
Expand Down
6 changes: 4 additions & 2 deletions src/containers/Storage/VDisks/VDisks.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import {VDiskWithDonorsStack} from '../../../components/VDisk/VDiskWithDonorsStack';
import type {Erasure} from '../../../types/api/storage';
import {cn} from '../../../utils/cn';
import type {PreparedVDisk} from '../../../utils/disks/types';
import type {StorageViewContext} from '../types';
Expand All @@ -11,10 +12,11 @@ const b = cn('ydb-storage-vdisks');
interface VDisksProps {
vDisks?: PreparedVDisk[];
viewContext?: StorageViewContext;
erasure?: Erasure;
}

export function VDisks({vDisks, viewContext}: VDisksProps) {
const vDisksWithDCMargins = useVDisksWithDCMargins(vDisks);
export function VDisks({vDisks, viewContext, erasure}: VDisksProps) {
const vDisksWithDCMargins = useVDisksWithDCMargins(vDisks, erasure);

return (
<div className={b('wrapper')}>
Expand Down
15 changes: 13 additions & 2 deletions src/containers/Storage/utils/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import React from 'react';

import {selectNodesMap} from '../../../store/reducers/nodesList';
import type {PreparedStorageGroup} from '../../../store/reducers/storage/types';
import type {Erasure} from '../../../types/api/storage';
import {valueIsDefined} from '../../../utils';
import type {PreparedVDisk} from '../../../utils/disks/types';
import {generateEvaluator} from '../../../utils/generateEvaluator';
Expand Down Expand Up @@ -84,12 +85,22 @@ export function getStorageGroupsInitialEntitiesCount(
return DEFAULT_ENTITIES_COUNT;
}

export function useVDisksWithDCMargins(vDisks: PreparedVDisk[] = []) {
function isErasureWithDifferentDC(erasure?: Erasure) {
// These erasure types suppose the data distributed across 3 different DC
return erasure === 'mirror-3-dc' || erasure === 'mirror-3of4';
Copy link
Collaborator

@astandrik astandrik Feb 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do these magic constant strings mean?

comment would be nice (and may be move to actual constants)

}

export function useVDisksWithDCMargins(vDisks: PreparedVDisk[] = [], erasure?: Erasure) {
const nodesMap = useTypedSelector(selectNodesMap);

return React.useMemo(() => {
const disksWithMargins: number[] = [];

// If single-dc erasure, all disks are in the same DC
if (!isErasureWithDifferentDC(erasure)) {
return disksWithMargins;
}

// Backend returns disks sorted by DC, so we don't need to apply any additional sorting
vDisks.forEach((disk, index) => {
const dc1 = nodesMap?.get(Number(disk?.NodeId))?.DC;
Expand All @@ -101,5 +112,5 @@ export function useVDisksWithDCMargins(vDisks: PreparedVDisk[] = []) {
});

return disksWithMargins;
}, [vDisks, nodesMap]);
}, [erasure, vDisks, nodesMap]);
}
41 changes: 39 additions & 2 deletions src/store/reducers/storage/__tests__/prepareGroupsDisks.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,19 @@ describe('prepareGroupsVDisk', () => {
AllocatedPercent: 12,

Donors: undefined,
PDisk: undefined,

PDisk: {
AllocatedPercent: NaN,
AllocatedSize: NaN,
AvailableSize: NaN,
NodeId: 224,
PDiskId: undefined,
Severity: 0,
SlotSize: undefined,
StringifiedId: undefined,
TotalSize: NaN,
Type: undefined,
},
};

const preparedData = prepareGroupsVDisk(vDiksDataWithoutPDisk);
Expand Down Expand Up @@ -124,6 +136,19 @@ describe('prepareGroupsVDisk', () => {
AvailableSize: 234461593600,
TotalSize: 265405071360,
AllocatedPercent: 12,

PDisk: {
AllocatedPercent: NaN,
AllocatedSize: NaN,
AvailableSize: NaN,
NodeId: 224,
PDiskId: undefined,
Severity: 0,
SlotSize: undefined,
StringifiedId: undefined,
TotalSize: NaN,
Type: undefined,
},
};

const preparedData = prepareGroupsVDisk(vDiksDataWithoutPDisk);
Expand Down Expand Up @@ -215,7 +240,19 @@ describe('prepareGroupsVDisk', () => {
AllocatedPercent: 12,

Donors: undefined,
PDisk: undefined,

PDisk: {
AllocatedPercent: NaN,
AllocatedSize: NaN,
AvailableSize: NaN,
NodeId: undefined,
PDiskId: undefined,
Severity: 0,
SlotSize: undefined,
StringifiedId: undefined,
TotalSize: NaN,
Type: undefined,
},
};

const preparedData = prepareGroupsVDisk(vDiksDataWithoutPDisk);
Expand Down
4 changes: 1 addition & 3 deletions src/store/reducers/storage/prepareGroupsDisks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,7 @@ export function prepareGroupsVDisk(data: TStorageVDisk = {}): PreparedVDisk {
VDiskId: whiteboardVDisk.VDiskId,
};

const preparedPDisk = PDisk
? prepareGroupsPDisk({...PDisk, NodeId: mergedVDiskData.NodeId})
: undefined;
const preparedPDisk = prepareGroupsPDisk({...PDisk, NodeId: mergedVDiskData.NodeId});
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add empty PDisk object even if there is no PDisk field


const PDiskId = preparedPDisk?.PDiskId ?? whiteboardVDisk?.PDiskId;

Expand Down
4 changes: 2 additions & 2 deletions src/store/reducers/storage/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import {z} from 'zod';

import type {EFlag} from '../../../types/api/enums';
import type {NodesGroupByField} from '../../../types/api/nodes';
import type {GroupsGroupByField} from '../../../types/api/storage';
import type {Erasure, GroupsGroupByField} from '../../../types/api/storage';
import type {PreparedPDisk, PreparedVDisk} from '../../../utils/disks/types';
import type {NodesUptimeFilterValues, PreparedNodeSystemState} from '../../../utils/nodes';

Expand Down Expand Up @@ -56,7 +56,7 @@ export interface PreparedStorageGroup {
PoolName?: string;
MediaType?: string;
Encryption?: boolean;
ErasureSpecies?: string;
ErasureSpecies?: Erasure;
Degraded: number;
Overall?: EFlag;

Expand Down
3 changes: 2 additions & 1 deletion src/types/api/cluster.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import type {EFlag} from './enums';
import type {Erasure} from './storage';
import type {TTabletStateInfo} from './tablet';

/**
Expand Down Expand Up @@ -38,7 +39,7 @@ export interface TClusterInfoV1 {

export interface TStorageStats {
PDiskFilter?: string;
ErasureSpecies?: string;
ErasureSpecies?: Erasure;
CurrentAvailableSize?: string;
/** uint64 */
CurrentAllocatedSize?: string;
Expand Down
9 changes: 7 additions & 2 deletions src/types/api/storage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ export type TStorageGroupInfo = TBSGroupStateInfo &

interface TBSGroupStateInfo {
GroupID?: number;
ErasureSpecies?: string;
ErasureSpecies?: Erasure;
VDisks?: TVDiskStateInfo[];
/** uint64 */
ChangeTime?: string;
Expand Down Expand Up @@ -137,7 +137,7 @@ export interface TGroupsStorageGroupInfo {
DiskSpace?: EFlag;
Kind?: string;
MediaType?: string;
ErasureSpecies?: string;
ErasureSpecies?: Erasure;
/** uint64 */
AllocationUnits?: string;
/**
Expand Down Expand Up @@ -228,6 +228,11 @@ export interface TStoragePDisk {
Whiteboard?: TPDiskStateInfo;
}

/**
* https://ydb.tech/docs/en/concepts/topology#cluster-config
*/
export type Erasure = 'none' | 'block-4-2' | 'mirror-3-dc' | 'mirror-3of4';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe enum and use it in isErasureWithDifferentDC ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like enums, you have to import them everywhere to satisfy TS, while for union string types every option will be suggested by autocomplete


// ==== Request types ====

export type EVersion = 'v1' | 'v2'; // only v2 versions works with sorting
Expand Down
Loading