jmsperu commented on PR #13074: URL: https://github.com/apache/cloudstack/pull/13074#issuecomment-4556978538
Thanks @abh1sar — all 9 review points addressed in commit f2210f4859: | # | Comment | Resolution | |---|---|---| | 1 | Strip GitHub-username refs from comments | done in `ChainDecision`, `composeParentBackupPaths` | | 2 | `backup.type` is the single source of truth | dropped `NASBackupChainKeys.TYPE` writes to `backup_details`; removed the `TYPE` constant; backupVO.type set by `takeBackup` is authoritative | | 3 | Just check the latest backup's bitmap | replaced `findLatestBackedUpBackupWithBitmap` (sort+scan) with `findLatestBackedUpBackup` (one max-by-date); `decideChain` now compares its bitmap inline and forces full on mismatch | | 4 | Reset active checkpoint on offering removal | `removeVMFromBackupOffering()` calls `clearVmActiveCheckpoint(vm.getId())` | | 5 | Drop manual volume-identity check in `composeParentBackupPaths` | done — count match is the only remaining safety check, since attach/detach is blocked while assigned and re-assignment clears the checkpoint per #4 | | 6 | Drop `BITMAP_RECREATED` everywhere | removed from `NASBackupProvider`, `BackupAnswer`, `LibvirtTakeBackupCommandWrapper`, `nasbackup.sh`, `NASBackupChainKeys` | | 7a | `deleteBackupAndSweepPendingAncestors` — drop always-true `findLiveChildren(parent).isEmpty()` check | done | | 7b | Rename to `deleteLeafBackupAndSweepPendingAncestors` | done | | 7c | Call `findChainParent` BEFORE delete | done (row is gone after `deleteBackupFileAndRow`) | | 8 | Drop BFS in `collectSubtree` | replaced with `findChainTail(root)` — highest `CHAIN_POSITION` for same `CHAIN_ID` — then walk leaf → root via `PARENT_BACKUP_ID`. `collectSubtree` removed. | Build is green and `NASBackupProviderTest` all 14 tests pass locally. On the `getVolumePoolsAndPaths` suggestion: that helper builds live primary-storage paths (`List<VolumeVO>` in, storage-pool-relative paths out), whereas `composeParentBackupPaths` builds NAS-relative backup-file paths from `List<Backup.VolumeInfo>` (UUID-keyed `root.<uuid>.qcow2` / `datadisk.<uuid>.qcow2` per the script's naming). Different shape — but the spirit (simpler path composition, drop the manual sanity checks) is in. Happy to align further if you'd prefer. Diff is 84 insertions / 133 deletions — net leaner. Standing by for the agent-side review you mentioned next. -- 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]
