Copilot commented on code in PR #13571:
URL: https://github.com/apache/cloudstack/pull/13571#discussion_r3546468945
##########
plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java:
##########
@@ -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;
+ }
+ if (!isDeletePending(b) && chainPosition(b) > position) {
+ return true;
Review Comment:
`hasLiveChildren` relies on `CHAIN_POSITION` comparisons. If
`CHAIN_POSITION` is missing/invalid for the target backup (or any peer), the
current logic can incorrectly conclude there are no live descendants and
physically delete a non-leaf backup. Given this is chain-integrity logic, it
should behave conservatively when positions are unknown (treat unknown
positions as having live descendants if any other live member exists in the
chain).
--
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]
