This is an automated email from the ASF dual-hosted git repository.
weizhouapache pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/cloudstack.git
The following commit(s) were added to refs/heads/main by this push:
new ba837fb8a80 Incremental nas backup fixes (#13571)
ba837fb8a80 is described below
commit ba837fb8a801a4175b49533318a6e7533f59edb4
Author: Abhisar Sinha <[email protected]>
AuthorDate: Fri Jul 10 11:50:19 2026 +0530
Incremental nas backup fixes (#13571)
* Avoid concurrent deletes modifying the same backup chain in parallel -
Backup deletes are serialized by using a per-VM GlobalLock
Backup delete checks if it has a live dependent backup not just a live
immediate child.
---
.../cloudstack/backup/NASBackupProvider.java | 187 +++++++---------
.../cloudstack/backup/NASBackupProviderTest.java | 243 ++++++++++++++++++++-
test/integration/smoke/test_backup_recovery_nas.py | 35 +--
3 files changed, 332 insertions(+), 133 deletions(-)
diff --git
a/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java
b/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java
index fe56fbf7c1a..f5fecc5de6f 100644
---
a/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java
+++
b/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java
@@ -38,6 +38,7 @@ import com.cloud.storage.dao.StoragePoolHostDao;
import com.cloud.storage.dao.VolumeDao;
import com.cloud.utils.Pair;
import com.cloud.utils.component.AdapterBase;
+import com.cloud.utils.db.GlobalLock;
import com.cloud.utils.exception.CloudRuntimeException;
import com.cloud.user.ResourceLimitService;
import com.cloud.vm.VMInstanceDetailVO;
@@ -83,6 +84,8 @@ import static
org.apache.cloudstack.backup.BackupManager.BackupFrameworkEnabled;
public class NASBackupProvider extends AdapterBase implements BackupProvider,
Configurable {
private static final Logger LOG =
LogManager.getLogger(NASBackupProvider.class);
+ private static final int CHAIN_LOCK_TIMEOUT_SECONDS = 300;
+
ConfigKey<Integer> NASBackupRestoreMountTimeout = new
ConfigKey<>("Advanced", Integer.class,
"nas.backup.restore.mount.timeout",
"30",
@@ -639,7 +642,7 @@ public class NASBackupProvider extends AdapterBase
implements BackupProvider, Co
}
} else {
logger.error("Failed to take backup for VM {}: {}",
vm.getInstanceName(), answer != null ? answer.getDetails() : "No answer
received");
- if (answer.getNeedsCleanup()) {
+ if (answer != null && answer.getNeedsCleanup()) {
logger.error("Backup cleanup failed for VM {}. Leaving the
backup in Error state. Backup should be manually deleted to free up the space",
vm.getInstanceName());
backupVO.setStatus(Backup.Status.Error);
backupDao.update(backupVO.getId(), backupVO);
@@ -912,26 +915,58 @@ public class NASBackupProvider extends AdapterBase
implements BackupProvider, Co
return deleteBackupFileAndRow(backup, backupRepository, host);
}
- // Snapshot-style cascade: defer the on-NAS rm + DB row while there
are live children,
- // mark this backup as delete-pending, and let the leaf's deletion
sweep it up later.
- // See DefaultSnapshotStrategy#deleteSnapshotChain for the same
pattern on incremental
- // snapshots. forced=true means the caller wants the entire subtree
gone right now.
- if (forced) {
- return cascadeDeleteSubtree(backup, backupRepository, host);
- }
+ // Concurrent deletes can mutate the same chain concurrently resulting
in inconsistent states.
+ // take a per-VM lock before modifying the chain.
+ final GlobalLock chainLock = acquireChainDeleteLock(backup.getVmId());
+ try {
+ Backup current = backupDao.findById(backup.getId());
+ if (current == null) {
+ LOG.debug("Backup {} was already removed by a concurrent chain
delete", backup.getUuid());
+ return true;
+ }
+ backup = current;
+
+ // Snapshot-style cascade: defer the on-NAS rm + DB row while
there are live children,
+ // mark this backup as delete-pending, and let the leaf's deletion
sweep it up later.
+ // See DefaultSnapshotStrategy#deleteSnapshotChain for the same
pattern on incremental
+ // snapshots. forced=true means the caller wants the entire
subtree gone right now.
+ if (forced) {
+ return cascadeDeleteSubtree(backup, backupRepository, host);
+ }
- List<Backup> liveChildren = findLiveChildren(backup);
- if (!liveChildren.isEmpty()) {
- markDeletePending(backup);
- LOG.debug("Backup {} has {} live child backup(s); marking as
delete-pending. The on-NAS file " +
- "and DB row will be removed once the last
descendant is gone, or pass forced=true.",
- backup.getUuid(), liveChildren.size());
- return true;
+ if (hasLiveChildren(backup)) {
+ markDeletePending(backup);
+ LOG.debug("Backup {} has live descendants in its chain;
setting status as Hidden. " +
+ "The on-NAS file and DB row will be removed
once the last descendant is gone, " +
+ "or pass forced=true.",
+ backup.getUuid());
+ return true;
+ }
+
+ // No live children — physically delete this backup, then walk up
the chain and
+ // collect any ancestors that were left in Hidden state.
+ return deleteLeafBackupAndSweepPendingAncestors(backup,
backupRepository, host);
+ } finally {
+ releaseChainDeleteLock(chainLock);
}
+ }
+
+ /**
+ * Take the per-VM lock that serializes chain-mutating backup deletes.
+ */
+ protected GlobalLock acquireChainDeleteLock(long vmId) {
+ GlobalLock lock = GlobalLock.getInternLock("nas.backup.chain.vm." +
vmId);
+ if (!lock.lock(CHAIN_LOCK_TIMEOUT_SECONDS)) {
+ lock.releaseRef();
+ throw new CloudRuntimeException(String.format(
+ "Timed out waiting for concurrent backup chain operations
on VM %d to finish, please try again", vmId));
+ }
+ return lock;
+ }
- // No live children — physically delete this backup, then walk up the
chain and
- // collect any ancestors that were left in delete-pending state.
- return deleteLeafBackupAndSweepPendingAncestors(backup,
backupRepository, host);
+ protected void releaseChainDeleteLock(GlobalLock lock) {
+ lock.unlock();
+ lock.releaseRef();
}
/**
@@ -951,7 +986,7 @@ public class NASBackupProvider extends AdapterBase
implements BackupProvider, Co
throw new CloudRuntimeException("Operation to delete backup timed
out, please try again");
}
if (answer == null || !answer.getResult()) {
- logger.warn("Failed to delete backup file for {} ({}); leaving DB
row intact",
+ logger.error("Failed to delete backup file for {} ({}); leaving DB
row intact",
backup.getUuid(), backup.getExternalId());
return false;
}
@@ -1000,53 +1035,24 @@ public class NASBackupProvider extends AdapterBase
implements BackupProvider, Co
}
/**
- * Return the live (not delete-pending, not Removed) children of {@code
parent} within the
- * same chain. Equivalent to "incrementals whose parent_backup_id points
at parent".
+ * Whether there are any live (not tombstoned/Hidden, not Removed)
descendants of {@code backup} within the
+ * same chain. This uses {@code CHAIN_POSITION} to detect dependents
deeper in the chain.
*/
- private List<Backup> findLiveChildren(Backup parent) {
- String parentUuid = parent.getUuid();
- String chainId = readDetail(parent, NASBackupChainKeys.CHAIN_ID);
- if (parentUuid == null || chainId == null) {
- return Collections.emptyList();
- }
- List<Backup> children = new ArrayList<>();
- for (Backup b : backupDao.listByVmId(null, parent.getVmId())) {
- if (b.getId() == parent.getId()) {
- continue;
- }
- if (!chainId.equals(readDetail(b, NASBackupChainKeys.CHAIN_ID))) {
- continue;
- }
- if (!parentUuid.equals(readDetail(b,
NASBackupChainKeys.PARENT_BACKUP_ID))) {
- continue;
- }
- if (isDeletePending(b)) {
- // Tombstoned children don't keep us alive — they're already
on the way out.
- continue;
- }
- children.add(b);
- }
- return children;
- }
-
- /**
- * Look up this backup's immediate parent in the chain (by {@code
PARENT_BACKUP_ID}).
- * Returns {@code null} if this is the full (no parent) or the parent row
is gone.
- *
- * <p>Prefer {@link #getChainOrderedLeafToRoot(Backup)} when walking the
whole chain —
- * this method hits the DB on each call and is O(N²) when used in a loop.
- */
- private Backup findChainParent(Backup backup) {
- String parentUuid = readDetail(backup,
NASBackupChainKeys.PARENT_BACKUP_ID);
- if (parentUuid == null || parentUuid.isEmpty()) {
- return null;
+ private boolean hasLiveChildren(Backup backup) {
+ String chainId = readDetail(backup, NASBackupChainKeys.CHAIN_ID);
+ if (chainId == null) {
+ return false;
}
+ int position = chainPosition(backup);
for (Backup b : backupDao.listByVmId(null, backup.getVmId())) {
- if (parentUuid.equals(b.getUuid())) {
- return b;
+ if (b.getId() == backup.getId() || !chainId.equals(readDetail(b,
NASBackupChainKeys.CHAIN_ID))) {
+ continue;
+ }
+ if (!isDeletePending(b) && chainPosition(b) > position) {
+ return true;
}
}
- return null;
+ return false;
}
/**
@@ -1078,7 +1084,7 @@ public class NASBackupProvider extends AdapterBase
implements BackupProvider, Co
/**
* Physically delete the leaf {@code backup}, then walk up the chain while
each ancestor
- * is in delete-pending state. Mirrors the snapshot subsystem pattern:
once a leaf is
+ * is in Hidden state. Mirrors the snapshot subsystem pattern: once a leaf
is
* gone, garbage-collect any tombstoned parents.
*
* <p>Caller must guarantee {@code backup} is a leaf (no live children).
Each tombstoned
@@ -1092,23 +1098,14 @@ public class NASBackupProvider extends AdapterBase
implements BackupProvider, Co
if (!deleteBackupFileAndRow(backup, repo, host)) {
return false;
}
- // Walk the snapshot from leaf+1 upward, deleting tombstoned ancestors
until a live
- // one is reached or the root is past.
- int leafIdx = indexOfBackupById(chain, backup.getId());
- if (leafIdx < 0) {
- // Leaf wasn't in its own CHAIN_ID list — degenerate case, nothing
more to sweep.
- return true;
- }
- for (int i = leafIdx + 1; i < chain.size(); i++) {
- Backup ancestor = chain.get(i);
- if (!isDeletePending(ancestor)) {
- break;
+ for (Backup member : chain) {
+ if (member.getId() == backup.getId()) {
+ continue;
}
- if (!deleteBackupFileAndRow(ancestor, repo, host)) {
- // Stop the sweep; the rest of the tombstoned chain will be
collected on a
- // future delete that re-runs the sweep.
- return true;
+ if (!isDeletePending(member)) {
+ break;
}
+ deleteBackupFileAndRow(member, repo, host);
}
return true;
}
@@ -1127,42 +1124,6 @@ public class NASBackupProvider extends AdapterBase
implements BackupProvider, Co
return true;
}
- private static int indexOfBackupById(List<Backup> chain, long id) {
- for (int i = 0; i < chain.size(); i++) {
- if (chain.get(i).getId() == id) {
- return i;
- }
- }
- return -1;
- }
-
- /**
- * Return the backup with the highest {@code CHAIN_POSITION} sharing
{@code root}'s
- * {@code CHAIN_ID}. Returns {@code root} if it has no chain metadata or
is itself the tail.
- */
- private Backup findChainTail(Backup root) {
- String chainId = readDetail(root, NASBackupChainKeys.CHAIN_ID);
- if (chainId == null) {
- return root;
- }
- Backup tail = root;
- int tailPos = chainPosition(root);
- for (Backup b : backupDao.listByVmId(null, root.getVmId())) {
- if (b.getId() == root.getId()) {
- continue;
- }
- if (!chainId.equals(readDetail(b, NASBackupChainKeys.CHAIN_ID))) {
- continue;
- }
- int pos = chainPosition(b);
- if (pos > tailPos) {
- tail = b;
- tailPos = pos;
- }
- }
- return tail;
- }
-
private int chainPosition(Backup b) {
String s = readDetail(b, NASBackupChainKeys.CHAIN_POSITION);
if (s == null) {
diff --git
a/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/NASBackupProviderTest.java
b/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/NASBackupProviderTest.java
index 3ba7dbad041..cd08378b926 100644
---
a/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/NASBackupProviderTest.java
+++
b/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/NASBackupProviderTest.java
@@ -55,6 +55,7 @@ import com.cloud.storage.dao.DiskOfferingDao;
import com.cloud.storage.dao.VolumeDao;
import com.cloud.user.ResourceLimitService;
import com.cloud.utils.Pair;
+import com.cloud.utils.db.GlobalLock;
import com.cloud.vm.VMInstanceDetailVO;
import com.cloud.vm.VMInstanceVO;
import com.cloud.vm.VirtualMachine;
@@ -679,9 +680,11 @@ public class NASBackupProviderTest {
// CHAIN_ID on the parent => not the no-chain fast path.
BackupDetailVO chainIdDetail = new BackupDetailVO(50L,
NASBackupChainKeys.CHAIN_ID, "chain-1", true);
+ BackupDetailVO chainPosDetail = new BackupDetailVO(50L,
NASBackupChainKeys.CHAIN_POSITION, "0", true);
Mockito.when(backupDetailsDao.findDetail(50L,
NASBackupChainKeys.CHAIN_ID)).thenReturn(chainIdDetail);
+ Mockito.when(backupDetailsDao.findDetail(50L,
NASBackupChainKeys.CHAIN_POSITION)).thenReturn(chainPosDetail);
- // A live child references parent-uuid via PARENT_BACKUP_ID.
+ // A live child sits deeper in the chain (higher CHAIN_POSITION).
BackupVO child = new BackupVO();
child.setVmId(vmId);
child.setBackupOfferingId(offeringId);
@@ -692,13 +695,14 @@ public class NASBackupProviderTest {
ReflectionTestUtils.setField(child, "uuid", "child-uuid");
BackupDetailVO childChainId = new BackupDetailVO(51L,
NASBackupChainKeys.CHAIN_ID, "chain-1", true);
- BackupDetailVO childParent = new BackupDetailVO(51L,
NASBackupChainKeys.PARENT_BACKUP_ID, "parent-uuid", true);
+ BackupDetailVO childChainPos = new BackupDetailVO(51L,
NASBackupChainKeys.CHAIN_POSITION, "1", true);
Mockito.when(backupDetailsDao.findDetail(51L,
NASBackupChainKeys.CHAIN_ID)).thenReturn(childChainId);
- Mockito.when(backupDetailsDao.findDetail(51L,
NASBackupChainKeys.PARENT_BACKUP_ID)).thenReturn(childParent);
+ Mockito.when(backupDetailsDao.findDetail(51L,
NASBackupChainKeys.CHAIN_POSITION)).thenReturn(childChainPos);
Mockito.when(backupDao.listByVmId(null,
vmId)).thenReturn(List.of(parent, child));
- // markDeletePending loads the row to flip its status to Hidden.
+ // Re-read under the chain lock + markDeletePending both load the row
by id.
Mockito.when(backupDao.findById(50L)).thenReturn(parent);
+
Mockito.doReturn(mock(GlobalLock.class)).when(nasBackupProvider).acquireChainDeleteLock(vmId);
boolean result = nasBackupProvider.deleteBackup(parent, false);
Assert.assertTrue(result);
@@ -771,13 +775,13 @@ public class NASBackupProviderTest {
parent.setStatus(Backup.Status.Hidden);
Mockito.when(backupDetailsDao.findDetail(50L,
NASBackupChainKeys.CHAIN_ID)).thenReturn(parentChainId);
Mockito.when(backupDetailsDao.findDetail(50L,
NASBackupChainKeys.CHAIN_POSITION)).thenReturn(parentChainPos);
- // Parent has no parent of its own (it's the full anchor).
- Mockito.when(backupDetailsDao.findDetail(50L,
NASBackupChainKeys.PARENT_BACKUP_ID)).thenReturn(null);
- // listByVmId is called once now (chain snapshot taken before the leaf
delete).
// We still use a mutable list + remove() answer so the DAO contract
is realistic.
java.util.List<Backup> liveBackups = new
java.util.ArrayList<>(List.of(parent, leaf));
Mockito.when(backupDao.listByVmId(null, vmId)).thenAnswer(inv -> new
java.util.ArrayList<>(liveBackups));
+ // The target row is re-read under the chain lock before any chain
decision.
+ Mockito.when(backupDao.findById(51L)).thenReturn(leaf);
+
Mockito.doReturn(mock(GlobalLock.class)).when(nasBackupProvider).acquireChainDeleteLock(vmId);
// Agent acknowledges every delete.
Mockito.when(agentManager.send(Mockito.anyLong(),
Mockito.any(DeleteBackupCommand.class)))
@@ -805,4 +809,229 @@ public class NASBackupProviderTest {
Mockito.verify(resourceLimitMgr, Mockito.times(2))
.decrementResourceCount(Mockito.anyLong(),
Mockito.eq(Resource.ResourceType.backup_storage), Mockito.any());
}
+
+ @Test
+ public void deletingLastLiveMemberCollectsDeeperOrphanTombstones()
+ throws AgentUnavailableException, OperationTimedoutException {
+ Long zoneId = 1L;
+ Long vmId = 2L;
+ Long hostId = 3L;
+ Long offeringId = 4L;
+
+ BackupVO full = new BackupVO();
+ full.setVmId(vmId);
+ full.setBackupOfferingId(offeringId);
+ full.setExternalId("i-2-2-VM/2026.05.10.10.00.00");
+ full.setZoneId(zoneId);
+ full.setStatus(Backup.Status.BackedUp);
+ ReflectionTestUtils.setField(full, "id", 50L);
+
+ // The last live incremental — the one being deleted.
+ BackupVO inc1 = new BackupVO();
+ inc1.setVmId(vmId);
+ inc1.setBackupOfferingId(offeringId);
+ inc1.setExternalId("i-2-2-VM/2026.05.10.10.30.00");
+ inc1.setZoneId(zoneId);
+ inc1.setStatus(Backup.Status.BackedUp);
+ ReflectionTestUtils.setField(inc1, "id", 51L);
+
+ // A stranded tombstone deeper in the chain: its own descendants are
long gone, so
+ // only a sweep triggered by an ancestor's deletion can ever collect
it.
+ BackupVO orphan = new BackupVO();
+ orphan.setVmId(vmId);
+ orphan.setBackupOfferingId(offeringId);
+ orphan.setExternalId("i-2-2-VM/2026.05.10.11.00.00");
+ orphan.setZoneId(zoneId);
+ orphan.setStatus(Backup.Status.Hidden);
+ ReflectionTestUtils.setField(orphan, "id", 52L);
+
+ VMInstanceVO vm = mock(VMInstanceVO.class);
+ Mockito.when(vm.getLastHostId()).thenReturn(hostId);
+ HostVO host = mock(HostVO.class);
+ Mockito.when(host.getStatus()).thenReturn(Status.Up);
+ Mockito.when(host.getId()).thenReturn(hostId);
+ Mockito.when(hostDao.findById(hostId)).thenReturn(host);
+
+ BackupRepositoryVO repo = new BackupRepositoryVO(1L, "nas",
"test-repo",
+ "nfs", "address", "sync", 1024L, null);
+
Mockito.when(backupRepositoryDao.findByBackupOfferingId(offeringId)).thenReturn(repo);
+
Mockito.when(vmInstanceDao.findByIdIncludingRemoved(vmId)).thenReturn(vm);
+
+ Mockito.when(backupDetailsDao.findDetail(50L,
NASBackupChainKeys.CHAIN_ID))
+ .thenReturn(new BackupDetailVO(50L,
NASBackupChainKeys.CHAIN_ID, "chain-1", true));
+ Mockito.when(backupDetailsDao.findDetail(50L,
NASBackupChainKeys.CHAIN_POSITION))
+ .thenReturn(new BackupDetailVO(50L,
NASBackupChainKeys.CHAIN_POSITION, "0", true));
+ Mockito.when(backupDetailsDao.findDetail(51L,
NASBackupChainKeys.CHAIN_ID))
+ .thenReturn(new BackupDetailVO(51L,
NASBackupChainKeys.CHAIN_ID, "chain-1", true));
+ Mockito.when(backupDetailsDao.findDetail(51L,
NASBackupChainKeys.CHAIN_POSITION))
+ .thenReturn(new BackupDetailVO(51L,
NASBackupChainKeys.CHAIN_POSITION, "1", true));
+ Mockito.when(backupDetailsDao.findDetail(52L,
NASBackupChainKeys.CHAIN_ID))
+ .thenReturn(new BackupDetailVO(52L,
NASBackupChainKeys.CHAIN_ID, "chain-1", true));
+ Mockito.when(backupDetailsDao.findDetail(52L,
NASBackupChainKeys.CHAIN_POSITION))
+ .thenReturn(new BackupDetailVO(52L,
NASBackupChainKeys.CHAIN_POSITION, "2", true));
+
+ Mockito.when(backupDao.listByVmId(null,
vmId)).thenReturn(List.of(full, inc1, orphan));
+ Mockito.when(backupDao.findById(51L)).thenReturn(inc1);
+
Mockito.doReturn(mock(GlobalLock.class)).when(nasBackupProvider).acquireChainDeleteLock(vmId);
+
+ Mockito.when(agentManager.send(Mockito.anyLong(),
Mockito.any(DeleteBackupCommand.class)))
+ .thenReturn(new BackupAnswer(new DeleteBackupCommand(null,
null, null, null), true, "ok"));
+
+ Assert.assertTrue(nasBackupProvider.deleteBackup(inc1, false));
+
+ // inc1 and the deeper orphan tombstone are physically removed; the
live full survives.
+ Mockito.verify(agentManager, Mockito.times(2))
+ .send(Mockito.anyLong(),
Mockito.any(DeleteBackupCommand.class));
+ Mockito.verify(backupDao).remove(51L);
+ Mockito.verify(backupDao).remove(52L);
+ Mockito.verify(backupDao, Mockito.never()).remove(50L);
+ }
+
+ @Test
+ public void deletingAncestorOfTombstoneWithLiveDescendantTombstonesIt()
+ throws AgentUnavailableException, OperationTimedoutException {
+ Long zoneId = 1L;
+ Long vmId = 2L;
+ Long hostId = 3L;
+ Long offeringId = 4L;
+
+ BackupVO full = new BackupVO();
+ full.setVmId(vmId);
+ full.setBackupOfferingId(offeringId);
+ full.setExternalId("i-2-2-VM/2026.05.10.10.00.00");
+ full.setZoneId(zoneId);
+ full.setStatus(Backup.Status.BackedUp);
+ ReflectionTestUtils.setField(full, "id", 50L);
+
+ BackupVO inc1 = new BackupVO();
+ inc1.setVmId(vmId);
+ inc1.setBackupOfferingId(offeringId);
+ inc1.setExternalId("i-2-2-VM/2026.05.10.10.30.00");
+ inc1.setZoneId(zoneId);
+ inc1.setStatus(Backup.Status.Hidden);
+ ReflectionTestUtils.setField(inc1, "id", 51L);
+
+ BackupVO inc2 = new BackupVO();
+ inc2.setVmId(vmId);
+ inc2.setBackupOfferingId(offeringId);
+ inc2.setExternalId("i-2-2-VM/2026.05.10.11.00.00");
+ inc2.setZoneId(zoneId);
+ inc2.setStatus(Backup.Status.BackedUp);
+ ReflectionTestUtils.setField(inc2, "id", 52L);
+
+ VMInstanceVO vm = mock(VMInstanceVO.class);
+ Mockito.when(vm.getLastHostId()).thenReturn(hostId);
+ HostVO host = mock(HostVO.class);
+ Mockito.when(host.getStatus()).thenReturn(Status.Up);
+ Mockito.when(hostDao.findById(hostId)).thenReturn(host);
+
+ BackupRepositoryVO repo = new BackupRepositoryVO(1L, "nas",
"test-repo",
+ "nfs", "address", "sync", 1024L, null);
+
Mockito.when(backupRepositoryDao.findByBackupOfferingId(offeringId)).thenReturn(repo);
+
Mockito.when(vmInstanceDao.findByIdIncludingRemoved(vmId)).thenReturn(vm);
+
+ Mockito.when(backupDetailsDao.findDetail(50L,
NASBackupChainKeys.CHAIN_ID))
+ .thenReturn(new BackupDetailVO(50L,
NASBackupChainKeys.CHAIN_ID, "chain-1", true));
+ Mockito.when(backupDetailsDao.findDetail(50L,
NASBackupChainKeys.CHAIN_POSITION))
+ .thenReturn(new BackupDetailVO(50L,
NASBackupChainKeys.CHAIN_POSITION, "0", true));
+ Mockito.when(backupDetailsDao.findDetail(51L,
NASBackupChainKeys.CHAIN_ID))
+ .thenReturn(new BackupDetailVO(51L,
NASBackupChainKeys.CHAIN_ID, "chain-1", true));
+ Mockito.when(backupDetailsDao.findDetail(52L,
NASBackupChainKeys.CHAIN_ID))
+ .thenReturn(new BackupDetailVO(52L,
NASBackupChainKeys.CHAIN_ID, "chain-1", true));
+ Mockito.when(backupDetailsDao.findDetail(52L,
NASBackupChainKeys.CHAIN_POSITION))
+ .thenReturn(new BackupDetailVO(52L,
NASBackupChainKeys.CHAIN_POSITION, "2", true));
+
+ Mockito.when(backupDao.listByVmId(null,
vmId)).thenReturn(List.of(full, inc1, inc2));
+ Mockito.when(backupDao.findById(50L)).thenReturn(full);
+
Mockito.doReturn(mock(GlobalLock.class)).when(nasBackupProvider).acquireChainDeleteLock(vmId);
+
+ Assert.assertTrue(nasBackupProvider.deleteBackup(full, false));
+
+ // The full anchor must NOT be physically deleted — inc2 (live) still
restores
+ // through inc1's and full's files. It becomes a tombstone instead.
+ Mockito.verify(agentManager, Mockito.never()).send(Mockito.anyLong(),
Mockito.any(DeleteBackupCommand.class));
+ Mockito.verify(backupDao, Mockito.never()).remove(Mockito.anyLong());
+ ArgumentCaptor<BackupVO> captor =
ArgumentCaptor.forClass(BackupVO.class);
+ Mockito.verify(backupDao).update(Mockito.eq(50L), captor.capture());
+ Assert.assertEquals(Backup.Status.Hidden,
captor.getValue().getStatus());
+ }
+
+ @Test
+ public void sweepContinuesPastFailedTombstoneDelete()
+ throws AgentUnavailableException, OperationTimedoutException {
+ Long zoneId = 1L;
+ Long vmId = 2L;
+ Long hostId = 3L;
+ Long offeringId = 4L;
+
+ BackupVO full = new BackupVO();
+ full.setVmId(vmId);
+ full.setBackupOfferingId(offeringId);
+ full.setExternalId("i-2-2-VM/2026.05.10.10.00.00");
+ full.setZoneId(zoneId);
+ full.setStatus(Backup.Status.Hidden);
+ ReflectionTestUtils.setField(full, "id", 50L);
+
+ BackupVO inc1 = new BackupVO();
+ inc1.setVmId(vmId);
+ inc1.setBackupOfferingId(offeringId);
+ inc1.setExternalId("i-2-2-VM/2026.05.10.10.30.00");
+ inc1.setZoneId(zoneId);
+ inc1.setStatus(Backup.Status.Hidden);
+ ReflectionTestUtils.setField(inc1, "id", 51L);
+
+ // The last live member — deleting it triggers the sweep of both
tombstones.
+ BackupVO inc2 = new BackupVO();
+ inc2.setVmId(vmId);
+ inc2.setBackupOfferingId(offeringId);
+ inc2.setExternalId("i-2-2-VM/2026.05.10.11.00.00");
+ inc2.setZoneId(zoneId);
+ inc2.setStatus(Backup.Status.BackedUp);
+ ReflectionTestUtils.setField(inc2, "id", 52L);
+
+ VMInstanceVO vm = mock(VMInstanceVO.class);
+ Mockito.when(vm.getLastHostId()).thenReturn(hostId);
+ HostVO host = mock(HostVO.class);
+ Mockito.when(host.getStatus()).thenReturn(Status.Up);
+ Mockito.when(host.getId()).thenReturn(hostId);
+ Mockito.when(hostDao.findById(hostId)).thenReturn(host);
+
+ BackupRepositoryVO repo = new BackupRepositoryVO(1L, "nas",
"test-repo",
+ "nfs", "address", "sync", 1024L, null);
+
Mockito.when(backupRepositoryDao.findByBackupOfferingId(offeringId)).thenReturn(repo);
+
Mockito.when(vmInstanceDao.findByIdIncludingRemoved(vmId)).thenReturn(vm);
+
+ Mockito.when(backupDetailsDao.findDetail(50L,
NASBackupChainKeys.CHAIN_ID))
+ .thenReturn(new BackupDetailVO(50L,
NASBackupChainKeys.CHAIN_ID, "chain-1", true));
+ Mockito.when(backupDetailsDao.findDetail(50L,
NASBackupChainKeys.CHAIN_POSITION))
+ .thenReturn(new BackupDetailVO(50L,
NASBackupChainKeys.CHAIN_POSITION, "0", true));
+ Mockito.when(backupDetailsDao.findDetail(51L,
NASBackupChainKeys.CHAIN_ID))
+ .thenReturn(new BackupDetailVO(51L,
NASBackupChainKeys.CHAIN_ID, "chain-1", true));
+ Mockito.when(backupDetailsDao.findDetail(51L,
NASBackupChainKeys.CHAIN_POSITION))
+ .thenReturn(new BackupDetailVO(51L,
NASBackupChainKeys.CHAIN_POSITION, "1", true));
+ Mockito.when(backupDetailsDao.findDetail(52L,
NASBackupChainKeys.CHAIN_ID))
+ .thenReturn(new BackupDetailVO(52L,
NASBackupChainKeys.CHAIN_ID, "chain-1", true));
+ Mockito.when(backupDetailsDao.findDetail(52L,
NASBackupChainKeys.CHAIN_POSITION))
+ .thenReturn(new BackupDetailVO(52L,
NASBackupChainKeys.CHAIN_POSITION, "2", true));
+
+ Mockito.when(backupDao.listByVmId(null,
vmId)).thenReturn(List.of(full, inc1, inc2));
+ Mockito.when(backupDao.findById(52L)).thenReturn(inc2);
+
Mockito.doReturn(mock(GlobalLock.class)).when(nasBackupProvider).acquireChainDeleteLock(vmId);
+
+ // Deletes run leaf-to-root: inc2 succeeds, inc1's rm FAILS, full
succeeds.
+ DeleteBackupCommand dummy = new DeleteBackupCommand(null, null, null,
null);
+ Mockito.when(agentManager.send(Mockito.anyLong(),
Mockito.any(DeleteBackupCommand.class)))
+ .thenReturn(new BackupAnswer(dummy, true, "ok"),
+ new BackupAnswer(dummy, false, "rm failed"),
+ new BackupAnswer(dummy, true, "ok"));
+
+ Assert.assertTrue(nasBackupProvider.deleteBackup(inc2, false));
+
+ // All three members were attempted; the failed inc1 keeps its row,
full is collected.
+ Mockito.verify(agentManager, Mockito.times(3))
+ .send(Mockito.anyLong(),
Mockito.any(DeleteBackupCommand.class));
+ Mockito.verify(backupDao).remove(52L);
+ Mockito.verify(backupDao, Mockito.never()).remove(51L);
+ Mockito.verify(backupDao).remove(50L);
+ }
}
diff --git a/test/integration/smoke/test_backup_recovery_nas.py
b/test/integration/smoke/test_backup_recovery_nas.py
index e55c1b6f0f9..41aa82bd085 100644
--- a/test/integration/smoke/test_backup_recovery_nas.py
+++ b/test/integration/smoke/test_backup_recovery_nas.py
@@ -56,13 +56,17 @@ class TestNASBackupAndRecovery(cloudstackTestCase):
# Check backup configuration values, set them to enable the nas
provider
backup_enabled_cfg = Configurations.list(cls.api_client,
name='backup.framework.enabled')
backup_provider_cfg = Configurations.list(cls.api_client,
name='backup.framework.provider.plugin')
+ incremental_backup_enabled_cfg = Configurations.list(cls.api_client,
name='nas.backup.incremental.enabled')
cls.backup_enabled = backup_enabled_cfg[0].value
cls.backup_provider = backup_provider_cfg[0].value
+ cls.incremental_backup_enabled =
incremental_backup_enabled_cfg[0].value
if cls.backup_enabled == "false":
cls.skipTest(cls, reason="Test can be run only if the config
backup.framework.enabled is true")
if cls.backup_provider != "nas":
Configurations.update(cls.api_client,
'backup.framework.provider.plugin', value='nas')
+ if cls.incremental_backup_enabled == "false":
+ Configurations.update(cls.api_client,
'nas.backup.incremental.enabled', value='true')
cls.account = Account.create(cls.api_client, cls.services["account"],
domainid=cls.domain.id)
@@ -94,6 +98,8 @@ class TestNASBackupAndRecovery(cloudstackTestCase):
if cls.backup_provider != "nas":
Configurations.update(cls.api_client,
'backup.framework.provider.plugin', value=cls.backup_provider)
+ if cls.incremental_backup_enabled == "false":
+ Configurations.update(cls.api_client,
'nas.backup.incremental.enabled', value="false")
except Exception as e:
raise Exception("Warning: Exception during cleanup : %s" % e)
@@ -519,10 +525,11 @@ class TestNASBackupAndRecovery(cloudstackTestCase):
self.backup_offering.removeOffering(self.apiclient, self.vm.id)
@attr(tags=["advanced", "backup"], required_hardware="true")
- def test_refuse_delete_full_with_children(self):
+ def test_delete_full_with_children_is_deferred(self):
"""
- Deleting a FULL that has surviving incrementals must fail without
forced=true.
- With forced=true it must succeed and remove the entire chain.
+ Deleting a FULL that has surviving incrementals succeeds but is
deferred: the
+ FULL is hidden from the backup list while its child survives, and it is
+ physically swept once the last descendant is deleted.
"""
self.backup_offering.assignOffering(self.apiclient, self.vm.id)
original_full_every = self._get_full_every()
@@ -535,19 +542,21 @@ class TestNASBackupAndRecovery(cloudstackTestCase):
backups = Backup.list(self.apiclient, self.vm.id)
backups.sort(key=lambda b: b.created)
- full = backups[0]
+ full, inc = backups[0], backups[1]
- failed = False
- try:
- Backup.delete(self.apiclient, full.id)
- except Exception:
- failed = True
- self.assertTrue(failed, "Deleting a FULL with children should be
refused without forced=true")
+ # Non-forced delete of the FULL is accepted but deferred: the FULL
is
+ # tombstoned (Hidden) and disappears from the list, the child
survives.
+ Backup.delete(self.apiclient, full.id)
+ remaining = Backup.list(self.apiclient, self.vm.id)
+ self.assertEqual(len(remaining), 1,
+ "Deleting a FULL with children should hide the FULL and keep
the child")
+ self.assertEqual(remaining[0].id, inc.id,
+ "The surviving backup should be the incremental child")
- # Forced delete should succeed and clear the whole chain
- Backup.delete(self.apiclient, full.id, forced=True)
+ # Deleting the last descendant sweeps the delete-pending FULL with
it.
+ Backup.delete(self.apiclient, inc.id)
remaining = Backup.list(self.apiclient, self.vm.id)
- self.assertIsNone(remaining, "Forced delete of FULL should remove
the entire chain")
+ self.assertIsNone(remaining, "Deleting the leaf should sweep the
hidden FULL, removing the chain")
finally:
self._set_full_every(original_full_every)
self.backup_offering.removeOffering(self.apiclient, self.vm.id)
