Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Disk Manager] Fix flappy test for snapshot service #3120

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -598,7 +598,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{
Expand All @@ -612,22 +612,25 @@ 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 {
// 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 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

чет я не понял комментария

ты пишешь про случае когда снапшот успешно создался

а иф проверяет то что создание с ошибой упало

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Пишу как раз чтобы объяснить, почему этот случай не проверяем. Для случая с ошибкой комментарий внутри

snapshotID, _, err := testcommon.GetIncremental(ctx, &types.Disk{
ZoneId: "zone-a",
DiskId: diskID,
})
require.NoError(t, err)

// Should wait here because checkpoint is deleted on |createOperation|
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

не понятный коммент

что за |createOperation| operation?

во-первых - какая-то тавтология
во-вторых - что это за операция такая? может ты имел в виду createSnapshot?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Сверху есть переменная 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 checkpoint should exist.
// on the disk. Otherwise base snapshot checkpoint should exist.
if len(snapshotID) > 0 {
require.Equal(t, snapshotID, baseSnapshotID)
testcommon.RequireCheckpoint(t, ctx, diskID, baseSnapshotID)
} else {
testcommon.RequireCheckpointsDoNotExist(t, ctx, diskID)
Expand Down
17 changes: 16 additions & 1 deletion cloud/disk_manager/internal/pkg/facade/testcommon/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -369,6 +369,7 @@ func WaitForCheckpointsDoNotExist(
t *testing.T,
ctx context.Context,
diskID string,
awaitedCheckpoints ...string,
) {

nbsClient := NewNbsTestingClient(t, ctx, "zone-a")
Expand All @@ -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
}

Expand Down
Loading