Skip to content
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

[Disk Manager] errors from blockstore should be internal by default #2980

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

gy2411
Copy link
Collaborator

@gy2411 gy2411 commented Feb 5, 2025

Before this fix, all error from blockstore with codes E_PRECONDITION_FAILED and E_RESOURCE_EXHAUSTED were treated as public. This is too wide range of errors to treat them all as public.

@gy2411 gy2411 added large-tests Launch large tests for PR disk_manager Add this label to run only cloud/disk_manager build and tests on PR labels Feb 5, 2025
Copy link
Contributor

github-actions bot commented Feb 5, 2025

Note

This is an automated comment that will be appended during run.

🟢 linux-x86_64-relwithdebinfo: all tests PASSED for commit 2d288f6.

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
856 856 0 0 0 0

clientErr := nbs_client.GetClientError(e)

if clientErr.Code == nbs_client.E_RESOURCE_EXHAUSTED &&
strings.Contains(clientErr.Message, "max disk count in group exceeded") {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

}

if clientErr.Code == nbs_client.E_PRECONDITION_FAILED &&
strings.Contains(clientErr.Message, "failed to add some disks") {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Конечно, не очень хорошо при определении публичности закладываться на тескт ошибки.
Но в то же время по-другому сходу и не сделаешь. Если не смотреть на текст, то остаётся смотреть только на код ошибки. https://github.com/ydb-platform/nbs/blob/main/cloud/blockstore/public/sdk/go/client/error.go#L10. А это уже грубовато.

Идеальная картина в моём представлении могла бы выглядеть так. Публичность ошибки определяется в том месте, где она создаётся (в данном случае в коде disk registry). Далее она пробразывается до disk manager в виде флажка. Но для этого придётся потрогать и код nbs, и код blockstore клиента. И в целом не уверен, стоит ли так делать.

Copy link
Contributor

github-actions bot commented Feb 6, 2025

Note

This is an automated comment that will be appended during run.

🟢 linux-x86_64-relwithdebinfo: all tests PASSED for commit acb7c9d.

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
856 856 0 0 0 0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disk_manager Add this label to run only cloud/disk_manager build and tests on PR large-tests Launch large tests for PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant