Issue-4293: added to Disk Registry, sending attach/detach requests for CMS events (ADD_DEVICE/ADD_HOST/REMOVE_DEVICE/REMOVE HOST) #4442

vladstepanyuk · 2025-10-02T09:01:33Z

Описание логики тут https://github.com/ydb-platform/nbs/blob/main/doc/blockstore/disk_registry/attach-detach-hw-devices-in-da-on-cms.md

github-actions · 2025-10-03T14:10:19Z

Note

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

🔴 linux-x86_64-relwithdebinfo: some tests FAILED for commit eec2a6b.

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED^?
5795	5793	0	0	1	1	0

🔴 linux-x86_64-relwithdebinfo: some tests FAILED for commit eec2a6b.

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED^?
5795	5792	0	1	1	1	0

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

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED^?
2	2	0	0	0	0	0

github-actions · 2025-10-06T08:40:42Z

Note

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

🔴 linux-x86_64-relwithdebinfo: some tests FAILED for commit cb70f2b.

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED^?
5842	5839	0	2	0	1	0

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

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED^?
4	4	0	0	0	0	0

cloud/blockstore/config/storage.proto

cloud/blockstore/libs/storage/disk_registry/disk_registry_actor_attach_detach_path.cpp

komarevtsev-d · 2025-10-06T10:50:25Z

cloud/blockstore/libs/storage/disk_registry/disk_registry_actor_attach_detach_path.cpp

+    }
+
+    if (AgentsWithAttachDetachRequestsInProgress.size() ==
+        Config->GetMaxInflightAttachDetachPathRequestsProcessing())


А зачем этот ограничитель? Чего боимся?

да просто на всякий случай, как будто лучше всегда ограничивать максимальную паралельность, чтобы например в случае чего на всем кластере cразу одновременно не начинать тяжелые операции открытия девайсов

cloud/blockstore/libs/storage/protos/disk.proto

komarevtsev-d · 2025-10-06T15:17:44Z

cloud/blockstore/libs/storage/protos/disk.proto

+    repeated TPathToGeneration PathsToDetach = 2;
+
+    // Generation of DR tablet.
+    uint32 DiskRegistryGeneration = 3;


А реально, зачем поколение таблетки? От чего страхуемся?
Гарантий от локальной базы должно хватать чтобы этого не требовалось

DR в поколении n-1 отправляет реквест с закрытием девайса
DR рестартует
DR с поколением n отправляет реквест с открытием девайса
DA получает сообщение от DR с поколением n реквест на открытие девайса
DA получет сообщение от DR с поколением n-1 реквест на закрытие девайса

DR думает что девайс открыт но девайс закрыт, локальная база же никак не гарантирует порядок сообщений к DA

Если к каждому запросу прицеплен seqNo, то DA поймет, что если он уже выполнил запрос с seqNo=n, запрос с SeqNo=n-1 выполнять не надо.

так я этот seqNo в оперативке храню, если Dr рестартанет, то и seqNo сбросится

можно конечно их в локальной базе хранить, но как мне кажется хранить эти seqNo в базе будет более геморно чем отправлять два счетчика - поколение таблетки и хранимый в оперативе seqNo

cloud/blockstore/libs/storage/disk_registry/disk_registry_actor_attach_detach_path.cpp

cloud/blockstore/libs/storage/disk_registry/disk_registry_state.cpp

github-actions · 2025-10-07T13:09:16Z

Note

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

🔴 linux-x86_64-relwithdebinfo: some tests FAILED for commit 93a9dfa.

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED^?
5842	5839	0	2	0	1	0

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

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED^?
4	4	0	0	0	0	0

github-actions · 2025-10-14T10:48:14Z

Note

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

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

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED^?
5867	5866	0	0	0	1	0

sharpeye · 2025-10-14T12:10:20Z

cloud/blockstore/libs/storage/protos/disk.proto

+//                              //                      //                                               //
+///////////////////////////////////////////////////////////////////////////////////////////////////////////
+//                              //                      //                                               //
+// PATH_ATTACH_STATE_ATTACHING  // No                   // active attach                                 //


Зачем нужны attaching/detaching состояния? Инфру держать, пока DA отвечает?

attaching - чтобы случайно не алоцировать диск на закрытом девайсе (как только мы поймем что девайс открыт мы переведем его в состояние PATH_ATTACH_STATE_ATTACHED), ну и чтобы не долбится с секьюр эрейзами в него пока мы его не откроем
detaching - чтобы держать инфру пока девайс не закроется

attaching - чтобы случайно не алоцировать диск на закрытом девайсе (как только мы поймем что девайс открыт мы переведем его в состояние PATH_ATTACH_STATE_ATTACHED), ну и чтобы не долбится с секьюр эрейзами в него пока мы его не откроем

А если помечать девайсы приаттаченными только после ответа DA?

Инфра приходит в DR, DR идет в DA; DA прицепляет девайс, отвечает в DR; DR помечает девайс прицепленным, отвечает в инфру.
Если DR перезапустился, инфра ретраит запрос.

detaching - чтобы держать инфру пока девайс не закроется

Инфра пришла с запросом Detach; DR помечает девайс отцепленным (в базе), отправляет запрос в DA; DA отцепляет девайс и отвечает в DR; DR отвечает в инфру.
Если DR перезапускается в середине; Инфра ретраит запрос, DR, не смотря на статус девайса, прокручивает фарш полностью.

А если помечать девайсы приаттаченными только после ответа DA?
Инфра приходит в DR, DR идет в DA; DA прицепляет девайс, отвечает в DR; DR помечает девайс прицепленным, отвечает в инфру.
Если DR перезапустился, инфра ретраит запрос.

Тогда надо будет распиливать как-то транзакции AddDevice/AddHost. Типа сначала проверить, что добавление этого девайса вообще возможно (запустить dry run транзакции), затем выполнить присоединение девайса и потом полноценную транзакцию AddDevice/AddHost.

Ну и тогда получается, что на присоединении у нас будет сначала запрос в DA, затем пишущая транзакция, а на отсоединении наоборот. Лично мне кажется, это может запутать потом, и лучше сделать одинаковым образом и на присоединении, и на отсоединении.

Плюс надо будет где-то (видимо, в каком-то акторе или внутри ДР-актора) прикапывать RequestInfo для ответа инфре, т. е. список Pending CMS запросов.

Плюс, если вдруг что-то посередине этой цепочки произойдет, то запросы ретраятся. А если вдруг ретраить не будут, то система окажется в каком-то странном состоянии, что вроде в DA девайс открыт и т. д., но и DR, и инфра думают, что девайс еще не присоединен и не введен, соответственно (например, если ответ от DA потерялся). В реальной жизни такого, конечно, быть не должно, что запросы не ретраятся, но вообще это вроде теоретически валидное поведение, которое при этом приводит систему в странное состояние с нарушенными инвариантами.

Инфра пришла с запросом Detach; DR помечает девайс отцепленным (в базе), отправляет запрос в DA; DA отцепляет девайс и отвечает в DR; DR отвечает в инфру.
Если DR перезапускается в середине; Инфра ретраит запрос, DR, не смотря на статус девайса, прокручивает фарш полностью.

Опять же, чтобы всё нормально работало, инфре снаружи надо постоянно что-то ретраить, иначе риск нарушенных инвариантов между DA и DR по состоянию «прикрепленности» девайсов. Плюс лишние закрытия девайсов, не страшно, но просто зачем, если их можно избежать?

Мне кажется, это выглядит гораздо сложнее и опаснее, чем добавление отдельного состояния, что девайс в процессе присоединения/отсоединения, с выполнением этих операций в асинхронном режиме. Так мы четко видим, какие девайсы полностью введены в использование, а какие только в процессе. Плюс ничего страшного у меня не произойдет, если вдруг запросы перестанут ретраиться, операция сама в асинхронном режиме «доползет» до конца. И оно как-то лучше ложится на существующую логику, не надо как-то распиливать транзакции, отвечающие за операции, или продлевать их исполнение запросами в DA.

…ts, replaced E_ARGUMENT with E_PRECONDITION_FAILED and corrected some other issues

github-actions · 2025-10-20T11:27:07Z

Note

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

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

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED^?
5867	5866	0	0	0	1	0

sharpeye · 2025-10-21T14:22:23Z

cloud/blockstore/libs/storage/protos/disk.proto

+    repeated TPathToGeneration PathsToDetach = 2;
+
+    // Generation of DR tablet.
+    uint32 DiskRegistryGeneration = 3;


Если к каждому запросу прицеплен seqNo, то DA поймет, что если он уже выполнил запрос с seqNo=n, запрос с SeqNo=n-1 выполнять не надо.

sharpeye · 2025-10-21T14:24:00Z

cloud/blockstore/libs/storage/protos/disk.proto

+//                              //                      //                                               //
+///////////////////////////////////////////////////////////////////////////////////////////////////////////
+//                              //                      //                                               //
+// PATH_ATTACH_STATE_ATTACHING  // No                   // active attach                                 //


attaching - чтобы случайно не алоцировать диск на закрытом девайсе (как только мы поймем что девайс открыт мы переведем его в состояние PATH_ATTACH_STATE_ATTACHED), ну и чтобы не долбится с секьюр эрейзами в него пока мы его не откроем

