jmsperu commented on code in PR #13074:
URL: https://github.com/apache/cloudstack/pull/13074#discussion_r3452405878
##########
plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java:
##########
@@ -168,6 +200,305 @@ protected Host getVMHypervisorHost(VirtualMachine vm) {
return
resourceManager.findOneRandomRunningHostByHypervisor(Hypervisor.HypervisorType.KVM,
vm.getDataCenterId());
}
+ /**
+ * Returned by {@link #decideChain(VirtualMachine)} to describe the next
backup's place in
+ * the chain: full vs incremental, the bitmap name to create, and (for
incrementals) the
+ * parent bitmap and parent file path.
+ */
+ static final class ChainDecision {
+ final String mode; // "full" or "incremental"
+ final String bitmapNew;
+ final String bitmapParent; // null for full
+ // Per-volume parent backup file paths, one per current VM volume in
deviceId order.
+ // null/empty for full. Replaces the single parentPath field — each
volume needs its
+ // own parent file because backup files are named after each volume's
own UUID
+ // (root.<uuid>.qcow2 / datadisk.<uuid>.qcow2), abh1sar review at line
340.
+ final List<String> parentPaths;
+ final String chainId; // chain identifier this backup belongs
to
+ final int chainPosition; // 0 for full, N for the Nth incremental
in the chain
+
+ private ChainDecision(String mode, String bitmapNew, String
bitmapParent, List<String> parentPaths,
+ String chainId, int chainPosition) {
+ this.mode = mode;
+ this.bitmapNew = bitmapNew;
+ this.bitmapParent = bitmapParent;
+ this.parentPaths = parentPaths;
+ this.chainId = chainId;
+ this.chainPosition = chainPosition;
+ }
+
+ static ChainDecision fullStart(String bitmapName) {
+ return new ChainDecision(NASBackupChainKeys.TYPE_FULL, bitmapName,
null, null,
+ UUID.randomUUID().toString(), 0);
+ }
+
+ static ChainDecision incremental(String bitmapNew, String
bitmapParent, List<String> parentPaths,
+ String chainId, int chainPosition) {
+ return new ChainDecision(NASBackupChainKeys.TYPE_INCREMENTAL,
bitmapNew, bitmapParent,
+ parentPaths, chainId, chainPosition);
+ }
+
+ boolean isIncremental() {
+ return NASBackupChainKeys.TYPE_INCREMENTAL.equals(mode);
+ }
+ }
+
+ /**
+ * Decides whether the next backup for {@code vm} should be a fresh full
or an incremental
+ * appended to the existing chain. Stopped VMs are always full (libvirt
{@code backup-begin}
+ * requires a running QEMU process). The {@code nas.backup.full.every}
ConfigKey controls
+ * how many backups (full + incrementals) form one chain before a new full
is forced.
+ *
+ * <p>The decision is anchored on the VM's {@code
nas.active_checkpoint_id} detail, which
+ * records the bitmap that currently exists on the running QEMU. After a
restore that
+ * detail is cleared, so the next backup is automatically full — even
though there may be
+ * a more recent "last backup taken" row in the database. This matches the
prescription in
+ * the PR review (avoid relying on "last backup" because that breaks after
restore).</p>
+ */
+ protected ChainDecision decideChain(VirtualMachine vm) {
+ final String newBitmap = "backup-" + System.currentTimeMillis() /
1000L;
+
+ // Master switch — when the operator disables incrementals at the zone
level every
+ // backup is taken as a fresh full. Existing chains stay restorable
because each
+ // backup's metadata is kept independently; restoring an incremental
still walks its
+ // own chain (the per-backup chain_id / parent_backup_id details
persist regardless
+ // of the live config). The next backup with this flag back on starts
a new chain.
+ Boolean incrementalEnabled =
NASBackupIncrementalEnabled.valueIn(vm.getDataCenterId());
+ if (incrementalEnabled == null || !incrementalEnabled) {
+ return ChainDecision.fullStart(newBitmap);
+ }
+
+ // Stopped VMs cannot do incrementals — script will also fall back,
but we make the
+ // decision here so we register the right type up-front.
+ if (VirtualMachine.State.Stopped.equals(vm.getState())) {
+ return ChainDecision.fullStart(newBitmap);
+ }
+
+ Integer fullEvery = NASBackupFullEvery.valueIn(vm.getDataCenterId());
+ if (fullEvery == null || fullEvery <= 1) {
+ // Disabled or every-backup-is-full mode.
+ return ChainDecision.fullStart(newBitmap);
+ }
+
+ // 1. If the VM has no active_checkpoint_id, there is no bitmap on the
host to use as
+ // a parent. This is the case after restore (we clear it), after VM
was just assigned
+ // to the offering, or on the very first backup.
+ String activeCheckpoint = readVmActiveCheckpoint(vm.getId());
+ if (activeCheckpoint == null) {
+ return ChainDecision.fullStart(newBitmap);
+ }
+
+ // 2. Find the latest BackedUp backup of this VM whose BITMAP_NAME
matches the VM's
+ // active_checkpoint_id. Only that backup is a safe parent — any
earlier backup
+ // would have a bitmap that QEMU may have rotated out. Per the
review:
+ // "The latest backup should have the bitmap_name equal to the
VM's
+ // active_checkpoint_id which will become the parent backup. If
not, force full."
+ Backup parent = findLatestBackedUpBackupWithBitmap(vm.getId(),
activeCheckpoint);
+ if (parent == null) {
+ LOG.debug("VM {} has active_checkpoint_id={} but no matching
backup found — forcing full",
+ vm.getInstanceName(), activeCheckpoint);
+ return ChainDecision.fullStart(newBitmap);
+ }
+
+ String parentChainId = readDetail(parent, NASBackupChainKeys.CHAIN_ID);
+ int parentChainPosition = chainPosition(parent);
+ if (parentChainId == null || parentChainPosition == Integer.MAX_VALUE)
{
+ return ChainDecision.fullStart(newBitmap);
+ }
+
+ // Force a fresh full when the chain has reached the configured length.
+ if (parentChainPosition + 1 >= fullEvery) {
+ return ChainDecision.fullStart(newBitmap);
+ }
+
+ // The script needs the parent backup's on-NAS file path PER VOLUME so
it can rebase
+ // each new qcow2 onto the matching parent. The paths are stored
relative to the NAS
+ // mount root — the script resolves them inside its mount session.
When alignment
+ // fails (volume count changed, etc.) compose returns null and we fall
back to full
+ // so we don't risk corrupting the chain.
+ List<String> parentPaths = composeParentBackupPaths(parent,
vm.getId());
+ if (parentPaths == null) {
+ LOG.debug("VM {} parent backup {} volume layout no longer matches
current VM — forcing full",
+ vm.getInstanceName(), parent.getUuid());
+ return ChainDecision.fullStart(newBitmap);
+ }
+ return ChainDecision.incremental(newBitmap, activeCheckpoint,
parentPaths,
+ parentChainId, parentChainPosition + 1);
+ }
+
+ /**
+ * Read the {@code nas.active_checkpoint_id} VM detail. Returns {@code
null} when no detail
+ * exists (post-restore, first backup, or after explicit reset).
+ */
+ private String readVmActiveCheckpoint(long vmId) {
+ VMInstanceDetailVO d = vmInstanceDetailsDao.findDetail(vmId,
NASBackupChainKeys.VM_ACTIVE_CHECKPOINT_ID);
+ if (d == null) {
+ return null;
+ }
+ String v = d.getValue();
+ return (v == null || v.isEmpty()) ? null : v;
+ }
+
+ /**
+ * Locate the most-recent {@code BackedUp} backup for {@code vmId} whose
bitmap name equals
+ * {@code bitmapName}. Used by {@link #decideChain} to anchor the next
incremental.
+ */
+ private Backup findLatestBackedUpBackupWithBitmap(long vmId, String
bitmapName) {
+ List<Backup> history = backupDao.listByVmId(null, vmId);
+ if (history == null || history.isEmpty()) {
+ return null;
+ }
+ history.sort(Comparator.comparing(Backup::getDate).reversed());
+ for (Backup b : history) {
+ if (!Backup.Status.BackedUp.equals(b.getStatus())) {
+ continue;
+ }
+ if (bitmapName.equals(readDetail(b,
NASBackupChainKeys.BITMAP_NAME))) {
+ return b;
+ }
+ }
+ return null;
+ }
+
+ private String readDetail(Backup backup, String key) {
+ BackupDetailVO d = backupDetailsDao.findDetail(backup.getId(), key);
+ return d == null ? null : d.getValue();
+ }
+
+ /**
+ * Compose the on-NAS path of EVERY parent backup file (one per VM volume)
in the same
+ * order the script will iterate the current VM's disks (deviceId asc).
Relative to the
+ * NAS mount, matches the layout written by {@code nasbackup.sh}:
+ * first disk -> {@code <backupPath>/root.<volUuid>.qcow2}
+ * others -> {@code <backupPath>/datadisk.<volUuid>.qcow2}
+ *
+ * Returns {@code null} if the parent's stored volume list can't be
aligned with the
+ * current VM's volumes (count mismatch / different volume identities). In
that case the
+ * caller should force a fresh full so we don't accidentally rebase a data
disk onto the
+ * root parent — exactly the bug abh1sar flagged at line 340.
+ */
+ private List<String> composeParentBackupPaths(Backup parent, long vmId) {
+ // backupPath is stored as externalId by createBackupObject — e.g.
+ // "i-2-1234-VM/2026.04.27.13.45.00".
+ String dir = parent.getExternalId();
+ if (dir == null || dir.isEmpty()) {
+ return null;
+ }
+
+ // Read the parent's backed-up volume list (uuid order = deviceId
order at the time
+ // the parent was taken). The script names files as root.<uuid>.qcow2
for the first
+ // entry and datadisk.<uuid>.qcow2 for subsequent entries — match that
here.
+ List<Backup.VolumeInfo> parentVols = parent.getBackedUpVolumes();
+ if (parentVols == null || parentVols.isEmpty()) {
+ return null;
+ }
+
+ // Sanity 1: the current VM must have the same number of volumes. If
it doesn't (volume
+ // added or removed since the parent), positional alignment is unsafe
— caller falls back to full.
+ List<VolumeVO> currentVols = volumeDao.findByInstance(vmId);
+ if (currentVols == null || currentVols.size() != parentVols.size()) {
+ return null;
+ }
+
+ // Sanity 2: VolumeDao.findByInstance() has no ordering guarantee.
Sort the current
+ // volumes by deviceId so positional comparison against parentVols
(also deviceId-ordered
+ // at backup time) is meaningful.
+ List<VolumeVO> currentSorted = new ArrayList<>(currentVols);
+ currentSorted.sort(Comparator.comparing(Volume::getDeviceId));
+
+ // Sanity 3: verify each current volume's UUID matches the parent's
recorded UUID at the
+ // same position. If any disk was detached + a different one attached
in its place, the
+ // chain cannot be safely continued — force a full instead of silently
rebasing onto the
+ // wrong parent file.
+ for (int i = 0; i < parentVols.size(); i++) {
+ String parentUuid = parentVols.get(i).getUuid();
+ String currentUuid = currentSorted.get(i).getUuid();
+ if (parentUuid == null || parentUuid.isEmpty()
+ || currentUuid == null || !parentUuid.equals(currentUuid))
{
+ LOG.debug("Volume identity mismatch at position {} for VM {}:
parent uuid {} vs current uuid {}. " +
+ "Forcing a full backup to start a fresh chain.",
+ i, vmId, parentUuid, currentUuid);
+ return null;
+ }
+ }
+
+ List<String> paths = new ArrayList<>(parentVols.size());
Review Comment:
`composeParentBackupPaths` builds the on-NAS *parent backup file* paths that
the agent rebases each new incremental qcow2 onto —
`<backupPath>/root.<uuid>.qcow2` for the first volume and
`<backupPath>/datadisk.<uuid>.qcow2` for the rest, uniform across source pool
types.
`getVolumePoolsAndPaths(List<VolumeVO>)` returns the *source
primary-storage* disk paths (`/mnt/<pool>/…`, `/dev/drbd/by-res/cs-…/0`, …) and
takes `VolumeVO` rather than the parent's `Backup.VolumeInfo`, so it can't
substitute here without pointing the rebase at source disks (and it doesn't
type-match). I verified the current NAS-path form end-to-end on a real VM (full
→ simulated restart → incremental rebases onto
`<backupPath>/root.<uuid>.qcow2`). Did you mean a different spot, or am I
misreading the intent?
##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -113,20 +126,104 @@ backup_running_vm() {
mount_operation
mkdir -p "$dest" || { echo "Failed to create backup directory $dest"; exit
1; }
+ # Determine effective mode for this run.
+ # Legacy callers (no -M argument) get the original full-only behavior with
no checkpoint.
+ local effective_mode="${MODE:-legacy-full}"
+ local make_checkpoint=0
+ case "$effective_mode" in
+ incremental)
+ if [[ -z "$BITMAP_PARENT" || -z "$BITMAP_NEW" || -z "$PARENT_PATHS" ]];
then
+ echo "incremental mode requires --bitmap-parent, --bitmap-new, and
--parent-paths"
+ cleanup
+ exit 1
+ fi
+ make_checkpoint=1
+ ;;
+ full)
+ if [[ -z "$BITMAP_NEW" ]]; then
+ echo "full mode requires --bitmap-new (the bitmap to create for the
next incremental)"
+ cleanup
+ exit 1
+ fi
+ make_checkpoint=1
+ ;;
+ legacy-full)
+ make_checkpoint=0
+ ;;
+ *)
+ echo "Unknown mode: $effective_mode"
+ cleanup
+ exit 1
+ ;;
+ esac
+
+ # When incremental, verify the parent bitmap still exists on the running
domain.
+ # CloudStack rebuilds the libvirt domain XML on every VM start, so libvirt's
checkpoint
+ # registry is wiped — but the bitmap may still exist on the qcow2 itself (we
pre-seed
+ # one on stopped-VM backups, see backup_stopped_vm). If the parent is
missing from
+ # libvirt's view, recreate it. If it's missing entirely (qcow2 too), this
falls through
+ # to a fresh-create which captures all writes since — slightly larger but
correct.
+ if [[ "$effective_mode" == "incremental" ]]; then
+ if ! virsh -c qemu:///system checkpoint-list "$VM" --name 2>/dev/null |
grep -qx "$BITMAP_PARENT"; then
+ cat > $dest/recreate-checkpoint.xml <<XML
+<domaincheckpoint><name>$BITMAP_PARENT</name><disks>
+$(virsh -c qemu:///system domblklist "$VM" --details 2>/dev/null | awk
'$2=="disk"{printf "<disk name=\"%s\"/>\n", $3}')
+</disks></domaincheckpoint>
+XML
+ if ! virsh -c qemu:///system checkpoint-create "$VM" --xmlfile
$dest/recreate-checkpoint.xml > /dev/null 2>&1; then
+ # If a bitmap of the same name already lives on the qcow2 (pre-seeded
by an
+ # offline backup) libvirt 7.2+ should reuse it during
checkpoint-create. Older
+ # libvirt fails the create — clean up the orphan bitmap and retry as a
fresh.
+ local retried_ok=1
+ for disk_path in $(virsh -c qemu:///system domblklist "$VM" --details
2>/dev/null | awk '$2=="disk"{print $4}'); do
+ [[ -f "$disk_path" ]] && qemu-img bitmap --remove "$disk_path"
"$BITMAP_PARENT" 2>/dev/null || true
+ done
+ if ! virsh -c qemu:///system checkpoint-create "$VM" --xmlfile
$dest/recreate-checkpoint.xml > /dev/null 2>&1; then
+ retried_ok=0
+ fi
+ if [[ "$retried_ok" == "0" ]]; then
+ echo "Failed to recreate parent bitmap $BITMAP_PARENT for $VM"
+ cleanup
+ exit 1
+ fi
+ fi
+ rm -f $dest/recreate-checkpoint.xml
+ fi
+ fi
+
Review Comment:
I kept the redefine in the script but reduced it to the minimal form: if the
parent checkpoint isn't registered, redefine from a synthesized minimal
`<domaincheckpoint><name/><creationTime/></domaincheckpoint>` (no persisted
full XML); and if the parent bitmap isn't present on the qcow2, fall back to
full in place. That's only a few lines and keeps the `backup-begin`/checkpoint
sequence cohesive in one place; I validated the redefine and the missing-bitmap
fallback end-to-end on libvirt 10.
I can still move it into `LibvirtTakeBackupCommandWrapper` as a follow-up if
you'd prefer the Java side to own it — flagging that it would split the
checkpoint/`backup-begin` sequence across the script and Java.
--
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]
