abh1sar commented on code in PR #13074:
URL: https://github.com/apache/cloudstack/pull/13074#discussion_r3408100947
##########
plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java:
##########
@@ -506,13 +874,170 @@ public boolean deleteBackup(Backup backup, boolean
forced) {
} catch (OperationTimedoutException e) {
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",
+ backup.getUuid(), backup.getExternalId());
+ return false;
+ }
+ backupDao.remove(backup.getId());
+ return true;
+ }
- if (answer != null && answer.getResult()) {
- return backupDao.remove(backup.getId());
+ /**
+ * Mark {@code backup} as delete-pending in {@code backup_details}.
Idempotent.
+ */
+ private void markDeletePending(Backup backup) {
+ BackupDetailVO existing = backupDetailsDao.findDetail(backup.getId(),
NASBackupChainKeys.DELETE_PENDING);
+ if (existing == null) {
+ backupDetailsDao.persist(new BackupDetailVO(backup.getId(),
+ NASBackupChainKeys.DELETE_PENDING, "true", true));
}
+ }
- logger.debug("There was an error removing the backup with id {}",
backup.getId());
- return false;
+ /**
+ * @return true if this backup carries the delete-pending tombstone.
+ */
+ private boolean isDeletePending(Backup backup) {
+ BackupDetailVO d = backupDetailsDao.findDetail(backup.getId(),
NASBackupChainKeys.DELETE_PENDING);
+ return d != null && "true".equalsIgnoreCase(d.getValue());
+ }
+
+ /**
+ * 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".
+ */
+ 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.
+ */
+ private Backup findChainParent(Backup backup) {
+ String parentUuid = readDetail(backup,
NASBackupChainKeys.PARENT_BACKUP_ID);
+ if (parentUuid == null || parentUuid.isEmpty()) {
+ return null;
+ }
+ for (Backup b : backupDao.listByVmId(null, backup.getVmId())) {
+ if (parentUuid.equals(b.getUuid())) {
+ return b;
+ }
+ }
+ return null;
+ }
+
+ /**
+ * 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
+ * gone, garbage-collect any tombstoned parents.
+ *
+ * <p>Caller must guarantee {@code backup} is a leaf (no live children).
Each tombstoned
+ * ancestor is by definition childless once its sole child is deleted
here, so no extra
+ * live-children check is needed inside the loop.
+ */
+ private boolean deleteLeafBackupAndSweepPendingAncestors(Backup backup,
BackupRepository repo, Host host) {
+ // Record the parent BEFORE the delete — deleteBackupFileAndRow
removes the backup row,
+ // after which findChainParent can't resolve PARENT_BACKUP_ID.
+ Backup parent = findChainParent(backup);
+ if (!deleteBackupFileAndRow(backup, repo, host)) {
+ return false;
+ }
+ while (parent != null && isDeletePending(parent)) {
+ Backup nextParent = findChainParent(parent);
+ if (!deleteBackupFileAndRow(parent, 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;
+ }
+ parent = nextParent;
+ }
+ return true;
+ }
+
+ /**
+ * Forced delete of {@code root}'s entire chain, leaf-first. NAS backups
form a linear
+ * chain (full → inc → inc → …), not a tree, so we don't need BFS — find
the tail
+ * (highest {@code CHAIN_POSITION} for {@code root}'s {@code CHAIN_ID})
and delete down.
+ * Each {@code rm} runs against a chain that still resolves.
+ */
+ private boolean cascadeDeleteSubtree(Backup root, BackupRepository repo,
Host host) {
+ Backup tail = findChainTail(root);
+ if (tail == null) {
+ return false;
+ }
+ // Walk leaf → root via PARENT_BACKUP_ID, deleting unconditionally. We
re-fetch parent
+ // before each delete because deleteBackupFileAndRow removes the
backup row.
+ Backup current = tail;
+ while (current != null) {
+ Backup nextParent = findChainParent(current);
Review Comment:
This is doing db query to get the parent for each backup in the chain.
findChainParent gets a list of all backups in the chain. Can it be modified
to return (or another method written) an ordered list of Backup objects with
the first object as the topmost parent? Then this loop can just walk that list.
Same comment for deleteLeafBackupAndSweepPendingAncestors()
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