А если помечать девайсы приаттаченными только после ответа DA?

Инфра приходит в DR, DR идет в DA; DA прицепляет девайс, отвечает в DR; DR помечает девайс прицепленным, отвечает в инфру.
Если DR перезапустился, инфра ретраит запрос.

detaching - чтобы держать инфру пока девайс не закроется

Инфра пришла с запросом Detach; DR помечает девайс отцепленным (в базе), отправляет запрос в DA; DA отцепляет девайс и отвечает в DR; DR отвечает в инфру.
Если DR перезапускается в середине; Инфра ретраит запрос, DR, не смотря на статус девайса, прокручивает фарш полностью.

sharpeye · 2025-10-21T14:25:49Z

cloud/blockstore/libs/storage/protos/disk.proto

    bool TemporaryAgent = 13;
+
+    // Attach state for each path.
+    map<string, EPathAttachState> PathAttachStates = 14;


Разве одного списка DetachedPaths не хватит? Если путь в этом списке, то он отцепплен, иначе - прицеплен.

sharpeye · 2025-10-21T14:27:12Z

cloud/blockstore/libs/storage/protos/disk.proto

+    // Allowed devices attached to the agent.
+    repeated TPathToGeneration AllowedPaths = 4;
+
+    // Unallowed devices attached to the agent.
+    repeated TPathToGeneration UnknownPaths = 5;


Зачем TPathToGeneration, а не просто string?

потому что все операции присоединения отсоединения у меня должны быть гарантировано упорядочены, чтобы девайс ни закрывался ни открывался если уже пришел более свежий запрос

хочется быть максимально увереным что никаких гонок не будет, потому что если вдруг аллоцируется диск на закрытом девайсе то это залипание пока не придет дежурный и не рестартанет DA

vladstepanyuk requested a review from komarevtsev-d October 2, 2025 09:01

vladstepanyuk added large-tests Launch large tests for PR blockstore Add this label to run only cloud/blockstore build and tests on PR labels Oct 2, 2025

vladstepanyuk requested review from SvartMetal and sharpeye October 6, 2025 12:15

komarevtsev-d reviewed Oct 6, 2025

View reviewed changes

cloud/blockstore/libs/storage/disk_registry/disk_registry_state.cpp Outdated Show resolved Hide resolved

vladstepanyuk requested a review from komarevtsev-d October 7, 2025 10:24

vladstepanyuk force-pushed the users/vladstepanyuk/issue-4293/1 branch from 93a9dfa to c9d19c6 Compare October 14, 2025 08:06

sharpeye reviewed Oct 14, 2025

View reviewed changes

vladstepanyuk requested a review from sharpeye October 15, 2025 07:52

vladstepanyuk added 10 commits October 20, 2025 08:50

issue-4293: added attaching detaching paths logic to disk registry

a978aa9

removed field from backup

8a49773

issue-4293: fix build

6972877

issue-4293: added some logs and refactoring

532cb75

issue-4293: fix build

73782a0

issue-4293: fix build

ba42303

issue-4293: fix test build

f0baab8

issue-4293: removed some attaches, optimized registration, add commen…

1944019

…ts, replaced E_ARGUMENT with E_PRECONDITION_FAILED and corrected some other issues

issue-4293: removed ProcessPathAttachStatesOnRegistration

81b41dd

issue-4293: attach detach all known paths on Add Host

d947de9

vladstepanyuk force-pushed the users/vladstepanyuk/issue-4293/1 branch from c9d19c6 to d947de9 Compare October 20, 2025 08:52

sharpeye reviewed Oct 21, 2025

View reviewed changes

Uh oh!

Issue-4293: added to Disk Registry, sending attach/detach requests for CMS events (ADD_DEVICE/ADD_HOST/REMOVE_DEVICE/REMOVE HOST) #4442

Are you sure you want to change the base?

Issue-4293: added to Disk Registry, sending attach/detach requests for CMS events (ADD_DEVICE/ADD_HOST/REMOVE_DEVICE/REMOVE HOST) #4442

Uh oh!

Conversation

vladstepanyuk commented Oct 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vladstepanyuk Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vladstepanyuk Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vladstepanyuk Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Oct 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vladstepanyuk commented Oct 2, 2025 •

edited

Loading

github-actions bot commented Oct 3, 2025 •

edited

Loading

github-actions bot commented Oct 6, 2025 •

edited

Loading

vladstepanyuk Oct 7, 2025 •

edited

Loading

vladstepanyuk Oct 22, 2025 •

edited

Loading

github-actions bot commented Oct 7, 2025 •

edited

Loading

vladstepanyuk Oct 22, 2025 •

edited

Loading