abh1sar commented on PR #13571:
URL: https://github.com/apache/cloudstack/pull/13571#issuecomment-4923224575
> One thing I'd like changed before merge:
>
> In `deleteLeafBackupAndSweepPendingAncestors`, the sweep now ignores the
result of `deleteBackupFileAndRow`:
>
> ```java
> for (Backup member : chain) {
> if (member.getId() == backup.getId()) continue;
> if (!isDeletePending(member)) break;
> deleteBackupFileAndRow(member, repo, host); // return value dropped
> }
> ```
>
> The previous version stopped the sweep on failure. Because the sweep walks
leaf→root, if a child's on-NAS delete fails and the loop continues, we can
delete that child's parent while the failed child still exists on the
repository, which orphans the child (its base data is gone and it becomes
unrestorable). Can we restore the stop-on-failure behaviour, i.e. break/return
when `deleteBackupFileAndRow` returns false, so a partial NAS failure leaves a
consistent chain for the next sweep to retry?
>
We are deleting Hidden backups that don't have any dependents. Hidden
backups without any live children are by design not restorable. Let's say we
have 4 Hidden backups in a chain and one live leaf backup. In case one of the
Hidden backup delete fails, and we stop walking the chain to delete rest of the
backups. These Hidden backups will never get deleted.
Removing the condition makes the delete best-effort. We delete whatever
Hidden backups we can (we know they don't have any live dependents, so it is
safe to delete.
> Two smaller points to confirm:
>
> 1. On the `forced` path, `deleteBackup` now holds the per-VM lock and then
calls `cascadeDeleteSubtree`. Could you confirm nothing under it re-enters
`deleteBackup` for the same VM in a way that unbalances the lock ref-count?
(`GlobalLock` is reentrant per thread, so it should be fine, just want to be
sure since that method isn't in this diff.)
That's right. deleteBackup is only called from BackupManagerImpl.
> 2. `hasLiveChildren` relies on `chainPosition`. If `CHAIN_POSITION` is
ever absent, the `>` comparison could misjudge dependents. Probably a non-issue
since position is always written, but a defensive default may be worth it.
Yes, chain position is always written. Not doing anything about it now.
Maybe we can add some defensive code later
--
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]
