NBS-4827 Fix review issues and tests flakiness

ydb-platform · Mar 15, 2024 · 3094168 · 3094168
1 parent 45142c4
commit 3094168
Show file tree

Hide file tree

Showing 5 changed files with 26 additions and 20 deletions.
diff --git 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
@@ -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
@@ -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,26 +153,26 @@ TAcquireShadowDiskActor::TAcquireShadowDiskActor(
         const TDevices& shadowDiskDevices,
         TShadowDiskActor::EAcquireReason acquireReason,
         bool readOnlyMount,
-        bool isWritesToSourceBlocked,
+        bool areWritesToSourceBlocked,
         ui64 mountSeqNumber,
         ui32 generation,
         TActorId parentActor)
     : ShadowDiskId(std::move(shadowDiskId))
     , AcquireReason(acquireReason)
     , ReadOnlyMount(readOnlyMount)
     , TotalTimeout(
-          isWritesToSourceBlocked
+          areWritesToSourceBlocked
               ? config->GetMaxAcquireShadowDiskTotalTimeoutWhenBlocked()
               : config->GetMaxAcquireShadowDiskTotalTimeoutWhenNonBlocked())
     , ParentActor(parentActor)
     , MountSeqNumber(mountSeqNumber)
     , 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
@@ -12,7 +12,6 @@
 
 #include <contrib/ydb/library/actors/core/actor_bootstrapped.h>
 #include <contrib/ydb/library/actors/core/events.h>
-#include <contrib/ydb/library/actors/interconnect/types.h>
 
 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<TMigrationTimeoutCalculator> 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
@@ -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.