Skip to content

Commit bae63ac

Browse files
committed
snapshot: preserve Xen snapshot chaining through hidden refs
Source local main commit: - bb635f652f snapshot: preserve Xen snapshot chaining through hidden refs Source Apache commits: - 2a60305 Fix snapshot chaining on Xen (apache#12597) Change summary: - add a DAO method that lists snapshot-store refs by snapshot id, role, and a set of states - update DefaultSnapshotStrategy.getSnapshotImageStoreRef(...) to consider both Ready and Hidden image-store refs - align the unit test with the new DAO method and remove redundant null-path stubbing - record Record 040 and mark 8608b4e as already satisfied in the history document Functional impact: - preserves Xen incremental snapshot chain lookup even when a parent snapshot is hidden on secondary storage - reduces the chance of losing the expected parent chain and falling back to an incorrect full backup path - keeps zone-scoped image-store lookup while widening acceptable persisted states Validation: - cherry-pick from main applied cleanly on ablestack-europa with no additional manual conflict resolution - mvn/mvnw-based tests not run in this environment by request
1 parent 51d1aa5 commit bae63ac

5 files changed

Lines changed: 48 additions & 7 deletions

File tree

developer/history/apache-main-sync-2026-04-17.md

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1058,8 +1058,37 @@
10581058
- Resolution notes:
10591059
- Preserved the europa `snapshotInfo.setVmSnapshotName(...)` behavior and added Apache's early `snapshotsForRollback.add(snapshotInfo)` registration alongside it
10601060

1061+
### Record 040 - include hidden image-store refs when resolving Xen snapshot chains
1062+
1063+
- Local branch: `main`
1064+
- Local commit: `bb635f652f`
1065+
- Source Apache commits:
1066+
- `2a60305792` Fix snapshot chaining on Xen (#12597)
1067+
- Summary:
1068+
- Add a DAO method that lists snapshot-store refs by snapshot id, role, and a set of states
1069+
- Update `DefaultSnapshotStrategy.getSnapshotImageStoreRef(...)` to consider both `Ready` and `Hidden` image-store refs
1070+
- Align the unit test with the new DAO method and remove the redundant null-path stubbing
1071+
- Functional impact:
1072+
- Preserves Xen incremental snapshot chain lookup even when a parent snapshot is hidden on secondary storage
1073+
- Reduces the chance of losing the expected parent chain and falling back to an incorrect full backup path
1074+
- Keeps snapshot image-store lookup limited to the target zone while widening the acceptable persisted states
1075+
- Validation:
1076+
- Apache cherry-pick required manual conflict resolution on `main` only in `SnapshotDataStoreDaoImpl` because this branch already carried a local `idStateNeqSearch` builder for non-destroyed snapshot lookups
1077+
- The resolved DAO keeps the local `idStateNeqSearch` behavior and adds Apache's `idEqRoleEqStateInSearch` path for `Ready`/`Hidden` image-store lookup
1078+
- Maven-based Java test execution has not been run yet in this environment by request
1079+
- Europa cherry-pick status:
1080+
- `Applied cleanly on ablestack-europa; local commit pending creation`
1081+
- Conflict notes:
1082+
- `SnapshotDataStoreDaoImpl` conflicted on `main` because Apache adds a new state-in search builder in the same initialization block where this branch already introduced `idStateNeqSearch`
1083+
- Resolution notes:
1084+
- Preserved the local `idStateNeqSearch` initialization and added the Apache `idEqRoleEqStateInSearch` builder without changing the branch-local non-destroyed lookup behavior
1085+
10611086
### Observed Already Satisfied
10621087

1088+
- `8608b4edd0` `Fix snapshot copy resource limit concurrency`
1089+
- Current branch state already wraps snapshot-chain copy reservation in `CheckedReservation` and routes per-snapshot copy through `copySnapshotToZone(..., shouldCheckResourceLimits)`
1090+
- `SnapshotManagerImplTest` no longer stubs the removed direct `checkResourceLimit(...)` call in the covered copy flow
1091+
- Treat as already satisfied instead of creating a duplicate local commit
10631092
- `2359061f66` `api: remove required flag of gatewayid in CreateStaticRouteCmd (#12786)`
10641093
- Current branch state already has `gatewayId` without `required = true`
10651094
- Treat as already satisfied instead of creating a duplicate local commit

engine/schema/src/main/java/org/apache/cloudstack/storage/datastore/db/SnapshotDataStoreDao.java

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,8 @@ public interface SnapshotDataStoreDao extends GenericDao<SnapshotDataStoreVO, Lo
5151

5252
SnapshotDataStoreVO findBySnapshotIdAndDataStoreRoleAndState(long snapshotId, DataStoreRole role, ObjectInDataStoreStateMachine.State state);
5353

54+
List<SnapshotDataStoreVO> listBySnapshotIdAndDataStoreRoleAndStateIn(long snapshotId, DataStoreRole role, ObjectInDataStoreStateMachine.State... state);
55+
5456
List<SnapshotDataStoreVO> listReadyByVolumeIdAndCheckpointPathNotNull(long volumeId);
5557

5658
SnapshotDataStoreVO findOneBySnapshotId(long snapshotId, long zoneId);

engine/schema/src/main/java/org/apache/cloudstack/storage/datastore/db/SnapshotDataStoreDaoImpl.java

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,7 @@ public class SnapshotDataStoreDaoImpl extends GenericDaoBase<SnapshotDataStoreVO
7070
private SearchBuilder<SnapshotDataStoreVO> stateSearch;
7171
private SearchBuilder<SnapshotDataStoreVO> idStateNinSearch;
7272
private SearchBuilder<SnapshotDataStoreVO> idStateNeqSearch;
73+
private SearchBuilder<SnapshotDataStoreVO> idEqRoleEqStateInSearch;
7374
protected SearchBuilder<SnapshotVO> snapshotVOSearch;
7475
private SearchBuilder<SnapshotDataStoreVO> snapshotCreatedSearch;
7576
private SearchBuilder<SnapshotDataStoreVO> dataStoreAndInstallPathSearch;
@@ -158,6 +159,11 @@ public boolean configure(String name, Map<String, Object> params) throws Configu
158159
idStateNeqSearch.and(STATE, idStateNeqSearch.entity().getState(), SearchCriteria.Op.NEQ);
159160
idStateNeqSearch.done();
160161

162+
idEqRoleEqStateInSearch = createSearchBuilder();
163+
idEqRoleEqStateInSearch.and(SNAPSHOT_ID, idEqRoleEqStateInSearch.entity().getSnapshotId(), SearchCriteria.Op.EQ);
164+
idEqRoleEqStateInSearch.and(STORE_ROLE, idEqRoleEqStateInSearch.entity().getRole(), SearchCriteria.Op.EQ);
165+
idEqRoleEqStateInSearch.and(STATE, idEqRoleEqStateInSearch.entity().getState(), SearchCriteria.Op.IN);
166+
161167
snapshotVOSearch = snapshotDao.createSearchBuilder();
162168
snapshotVOSearch.and(VOLUME_ID, snapshotVOSearch.entity().getVolumeId(), SearchCriteria.Op.EQ);
163169
snapshotVOSearch.done();
@@ -394,6 +400,15 @@ public SnapshotDataStoreVO findBySnapshotIdAndDataStoreRoleAndState(long snapsho
394400
return findOneBy(sc);
395401
}
396402

403+
@Override
404+
public List<SnapshotDataStoreVO> listBySnapshotIdAndDataStoreRoleAndStateIn(long snapshotId, DataStoreRole role, State... state) {
405+
SearchCriteria<SnapshotDataStoreVO> sc = idEqRoleEqStateInSearch.create();
406+
sc.setParameters(SNAPSHOT_ID, snapshotId);
407+
sc.setParameters(STORE_ROLE, role);
408+
sc.setParameters(STATE, (Object[])state);
409+
return listBy(sc);
410+
}
411+
397412
@Override
398413
public SnapshotDataStoreVO findOneBySnapshotId(long snapshotId, long zoneId) {
399414
try (TransactionLegacy transactionLegacy = TransactionLegacy.currentTxn()) {

engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,7 @@ public class DefaultSnapshotStrategy extends SnapshotStrategyBase {
119119
private final List<Snapshot.State> snapshotStatesAbleToDeleteSnapshot = Arrays.asList(Snapshot.State.Destroying, Snapshot.State.Destroyed, Snapshot.State.Error, Snapshot.State.Hidden);
120120

121121
public SnapshotDataStoreVO getSnapshotImageStoreRef(long snapshotId, long zoneId) {
122-
List<SnapshotDataStoreVO> snaps = snapshotStoreDao.listReadyBySnapshot(snapshotId, DataStoreRole.Image);
122+
List<SnapshotDataStoreVO> snaps = snapshotStoreDao.listBySnapshotIdAndDataStoreRoleAndStateIn(snapshotId, DataStoreRole.Image, State.Ready, State.Hidden);
123123
for (SnapshotDataStoreVO ref : snaps) {
124124
if (zoneId == dataStoreMgr.getStoreZoneId(ref.getDataStoreId(), ref.getRole())) {
125125
return ref;

engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategyTest.java

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -257,11 +257,6 @@ public void verifyIfTheSnapshotIsBeingUsedByAnyVolumeTestDetailsIsNotEmptyThrowC
257257

258258
@Test
259259
public void testGetSnapshotImageStoreRefNull() {
260-
SnapshotDataStoreVO ref1 = Mockito.mock(SnapshotDataStoreVO.class);
261-
Mockito.when(ref1.getDataStoreId()).thenReturn(1L);
262-
Mockito.when(ref1.getRole()).thenReturn(DataStoreRole.Image);
263-
Mockito.when(snapshotDataStoreDao.listReadyBySnapshot(Mockito.anyLong(), Mockito.any(DataStoreRole.class))).thenReturn(List.of(ref1));
264-
Mockito.when(dataStoreManager.getStoreZoneId(1L, DataStoreRole.Image)).thenReturn(2L);
265260
Assert.assertNull(defaultSnapshotStrategySpy.getSnapshotImageStoreRef(1L, 1L));
266261
}
267262

@@ -270,7 +265,7 @@ public void testGetSnapshotImageStoreRefNotNull() {
270265
SnapshotDataStoreVO ref1 = Mockito.mock(SnapshotDataStoreVO.class);
271266
Mockito.when(ref1.getDataStoreId()).thenReturn(1L);
272267
Mockito.when(ref1.getRole()).thenReturn(DataStoreRole.Image);
273-
Mockito.when(snapshotDataStoreDao.listReadyBySnapshot(Mockito.anyLong(), Mockito.any(DataStoreRole.class))).thenReturn(List.of(ref1));
268+
Mockito.when(snapshotDataStoreDao.listBySnapshotIdAndDataStoreRoleAndStateIn(Mockito.anyLong(), Mockito.any(DataStoreRole.class), Mockito.any(), Mockito.any())).thenReturn(List.of(ref1));
274269
Mockito.when(dataStoreManager.getStoreZoneId(1L, DataStoreRole.Image)).thenReturn(1L);
275270
Assert.assertNotNull(defaultSnapshotStrategySpy.getSnapshotImageStoreRef(1L, 1L));
276271
}

0 commit comments

Comments
 (0)