[Disk Manager] retry with new checkpoint id when create snapshot if shadow disk failed during filling #2691

gy2411 · 2024-12-13T09:47:25Z

#1950

Сейчас есть баг при создании снапшотов из disk registy based дисков. Чекпоинты для таких дисков делаются через теневой диск (shadow disk). Сейчас, если вот тут мы от чекпоинта с теневым диском получаем статус Error, то мы удаляем чекпоинт и ретраим таск. При ретрае таск падает при попытке создать чекпоинт с тем же id, что был у удалённого чекпоинта.

Теперь вместо этого мы при ретрае создаём чекпоинт с новым checkpoint id.

Также для воспроизведения падений теневого диска в интеграционных тестах нам нужно ходить в disk registry. Добавляю в nbs client нужный для этого код.

См. больше деталей в комментариях в #1950

Надо понимать, что эта правка ещё не полностью решает проблему с падением теневого диска. Подробнее написал в issue #1950.

github-actions · 2024-12-13T13:12:25Z

Note

This is an automated comment that will be appended during run.

🟢 linux-x86_64-relwithdebinfo: all tests PASSED for commit 35da72c.

TESTS	PASSED	ERRORS	FAILED	SKIPPED	MUTED^?
6618	6618	0	0	0	0

gy2411 · 2024-12-13T16:03:05Z

cloud/disk_manager/internal/pkg/services/snapshots/create_snapshot_from_disk_task.go

