[Disk Manager] Fix migration for incremental snapshots #4775
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request fixes migration issues for incremental snapshots in the Disk Manager. The core fix ensures that snapshots are only migrated after their base snapshots have been fully migrated, preventing corruption in incremental snapshot chains. This is achieved by adding a check in the migration task to verify base snapshot migration status before proceeding with dependent snapshots.
Key changes:
- Added dependency checking logic to ensure base snapshots are migrated before incremental snapshots
- Extended test coverage with a new parameter
with_nemesisand comprehensive test for incremental snapshot migration scenarios - Modified existing test parametrization to include nemesis testing configuration
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| cloud/disk_manager/internal/pkg/dataplane/migrate_snapshot_database_task.go | Implements checkBaseSnapshotIsMigrated function to verify base snapshot migration status before migrating dependent snapshots |
| cloud/disk_manager/test/snapshot_migration_test/test.py | Adds with_nemesis parameter to existing tests and introduces comprehensive test for incremental snapshot chain migration during database migration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cloud/disk_manager/internal/pkg/dataplane/migrate_snapshot_database_task.go
Show resolved
Hide resolved
cloud/disk_manager/internal/pkg/dataplane/migrate_snapshot_database_task.go
Outdated
Show resolved
Hide resolved
cloud/disk_manager/internal/pkg/dataplane/migrate_snapshot_database_task.go
Show resolved
Hide resolved
|
Note This is an automated comment that will be appended during run. 🔴 linux-x86_64-relwithdebinfo: some tests FAILED for commit 2e5e813.
🔴 linux-x86_64-relwithdebinfo: some tests FAILED for commit 2e5e813.
🔴 linux-x86_64-relwithdebinfo: some tests FAILED for commit 2e5e813.
|
jkuradobery
added
large-tests
|
Note This is an automated comment that will be appended during run. 🔴 linux-x86_64-relwithdebinfo: some tests FAILED for commit b60acd4.
🔴 linux-x86_64-relwithdebinfo: some tests FAILED for commit b60acd4.
🔴 linux-x86_64-relwithdebinfo: some tests FAILED for commit b60acd4.
|
cloud/disk_manager/internal/pkg/dataplane/migrate_snapshot_database_task.go
Outdated
Show resolved
Hide resolved
| base_snapshot_ids = {} | ||
| for record in database_entries: | ||
| base_snapshot_ids[record['id']] = record['base_snapshot_id'] | ||
|
|
There was a problem hiding this comment.
А не получится тут ещё проверить, что снапшоты в правильном порядке мигрировали? Скажем, сравнить поле created_at?
There was a problem hiding this comment.
Кстати, а чем плохо, если инкрементальный снапшот мигрирует раньше базового?
There was a problem hiding this comment.
А не получится тут ещё проверить, что снапшоты в правильном порядке мигрировали? Скажем, сравнить поле created_at?
Так нам не супер обязательно полагаться на время миграции. Нам достаточно положиться на инвариант "цепочка снапшотов раньше == цепочке снапшотов сейчас", который и проверяется в тесте.
There was a problem hiding this comment.
Кстати, а чем плохо, если инкрементальный снапшот мигрирует раньше базового?
Мы мигрируем все снапшоты де-факто как не инкрементальные, гарантия порядка нам дает: 1) что снапшот из диска из которого есть инкрементальные снапшоты возьмет последний чекпоинт.
2) Что цепочка инкрементальности сохранится и в будущем можно будет дописать в миграцию возможность мигрировать инкрементальные снапшоты как инкрементальные. То есть эта правка не настолько критична и мигрировать инкрементальные снапшоты вполне можно и сейчас.
|
Note This is an automated comment that will be appended during run. 🔴 linux-x86_64-relwithdebinfo: some tests FAILED for commit 2699fa8.
🔴 linux-x86_64-relwithdebinfo: some tests FAILED for commit 2699fa8.
🔴 linux-x86_64-relwithdebinfo: some tests FAILED for commit 2699fa8.
|
No description provided.