From b698180a380ccf8946755587155deacbebc19f36 Mon Sep 17 00:00:00 2001 From: sharpeye Date: Mon, 23 Sep 2024 15:18:43 +0000 Subject: [PATCH] issue-2076: fix affected disk tracking --- .../disk_registry/disk_registry_state.cpp | 24 +- .../disk_registry_state_ut_mirrored_disks.cpp | 249 +++++++++++++++++- 2 files changed, 257 insertions(+), 16 deletions(-) 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 57e02bb1c55..4a799ebf40b 100644 --- a/cloud/blockstore/libs/storage/disk_registry/disk_registry_state.cpp +++ b/cloud/blockstore/libs/storage/disk_registry/disk_registry_state.cpp @@ -1208,7 +1208,7 @@ NProto::TError TDiskRegistryState::ReplaceDevice( timestamp, message); if (HasError(error)) { - TryUpdateDiskState(db, diskId, timestamp); + *diskStateUpdated = TryUpdateDiskState(db, diskId, timestamp); return error; } @@ -4749,7 +4749,7 @@ void TDiskRegistryState::ApplyAgentStateChange( UpdateAgent(db, agent); DeviceList.UpdateDevices(agent, DevicePoolConfigs); - THashSet diskIds; + THashMap maybeAffectedDisks; for (const auto& d: agent.GetDevices()) { const auto& deviceId = d.GetDeviceUUID(); @@ -4761,13 +4761,13 @@ void TDiskRegistryState::ApplyAgentStateChange( auto& disk = Disks[diskId]; + maybeAffectedDisks.emplace(diskId, disk.State); + // check if deviceId is target for migration if (RestartDeviceMigration(timestamp, db, diskId, disk, deviceId)) { continue; } - bool isAffected = true; - if (agent.GetState() == NProto::AGENT_STATE_WARNING) { if (disk.MigrationSource2Target.contains(deviceId)) { // migration already started @@ -4818,24 +4818,18 @@ void TDiskRegistryState::ApplyAgentStateChange( ReportMirroredDiskDeviceReplacementFailure( FormatError(error)); } - - if (!updated) { - isAffected = false; - } } } CancelDeviceMigration(timestamp, db, diskId, disk, deviceId); } - - if (isAffected) { - diskIds.emplace(std::move(diskId)); - } } - for (auto& id: diskIds) { - if (TryUpdateDiskState(db, id, timestamp)) { - affectedDisks.push_back(std::move(id)); + for (auto& [diskId, oldState]: maybeAffectedDisks) { + auto& disk = Disks[diskId]; + TryUpdateDiskState(db, diskId, disk, timestamp); + if (oldState != disk.State) { + affectedDisks.push_back(diskId); } } } diff --git a/cloud/blockstore/libs/storage/disk_registry/disk_registry_state_ut_mirrored_disks.cpp b/cloud/blockstore/libs/storage/disk_registry/disk_registry_state_ut_mirrored_disks.cpp index 3c44fdf2224..8175a71fa84 100644 --- a/cloud/blockstore/libs/storage/disk_registry/disk_registry_state_ut_mirrored_disks.cpp +++ b/cloud/blockstore/libs/storage/disk_registry/disk_registry_state_ut_mirrored_disks.cpp @@ -1806,6 +1806,129 @@ Y_UNIT_TEST_SUITE(TDiskRegistryStateMirroredDisksTest) rt); } + Y_UNIT_TEST(ShouldReturnAffectedDiskListOnAgentFailure) + { + TTestExecutor executor; + executor.WriteTx([&] (TDiskRegistryDatabase db) { + db.InitSchema(); + }); + + auto agentConfig1 = AgentConfig(1, { + Device("dev-1", "uuid-1", "rack-1"), + Device("dev-2", "uuid-2", "rack-1"), + }); + + auto agentConfig2 = AgentConfig(2, { + Device("dev-1", "uuid-3", "rack-2"), + Device("dev-2", "uuid-4", "rack-2"), + }); + + auto agentConfig3 = AgentConfig(3, { + Device("dev-1", "uuid-5", "rack-3"), + }); + + TDiskRegistryState state = TDiskRegistryStateBuilder() + .WithKnownAgents({ + agentConfig1, + agentConfig2, + agentConfig3 + }) + .Build(); + + // Create a mirror-2 disk + executor.WriteTx([&] (TDiskRegistryDatabase db) { + TVector devices; + TVector> replicas; + TVector migrations; + TVector deviceReplacementIds; + auto error = AllocateMirroredDisk( + db, + state, + "disk-1", + 20_GB, + 1, + devices, + replicas, + migrations, + deviceReplacementIds); + UNIT_ASSERT_SUCCESS(error); + UNIT_ASSERT_VALUES_EQUAL(2, devices.size()); + UNIT_ASSERT_VALUES_EQUAL("uuid-1", devices[0].GetDeviceUUID()); + UNIT_ASSERT_VALUES_EQUAL("uuid-2", devices[1].GetDeviceUUID()); + UNIT_ASSERT_VALUES_EQUAL(1, replicas.size()); + UNIT_ASSERT_VALUES_EQUAL(2, replicas[0].size()); + UNIT_ASSERT_VALUES_EQUAL("uuid-3", replicas[0][0].GetDeviceUUID()); + UNIT_ASSERT_VALUES_EQUAL("uuid-4", replicas[0][1].GetDeviceUUID()); + ASSERT_VECTORS_EQUAL(TVector{}, deviceReplacementIds); + }); + + // All replicas must be online + + { + TDiskInfo replicaInfo; + UNIT_ASSERT_SUCCESS(state.GetDiskInfo("disk-1/0", replicaInfo)); + UNIT_ASSERT_VALUES_EQUAL( + NProto::EDiskState_Name(NProto::DISK_STATE_ONLINE), + NProto::EDiskState_Name(replicaInfo.State)); + } + + { + TDiskInfo replicaInfo; + UNIT_ASSERT_SUCCESS(state.GetDiskInfo("disk-1/1", replicaInfo)); + UNIT_ASSERT_VALUES_EQUAL( + NProto::EDiskState_Name(NProto::DISK_STATE_ONLINE), + NProto::EDiskState_Name(replicaInfo.State)); + } + + // Breaking the first agent + + executor.WriteTx([&] (TDiskRegistryDatabase db) mutable { + TVector affectedDisks; + TDuration timeout; + auto error = state.UpdateAgentState( + db, + agentConfig1.GetAgentId(), + NProto::AGENT_STATE_UNAVAILABLE, + Now(), + "unreachable", + affectedDisks); + + // Now first replica (disk-1/0) should be broken + + TDiskInfo replicaInfo; + UNIT_ASSERT_SUCCESS(state.GetDiskInfo("disk-1/0", replicaInfo)); + UNIT_ASSERT_VALUES_EQUAL( + NProto::EDiskState_Name(NProto::DISK_STATE_TEMPORARILY_UNAVAILABLE), + NProto::EDiskState_Name(replicaInfo.State)); + + // And second one (disk-1/1) should be OK + + replicaInfo = {}; + UNIT_ASSERT_SUCCESS(state.GetDiskInfo("disk-1/1", replicaInfo)); + UNIT_ASSERT_VALUES_EQUAL( + NProto::EDiskState_Name(NProto::DISK_STATE_ONLINE), + NProto::EDiskState_Name(replicaInfo.State)); + + UNIT_ASSERT_VALUES_EQUAL(S_OK, error.GetCode()); + UNIT_ASSERT_VALUES_EQUAL(TDuration::Zero(), timeout); + + // The state of 'disk-1/0' has been changed so 'disk-1/0' should be + // in 'affectedDisks' + ASSERT_VECTORS_EQUAL(TVector{"disk-1/0"}, affectedDisks); + + auto replaced = state.GetAutomaticallyReplacedDevices(); + UNIT_ASSERT_VALUES_EQUAL(1, replaced.size()); + UNIT_ASSERT_VALUES_EQUAL("uuid-1", replaced[0].DeviceId); + + UNIT_ASSERT_SUCCESS(state.MarkReplacementDevice( + Now(), + db, + "disk-1", + "uuid-5", + false)); + }); + } + Y_UNIT_TEST(ShouldProperlyCleanupAutomaticallyReplacedDevices) { TTestExecutor executor; @@ -2400,6 +2523,13 @@ Y_UNIT_TEST_SUITE(TDiskRegistryStateMirroredDisksTest) const auto changeStateTs = Now(); executor.WriteTx([&] (TDiskRegistryDatabase db) mutable { + TDiskInfo diskInfo; + UNIT_ASSERT_SUCCESS(state.GetDiskInfo("disk-1/0", diskInfo)); + UNIT_ASSERT_VALUES_EQUAL( + NProto::EDiskState_Name( + NProto::DISK_STATE_ONLINE), + NProto::EDiskState_Name(diskInfo.State)); + TVector affectedDisks; TDuration timeout; auto error = state.UpdateAgentState( @@ -2412,7 +2542,14 @@ Y_UNIT_TEST_SUITE(TDiskRegistryStateMirroredDisksTest) UNIT_ASSERT_VALUES_EQUAL(S_OK, error.GetCode()); UNIT_ASSERT_VALUES_EQUAL(TDuration::Zero(), timeout); - UNIT_ASSERT_VALUES_EQUAL(0, affectedDisks.size()); + ASSERT_VECTORS_EQUAL(TVector{"disk-1/0"}, affectedDisks); + + diskInfo = {}; + UNIT_ASSERT_SUCCESS(state.GetDiskInfo("disk-1/0", diskInfo)); + UNIT_ASSERT_VALUES_EQUAL( + NProto::EDiskState_Name( + NProto::DISK_STATE_TEMPORARILY_UNAVAILABLE), + NProto::EDiskState_Name(diskInfo.State)); }); UNIT_ASSERT_VALUES_EQUAL(1, criticalEvents->Val()); @@ -2482,6 +2619,116 @@ Y_UNIT_TEST_SUITE(TDiskRegistryStateMirroredDisksTest) }); } + Y_UNIT_TEST(ShouldReportDeviceReplacementFailureUponDeviceFailure) + { + TTestExecutor executor; + executor.WriteTx([&] (TDiskRegistryDatabase db) { + db.InitSchema(); + }); + + auto agentConfig1 = AgentConfig(1, { + Device("dev-1", "uuid-1", "rack-1"), + }); + + auto agentConfig2 = AgentConfig(2, { + Device("dev-1", "uuid-2", "rack-2"), + }); + + auto agentConfig3 = AgentConfig(3, { + Device("dev-1", "uuid-3", "rack-3"), + }); + + TDiskRegistryState state = TDiskRegistryStateBuilder() + .WithKnownAgents({ + agentConfig1, + agentConfig2, + agentConfig3, + }) + .Build(); + + // Creating a mirror-3 (one device per replica) + + executor.WriteTx([&] (TDiskRegistryDatabase db) { + TVector devices; + TVector> replicas; + TVector migrations; + TVector deviceReplacementIds; + auto error = AllocateMirroredDisk( + db, + state, + "disk-1", + 10_GB, + 2, + devices, + replicas, + migrations, + deviceReplacementIds); + UNIT_ASSERT_SUCCESS(error); + UNIT_ASSERT_VALUES_EQUAL(1, devices.size()); + UNIT_ASSERT_VALUES_EQUAL("uuid-1", devices[0].GetDeviceUUID()); + UNIT_ASSERT_VALUES_EQUAL(2, replicas.size()); + UNIT_ASSERT_VALUES_EQUAL(1, replicas[0].size()); + UNIT_ASSERT_VALUES_EQUAL("uuid-2", replicas[0][0].GetDeviceUUID()); + UNIT_ASSERT_VALUES_EQUAL(1, replicas[1].size()); + UNIT_ASSERT_VALUES_EQUAL("uuid-3", replicas[1][0].GetDeviceUUID()); + ASSERT_VECTORS_EQUAL(TVector{}, deviceReplacementIds); + }); + + auto monitoring = CreateMonitoringServiceStub(); + auto rootGroup = monitoring->GetCounters() + ->GetSubgroup("counters", "blockstore"); + + auto serverGroup = rootGroup->GetSubgroup("component", "server"); + InitCriticalEventsCounter(serverGroup); + + auto criticalEvents = serverGroup->FindCounter( + "AppCriticalEvents/MirroredDiskDeviceReplacementFailure"); + + UNIT_ASSERT_VALUES_EQUAL(0, criticalEvents->Val()); + + executor.WriteTx([&] (TDiskRegistryDatabase db) mutable { + + // A first replica should be OK + + TDiskInfo diskInfo; + UNIT_ASSERT_SUCCESS(state.GetDiskInfo("disk-1/0", diskInfo)); + UNIT_ASSERT_VALUES_EQUAL( + NProto::EDiskState_Name( + NProto::DISK_STATE_ONLINE), + NProto::EDiskState_Name(diskInfo.State)); + + // Breakin a device + + TString affectedDisk; + TDuration timeout; + auto error = state.UpdateDeviceState( + db, + "uuid-1", + NProto::DEVICE_STATE_ERROR, + Now(), + "test", + affectedDisk); + + // Now the first replca is not OK, its state has been changed to + // ERROR so we should see disk-1/0 in affectedDisk + + UNIT_ASSERT_VALUES_EQUAL(S_OK, error.GetCode()); + UNIT_ASSERT_VALUES_EQUAL(TDuration::Zero(), timeout); + UNIT_ASSERT_VALUES_EQUAL("disk-1/0", affectedDisk); + + // Ensuring that disk-1/0 is broken + + diskInfo = {}; + UNIT_ASSERT_SUCCESS(state.GetDiskInfo("disk-1/0", diskInfo)); + UNIT_ASSERT_VALUES_EQUAL( + NProto::EDiskState_Name( + NProto::DISK_STATE_ERROR), + NProto::EDiskState_Name(diskInfo.State)); + }); + + UNIT_ASSERT_VALUES_EQUAL(1, criticalEvents->Val()); + } + Y_UNIT_TEST(ShouldStopAutomaticReplacementIfReplacementRateIsTooHigh) { TTestExecutor executor;