issue-4293: order all attach detach requests with DR and DA generations #4911

vladstepanyuk · 2025-12-25T18:31:49Z

#4293
To avoid bugs, when the Disk Registry thinks that the Disk Agent has attached Path, but because of a race condition, the Path is detached, we should handle all attach or detach requests with some generation number. We can keep a counter in RAM for all Disk Agents lets call it Disk Agent Generation, and increment it with each CMS action like AddHost/AddDevice or RemoveDevice/RemoveHost. When attaching or closing Path via TEvAttachPathRequest/TEvDetachPathRequest, or as a result of registration, we should pass Disk Agent and Disk Registry tablet generations. A Disk Agent Generation needs to order attach and detach requests sent in one DR generation. In this way, we can order all attach and detach requests and reject outdated ones.

drbasic · 2025-12-26T06:23:43Z

cloud/blockstore/libs/storage/disk_agent/disk_agent_state.cpp

+    ui64 diskAgentGeneration)
+{
+    if (diskRegistryGeneration < DiskRegistryGeneration) {
+        return MakeError(E_ARGUMENT, "outdated disk registry generation");


это не ретрабельная ошибка. Что должен делать клиент когда ее получит?

Клиент - это таблетка DiskRegistry, так что, если появился DiskRegistry с большим поколением, то мы можем добить эту таблетку или хотя бы перестать слать запросы, которые в любом случае будут проигнорированы.

cloud/blockstore/libs/storage/disk_agent/testlib/test_env.h

cloud/blockstore/libs/storage/disk_agent/disk_agent_actor.h

drbasic · 2025-12-26T06:32:08Z

cloud/blockstore/libs/storage/disk_agent/disk_agent_actor_attach_detach_path.cpp

+    }
+
+    // Filter from unknown paths.
+    auto unknownPaths = std::ranges::partition(


может имеется ввиду новые пути?

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

drbasic · 2025-12-26T06:35:41Z

cloud/blockstore/libs/storage/disk_agent/disk_agent_actor_attach_detach_path.cpp

+        pathsToPerformAttachDetachRange.begin(),
+        pathsToPerformAttachDetachRange.end());
+
+    if (auto error = State->CheckAttachDetachPathsRequestGeneration(


вот это условие нет смысла проверить в самом начале?

Ну до него только две проверки: на то, что фича включена, и на то, что нет уже исполняющегося запроса.
Функция CheckAttachDetachPathsRequestGeneration не только проверяет поколения, но и перещелкивает их, поэтому, кажется, будет нехорошо перещелкивать поколение, пока исполняется какой-то запрос или если фича вообще выключена.

cloud/blockstore/libs/storage/disk_agent/disk_agent_actor_attach_detach_path.cpp

github-actions · 2025-12-26T07:49:24Z

Note

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

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

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED^?
6082	6081	0	0	0	1	0

github-actions · 2025-12-26T14:48:48Z

Note

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

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

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED^?
6082	6081	0	0	0	1	0

drbasic · 2025-12-29T06:47:40Z

cloud/blockstore/libs/storage/disk_agent/disk_agent_actor.h

+    };
+
+    TResultOrError<TCheckAttachDetachPathRequestResult>
+    CheckAttachDetachPathsRequest(


это все константные методы?

sharpeye · 2025-12-29T10:19:19Z

It'll be helpful to have a high-level description of what is going on in this PR, not just a reference to an issue.

vladstepanyuk · 2025-12-29T12:24:09Z

It'll be helpful to have a high-level description of what is going on in this PR, not just a reference to an issue.

added description

drbasic · 2025-12-29T16:29:18Z

cloud/blockstore/libs/storage/disk_agent/disk_agent_private.h

        TVector<TString> PathsToAttach;
        TVector<TString> AlreadyAttachedPaths;
+
+        ui64 DiskRegistryGeneration;


инициализировать 0 ?

drbasic · 2025-12-29T16:29:26Z

cloud/blockstore/libs/storage/disk_agent/disk_agent_private.h

        TVector<TString> PathsToDetach;
+        TVector<TString> AlreadyDetachedPaths;
+
+        ui64 DiskRegistryGeneration;


drbasic · 2025-12-29T16:30:01Z

cloud/blockstore/libs/storage/disk_agent/disk_agent_actor.h

+        TVector<TString> paths,
+        EAction action);
+
+    TResultOrError<TCheckAttachDetachPathRequestResult> CheckAttachPathsRequest(


а зачем три метода, одного не достаточно?

drbasic · 2025-12-29T16:30:48Z

cloud/blockstore/libs/storage/disk_agent/disk_agent_actor_attach_detach_path.cpp

+    }
+
+    return TCheckAttachDetachPathRequestResult{
+        .AlreadyInWantedStatePaths = alreadyAttachedPaths,


std::move(alreadyAttachedPaths) ?

drbasic · 2025-12-29T16:32:10Z

cloud/blockstore/libs/storage/disk_agent/disk_agent_actor_attach_detach_path.cpp

-        [actorSystem, daId, pathsToDetach = std::move(pathsToDetach)](
-            auto) mutable
+        [actorSystem,
+         daId,


selfId = ctx.SelfID;

drbasic · 2025-12-29T16:32:52Z

cloud/blockstore/libs/storage/disk_agent/disk_agent_actor_attach_detach_path.cpp

+         pathsToDetach = std::move(pathsToDetach),
+         alreadyDetachedPaths = std::move(alreadyDetachedPaths),
+         diskRegistryGeneration = record.GetDiskRegistryGeneration(),
+         diskAgentGeneration = record.GetDiskAgentGeneration()](auto) mutable


параметр не важен?

drbasic · 2025-12-29T16:36:24Z

cloud/blockstore/libs/storage/disk_agent/disk_agent_actor_ut.cpp

+            TRequest request;
+            request.DiskAgentGeneration = GenerateDiskAgentGeneration();
+
+            auto devicesInRequest =


ui64 писать так же сложно как auto, но читать в два раза понятнее

drbasic · 2025-12-29T16:37:21Z

cloud/blockstore/libs/storage/disk_agent/disk_agent_actor_ut.cpp

    }
-}

+    Y_UNIT_TEST_F(ShouldRejectOldattachDetachRequests, TFixture)


sharpeye · 2025-12-29T10:44:33Z

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

+    uint64 DiskAgentGeneration = 3;
+
+    // Disk Registry Tablet generation needed to order attach/detach path
+    // requests sent from two different DR generations.
+    uint32 DiskRegistryGeneration = 4;


В THeader есть RequestId и RequestGeneration - не подойдут? https://github.com/ydb-platform/nbs/blob/main/cloud/blockstore/public/api/protos/headers.proto#L46-L52

sharpeye · 2026-01-09T14:29:13Z

cloud/blockstore/libs/storage/disk_agent/disk_agent_actor_ut.cpp

+            FindProcessesWithOpenFile(Devices[0]).size());
+    }
+
+    Y_UNIT_TEST_F(AttachDetachPathStressTest, TFixture)


