Fix volume snapshot physical size after migration between secondary storages #12166
Conversation
|
@blueorangutan package |
|
@hsato03 a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 4.20 #12166 +/- ##
============================================
- Coverage 16.18% 16.18% -0.01%
- Complexity 13300 13301 +1
============================================
Files 5657 5657
Lines 498478 498478
Branches 60501 60501
============================================
- Hits 80668 80664 -4
- Misses 408827 408830 +3
- Partials 8983 8984 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where volume snapshot physical size was incorrectly set to 0 after migration between secondary storages using the migrateSecondaryStorageData and migrateResourceToAnotherSecondaryStorage APIs.
- Corrected the lookup of the destination snapshot store to use destination data store ID and snapshot ID instead of source parameters
- Aligned the snapshot handling pattern with the existing VolumeInfo and TemplateInfo handling in the same method
Comments suppressed due to low confidence (1)
engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/SecondaryStorageServiceImpl.java:291
- The fixed bug in the
updateDataObjectmethod lacks test coverage. Consider adding a test case to verify that after migrating a snapshot between secondary storages, the physical size is correctly preserved. This would prevent similar regressions in the future.
private void updateDataObject(DataObject srcData, DataObject destData) {
if (destData instanceof SnapshotInfo) {
SnapshotDataStoreVO snapshotStore = snapshotStoreDao.findBySourceSnapshot(srcData.getId(), DataStoreRole.Image);
SnapshotDataStoreVO destSnapshotStore = snapshotStoreDao.findByStoreSnapshot(DataStoreRole.Image, destData.getDataStore().getId(), destData.getId());
if (snapshotStore != null && destSnapshotStore != null) {
destSnapshotStore.setPhysicalSize(snapshotStore.getPhysicalSize());
destSnapshotStore.setCreated(snapshotStore.getCreated());
if (snapshotStore.getParentSnapshotId() != destSnapshotStore.getParentSnapshotId()) {
destSnapshotStore.setParentSnapshotId(snapshotStore.getParentSnapshotId());
}
snapshotStoreDao.update(destSnapshotStore.getId(), destSnapshotStore);
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 15860 |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-14899)
|
|
@hsato03 should this go in 4.20 branch? |
hsato03
force-pushed
the
fix-snapshot-size-after-migration
branch
from
e81766a to
dc7c20d
Compare
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
Sure, I changed the base branch to |
|
@blueorangutan package |
|
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 15941 |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-14915)
|
|
@blueorangutan test |
|
@RosiKyu a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-15167)
|
There was a problem hiding this comment.
LGTM
Tested on KVM environment with 2 secondary storages. Verified snapshot physical size is correctly preserved after migration using both migrateResourceToAnotherSecondaryStorage and migrateSecondaryStorageData APIs. Also confirmed migrated snapshot remains functional by creating and attaching a volume from it. LGTM.
Test Execution Summary
| Test Case | Description | API Tested | Result |
|---|---|---|---|
| TC1 | Snapshot physical size preserved after single resource migration | migrateResourceToAnotherSecondaryStorage | PASS |
| TC2 | Snapshot physical size preserved after bulk migration | migrateSecondaryStorageData | PASS |
| TC3 | Migrated snapshot remains usable | createVolume + attachVolume | PASS |
Detailed Test Report:
TC1: Verify volume snapshot physical size is preserved after migration between secondary storages
Objective Verify that when a volume snapshot is migrated from one secondary storage to another using the migrateResourceToAnotherSecondaryStorage API, its physical size is correctly preserved and not set to 0.
Test Steps
- Deploy a VM with a root volume
- Stop the VM
- Create a volume snapshot
- Record the snapshot's physical size from the database
- Migrate the snapshot to a different secondary storage using migrateResourceToAnotherSecondaryStorage
- Verify the physical size is preserved on the destination secondary storage
Expected Result The snapshot's physical size on the destination secondary storage should match the original physical size (1510604800 bytes), not 0.
Actual Result The snapshot's physical size was correctly preserved as 1510604800 bytes after migration to the destination secondary storage.
Test Evidence
Environment: 1 MS, 2 Secondary Storages
Secondary storages:
(localcloud) 🐱 > list imagestores
{
"count": 2,
"imagestore": [
{
"id": "09dbab49-82da-43e1-9031-815dd0fc5fb8",
"name": "NFS://10.0.32.4/acs/secondary/ref-trl-10708-k-Mol9-rositsa-kyuchukova/ref-trl-10708-k-Mol9-rositsa-kyuchukova-sec1",
...
},
{
"id": "d6d105da-939e-4e60-b369-8a4a5ed6bb0a",
"name": "NFS://10.0.32.4/acs/secondary/ref-trl-10708-k-Mol9-rositsa-kyuchukova/ref-trl-10708-k-Mol9-rositsa-kyuchukova-sec2",
...
}
]
}
- Deploy and stop VM:
(localcloud) 🐱 > deploy virtualmachine name=test-vm serviceofferingid=5de91f25-5d53-485f-8c63-050499315fe1 templateid=e2b7cd99-f87b-11f0-983f-1e00fa000343 zoneid=6b3966e1-7372-4033-bf45-1af3c5bcbc25 networkids=6bde88da-a68b-4bf0-938a-c849bf85c85a
{
"virtualmachine": {
"id": "4e98d18e-3f1c-4d9f-8b64-b2facc7e8e71",
"name": "test-vm",
"state": "Running",
...
}
}
- Create snapshot (after stopping VM):
(localcloud) 🐱 > create snapshot volumeid=b196a9aa-2d9e-4d8f-9a72-01ea3820bb84
{
"snapshot": {
"id": "0d9550f3-7f93-4c5f-91f5-bd81b12daca7",
"name": "test-vm_ROOT-4_20260126065644",
"physicalsize": 1510604800,
"state": "BackedUp",
"volumeid": "b196a9aa-2d9e-4d8f-9a72-01ea3820bb84",
...
}
}
- Verify snapshot physical size before migration (on sec1, store_id=1):
mysql> SELECT id, snapshot_id, store_id, store_role, state, physical_size, install_path
FROM cloud.snapshot_store_ref
WHERE snapshot_id IN (SELECT id FROM cloud.snapshots WHERE uuid='0d9550f3-7f93-4c5f-91f5-bd81b12daca7');
+----+-------------+----------+------------+-------+---------------+------------------------------------------------------------------------------------------+
| id | snapshot_id | store_id | store_role | state | physical_size | install_path |
+----+-------------+----------+------------+-------+---------------+------------------------------------------------------------------------------------------+
| 1 | 1 | 1 | Primary | Ready | 8589934592 | /mnt/a4e6c7f4-aad3-3063-bef4-45d2face66df/snapshots/5ce8ff95-9983-4b1f-895b-de7612d487c0 |
| 2 | 1 | 1 | Image | Ready | 1510604800 | snapshots/2/3/5ce8ff95-9983-4b1f-895b-de7612d487c0 |
+----+-------------+----------+------------+-------+---------------+------------------------------------------------------------------------------------------+
- Migrate snapshot from sec1 to sec2:*
(localcloud) 🐱 > migrateResourceToAnotherSecondaryStorage snapshots=0d9550f3-7f93-4c5f-91f5-bd81b12daca7 srcpool=09dbab49-82da-43e1-9031-815dd0fc5fb8 destpool=d6d105da-939e-4e60-b369-8a4a5ed6bb0a
{
"imagestore": {
"message": "Migration completed. successful migrations: 1",
"success": true
}
}
- Verify physical size is preserved after migration (on sec2, store_id=2):
mysql> SELECT id, snapshot_id, store_id, store_role, state, physical_size, install_path
FROM cloud.snapshot_store_ref
WHERE snapshot_id IN (SELECT id FROM cloud.snapshots WHERE uuid='0d9550f3-7f93-4c5f-91f5-bd81b12daca7');
+----+-------------+----------+------------+-------+---------------+------------------------------------------------------------------------------------------+
| id | snapshot_id | store_id | store_role | state | physical_size | install_path |
+----+-------------+----------+------------+-------+---------------+------------------------------------------------------------------------------------------+
| 1 | 1 | 1 | Primary | Ready | 8589934592 | /mnt/a4e6c7f4-aad3-3063-bef4-45d2face66df/snapshots/5ce8ff95-9983-4b1f-895b-de7612d487c0 |
| 3 | 1 | 2 | Image | Ready | 1510604800 | snapshots/2/3/5ce8ff95-9983-4b1f-895b-de7612d487c0 |
+----+-------------+----------+------------+-------+---------------+------------------------------------------------------------------------------------------+
Test Result: PASS The physical size (1510604800 bytes) was correctly preserved on the destination secondary storage (store_id=2) after migration.
TC2: Verify volume snapshot physical size is preserved using migrateSecondaryStorageData API (bulk migration)
Objective: Verify that when volume snapshots are migrated using the bulk migrateSecondaryStorageData API, the physical size is correctly preserved and not set to 0.
Test Steps:
- Using the existing snapshot from TC1 (currently on sec2)
- Migrate all data from sec2 to sec1 using migrateSecondaryStorageData with migrationtype=Complete
- Verify the physical size is preserved on the destination secondary storage
Expected Result: The snapshot's physical size on the destination secondary storage should match the original physical size (1510604800 bytes), not 0.
Actual Result: The snapshot's physical size was correctly preserved as 1510604800 bytes after bulk migration to the destination secondary storage.
Test Evidence:
- Initial state - snapshot on sec2 (store_id=2) from TC1:
mysql> SELECT id, snapshot_id, store_id, store_role, state, physical_size, install_path
FROM cloud.snapshot_store_ref
WHERE snapshot_id IN (SELECT id FROM cloud.snapshots WHERE uuid='0d9550f3-7f93-4c5f-91f5-bd81b12daca7');
+----+-------------+----------+------------+-------+---------------+------------------------------------------------------------------------------------------+
| id | snapshot_id | store_id | store_role | state | physical_size | install_path |
+----+-------------+----------+------------+-------+---------------+------------------------------------------------------------------------------------------+
| 1 | 1 | 1 | Primary | Ready | 8589934592 | /mnt/a4e6c7f4-aad3-3063-bef4-45d2face66df/snapshots/5ce8ff95-9983-4b1f-895b-de7612d487c0 |
| 3 | 1 | 2 | Image | Ready | 1510604800 | snapshots/2/3/5ce8ff95-9983-4b1f-895b-de7612d487c0 |
+----+-------------+----------+------------+-------+---------------+------------------------------------------------------------------------------------------+
- Bulk migrate from sec2 to sec1:*
(localcloud) 🐱 > migrateSecondaryStorageData srcpool=d6d105da-939e-4e60-b369-8a4a5ed6bb0a destpools=09dbab49-82da-43e1-9031-815dd0fc5fb8 migrationtype=Complete
{
"imagestore": {
"message": "Migration completed. successful migrations: 3",
"migrationtype": "COMPLETE",
"success": true
}
}
- Verify physical size is preserved after migration (on sec1, store_id=1):
mysql> SELECT id, snapshot_id, store_id, store_role, state, physical_size, install_path
FROM cloud.snapshot_store_ref
WHERE snapshot_id IN (SELECT id FROM cloud.snapshots WHERE uuid='0d9550f3-7f93-4c5f-91f5-bd81b12daca7');
+----+-------------+----------+------------+-------+---------------+------------------------------------------------------------------------------------------+
| id | snapshot_id | store_id | store_role | state | physical_size | install_path |
+----+-------------+----------+------------+-------+---------------+------------------------------------------------------------------------------------------+
| 1 | 1 | 1 | Primary | Ready | 8589934592 | /mnt/a4e6c7f4-aad3-3063-bef4-45d2face66df/snapshots/5ce8ff95-9983-4b1f-895b-de7612d487c0 |
| 4 | 1 | 1 | Image | Ready | 1510604800 | snapshots/2/3/5ce8ff95-9983-4b1f-895b-de7612d487c0 |
+----+-------------+----------+------------+-------+---------------+------------------------------------------------------------------------------------------+
Test Result: PASS The physical size (1510604800 bytes) was correctly preserved on sec1 (store_id=1) after bulk migration using migrateSecondaryStorageData
TC3: Verify migrated snapshot remains usable (create and attach volume from snapshot)
Objective: Verify that a snapshot which has been migrated between secondary storages remains fully functional and can be used to create new volumes.
Test Steps:
- Using the snapshot from TC1/TC2 that has been migrated between secondary storages
- Create a new volume from the migrated snapshot
- Attach the volume to a VM to verify it's fully usable
Expected Result: Volume should be created successfully from the migrated snapshot and attach to the VM without errors.
Actual Result: Volume was created successfully with state "Ready" and attached to the VM.
Test Evidence:
- create volume from migrated snapshot:
(localcloud) 🐱 > create volume name=vol-from-snapshot snapshotid=0d9550f3-7f93-4c5f-91f5-bd81b12daca7
{
"volume": {
"id": "c28f97c6-3a19-48cc-9dd4-2641a59695b4",
"name": "vol-from-snapshot",
"snapshotid": "0d9550f3-7f93-4c5f-91f5-bd81b12daca7",
"size": 8589934592,
"state": "Ready",
"storage": "ref-trl-10708-k-Mol9-rositsa-kyuchukova-kvm-pri2",
"type": "DATADISK",
...
}
}
- Attach volume to VM:
(localcloud) 🐱 > attach volume id=c28f97c6-3a19-48cc-9dd4-2641a59695b4 virtualmachineid=4e98d18e-3f1c-4d9f-8b64-b2facc7e8e71
{
"volume": {
"id": "c28f97c6-3a19-48cc-9dd4-2641a59695b4",
"name": "vol-from-snapshot",
"attached": "2026-01-26T07:15:30+0000",
"deviceid": 1,
"state": "Ready",
"virtualmachineid": "4e98d18e-3f1c-4d9f-8b64-b2facc7e8e71",
"vmdisplayname": "test-vm",
"vmstate": "Stopped",
...
}
}
Test Result: PASS
* 4.22: fix install path for systemvm templates when introducing new sec storage (#11605) fix Sensitive Data Exposure Through Exception Logging in OVM Hypervis… (#12032) Fix snapshot physical size after migration (#12166) ConfigDrive: use file absolute path instead of canonical path to create ISO (#11623) Add log for null templateVO (#12406) snapshot: fix listSnapshots for volume which got delete and whose storage pool got deleted (#12433) Notify user if template upgrade is not required (#12483) Fix: proper permissions for systemvm template registrations on hardened systems (#12098) Allow modification of user vm details if user.vm.readonly.details is empty (#10456) NPE fix while deleting storage pool when pool has detached volumes (#12451)
8 participants
Description
When migrating a volume snapshot from one secondary storage to another using the
migrateSecondaryStorageDataandmigrateResourceToAnotherSecondaryStorageAPIs, its physical size is set to0.This behavior was changed to preserve the physical size of the snapshot after migration.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
I migrated a volume snapshot using the
migrateSecondaryStorageDataandmigrateResourceToAnotherSecondaryStorageAPIs and verified that its physical size had not changed.How did you try to break this feature and the system with this change?