-	if err != nil {
-		return err
-	}
+	if t.state.FinalCheckpointID == "" {


Мотивировка этого if (и вообще мотивировка поля FinalCheckpointID) -- мы не должны обновлять чекпоинт, если мы уже зашедуллили dataplane таск с предыдущим чекпоинтом.

gy2411 · 2024-12-13T16:05:19Z

cloud/disk_manager/internal/pkg/services/snapshots/create_snapshot_from_disk_task.go


-	// NBS-1873: should always delete checkpoint.
-	err = nbsClient.DeleteCheckpoint(ctx, disk.DiskId, checkpointID)
+	err = t.deletePreviousCheckpoint(ctx, nbsClient)


Предыдущий чекпоинт тоже надо удалить -- таск мог пойти на отмену после инкремента FailedCheckpointsCount, но до удаления старого чекпоинта.

gy2411 · 2024-12-13T16:07:32Z

cloud/disk_manager/internal/pkg/facade/snapshot_service_test/snapshot_service_test.go

+	)
+}
+
+func TestCreateSnapshotFromDiskWithFailedShadowDiskLong(t *testing.T) {


Сделал два варианта теста с разными границами на интервал случайной зажержки, чтобы меньше зависеть от конкретных таймингов работы таска.

gy2411 · 2024-12-13T16:10:18Z

cloud/disk_manager/internal/pkg/clients/nbs/testing_client.go

+	}
+
+	input := fmt.Sprintf(
+		"{\"DisableAgent\":{\"AgentId\":\"%v\",\"DeviceUUIDs\":%v},\"Message\":\"%v\"}",


Хотя ручка и называется "DisableAgent", она не будет ломать весь агент, если ей передать непустой спасок девайсов. Она сломает только девайсы из этого списка.

Сломает -- значит, девайсы начнут отдавать ошибку в ответ на все запросы чтения и записи.

gy2411 · 2024-12-13T16:13:15Z

cloud/disk_manager/internal/pkg/services/snapshots/create_snapshot_from_disk_task.go

@@ -278,6 +293,7 @@ func (t *createSnapshotFromDiskTask) GetResponse() proto.Message {

 func (t *createSnapshotFromDiskTask) ensureCheckpointReady(


Тут есть конфликт с #2612 (там эта функция уносится в метод nbs-клиента). Придётся его порезолвить.

Кстати, в том pr делается удаление чекпоинта в методе EnsureCheckpointReady, а в этом pr предлагается вынести его за пределы EnsureCheckpointReady. Это важно: иначе будет возможен плохой сценарий, при котором таск пойдёт на ретрай уже после удаления чекпоинта, но еще не успев увеличить FailedCheckpointsCount.

cloud/disk_manager/test/recipe/nbs_launcher.py

github-actions · 2024-12-13T19:22:51Z

Note

This is an automated comment that will be appended during run.

🟢 linux-x86_64-relwithdebinfo: all tests PASSED for commit 3b88f28.

TESTS	PASSED	ERRORS	FAILED	SKIPPED	MUTED^?
6619	6619	0	0	0	0

github-actions · 2024-12-14T18:38:43Z

Note

This is an automated comment that will be appended during run.

🟢 linux-x86_64-relwithdebinfo: all tests PASSED for commit e5ab782.

TESTS	PASSED	ERRORS	FAILED	SKIPPED	MUTED^?
6624	6624	0	0	0	0

cloud/disk_manager/internal/pkg/services/snapshots/protos/create_snapshot_from_disk_task.proto

BarkovBG · 2025-01-14T15:23:00Z

cloud/disk_manager/internal/pkg/services/snapshots/protos/create_snapshot_from_disk_task.proto

+
+    // Needed for shadow disk based checkpoints.
+    int32 FailedCheckpointsCount = 7;
+    string FinalCheckpointID = 8;


отступ, так как коммент относится только в переменной FailedCheckpointsCount

Казалось бы, к FinalCheckpointID этот комментарий тоже относится. Без теневых дисков было бы незачем созранять id чекпоинта персистентно.

cloud/disk_manager/internal/pkg/services/snapshots/create_snapshot_from_disk_task.go

BarkovBG · 2025-01-14T15:31:07Z

cloud/disk_manager/internal/pkg/services/snapshots/create_snapshot_from_disk_task.go

+	return t.makeCheckpointID(int(t.state.FailedCheckpointsCount))
+}
+
+func (t *createSnapshotFromDiskTask) makeCheckpointID(index int) string {


вообще этот подход ломает семантику snapshotID = checkpointID - это точно ничего не сломает?

Насколько я вижу по коду, ничего сломаться не должно. У нас везде разграничены checkpoint id и snapshot id. Мы на это полагаемся разве что в некоторых тестах, в которых не должны происходить поломки чекпоинтов.

Но конечно хотелось бы как-то более нажёжно убедиться, что ничего не сломается... Есть радикальная идея -- делать первый же чекпоинт с другим id (скажем, не snapshot_id, а snapshot_id_0). Чтобы ситуация с различными id снапшота и чекпоинта воспроизволась не только в случаях поломки теневого диска и наблюдалась во всех имеющихся ныне тестах.

BarkovBG · 2025-01-14T15:36:15Z

cloud/disk_manager/internal/pkg/clients/nbs/disk_registry_state.go

@@ -0,0 +1,62 @@
+package nbs


предлагаю упростить и если уж тащить внутренности dr в дм, то только в тестового клиента (и я бы даже делал это без тестов)

А что ты понимаешь под "упростить"?

Хм, а в чём принципиальная разница между тестовым клиентом и отдельным файликом disk_registry_state.go? Они ведь всё равно в одном модуле находятся.

Если не писать тесты на методы работы с disk registry, то появляется опасение, что выключение девайса не сработает. И тогда интеграционный тест будет работать вхолостую -- он будет завершаться успехом, но при этом по факту никогда не будет выключать девайс. Хочется обезопаситься от этого.

Было бы здорово в самом интеграционном тесте как-то проверить, что девайс действительно был выключен и что действительно был налит новый чекпоинт. Но ведь так происходит не всегда -- из-за рандома с таймингами могут быть сценарии, когда девайс не ломался.

то, что написаны тесты - никогда не плохо

но такой большой кусок кода, особенно не используемый ДМом в проде тащить в nbs клиента неправильно - мы вообще в DA не ходим

в клиенте иметь этот код также будет причиной вопросов что это, зачем это, давайте сделаем по аналогии и тп

мы в целом уже давно хотели это исправить и мне кажется этот момент настал, тк этот код нетривиальный (тк это внутренности DA) #892

cloud/disk_manager/internal/pkg/clients/nbs/tests/ya.make

cloud/disk_manager/test/recipe/nbs_launcher.py

…f shadow disk failed during its filling

…s update when retry shadow disk fail

gy2411 added the large-tests Launch large tests for PR label Dec 13, 2024

gy2411 mentioned this pull request Dec 13, 2024

[Disk manager] Retry snapshot creation on shadow disk fail #1950

Open

gy2411 commented Dec 13, 2024

View reviewed changes

cloud/disk_manager/test/recipe/nbs_launcher.py Show resolved Hide resolved

BarkovBG reviewed Jan 14, 2025

View reviewed changes

gy2411 force-pushed the users/gayurgin/retry_with_new_checkpoint_id_when_create_snapshot branch 2 times, most recently from 7cd05e6 to ff6d6f1 Compare January 16, 2025 10:34

gy2411 added 9 commits January 16, 2025 13:45

[Disk Manager] add methods for disk registry to nbs client

7e5ef27

[Disk Manager] retry with new checkpoint id in create snapshot task i…

1b5ba68

…f shadow disk failed during its filling

[Disk Manager] add test on snapshot creation with shadow disk failure

76099cb

[Disk Manager] enable shadow disks in disk manager large tests

7d546cb

minor improvements

77e5bf0

[Disk Manager] add tests on disk registry methods of nbs client

40b43db

[Disk Manager] create snapshot from disk: add unit test on checkpoint…

0e0840e

…s update when retry shadow disk fail

fix tests names

3a572dd

minor fixes

560c54f

gy2411 force-pushed the users/gayurgin/retry_with_new_checkpoint_id_when_create_snapshot branch from ff6d6f1 to 560c54f Compare January 16, 2025 10:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Disk Manager] retry with new checkpoint id when create snapshot if shadow disk failed during filling #2691

[Disk Manager] retry with new checkpoint id when create snapshot if shadow disk failed during filling #2691

gy2411 commented Dec 13, 2024 •

edited

Loading

github-actions bot commented Dec 13, 2024

gy2411 Dec 13, 2024 •

edited

Loading

gy2411 Dec 13, 2024

gy2411 Dec 13, 2024

gy2411 Dec 13, 2024

gy2411 Dec 13, 2024 •

edited

Loading

github-actions bot commented Dec 13, 2024

github-actions bot commented Dec 14, 2024

BarkovBG Jan 14, 2025

gy2411 Jan 16, 2025

BarkovBG Jan 14, 2025

gy2411 Jan 15, 2025

BarkovBG Jan 14, 2025

gy2411 Jan 15, 2025

gy2411 Jan 15, 2025

BarkovBG Jan 16, 2025

		@@ -278,6 +293,7 @@ func (t *createSnapshotFromDiskTask) GetResponse() proto.Message {

		func (t *createSnapshotFromDiskTask) ensureCheckpointReady(

[Disk Manager] retry with new checkpoint id when create snapshot if shadow disk failed during filling #2691

Are you sure you want to change the base?

[Disk Manager] retry with new checkpoint id when create snapshot if shadow disk failed during filling #2691

Conversation

gy2411 commented Dec 13, 2024 • edited Loading

github-actions bot commented Dec 13, 2024

gy2411 Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gy2411 Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Dec 13, 2024

github-actions bot commented Dec 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gy2411 commented Dec 13, 2024 •

edited

Loading

gy2411 Dec 13, 2024 •

edited

Loading

gy2411 Dec 13, 2024 •

edited

Loading