jmsperu commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4768508884
@abh1sar — on the delete-logic / resource-accounting point: I dug into how `deleteCheckedBackup` interacts with the NAS chain delete and confirmed two concrete problems: 1. **Tombstone case (live children):** the NAS provider keeps the row as `delete-pending` for chain tracking, but `deleteCheckedBackup` then calls `backupDao.remove(target)` — which destroys the tombstone the later sweep relies on. 2. **Leaf + sweep case:** the provider physically removes the leaf row **plus the swept delete-pending ancestors** (N rows), but `deleteCheckedBackup` re-removes the leaf (already gone → `false` → spurious failure) and decrements `backup` count / `backup_storage` usage for **only one** backup — leaking the other N−1. Comparing with `SnapshotManagerImpl.deleteSnapshot`: the strategy owns storage + chain/state, and the manager decrements based on the **post-delete state** of the entity rather than blindly removing the row. **Proposed rule (rigid, exactly-once):** a backup's `backup` count + `backup_storage` usage is decremented **once, at the single point its row+file are physically removed** — i.e. inside `deleteBackupFileAndRow`, which runs for the leaf *and* every swept ancestor. `deleteCheckedBackup` keeps only the `CheckedReservation` limit guard and stops calling `decrementResourceCount`/`backupDao.remove` for NAS chain backups; a tombstone (`delete-pending`) is **not** decremented until the sweep finally removes it. One open question before I implement — the manager currently decrements for **all** providers (Veeam/Networker too). To keep their accounting intact while the NAS provider owns chain accounting, do you prefer: (a) a `BackupProvider` capability flag (e.g. `handlesChainDeleteAccounting()`, default `false`; NAS returns `true`) so the manager skips its own decrement/remove only for such providers, or (b) `deleteBackup` returning the set/count of backups actually removed, so the manager decrements per-removed? I'm leaning (a) as the smaller, more rigid change — happy to implement whichever you prefer. -- 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]
