From 6574310571e17851682055c22b3c58346e6bfecd Mon Sep 17 00:00:00 2001 From: Anton Myagkov Date: Wed, 10 Jul 2024 11:49:52 +0200 Subject: [PATCH] issue 1394: optimize cleanup devices (#1476) Issue: #1394 1. Pass all devices as an argument for MarkDevicesAsClean 2. Update each agent only once. TryUpdateDevices iterates via devices and save agents in the map to call UpdateAgent and DeviceList.UpdateDevices for each agent only once. --- .../disk_registry_actor_secure_erase.cpp | 11 +- .../disk_registry/disk_registry_state.cpp | 67 +++++++--- .../disk_registry/disk_registry_state.h | 25 +++- .../disk_registry/disk_registry_state_ut.cpp | 125 +++++++++++++++--- .../disk_registry_state_ut_agents_info.cpp | 2 +- .../disk_registry_state_ut_create.cpp | 11 +- .../disk_registry_state_ut_migration.cpp | 20 +-- .../disk_registry/testlib/test_state.cpp | 13 +- .../disk_registry/testlib/test_state.h | 2 +- 9 files changed, 208 insertions(+), 68 deletions(-) diff --git a/cloud/blockstore/libs/storage/disk_registry/disk_registry_actor_secure_erase.cpp b/cloud/blockstore/libs/storage/disk_registry/disk_registry_actor_secure_erase.cpp index 229c6d5628f..48beadfc001 100644 --- a/cloud/blockstore/libs/storage/disk_registry/disk_registry_actor_secure_erase.cpp +++ b/cloud/blockstore/libs/storage/disk_registry/disk_registry_actor_secure_erase.cpp @@ -278,16 +278,9 @@ void TDiskRegistryActor::ExecuteCleanupDevices( TTransactionContext& tx, TTxDiskRegistry::TCleanupDevices& args) { - Y_UNUSED(ctx); - TDiskRegistryDatabase db(tx.DB); - - for (const auto& uuid: args.Devices) { - auto diskId = State->MarkDeviceAsClean(ctx.Now(), db, uuid); - if (diskId) { - args.SyncDeallocatedDisks.push_back(std::move(diskId)); - } - } + args.SyncDeallocatedDisks = + State->MarkDevicesAsClean(ctx.Now(), db, args.Devices); } void TDiskRegistryActor::CompleteCleanupDevices( diff --git a/cloud/blockstore/libs/storage/disk_registry/disk_registry_state.cpp b/cloud/blockstore/libs/storage/disk_registry/disk_registry_state.cpp index ffa56cae87b..3611a7fc997 100644 --- a/cloud/blockstore/libs/storage/disk_registry/disk_registry_state.cpp +++ b/cloud/blockstore/libs/storage/disk_registry/disk_registry_state.cpp @@ -3601,21 +3601,37 @@ bool TDiskRegistryState::MarkDeviceAsDirty( return true; } -TString TDiskRegistryState::MarkDeviceAsClean( +TDiskRegistryState::TDiskId TDiskRegistryState::MarkDeviceAsClean( TInstant now, TDiskRegistryDatabase& db, const TDeviceId& uuid) { - DeviceList.MarkDeviceAsClean(uuid); - db.DeleteDirtyDevice(uuid); + auto ret = MarkDevicesAsClean(now, db, TVector{uuid}); + return ret.empty() ? "" : ret[0]; +} - if (!DeviceList.IsSuspendedDevice(uuid)) { - db.DeleteSuspendedDevice(uuid); +TVector TDiskRegistryState::MarkDevicesAsClean( + TInstant now, + TDiskRegistryDatabase& db, + const TVector& uuids) +{ + for (const auto& uuid: uuids) { + DeviceList.MarkDeviceAsClean(uuid); + db.DeleteDirtyDevice(uuid); + + if (!DeviceList.IsSuspendedDevice(uuid)) { + db.DeleteSuspendedDevice(uuid); + } } - TryUpdateDevice(now, db, uuid); + TVector ret; + for (const auto& uuid: TryUpdateDevices(now, db, uuids)) { + if (auto diskId = PendingCleanup.EraseDevice(uuid); !diskId.empty()) { + ret.push_back(std::move(diskId)); + } + } - return PendingCleanup.EraseDevice(uuid); + return ret; } bool TDiskRegistryState::TryUpdateDevice( @@ -3623,19 +3639,38 @@ bool TDiskRegistryState::TryUpdateDevice( TDiskRegistryDatabase& db, const TDeviceId& uuid) { - Y_UNUSED(now); + return !TryUpdateDevices(now, db, {uuid}).empty(); +} - auto [agent, device] = FindDeviceLocation(uuid); - if (!agent || !device) { - return false; - } +TVector TDiskRegistryState::TryUpdateDevices( + TInstant now, + TDiskRegistryDatabase& db, + const TVector& uuids) +{ + TVector ret; + ret.reserve(uuids.size()); - AdjustDeviceIfNeeded(*device, {}); + TSet agentsMap; + for (const auto& uuid: uuids) { + auto [agent, device] = FindDeviceLocation(uuid); + if (!agent || !device) { + continue; + } + ret.push_back(uuid); + agentsMap.emplace(agent->agentid()); + AdjustDeviceIfNeeded(*device, now); + } - UpdateAgent(db, *agent); - DeviceList.UpdateDevices(*agent, DevicePoolConfigs); + for (const auto& agentId: agentsMap) { + auto* agent = AgentList.FindAgent(agentId); + if (!agent) { + continue; + } + UpdateAgent(db, *agent); + DeviceList.UpdateDevices(*agent, DevicePoolConfigs); + } - return true; + return ret; } TVector TDiskRegistryState::CollectBrokenDevices( diff --git a/cloud/blockstore/libs/storage/disk_registry/disk_registry_state.h b/cloud/blockstore/libs/storage/disk_registry/disk_registry_state.h index 88f9b4a951f..8386f958d70 100644 --- a/cloud/blockstore/libs/storage/disk_registry/disk_registry_state.h +++ b/cloud/blockstore/libs/storage/disk_registry/disk_registry_state.h @@ -493,10 +493,22 @@ class TDiskRegistryState TVector GetBrokenDevices() const; TVector GetDirtyDevices() const; - TString MarkDeviceAsClean( + + /// Mark selected device as clean and remove it + /// from lists of suspended/dirty/pending cleanup devices + /// @return disk id where selected device was allocated + TDiskId MarkDeviceAsClean( TInstant now, TDiskRegistryDatabase& db, const TDeviceId& uuid); + + /// Mark selected devices as clean and remove them + /// from lists of suspended/dirty/pending cleanup devices + /// @return vector of disk ids where selected devices were allocated + TVector MarkDevicesAsClean( + TInstant now, + TDiskRegistryDatabase& db, + const TVector& uuids); bool MarkDeviceAsDirty(TDiskRegistryDatabase& db, const TDeviceId& uuid); NProto::TError CreatePlacementGroup( @@ -1123,11 +1135,22 @@ class TDiskRegistryState TDiskRegistryDatabase& db, const TString& diskId); + /// Try to update configuration of selected device and its agent + /// in the disk registry database + /// @return true if the device updates successfully; otherwise, return false bool TryUpdateDevice( TInstant now, TDiskRegistryDatabase& db, const TDeviceId& uuid); + /// Try to update configuration of selected devices and their agents + /// in the disk registry database + /// @return List of updated devices + TVector TryUpdateDevices( + TInstant now, + TDiskRegistryDatabase& db, + const TVector& uuids); + TDeviceList::TAllocationQuery MakeMigrationQuery( const TDiskId& sourceDiskId, const NProto::TDeviceConfig& sourceDevice); diff --git a/cloud/blockstore/libs/storage/disk_registry/disk_registry_state_ut.cpp b/cloud/blockstore/libs/storage/disk_registry/disk_registry_state_ut.cpp index a1265d03510..536a498389c 100644 --- a/cloud/blockstore/libs/storage/disk_registry/disk_registry_state_ut.cpp +++ b/cloud/blockstore/libs/storage/disk_registry/disk_registry_state_ut.cpp @@ -1271,11 +1271,13 @@ Y_UNIT_TEST_SUITE(TDiskRegistryStateTest) Device("dev-3", "uuid-7", "rack-1") }); - TDiskRegistryState state = TDiskRegistryStateBuilder() - .WithKnownAgents({ agent1, agent2 }) - .WithDisks({ Disk("disk-1", {"uuid-1"}) }) - .WithDirtyDevices({ "uuid-4", "uuid-7" }) - .Build(); + TDiskRegistryState state = + TDiskRegistryStateBuilder() + .WithKnownAgents({agent1, agent2}) + .WithDisks({Disk("disk-1", {"uuid-1"})}) + .WithDirtyDevices( + {TDirtyDevice{"uuid-4", {}}, TDirtyDevice{"uuid-7", {}}}) + .Build(); executor.WriteTx([&] (TDiskRegistryDatabase db) { TVector devices; @@ -8292,18 +8294,29 @@ Y_UNIT_TEST_SUITE(TDiskRegistryStateTest) }), }; - TDiskRegistryState state = TDiskRegistryStateBuilder() - .WithKnownAgents(agents) - .WithDisks({ - Disk("disk-1", {"uuid-1.1"}), - Disk("disk-2", {"uuid-2.2"}), - Disk("disk-3", {"uuid-3.4", "uuid-3.5"}), - Disk("disk-4", {"uuid-4.1"}), - }) - .WithDirtyDevices({"uuid-2.1", "uuid-3.3", "uuid-3.6", "uuid-5.2"}) - .AddDevicePoolConfig("local-ssd", 10_GB, NProto::DEVICE_POOL_KIND_LOCAL) - .AddDevicePoolConfig("rot", 10_GB, NProto::DEVICE_POOL_KIND_GLOBAL) - .Build(); + TDiskRegistryState state = + TDiskRegistryStateBuilder() + .WithKnownAgents(agents) + .WithDisks({ + Disk("disk-1", {"uuid-1.1"}), + Disk("disk-2", {"uuid-2.2"}), + Disk("disk-3", {"uuid-3.4", "uuid-3.5"}), + Disk("disk-4", {"uuid-4.1"}), + }) + .WithDirtyDevices( + {TDirtyDevice{"uuid-2.1", {}}, + TDirtyDevice{"uuid-3.3", {}}, + TDirtyDevice{"uuid-3.6", {}}, + TDirtyDevice{"uuid-5.2", {}}}) + .AddDevicePoolConfig( + "local-ssd", + 10_GB, + NProto::DEVICE_POOL_KIND_LOCAL) + .AddDevicePoolConfig( + "rot", + 10_GB, + NProto::DEVICE_POOL_KIND_GLOBAL) + .Build(); const auto poolNames = state.GetPoolNames(); ASSERT_VECTORS_EQUAL( @@ -11535,6 +11548,84 @@ Y_UNIT_TEST_SUITE(TDiskRegistryStateTest) UNIT_ASSERT_VALUES_EQUAL(errorMessage, d.GetStateMessage()); }); } + + Y_UNIT_TEST(ShouldCleanMultipleDevicesFromOneDisk) + { + TTestExecutor executor; + executor.WriteTx([&](TDiskRegistryDatabase db) { db.InitSchema(); }); + + const auto agent1 = AgentConfig( + 1, + {Device("dev-1", "uuid-1", "rack-1")}); + + const auto agent2 = AgentConfig( + 2, + {Device("dev-2", "uuid-2", "rack-1")}); + + TDiskRegistryState state = + TDiskRegistryStateBuilder() + .WithKnownAgents({agent1, agent2}) + .WithDisks({Disk("disk-1", {"uuid-1", "uuid-2"})}) + .WithDirtyDevices( + {TDirtyDevice{"uuid-1", "disk-1"}, + TDirtyDevice{"uuid-2", "disk-1"}}) + .Build(); + + UNIT_ASSERT_EQUAL(state.GetDirtyDevices().size(), 2); + + executor.WriteTx( + [&](TDiskRegistryDatabase db) + { + const auto& cleanDisks = state.MarkDevicesAsClean( + Now(), + db, + TVector{"uuid-1", "uuid-2"}); + UNIT_ASSERT_EQUAL(cleanDisks.size(), 1); + UNIT_ASSERT_EQUAL(cleanDisks[0], "disk-1"); + }); + + UNIT_ASSERT(state.GetDirtyDevices().empty()); + } + + Y_UNIT_TEST(ShouldCleanMultipleDevicesFromDifferentDisks) + { + TTestExecutor executor; + executor.WriteTx([&](TDiskRegistryDatabase db) { db.InitSchema(); }); + + const auto agent1 = AgentConfig( + 1, + {Device("dev-1", "uuid-1", "rack-1")}); + + const auto agent2 = AgentConfig( + 2, + {Device("dev-2", "uuid-2", "rack-1")}); + + TDiskRegistryState state = + TDiskRegistryStateBuilder() + .WithKnownAgents({agent1, agent2}) + .WithDisks( + {Disk("disk-1", {"uuid-1"}), Disk("disk-1", {"uuid-2"})}) + .WithDirtyDevices( + {TDirtyDevice{"uuid-1", "disk-1"}, + TDirtyDevice{"uuid-2", "disk-2"}}) + .Build(); + + UNIT_ASSERT_EQUAL(state.GetDirtyDevices().size(), 2); + + executor.WriteTx( + [&](TDiskRegistryDatabase db) + { + const auto& cleanDisks = state.MarkDevicesAsClean( + Now(), + db, + TVector{"uuid-1", "uuid-2"}); + UNIT_ASSERT_EQUAL(cleanDisks.size(), 2); + UNIT_ASSERT_EQUAL(cleanDisks[0], "disk-1"); + UNIT_ASSERT_EQUAL(cleanDisks[1], "disk-2"); + }); + + UNIT_ASSERT(state.GetDirtyDevices().empty()); + } } } // namespace NCloud::NBlockStore::NStorage diff --git a/cloud/blockstore/libs/storage/disk_registry/disk_registry_state_ut_agents_info.cpp b/cloud/blockstore/libs/storage/disk_registry/disk_registry_state_ut_agents_info.cpp index 0c0a7b7db57..e0651ab5464 100644 --- a/cloud/blockstore/libs/storage/disk_registry/disk_registry_state_ut_agents_info.cpp +++ b/cloud/blockstore/libs/storage/disk_registry/disk_registry_state_ut_agents_info.cpp @@ -110,7 +110,7 @@ Y_UNIT_TEST_SUITE(TDiskRegistryStateQueryAgentsInfoTest) return config; }()) .WithAgents(agents) - .WithDirtyDevices({"uuid-1.1"}) + .WithDirtyDevices({TDirtyDevice{"uuid-1.1", {}}}) .Build(); auto agentsInfo = state.QueryAgentsInfo(); diff --git a/cloud/blockstore/libs/storage/disk_registry/disk_registry_state_ut_create.cpp b/cloud/blockstore/libs/storage/disk_registry/disk_registry_state_ut_create.cpp index dece6807d36..87db2992bae 100644 --- a/cloud/blockstore/libs/storage/disk_registry/disk_registry_state_ut_create.cpp +++ b/cloud/blockstore/libs/storage/disk_registry/disk_registry_state_ut_create.cpp @@ -59,11 +59,12 @@ Y_UNIT_TEST_SUITE(TDiskRegistryStateCreateTest) }) }; - TDiskRegistryState state = TDiskRegistryStateBuilder() - .WithKnownAgents(agents) - .WithDisks({ Disk("disk-1", {"uuid-1.1"}) }) - .WithDirtyDevices({"uuid-2.1"}) - .Build(); + TDiskRegistryState state = + TDiskRegistryStateBuilder() + .WithKnownAgents(agents) + .WithDisks({Disk("disk-1", {"uuid-1.1"})}) + .WithDirtyDevices({TDirtyDevice{"uuid-2.1", {}}}) + .Build(); auto deviceByName = [] (auto agentId, auto name) { NProto::TDeviceConfig config; diff --git a/cloud/blockstore/libs/storage/disk_registry/disk_registry_state_ut_migration.cpp b/cloud/blockstore/libs/storage/disk_registry/disk_registry_state_ut_migration.cpp index b5ac3c6c9af..1939f31d670 100644 --- a/cloud/blockstore/libs/storage/disk_registry/disk_registry_state_ut_migration.cpp +++ b/cloud/blockstore/libs/storage/disk_registry/disk_registry_state_ut_migration.cpp @@ -66,14 +66,18 @@ Y_UNIT_TEST_SUITE(TDiskRegistryStateMigrationTest) }) }; - TDiskRegistryState state = TDiskRegistryStateBuilder() - .WithKnownAgents(agents) - .WithDisks({ - Disk("foo", { "uuid-1.1", "uuid-1.2" }), // rack-1 - Disk("bar", { "uuid-2.1" }) // rack-2 - }) - .WithDirtyDevices({"uuid-4.1", "uuid-4.2", "uuid-4.3"}) - .Build(); + TDiskRegistryState state = + TDiskRegistryStateBuilder() + .WithKnownAgents(agents) + .WithDisks({ + Disk("foo", {"uuid-1.1", "uuid-1.2"}), // rack-1 + Disk("bar", {"uuid-2.1"}) // rack-2 + }) + .WithDirtyDevices( + {TDirtyDevice{"uuid-4.1", {}}, + TDirtyDevice{"uuid-4.2", {}}, + TDirtyDevice{"uuid-4.3", {}}}) + .Build(); UNIT_ASSERT(state.IsMigrationListEmpty()); diff --git a/cloud/blockstore/libs/storage/disk_registry/testlib/test_state.cpp b/cloud/blockstore/libs/storage/disk_registry/testlib/test_state.cpp index 65a37d4ab42..ff8215b53c6 100644 --- a/cloud/blockstore/libs/storage/disk_registry/testlib/test_state.cpp +++ b/cloud/blockstore/libs/storage/disk_registry/testlib/test_state.cpp @@ -561,16 +561,9 @@ TDiskRegistryStateBuilder& TDiskRegistryStateBuilder::WithDisks( } TDiskRegistryStateBuilder& TDiskRegistryStateBuilder::WithDirtyDevices( - TVector dirtyDevices) -{ - DirtyDevices.clear(); - DirtyDevices.reserve(dirtyDevices.size()); - for (auto& s: dirtyDevices) { - DirtyDevices.emplace_back(TDirtyDevice { - .Id = std::move(s) - }); - } - + TVector dirtyDevices) +{ + DirtyDevices = std::move(dirtyDevices); return *this; } diff --git a/cloud/blockstore/libs/storage/disk_registry/testlib/test_state.h b/cloud/blockstore/libs/storage/disk_registry/testlib/test_state.h index dceef794121..df8391064b7 100644 --- a/cloud/blockstore/libs/storage/disk_registry/testlib/test_state.h +++ b/cloud/blockstore/libs/storage/disk_registry/testlib/test_state.h @@ -276,7 +276,7 @@ struct TDiskRegistryStateBuilder TDiskRegistryStateBuilder& WithDisks(TVector disks); - TDiskRegistryStateBuilder& WithDirtyDevices(TVector dirtyDevices); + TDiskRegistryStateBuilder& WithDirtyDevices(TVector dirtyDevices); TDiskRegistryStateBuilder& WithSuspendedDevices( TVector suspendedDevices);