StressTest - это что-то про другой тип теста, не UT

sharpeye · 2026-01-09T14:31:25Z

cloud/blockstore/libs/storage/disk_agent/disk_agent_actor_ut.cpp

+        Runtime->DispatchEvents(TDispatchOptions(), TDuration::Seconds(1));
+
+        TVector<TString> paths;
+        for (const auto& fPath: PartLabels) {


sharpeye · 2026-01-09T14:33:36Z

cloud/blockstore/libs/storage/disk_agent/disk_agent_actor_attach_detach_path.cpp

+    if (auto error = State->CheckAttachDetachPathsRequestGeneration(
+            diskRegistryGeneration,
+            diskAgentGeneration);
+        HasError(error) && pathsToPerformAttachDetach)


почему pathsToPerformAttachDetach влияет на ошибку?

sharpeye · 2026-01-09T14:55:18Z

cloud/blockstore/libs/storage/disk_agent/disk_agent_state.h

+    ui64 DiskRegistryGeneration = 0;
+    ui64 DiskAgentGeneration = 0;


Это лучше унести в актор

sharpeye · 2026-01-09T16:24:33Z

cloud/blockstore/libs/storage/disk_agent/disk_agent_actor_attach_detach_path.cpp

+auto TDiskAgentActor::CheckAttachDetachPathsRequest(
+    ui64 diskRegistryGeneration,
+    ui64 diskAgentGeneration,
+    TVector<TString> paths,
+    EAction action) -> TResultOrError<TCheckAttachDetachPathRequestResult>


Это можно распилить на части: (1) проверка, что фичка включена, (2) обновление поколения запроса (3) распиливание paths на две части ([attached, detached)

2176242

issue-4293: order all attach detach requests with dr and da generations

b0c9c33

vladstepanyuk requested review from drbasic and sharpeye December 25, 2025 18:31

vladstepanyuk changed the title ~~issue-4293: order all attach detach requests with dr and da generations~~ issue-4293: order all attach detach requests with DR and DA generations Dec 25, 2025

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

issue-4293: added proto fields

87b8546

drbasic reviewed Dec 26, 2025

View reviewed changes

cloud/blockstore/libs/storage/disk_agent/testlib/test_env.h Outdated Show resolved Hide resolved

drbasic reviewed Dec 26, 2025

View reviewed changes

cloud/blockstore/libs/storage/disk_agent/disk_agent_actor.h Outdated Show resolved Hide resolved

drbasic reviewed Dec 26, 2025

View reviewed changes

cloud/blockstore/libs/storage/disk_agent/disk_agent_actor_attach_detach_path.cpp Outdated Show resolved Hide resolved

vladstepanyuk added 2 commits December 26, 2025 10:08

issue-4293: corrected issues

828e6af

issue-4293: fmt

80b9fa4

vladstepanyuk requested a review from drbasic December 26, 2025 10:13

issue-4293: fix build

a959b89

drbasic reviewed Dec 29, 2025

View reviewed changes

sharpeye reviewed Jan 9, 2026

View reviewed changes

		ui64 DiskRegistryGeneration = 0;
		ui64 DiskAgentGeneration = 0;

issue-4293: order all attach detach requests with DR and DA generations #4911

Are you sure you want to change the base?

issue-4293: order all attach detach requests with DR and DA generations #4911

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vladstepanyuk Dec 26, 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!

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!

Uh oh!

github-actions bot commented Dec 26, 2025

Uh oh!

github-actions bot commented Dec 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharpeye commented Dec 29, 2025

Uh oh!

vladstepanyuk commented Dec 29, 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!

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!

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 Dec 25, 2025 •

edited

Loading

vladstepanyuk Dec 26, 2025 •

edited

Loading