From e9a2aea63dbec2a5f1175a42aead7e1371555d1a Mon Sep 17 00:00:00 2001 From: Rattysed <46933547+Rattysed@users.noreply.github.com> Date: Fri, 28 Feb 2025 14:41:36 +0000 Subject: [PATCH 1/9] Fix test --- .../snapshot_service_test.go | 31 ++++++------------- 1 file changed, 9 insertions(+), 22 deletions(-) diff --git a/cloud/disk_manager/internal/pkg/facade/snapshot_service_test/snapshot_service_test.go b/cloud/disk_manager/internal/pkg/facade/snapshot_service_test/snapshot_service_test.go index 1b03bcccd7..7a7967cbf8 100644 --- a/cloud/disk_manager/internal/pkg/facade/snapshot_service_test/snapshot_service_test.go +++ b/cloud/disk_manager/internal/pkg/facade/snapshot_service_test/snapshot_service_test.go @@ -12,7 +12,6 @@ import ( "github.com/ydb-platform/nbs/cloud/disk_manager/internal/pkg/clients/nbs" "github.com/ydb-platform/nbs/cloud/disk_manager/internal/pkg/common" "github.com/ydb-platform/nbs/cloud/disk_manager/internal/pkg/facade/testcommon" - "github.com/ydb-platform/nbs/cloud/disk_manager/internal/pkg/types" ) //////////////////////////////////////////////////////////////////////////////// @@ -598,7 +597,7 @@ func TestSnapshotServiceDeleteIncrementalSnapshotWhileCreating(t *testing.T) { require.NotEmpty(t, createOperation) // Need to add some variance for better testing. - common.WaitForRandomDuration(1*time.Second, 3*time.Second) + common.WaitForRandomDuration(1*time.Millisecond, 3*time.Second) reqCtx = testcommon.GetRequestContext(t, ctx) deleteOperation, err := client.DeleteSnapshot(reqCtx, &disk_manager.DeleteSnapshotRequest{ @@ -612,26 +611,14 @@ func TestSnapshotServiceDeleteIncrementalSnapshotWhileCreating(t *testing.T) { err = internal_client.WaitOperation(ctx, client, deleteOperation.Id) require.NoError(t, err) - // In case of successful snapshot1 creation, snapshot1 should be deleted - // and checkpoint should be deleted too. - // Otherwise we should check incremental table. - if creationErr == nil { - testcommon.RequireCheckpointsDoNotExist(t, ctx, diskID) - } else { - snapshotID, _, err := testcommon.GetIncremental(ctx, &types.Disk{ - ZoneId: "zone-a", - DiskId: diskID, - }) - require.NoError(t, err) - - // In case of snapshot1 creation failure base snapshot may be already - // deleted from incremental table and then checkpoint should not exist - // on the disk. Otherwise checkpoint should exist. - if len(snapshotID) > 0 { - testcommon.RequireCheckpoint(t, ctx, diskID, baseSnapshotID) - } else { - testcommon.RequireCheckpointsDoNotExist(t, ctx, diskID) - } + // If snapshot creation ends up successfuly, there may be two cases: + // Snapshot was created and then deleted, so should be no checkpoints left + // or snapshot deletion ended up before creation, snapshot was not deleted, + // so there should be a checkpoint. + if creationErr != nil { + // should wait here because checkpoint is deleted on |createOperation| + // operation cancel (and exact time of this event is unknown). + testcommon.WaitForCheckpointsDoNotExist(t, ctx, diskID) } snapshotID2 := t.Name() + "2" From 688c6cc1dca2b60b0a466775d4409f39f3676ac6 Mon Sep 17 00:00:00 2001 From: Rattysed <46933547+Rattysed@users.noreply.github.com> Date: Mon, 3 Mar 2025 16:31:00 +0000 Subject: [PATCH 2/9] fix test --- .../snapshot_service_test.go | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/cloud/disk_manager/internal/pkg/facade/snapshot_service_test/snapshot_service_test.go b/cloud/disk_manager/internal/pkg/facade/snapshot_service_test/snapshot_service_test.go index 7a7967cbf8..f3de2e5615 100644 --- a/cloud/disk_manager/internal/pkg/facade/snapshot_service_test/snapshot_service_test.go +++ b/cloud/disk_manager/internal/pkg/facade/snapshot_service_test/snapshot_service_test.go @@ -12,6 +12,7 @@ import ( "github.com/ydb-platform/nbs/cloud/disk_manager/internal/pkg/clients/nbs" "github.com/ydb-platform/nbs/cloud/disk_manager/internal/pkg/common" "github.com/ydb-platform/nbs/cloud/disk_manager/internal/pkg/facade/testcommon" + "github.com/ydb-platform/nbs/cloud/disk_manager/internal/pkg/types" ) //////////////////////////////////////////////////////////////////////////////// @@ -598,6 +599,7 @@ func TestSnapshotServiceDeleteIncrementalSnapshotWhileCreating(t *testing.T) { // Need to add some variance for better testing. common.WaitForRandomDuration(1*time.Millisecond, 3*time.Second) + // common.WaitForRandomDuration(2000*time.Millisecond, 3*time.Second) reqCtx = testcommon.GetRequestContext(t, ctx) deleteOperation, err := client.DeleteSnapshot(reqCtx, &disk_manager.DeleteSnapshotRequest{ @@ -616,9 +618,18 @@ func TestSnapshotServiceDeleteIncrementalSnapshotWhileCreating(t *testing.T) { // or snapshot deletion ended up before creation, snapshot was not deleted, // so there should be a checkpoint. if creationErr != nil { - // should wait here because checkpoint is deleted on |createOperation| - // operation cancel (and exact time of this event is unknown). - testcommon.WaitForCheckpointsDoNotExist(t, ctx, diskID) + snapshotID, _, err := testcommon.GetIncremental(ctx, &types.Disk{ + ZoneId: "zone-a", + DiskId: diskID, + }) + require.NoError(t, err) + // If snapshot creation is cancelled, there may be two cases: + // it was cancelled before or after changing base snapshot + if snapshotID == baseSnapshotID { + testcommon.RequireCheckpoint(t, ctx, diskID, snapshotID) + } else { + testcommon.WaitForCheckpointsDoNotExist(t, ctx, diskID) + } } snapshotID2 := t.Name() + "2" From 332ec142b04b1ab028caf51d4779c69df0363d93 Mon Sep 17 00:00:00 2001 From: Rattysed <46933547+Rattysed@users.noreply.github.com> Date: Tue, 4 Mar 2025 12:42:24 +0000 Subject: [PATCH 3/9] add variety into waitForCheckpointsDoNotExist --- .../snapshot_service_test.go | 12 ++++++++---- .../internal/pkg/facade/testcommon/common.go | 17 ++++++++++++++++- 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/cloud/disk_manager/internal/pkg/facade/snapshot_service_test/snapshot_service_test.go b/cloud/disk_manager/internal/pkg/facade/snapshot_service_test/snapshot_service_test.go index f3de2e5615..1584ebfca1 100644 --- a/cloud/disk_manager/internal/pkg/facade/snapshot_service_test/snapshot_service_test.go +++ b/cloud/disk_manager/internal/pkg/facade/snapshot_service_test/snapshot_service_test.go @@ -623,10 +623,14 @@ func TestSnapshotServiceDeleteIncrementalSnapshotWhileCreating(t *testing.T) { DiskId: diskID, }) require.NoError(t, err) - // If snapshot creation is cancelled, there may be two cases: - // it was cancelled before or after changing base snapshot - if snapshotID == baseSnapshotID { - testcommon.RequireCheckpoint(t, ctx, diskID, snapshotID) + + // In case of snapshot1 creation failure base snapshot may be already + // deleted from incremental table and then checkpoint should not exist + // on the disk. Otherwise base snapshot checkpoint should exist. + if len(snapshotID) > 0 { + testcommon.WaitForCheckpointsDoNotExist(t, ctx, diskID, snapshotID1) + require.Equal(t, snapshotID, baseSnapshotID) + testcommon.RequireCheckpoint(t, ctx, diskID, baseSnapshotID) } else { testcommon.WaitForCheckpointsDoNotExist(t, ctx, diskID) } diff --git a/cloud/disk_manager/internal/pkg/facade/testcommon/common.go b/cloud/disk_manager/internal/pkg/facade/testcommon/common.go index 258a03a309..53419b141a 100644 --- a/cloud/disk_manager/internal/pkg/facade/testcommon/common.go +++ b/cloud/disk_manager/internal/pkg/facade/testcommon/common.go @@ -369,6 +369,7 @@ func WaitForCheckpointsDoNotExist( t *testing.T, ctx context.Context, diskID string, + awaitedCheckpoints ...string, ) { nbsClient := NewNbsTestingClient(t, ctx, "zone-a") @@ -377,7 +378,21 @@ func WaitForCheckpointsDoNotExist( checkpoints, err := nbsClient.GetCheckpoints(ctx, diskID) require.NoError(t, err) - if len(checkpoints) == 0 { + found := false + + for _, awaitedCheckpoint := range awaitedCheckpoints { + for _, checkpoint := range checkpoints { + if awaitedCheckpoint == checkpoint { + found = true + break + } + } + if found { + break + } + } + + if !found { return } From efc047ee8f9d94943f2bee38b171a738a5366a53 Mon Sep 17 00:00:00 2001 From: Rattysed <46933547+Rattysed@users.noreply.github.com> Date: Tue, 4 Mar 2025 12:52:41 +0000 Subject: [PATCH 4/9] change check in test --- .../facade/snapshot_service_test/snapshot_service_test.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/cloud/disk_manager/internal/pkg/facade/snapshot_service_test/snapshot_service_test.go b/cloud/disk_manager/internal/pkg/facade/snapshot_service_test/snapshot_service_test.go index 1584ebfca1..ffd54fa018 100644 --- a/cloud/disk_manager/internal/pkg/facade/snapshot_service_test/snapshot_service_test.go +++ b/cloud/disk_manager/internal/pkg/facade/snapshot_service_test/snapshot_service_test.go @@ -624,15 +624,17 @@ func TestSnapshotServiceDeleteIncrementalSnapshotWhileCreating(t *testing.T) { }) require.NoError(t, err) + // Should wait here because checkpoint is deleted on |createOperation| + // operation cancel (and exact time of this event is unknown). + testcommon.WaitForCheckpointsDoNotExist(t, ctx, diskID, snapshotID1) // In case of snapshot1 creation failure base snapshot may be already // deleted from incremental table and then checkpoint should not exist // on the disk. Otherwise base snapshot checkpoint should exist. if len(snapshotID) > 0 { - testcommon.WaitForCheckpointsDoNotExist(t, ctx, diskID, snapshotID1) require.Equal(t, snapshotID, baseSnapshotID) testcommon.RequireCheckpoint(t, ctx, diskID, baseSnapshotID) } else { - testcommon.WaitForCheckpointsDoNotExist(t, ctx, diskID) + testcommon.RequireCheckpointsDoNotExist(t, ctx, diskID) } } From f6eed5f157eb62072e5cbefa16e297be7872647b Mon Sep 17 00:00:00 2001 From: Rattysed <46933547+Rattysed@users.noreply.github.com> Date: Tue, 4 Mar 2025 12:53:50 +0000 Subject: [PATCH 5/9] remove extra line --- .../pkg/facade/snapshot_service_test/snapshot_service_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/cloud/disk_manager/internal/pkg/facade/snapshot_service_test/snapshot_service_test.go b/cloud/disk_manager/internal/pkg/facade/snapshot_service_test/snapshot_service_test.go index ffd54fa018..127416721a 100644 --- a/cloud/disk_manager/internal/pkg/facade/snapshot_service_test/snapshot_service_test.go +++ b/cloud/disk_manager/internal/pkg/facade/snapshot_service_test/snapshot_service_test.go @@ -599,7 +599,6 @@ func TestSnapshotServiceDeleteIncrementalSnapshotWhileCreating(t *testing.T) { // Need to add some variance for better testing. common.WaitForRandomDuration(1*time.Millisecond, 3*time.Second) - // common.WaitForRandomDuration(2000*time.Millisecond, 3*time.Second) reqCtx = testcommon.GetRequestContext(t, ctx) deleteOperation, err := client.DeleteSnapshot(reqCtx, &disk_manager.DeleteSnapshotRequest{ From 7df69703c99420739c2ddcc7a875270a6f143ab1 Mon Sep 17 00:00:00 2001 From: Rattysed <46933547+Rattysed@users.noreply.github.com> Date: Tue, 4 Mar 2025 21:30:29 +0000 Subject: [PATCH 6/9] fix method that was broken before --- .../internal/pkg/facade/testcommon/common.go | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/cloud/disk_manager/internal/pkg/facade/testcommon/common.go b/cloud/disk_manager/internal/pkg/facade/testcommon/common.go index 53419b141a..070ea03463 100644 --- a/cloud/disk_manager/internal/pkg/facade/testcommon/common.go +++ b/cloud/disk_manager/internal/pkg/facade/testcommon/common.go @@ -378,21 +378,20 @@ func WaitForCheckpointsDoNotExist( checkpoints, err := nbsClient.GetCheckpoints(ctx, diskID) require.NoError(t, err) - found := false + checkpointSet := make(map[string]struct{}, len(checkpoints)) + for _, checkpoint := range checkpoints { + checkpointSet[checkpoint] = struct{}{} + } + found := false for _, awaitedCheckpoint := range awaitedCheckpoints { - for _, checkpoint := range checkpoints { - if awaitedCheckpoint == checkpoint { - found = true - break - } - } - if found { + if _, exists := checkpointSet[awaitedCheckpoint]; exists { + found = true break } } - if !found { + if len(checkpoints) == 0 || (!found && len(awaitedCheckpoints) > 0) { return } From c4641ae4ac268959eec750434a2a7cbdaf3d37fa Mon Sep 17 00:00:00 2001 From: Rattysed <46933547+Rattysed@users.noreply.github.com> Date: Thu, 6 Mar 2025 11:38:16 +0000 Subject: [PATCH 7/9] separate methods --- .../snapshot_service_test.go | 2 +- .../internal/pkg/facade/testcommon/common.go | 39 ++++++++++++------- 2 files changed, 27 insertions(+), 14 deletions(-) diff --git a/cloud/disk_manager/internal/pkg/facade/snapshot_service_test/snapshot_service_test.go b/cloud/disk_manager/internal/pkg/facade/snapshot_service_test/snapshot_service_test.go index 127416721a..fc91e18777 100644 --- a/cloud/disk_manager/internal/pkg/facade/snapshot_service_test/snapshot_service_test.go +++ b/cloud/disk_manager/internal/pkg/facade/snapshot_service_test/snapshot_service_test.go @@ -625,7 +625,7 @@ func TestSnapshotServiceDeleteIncrementalSnapshotWhileCreating(t *testing.T) { // Should wait here because checkpoint is deleted on |createOperation| // operation cancel (and exact time of this event is unknown). - testcommon.WaitForCheckpointsDoNotExist(t, ctx, diskID, snapshotID1) + testcommon.WaitForCheckpointDoesNotExist(t, ctx, diskID, snapshotID1) // In case of snapshot1 creation failure base snapshot may be already // deleted from incremental table and then checkpoint should not exist // on the disk. Otherwise base snapshot checkpoint should exist. diff --git a/cloud/disk_manager/internal/pkg/facade/testcommon/common.go b/cloud/disk_manager/internal/pkg/facade/testcommon/common.go index 070ea03463..074c8b2a8c 100644 --- a/cloud/disk_manager/internal/pkg/facade/testcommon/common.go +++ b/cloud/disk_manager/internal/pkg/facade/testcommon/common.go @@ -5,6 +5,7 @@ import ( "fmt" "net/http" "os" + "slices" "strconv" "strings" "testing" @@ -369,7 +370,6 @@ func WaitForCheckpointsDoNotExist( t *testing.T, ctx context.Context, diskID string, - awaitedCheckpoints ...string, ) { nbsClient := NewNbsTestingClient(t, ctx, "zone-a") @@ -378,26 +378,39 @@ func WaitForCheckpointsDoNotExist( checkpoints, err := nbsClient.GetCheckpoints(ctx, diskID) require.NoError(t, err) - checkpointSet := make(map[string]struct{}, len(checkpoints)) - for _, checkpoint := range checkpoints { - checkpointSet[checkpoint] = struct{}{} + if len(checkpoints) == 0 { + return } - found := false - for _, awaitedCheckpoint := range awaitedCheckpoints { - if _, exists := checkpointSet[awaitedCheckpoint]; exists { - found = true - break - } - } + logging.Warn( + ctx, + "WaitForCheckpointsDoNotExist proceeding to next iteration", + ) + + <-time.After(100 * time.Millisecond) + } +} + +func WaitForCheckpointDoesNotExist( + t *testing.T, + ctx context.Context, + diskID string, + checkpointID string, +) { + + nbsClient := NewNbsTestingClient(t, ctx, "zone-a") + + for { + checkpoints, err := nbsClient.GetCheckpoints(ctx, diskID) + require.NoError(t, err) - if len(checkpoints) == 0 || (!found && len(awaitedCheckpoints) > 0) { + if !slices.Contains(checkpoints, checkpointID) { return } logging.Warn( ctx, - "WaitForCheckpointsDoNotExist proceeding to next iteration", + "WaitForCheckpointDoesNotExist proceeding to next iteration", ) <-time.After(100 * time.Millisecond) From 7959f6fc3537b1f7351c327a7c9e567462caefd9 Mon Sep 17 00:00:00 2001 From: Rattysed <46933547+Rattysed@users.noreply.github.com> Date: Fri, 7 Mar 2025 08:01:36 +0000 Subject: [PATCH 8/9] remove copypaste --- .../snapshot_service_test.go | 18 +++++----- .../internal/pkg/facade/testcommon/common.go | 33 +++---------------- 2 files changed, 14 insertions(+), 37 deletions(-) diff --git a/cloud/disk_manager/internal/pkg/facade/snapshot_service_test/snapshot_service_test.go b/cloud/disk_manager/internal/pkg/facade/snapshot_service_test/snapshot_service_test.go index fc91e18777..16e4ccc57d 100644 --- a/cloud/disk_manager/internal/pkg/facade/snapshot_service_test/snapshot_service_test.go +++ b/cloud/disk_manager/internal/pkg/facade/snapshot_service_test/snapshot_service_test.go @@ -178,7 +178,8 @@ func TestSnapshotServiceCancelCreateSnapshotFromDisk(t *testing.T) { // Should wait here because checkpoint is deleted on operation cancel (and // exact time of this event is unknown). - testcommon.WaitForCheckpointsDoNotExist(t, ctx, diskID) + testcommon.WaitForCheckpointDoesNotExist(t, ctx, diskID, snapshotID) + testcommon.RequireCheckpointsDoNotExist(t, ctx, diskID) testcommon.CheckConsistency(t, ctx) } @@ -612,10 +613,10 @@ func TestSnapshotServiceDeleteIncrementalSnapshotWhileCreating(t *testing.T) { err = internal_client.WaitOperation(ctx, client, deleteOperation.Id) require.NoError(t, err) - // If snapshot creation ends up successfuly, there may be two cases: - // Snapshot was created and then deleted, so should be no checkpoints left - // or snapshot deletion ended up before creation, snapshot was not deleted, - // so there should be a checkpoint. + // If snapshot creation ends up successfuly we don't care because, + // there may be two cases: Snapshot was created and then deleted, + // so should be no checkpoints left or snapshot deletion ended up before + // creation, snapshot was not deleted, so there should be a checkpoint. if creationErr != nil { snapshotID, _, err := testcommon.GetIncremental(ctx, &types.Disk{ ZoneId: "zone-a", @@ -623,8 +624,8 @@ func TestSnapshotServiceDeleteIncrementalSnapshotWhileCreating(t *testing.T) { }) require.NoError(t, err) - // Should wait here because checkpoint is deleted on |createOperation| - // operation cancel (and exact time of this event is unknown). + // Should wait here because checkpoint is deleted on snapshotID1 + // creation operation cancel (and exact time of this event is unknown). testcommon.WaitForCheckpointDoesNotExist(t, ctx, diskID, snapshotID1) // In case of snapshot1 creation failure base snapshot may be already // deleted from incremental table and then checkpoint should not exist @@ -787,7 +788,8 @@ func TestSnapshotServiceDeleteSnapshotWhenCreationIsInFlight(t *testing.T) { if err != nil { // Should wait here because checkpoint is deleted on |createOp| // operation cancel (and exact time of this event is unknown). - testcommon.WaitForCheckpointsDoNotExist(t, ctx, diskID) + testcommon.WaitForCheckpointDoesNotExist(t, ctx, diskID, snapshotID) + testcommon.RequireCheckpointsDoNotExist(t, ctx, diskID) } testcommon.CheckConsistency(t, ctx) diff --git a/cloud/disk_manager/internal/pkg/facade/testcommon/common.go b/cloud/disk_manager/internal/pkg/facade/testcommon/common.go index 074c8b2a8c..071a2d24bd 100644 --- a/cloud/disk_manager/internal/pkg/facade/testcommon/common.go +++ b/cloud/disk_manager/internal/pkg/facade/testcommon/common.go @@ -366,31 +366,6 @@ func RequireCheckpointsDoNotExist( require.Empty(t, checkpoints) } -func WaitForCheckpointsDoNotExist( - t *testing.T, - ctx context.Context, - diskID string, -) { - - nbsClient := NewNbsTestingClient(t, ctx, "zone-a") - - for { - checkpoints, err := nbsClient.GetCheckpoints(ctx, diskID) - require.NoError(t, err) - - if len(checkpoints) == 0 { - return - } - - logging.Warn( - ctx, - "WaitForCheckpointsDoNotExist proceeding to next iteration", - ) - - <-time.After(100 * time.Millisecond) - } -} - func WaitForCheckpointDoesNotExist( t *testing.T, ctx context.Context, @@ -399,8 +374,10 @@ func WaitForCheckpointDoesNotExist( ) { nbsClient := NewNbsTestingClient(t, ctx, "zone-a") + ticker := time.NewTicker(time.Millisecond * 100) + defer ticker.Stop() - for { + for range ticker.C { checkpoints, err := nbsClient.GetCheckpoints(ctx, diskID) require.NoError(t, err) @@ -408,12 +385,10 @@ func WaitForCheckpointDoesNotExist( return } - logging.Warn( + logging.Debug( ctx, "WaitForCheckpointDoesNotExist proceeding to next iteration", ) - - <-time.After(100 * time.Millisecond) } } From 12f3e4b6820ebc1e28d7b7409d74200342e635ac Mon Sep 17 00:00:00 2001 From: Rattysed <46933547+Rattysed@users.noreply.github.com> Date: Mon, 10 Mar 2025 18:56:38 +0000 Subject: [PATCH 9/9] Swap vars in require.equal --- .../pkg/facade/snapshot_service_test/snapshot_service_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cloud/disk_manager/internal/pkg/facade/snapshot_service_test/snapshot_service_test.go b/cloud/disk_manager/internal/pkg/facade/snapshot_service_test/snapshot_service_test.go index 16e4ccc57d..bd9d273bdd 100644 --- a/cloud/disk_manager/internal/pkg/facade/snapshot_service_test/snapshot_service_test.go +++ b/cloud/disk_manager/internal/pkg/facade/snapshot_service_test/snapshot_service_test.go @@ -631,7 +631,7 @@ func TestSnapshotServiceDeleteIncrementalSnapshotWhileCreating(t *testing.T) { // deleted from incremental table and then checkpoint should not exist // on the disk. Otherwise base snapshot checkpoint should exist. if len(snapshotID) > 0 { - require.Equal(t, snapshotID, baseSnapshotID) + require.Equal(t, baseSnapshotID, snapshotID) testcommon.RequireCheckpoint(t, ctx, diskID, baseSnapshotID) } else { testcommon.RequireCheckpointsDoNotExist(t, ctx, diskID)