-
Notifications
You must be signed in to change notification settings - Fork 741
50 shades of storage #30583
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.
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
base: main
Are you sure you want to change the base?
50 shades of storage #30583
Conversation
|
⚪
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ ⚪ Ya make output | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
🟢 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request adds new storage capacity metrics to the viewer API for monitoring storage health and capacity across nodes and storage groups. The PR introduces four new metrics: MaxPDiskUsage, MaxVDiskSlotUsage, MaxVDiskRawUsage, and CapacityAlert, along with support for sorting, grouping, and filtering by these metrics.
Key changes:
- Added new storage capacity metrics (MaxPDiskUsage, MaxVDiskSlotUsage, MaxVDiskRawUsage, CapacityAlert) to track storage utilization
- Implemented calculation methods for aggregating VDisk and PDisk metrics at node and group levels
- Extended API documentation to include new fields for sorting, grouping, and filtering operations
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 18 comments.
Show a summary per file
| File | Description |
|---|---|
| ydb/core/viewer/viewer_nodes.h | Added new enum fields, struct members, calculation methods (CalcVDisks, CalcPDisks), and whiteboard request initialization for node-level storage metrics |
| ydb/core/viewer/viewer_groups.h | Added new enum fields, struct members, CalcCapacityMetrics method, and whiteboard request handling for group-level storage metrics |
| ydb/core/viewer/tests/test.py | Updated test normalization to include new fields and added new test cases for grouping by CapacityAlert |
| ydb/core/viewer/tests/canondata/result.json | Updated canonical test output to reflect new fields in API responses |
| ydb/core/viewer/protos/viewer.proto | Added new optional fields to TStorageGroupInfo and TNodeInfo messages |
| ydb/core/viewer/pdisk_info.h | Modified whiteboard requests to request all fields instead of defaults |
| ydb/core/viewer/json_handlers_viewer.cpp | Incremented API version numbers for nodes and groups handlers |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
79f1c20 to
54c8947
Compare
|
⚪ ⚪ Ya make output | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
| case EGroupFields::MaxVDiskSlotUsage: | ||
| SortCollection(GroupView, [](const TGroup* group) { return group->MaxVDiskSlotUsage; }, ReverseSort); | ||
| break; | ||
| case EGroupFields::MaxNormalizedOccupancy: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| case EGroupFields::MaxNormalizedOccupancy: | |
| case EGroupFields::MaxNormalizedOccupancy: | |
| case EGroupFields::CapacityAlert: | |
| SortCollection(GroupView, [](const TGroup* group) { return group->MaxNormalizedOccupancy; }, ReverseSort); | |
| break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Поправочка. CapacityAlert надо сортировать по MaxVDiskSlotUsage
| case EGroupFields::MaxNormalizedOccupancy: | ||
| return (float)MaxNormalizedOccupancy; | ||
| case EGroupFields::CapacityAlert: | ||
| return (ui64)CapacityAlert; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| case EGroupFields::MaxNormalizedOccupancy: | |
| return (float)MaxNormalizedOccupancy; | |
| case EGroupFields::CapacityAlert: | |
| return (ui64)CapacityAlert; | |
| case EGroupFields::MaxNormalizedOccupancy: | |
| case EGroupFields::CapacityAlert: | |
| return (float)MaxNormalizedOccupancy; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Это нужно чтобы диски с одинаковым алертом (все зеленые) были отсортированы
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Я сейчас осознал, что CapacityAlert корректнее сортировать по MaxVDiskSlotUsage, а не по MaxNormalizedOccupancy.
Давай лучше сделаем так:
case EGroupFields::MaxPDiskUsage:
return (float)MaxPDiskUsage;
case EGroupFields::MaxVDiskSlotUsage:
case EGroupFields::CapacityAlert:
return (float)MaxVDiskSlotUsage;
case EGroupFields::MaxNormalizedOccupancy:
return (float)MaxNormalizedOccupancy;И для узлов и для Group.ApplySort тоже
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
В void FillVDiskFromVSlotInfo кажется не хватает новых полей по аналогии с https://github.com/ydb-platform/ydb/pull/27792/files#diff-8b1badd7d4809ec27a64cdcb492e027aa5a275f28fe14fa9abf6581e5f4b8838R1391
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
И в ProcessResponses не хватает инициализации полей https://github.com/ydb-platform/ydb/pull/27792/files#diff-8b1badd7d4809ec27a64cdcb492e027aa5a275f28fe14fa9abf6581e5f4b8838R1455
В канонизированных результатах group.MaxPDiskUsage=0, а должен быть non-zero-number
| PileName, | ||
| MaxPDiskUsage, | ||
| MaxVDiskSlotUsage, | ||
| MaxVDiskRawUsage, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| MaxVDiskRawUsage, |
Предлагаю убрать MaxVDiskRawUsage для узлов. Для групп эта метрика нужна для глубокого траблшутинга (см https://github.com/ydb-platform/ydb-rfc/blob/main/164_capacity_metrics.md#рекомендации), а для узлов только будет сбивать с толку. Пользователю мы хотим рассказывать только про PDiskUsage и VDiskSlotUsage (я рассчитываю что они просто заменят собой group.DiskUsage и group.Usage)
| case EGroupFields::MaxNormalizedOccupancy: | ||
| return (float)MaxNormalizedOccupancy; | ||
| case EGroupFields::CapacityAlert: | ||
| return (ui64)CapacityAlert; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Я сейчас осознал, что CapacityAlert корректнее сортировать по MaxVDiskSlotUsage, а не по MaxNormalizedOccupancy.
Давай лучше сделаем так:
case EGroupFields::MaxPDiskUsage:
return (float)MaxPDiskUsage;
case EGroupFields::MaxVDiskSlotUsage:
case EGroupFields::CapacityAlert:
return (float)MaxVDiskSlotUsage;
case EGroupFields::MaxNormalizedOccupancy:
return (float)MaxNormalizedOccupancy;И для узлов и для Group.ApplySort тоже
| case EGroupFields::MaxVDiskSlotUsage: | ||
| SortCollection(GroupView, [](const TGroup* group) { return group->MaxVDiskSlotUsage; }, ReverseSort); | ||
| break; | ||
| case EGroupFields::MaxNormalizedOccupancy: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Поправочка. CapacityAlert надо сортировать по MaxVDiskSlotUsage
| float MaxVDiskRawUsage = 0; | ||
| float MaxNormalizedOccupancy = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| float MaxVDiskRawUsage = 0; | |
| float MaxNormalizedOccupancy = 0; |
Как я уже сказал, MaxVDiskRawUsage для узлов я считаю не нужен. MaxNormalizedOccupancy тоже. В моей версии патча он нужен был для сортировки по CapacityAlert, но сейчас я понял что лучше для этого использовать MaxVDiskSlotUsage
| case ENodeFields::CapacityAlert: | ||
| return static_cast<int>(CapacityAlert); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| case ENodeFields::CapacityAlert: | |
| return static_cast<int>(CapacityAlert); | |
| case ENodeFields::CapacityAlert: | |
| return MaxVDiskSlotUsage; |
| case ENodeFields::MaxVDiskRawUsage: | ||
| SortCollection(NodeView, [](const TNode* node) { return node->MaxVDiskRawUsage; }, ReverseSort); | ||
| NeedSort = false; | ||
| break; | ||
| case ENodeFields::MaxVDiskSlotUsage: | ||
| SortCollection(NodeView, [](const TNode* node) { return node->MaxVDiskSlotUsage; }, ReverseSort); | ||
| NeedSort = false; | ||
| break; | ||
| case ENodeFields::CapacityAlert: | ||
| SortCollection(NodeView, [](const TNode* node) { return node->MaxNormalizedOccupancy; }, ReverseSort); | ||
| NeedSort = false; | ||
| break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| case ENodeFields::MaxVDiskRawUsage: | |
| SortCollection(NodeView, [](const TNode* node) { return node->MaxVDiskRawUsage; }, ReverseSort); | |
| NeedSort = false; | |
| break; | |
| case ENodeFields::MaxVDiskSlotUsage: | |
| SortCollection(NodeView, [](const TNode* node) { return node->MaxVDiskSlotUsage; }, ReverseSort); | |
| NeedSort = false; | |
| break; | |
| case ENodeFields::CapacityAlert: | |
| SortCollection(NodeView, [](const TNode* node) { return node->MaxNormalizedOccupancy; }, ReverseSort); | |
| NeedSort = false; | |
| break; | |
| case ENodeFields::MaxVDiskSlotUsage: | |
| case ENodeFields::CapacityAlert: | |
| SortCollection(NodeView, [](const TNode* node) { return node->MaxVDiskSlotUsage; }, ReverseSort); | |
| NeedSort = false; | |
| break; |
Changelog entry
...
Changelog category
Description for reviewers
...