Skip to content

Commit 4f3375a

Browse files
committed
fix(nas-backup): don't persist unconfirmed bitmap; gate sanity_checks
Two more Copilot review items on PR #13074: * persistChainMetadata: only store nas.bitmap_name when the agent confirms it via BITMAP_CREATED=. Previously fell back to decision.bitmapNew if the agent didn't emit it, which could anchor the next incremental on a bitmap that doesn't exist (stopped-VM pre-seed failure was the trigger Copilot flagged, but the same risk applies any time the marker is lost). Leaving it empty correctly forces the next backup to a fresh full instead. * nasbackup.sh: sanity_checks (QEMU >= 4.2 / libvirt >= 7.2) is only needed for the incremental backup-begin path. Gate it to `OP=backup && -n MODE` so legacy full-only callers and delete / stats / rebase operations still work on older host versions.
1 parent 3e2e144 commit 4f3375a

2 files changed

Lines changed: 15 additions & 7 deletions

File tree

plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -436,11 +436,14 @@ private List<String> composeParentBackupPaths(Backup parent, long vmId) {
436436
* other providers can implement their own chain semantics without schema changes.
437437
*/
438438
private void persistChainMetadata(Backup backup, ChainDecision decision, String bitmapFromAgent) {
439-
// Prefer the bitmap name confirmed by the agent (BITMAP_CREATED= line). Fall back to
440-
// what we asked it to create — they should match.
441-
String bitmap = bitmapFromAgent != null ? bitmapFromAgent : decision.bitmapNew;
442-
if (bitmap != null) {
443-
backupDetailsDao.persist(new BackupDetailVO(backup.getId(), NASBackupChainKeys.BITMAP_NAME, bitmap, true));
439+
// Only persist nas.bitmap_name when the agent confirmed it via BITMAP_CREATED=. If we
440+
// fall back to decision.bitmapNew when the agent didn't emit BITMAP_CREATED= (e.g.,
441+
// stopped-VM path where the qemu-img pre-seed failed, or running-VM path where libvirt
442+
// backup-begin succeeded but the bitmap line wasn't surfaced for any reason), we'd
443+
// anchor the next incremental on a bitmap that doesn't exist on the host. Better to
444+
// leave it empty so the next backup sees no checkpoint and starts a fresh full chain.
445+
if (bitmapFromAgent != null && !bitmapFromAgent.isEmpty()) {
446+
backupDetailsDao.persist(new BackupDetailVO(backup.getId(), NASBackupChainKeys.BITMAP_NAME, bitmapFromAgent, true));
444447
}
445448
backupDetailsDao.persist(new BackupDetailVO(backup.getId(), NASBackupChainKeys.CHAIN_ID, decision.chainId, true));
446449
backupDetailsDao.persist(new BackupDetailVO(backup.getId(), NASBackupChainKeys.CHAIN_POSITION,

scripts/vm/hypervisor/kvm/nasbackup.sh

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -607,8 +607,13 @@ while [[ $# -gt 0 ]]; do
607607
esac
608608
done
609609

610-
# Perform Initial sanity checks
611-
sanity_checks
610+
# QEMU >= 4.2 and libvirt >= 7.2 are only required for backup-begin (incremental
611+
# checkpoints and per-bitmap exports). Legacy full-only backups, plus delete /
612+
# stats / rebase operations, run on older versions just fine. Gate the version
613+
# check to the paths that actually need it to preserve backward compatibility.
614+
if [ "$OP" = "backup" ] && [ -n "$MODE" ]; then
615+
sanity_checks
616+
fi
612617

613618
if [ "$OP" = "backup" ]; then
614619
STATE=$(virsh -c qemu:///system list | awk -v vm="$VM" '$2 == vm {print $3}')

0 commit comments

Comments
 (0)