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 fe56fbf7c1ae..f5fecc5de6f1 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.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 @@ 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 NASBackupRestoreMountTimeout = new ConfigKey<>("Advanced", Integer.class, "nas.backup.restore.mount.timeout", "30", @@ -639,7 +642,7 @@ public Pair takeBackup(final VirtualMachine vm, Boolean quiesce } } 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 boolean deleteBackup(Backup backup, boolean forced) { 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 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 @@ private boolean deleteBackupFileAndRow(Backup backup, BackupRepository repo, Hos 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 @@ private boolean isDeletePending(Backup backup) { } /** - * 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 findLiveChildren(Backup parent) { - String parentUuid = parent.getUuid(); - String chainId = readDetail(parent, NASBackupChainKeys.CHAIN_ID); - if (parentUuid == null || chainId == null) { - return Collections.emptyList(); - } - List 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. - * - *

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 @@ private List getChainOrderedLeafToRoot(Backup member) { /** * 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. * *

Caller must guarantee {@code backup} is a leaf (no live children). Each tombstoned @@ -1092,23 +1098,14 @@ private boolean deleteLeafBackupAndSweepPendingAncestors(Backup backup, BackupRe 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 @@ private boolean cascadeDeleteSubtree(Backup root, BackupRepository repo, Host ho return true; } - private static int indexOfBackupById(List 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 3ba7dbad0416..cd08378b926a 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.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 void deleteWithLiveChildMarksDeletePendingAndPreservesFile() // 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 void deleteWithLiveChildMarksDeletePendingAndPreservesFile() 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 void deletingLeafSweepsUpDeletePendingParent() 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 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 void deletingLeafSweepsUpDeletePendingParent() 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 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 e55c1b6f0f93..41aa82bd0857 100644 --- a/test/integration/smoke/test_backup_recovery_nas.py +++ b/test/integration/smoke/test_backup_recovery_nas.py @@ -56,13 +56,17 @@ def setUpClass(cls): # 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 @@ def tearDownClass(cls): 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 @@ def test_delete_middle_incremental_repairs_chain(self): 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 @@ def test_refuse_delete_full_with_children(self): 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)