From 309416849743a8a205dc1b4a07d83870be65d239 Mon Sep 17 00:00:00 2001 From: Kirill Pleshivtsev Date: Fri, 15 Mar 2024 12:11:03 +0700 Subject: [PATCH] NBS-4827 Fix review issues and tests flakiness --- cloud/blockstore/config/storage.proto | 2 +- .../migration_timeout_calculator.cpp | 5 ++--- .../storage/volume/actors/shadow_disk_actor.cpp | 14 ++++++-------- .../libs/storage/volume/actors/shadow_disk_actor.h | 13 ++++++------- .../libs/storage/volume/volume_ut_checkpoint.cpp | 12 +++++++++++- 5 files changed, 26 insertions(+), 20 deletions(-) diff --git a/cloud/blockstore/config/storage.proto b/cloud/blockstore/config/storage.proto index bf99ae6cbcd..0143188c39c 100644 --- a/cloud/blockstore/config/storage.proto +++ b/cloud/blockstore/config/storage.proto @@ -928,7 +928,7 @@ message TStorageServiceConfig optional string CachedDiskAgentSessionsPath = 351; // Max bandwidth used for shadow disk fill in MiB/s (actual bandwidth is x2 - // due to the need to read and write). + // due to the need to read and write). optional uint32 MaxShadowDiskFillBandwidth = 352; // Minimum delay between shadow disk acquire attempts when writes to the // source disk are blocked (in ms). diff --git a/cloud/blockstore/libs/storage/partition_nonrepl/migration_timeout_calculator.cpp b/cloud/blockstore/libs/storage/partition_nonrepl/migration_timeout_calculator.cpp index f66edaee701..2eb4cdaf8e2 100644 --- a/cloud/blockstore/libs/storage/partition_nonrepl/migration_timeout_calculator.cpp +++ b/cloud/blockstore/libs/storage/partition_nonrepl/migration_timeout_calculator.cpp @@ -19,9 +19,8 @@ TMigrationTimeoutCalculator::TMigrationTimeoutCalculator( TDuration TMigrationTimeoutCalculator::CalculateTimeout( TBlockRange64 nextProcessingRange) const { - // migration range is 4_MB - const auto migrationFactorPerAgent = MaxMigrationBandwidthMiBs / 4; + const auto migrationFactorPerAgent = MaxMigrationBandwidthMiBs / 4.0; if (PartitionConfig->GetUseSimpleMigrationBandwidthLimiter()) { return TDuration::Seconds(1) / migrationFactorPerAgent; @@ -41,7 +40,7 @@ TDuration TMigrationTimeoutCalculator::CalculateTimeout( const auto factor = Max(migrationFactorPerAgent * agentDeviceCount / ExpectedDiskAgentSize, - 1U); + 1.0); return TDuration::Seconds(1) / factor; } diff --git a/cloud/blockstore/libs/storage/volume/actors/shadow_disk_actor.cpp b/cloud/blockstore/libs/storage/volume/actors/shadow_disk_actor.cpp index 0a99a6e3df9..49b1898349f 100644 --- a/cloud/blockstore/libs/storage/volume/actors/shadow_disk_actor.cpp +++ b/cloud/blockstore/libs/storage/volume/actors/shadow_disk_actor.cpp @@ -96,7 +96,7 @@ class TAcquireShadowDiskActor const TDevices& shadowDiskDevices, TShadowDiskActor::EAcquireReason acquireReason, bool readOlyMount, - bool isWritesToSourceBlocked, + bool areWritesToSourceBlocked, ui64 mountSeqNumber, ui32 generation, TActorId parentActor); @@ -153,7 +153,7 @@ TAcquireShadowDiskActor::TAcquireShadowDiskActor( const TDevices& shadowDiskDevices, TShadowDiskActor::EAcquireReason acquireReason, bool readOnlyMount, - bool isWritesToSourceBlocked, + bool areWritesToSourceBlocked, ui64 mountSeqNumber, ui32 generation, TActorId parentActor) @@ -161,7 +161,7 @@ TAcquireShadowDiskActor::TAcquireShadowDiskActor( , AcquireReason(acquireReason) , ReadOnlyMount(readOnlyMount) , TotalTimeout( - isWritesToSourceBlocked + areWritesToSourceBlocked ? config->GetMaxAcquireShadowDiskTotalTimeoutWhenBlocked() : config->GetMaxAcquireShadowDiskTotalTimeoutWhenNonBlocked()) , ParentActor(parentActor) @@ -169,10 +169,10 @@ TAcquireShadowDiskActor::TAcquireShadowDiskActor( , Generation(generation) , ShadowDiskDevices(shadowDiskDevices) , RetryDelayProvider( - isWritesToSourceBlocked + areWritesToSourceBlocked ? config->GetMinAcquireShadowDiskRetryDelayWhenBlocked() : config->GetMinAcquireShadowDiskRetryDelayWhenNonBlocked(), - isWritesToSourceBlocked + areWritesToSourceBlocked ? config->GetMaxAcquireShadowDiskRetryDelayWhenBlocked() : config->GetMaxAcquireShadowDiskRetryDelayWhenNonBlocked()) { @@ -768,8 +768,6 @@ void TShadowDiskActor::CreateShadowDiskPartitionActor( PoisonPillHelper.TakeOwnership(ctx, DstActorId); if (State == EActorState::WaitAcquireForRead) { - STORAGE_CHECK_PRECONDITION(Acquired()); - // Ready to serve checkpoint reads. State = EActorState::CheckpointReady; } else { @@ -779,7 +777,6 @@ void TShadowDiskActor::CreateShadowDiskPartitionActor( // Ready to fill shadow disk with data. State = EActorState::Preparing; - STORAGE_CHECK_PRECONDITION(Acquired()); TNonreplicatedPartitionMigrationCommonActor::InitWork( ctx, @@ -798,6 +795,7 @@ void TShadowDiskActor::CreateShadowDiskPartitionActor( SrcConfig->GetBlockCount())); } + STORAGE_CHECK_PRECONDITION(Acquired()); SchedulePeriodicalReAcquire(ctx); } diff --git a/cloud/blockstore/libs/storage/volume/actors/shadow_disk_actor.h b/cloud/blockstore/libs/storage/volume/actors/shadow_disk_actor.h index 0086d789b52..2030300c006 100644 --- a/cloud/blockstore/libs/storage/volume/actors/shadow_disk_actor.h +++ b/cloud/blockstore/libs/storage/volume/actors/shadow_disk_actor.h @@ -12,7 +12,6 @@ #include #include -#include namespace NCloud::NBlockStore::NStorage { @@ -91,17 +90,17 @@ class TShadowDiskActor final const TString ShadowDiskId; const ui64 MountSeqNumber = 0; const ui32 Generation = 0; - const TActorId VolumeActorId; - const TActorId SrcActorId; + const NActors::TActorId VolumeActorId; + const NActors::TActorId SrcActorId; TNonreplicatedPartitionConfigPtr DstConfig; - TActorId DstActorId; + NActors::TActorId DstActorId; ui64 ProcessedBlockCount = 0; EActorState State = EActorState::Error; std::optional TimeoutCalculator; - TActorId AcquireActorId; + NActors::TActorId AcquireActorId; // The list of devices received on first acquire. TDevices ShadowDiskDevices; @@ -117,8 +116,8 @@ class TShadowDiskActor final ui64 mountSeqNumber, ui32 generation, TNonreplicatedPartitionConfigPtr srcConfig, - TActorId volumeActorId, - TActorId srcActorId, + NActors::TActorId volumeActorId, + NActors::TActorId srcActorId, const TActiveCheckpointInfo& checkpointInfo); ~TShadowDiskActor() override; diff --git a/cloud/blockstore/libs/storage/volume/volume_ut_checkpoint.cpp b/cloud/blockstore/libs/storage/volume/volume_ut_checkpoint.cpp index aef553588b2..4a46c87a428 100644 --- a/cloud/blockstore/libs/storage/volume/volume_ut_checkpoint.cpp +++ b/cloud/blockstore/libs/storage/volume/volume_ut_checkpoint.cpp @@ -3359,12 +3359,18 @@ Y_UNIT_TEST_SUITE(TVolumeCheckpointTest) NCloud::NProto::STORAGE_MEDIA_SSD_NONREPLICATED); } - Y_UNIT_TEST(ShouldCreateCheckpointWithShadowDiskMirror) + Y_UNIT_TEST(ShouldCreateCheckpointWithShadowDiskMirror2) { DoShouldCreateCheckpointWithShadowDisk( NCloud::NProto::STORAGE_MEDIA_SSD_MIRROR2); } + Y_UNIT_TEST(ShouldCreateCheckpointWithShadowDiskMirror3) + { + DoShouldCreateCheckpointWithShadowDisk( + NCloud::NProto::STORAGE_MEDIA_SSD_MIRROR3); + } + Y_UNIT_TEST(ShouldCreateCheckpointWithShadowDiskHddNonrepl) { DoShouldCreateCheckpointWithShadowDisk( @@ -3553,6 +3559,7 @@ Y_UNIT_TEST_SUITE(TVolumeCheckpointTest) { NProto::TStorageServiceConfig config; config.SetUseShadowDisksForNonreplDiskCheckpoints(true); + config.SetMaxShadowDiskFillBandwidth(1); auto runtime = PrepareTestActorRuntime(config); const ui64 expectedBlockCount = 32768; @@ -3631,6 +3638,7 @@ Y_UNIT_TEST_SUITE(TVolumeCheckpointTest) expectedBlockCount); volume.SendToPipe(std::move(request)); + runtime->DispatchEvents({}, TDuration::MilliSeconds(100)); } // Steal the acquire response. @@ -3675,6 +3683,7 @@ Y_UNIT_TEST_SUITE(TVolumeCheckpointTest) { NProto::TStorageServiceConfig config; config.SetUseShadowDisksForNonreplDiskCheckpoints(true); + config.SetMaxShadowDiskFillBandwidth(1); config.SetMaxAcquireShadowDiskTotalTimeoutWhenBlocked(5000); auto runtime = PrepareTestActorRuntime(config); @@ -3740,6 +3749,7 @@ Y_UNIT_TEST_SUITE(TVolumeCheckpointTest) expectedBlockCount); volume.SendToPipe(std::move(request)); + runtime->DispatchEvents({}, TDuration::MilliSeconds(100)); } // Steal the acquire response.