Skip to content

Incremental nas backup fixes #13571

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
abh1sar wants to merge 3 commits into
base: main
Choose a base branch
from
+332 −133
Open

Incremental nas backup fixes #13571

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9aa4f52
Avoid concurrent deletes modifying the same backup chain in parallel …
abh1sar Jul 8, 2026
bc5ef78
fix comment
abh1sar Jul 8, 2026
a8937e2
Fix integration test failures
abh1sar Jul 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
187 changes: 74 additions & 113 deletions plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<Integer> NASBackupRestoreMountTimeout = new ConfigKey<>("Advanced", Integer.class,
"nas.backup.restore.mount.timeout",
"30",
Expand Down Expand Up @@ -639,7 +642,7 @@ public Pair<Boolean, Backup> 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);
Expand Down Expand Up @@ -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<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();
}

/**
Expand All @@ -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;
}
Expand Down Expand Up @@ -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<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;
}
Comment thread
abh1sar marked this conversation as resolved.
if (!isDeletePending(b) && chainPosition(b) > position) {
return true;
Comment thread
abh1sar marked this conversation as resolved.
}
}
return null;
return false;
}

/**
Expand Down Expand Up @@ -1078,7 +1084,7 @@ private List<Backup> 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.
*
* <p>Caller must guarantee {@code backup} is a leaf (no live children). Each tombstoned
Expand All @@ -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;
}
Expand All @@ -1127,42 +1124,6 @@ private boolean cascadeDeleteSubtree(Backup root, BackupRepository repo, Host ho
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) {
Expand Down
Loading