abh1sar commented on code in PR #13074:
URL: https://github.com/apache/cloudstack/pull/13074#discussion_r3457076881
##########
plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java:
##########
@@ -495,9 +863,42 @@ public boolean deleteBackup(Backup backup, boolean forced)
{
throw new CloudRuntimeException(String.format("Unable to find a
running KVM host in zone %d to delete backup %s", backup.getZoneId(),
backup.getUuid()));
}
- DeleteBackupCommand command = new
DeleteBackupCommand(backup.getExternalId(), backupRepository.getType(),
- backupRepository.getAddress(),
backupRepository.getMountOptions());
+ // Backups outside any tracked chain (legacy or non-incremental
providers) are
+ // deleted straight away — no children semantics apply.
+ if (readDetail(backup, NASBackupChainKeys.CHAIN_ID) == null) {
+ 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);
+ }
+
Review Comment:
set vm's active checkpoint_id to null if the backup corresponding to the
active_checkpoint_id is deleted.
##########
server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java:
##########
@@ -1701,6 +1701,14 @@ private boolean deleteCheckedBackup(Boolean forced,
BackupProvider backupProvide
reservationDao, resourceLimitMgr)) {
boolean result = backupProvider.deleteBackup(backup, forced);
if (result) {
+ // Chain-aware providers (e.g. NAS) physically remove several
backups per call
+ // (leaf + swept delete-pending ancestors) and decrement
resource count/usage and
+ // remove each DB row themselves, exactly once per removed
backup. Decrementing or
+ // removing again here would double-handle and destroy
delete-pending tombstones,
+ // so defer entirely to the provider for those.
+ if (backupProvider.handlesChainDeleteResourceAccounting()) {
+ return true;
Review Comment:
call `checkAndGenerateUsageForLastBackupDeletedAfterOfferingRemove` before
returning true
##########
plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java:
##########
@@ -506,13 +907,215 @@ 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());
+ // Exactly-once resource accounting: decrement at the single point a
backup row + file are
+ // physically removed. This runs for the leaf and for every swept
delete-pending ancestor,
+ // so a chain delete decrements once per actually-removed backup. The
manager skips its own
+ // accounting for this provider (see
handlesChainDeleteResourceAccounting()).
+ long size = backup.getSize() != null ? backup.getSize() : 0L;
+ resourceLimitMgr.decrementResourceCount(backup.getAccountId(),
Resource.ResourceType.backup);
+ resourceLimitMgr.decrementResourceCount(backup.getAccountId(),
Resource.ResourceType.backup_storage, size);
+ 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);
Review Comment:
Sorry, about going back on this but can we use a new `Backup.Status.Hidden`
state similar to `Snapshot.State.Hidden` instead of the `DELETE_PENDING` detail?
We'll also need a change in BackupManagerImpl.listBackups to not return
backups that are `Hidden`
Also, no other operations such as
Restore/RestoreandAttachVolume/CreateInstanceFromBackup/Delete should be
allowed on a `Hidden` backup
--
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]
