Copilot commented on code in PR #13074:
URL: https://github.com/apache/cloudstack/pull/13074#discussion_r3152913638
##########
plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java:
##########
@@ -168,6 +182,168 @@ 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
+ final String parentPath; // null for full
+ 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, String parentPath,
+ String chainId, int chainPosition) {
+ this.mode = mode;
+ this.bitmapNew = bitmapNew;
+ this.bitmapParent = bitmapParent;
+ this.parentPath = parentPath;
+ 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, String parentPath,
+ String chainId, int chainPosition) {
+ return new ChainDecision(NASBackupChainKeys.TYPE_INCREMENTAL,
bitmapNew, bitmapParent,
+ parentPath, 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.
+ */
+ protected ChainDecision decideChain(VirtualMachine vm) {
+ final String newBitmap = "backup-" + System.currentTimeMillis() /
1000L;
+
+ // 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);
+ }
+
+ // Walk this VM's backups newest→oldest, find the most recent BackedUp
backup that has a
+ // bitmap stored. If we don't find one, this is the first backup in a
chain — start full.
+ List<Backup> history = backupDao.listByVmId(vm.getDataCenterId(),
vm.getId());
+ if (history == null || history.isEmpty()) {
+ return ChainDecision.fullStart(newBitmap);
+ }
+ history.sort(Comparator.comparing(Backup::getDate).reversed());
+
+ Backup parent = null;
+ String parentBitmap = null;
+ String parentChainId = null;
+ int parentChainPosition = -1;
+ for (Backup b : history) {
+ if (!Backup.Status.BackedUp.equals(b.getStatus())) {
+ continue;
+ }
+ String bm = readDetail(b, NASBackupChainKeys.BITMAP_NAME);
+ if (bm == null) {
+ continue;
+ }
+ parent = b;
+ parentBitmap = bm;
+ parentChainId = readDetail(b, NASBackupChainKeys.CHAIN_ID);
+ String posStr = readDetail(b, NASBackupChainKeys.CHAIN_POSITION);
+ try {
+ parentChainPosition = posStr == null ? 0 :
Integer.parseInt(posStr);
+ } catch (NumberFormatException e) {
+ parentChainPosition = 0;
+ }
+ break;
+ }
+ if (parent == null || parentBitmap == null || parentChainId == null) {
+ 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 so it can
rebase the new
+ // qcow2 onto it. The path is stored relative to the NAS mount point —
the script
+ // resolves it inside its mount session.
+ String parentPath = composeParentBackupPath(parent);
+ return ChainDecision.incremental(newBitmap, parentBitmap, parentPath,
+ parentChainId, parentChainPosition + 1);
+ }
+
+ 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 a parent backup's root-disk qcow2. Relative
to the NAS mount,
+ * matches the layout written by {@code nasbackup.sh} ({@code
<backupPath>/root.<volUuid>.qcow2}).
+ */
+ private String composeParentBackupPath(Backup parent) {
+ // backupPath is stored as externalId by createBackupObject — e.g.
"i-2-1234-VM/2026.04.27.13.45.00".
+ // Volume UUID for the root volume is what the script keys backup
files on.
+ VolumeVO rootVolume =
volumeDao.getInstanceRootVolume(parent.getVmId());
+ String volUuid = rootVolume == null ? "root" : rootVolume.getUuid();
+ return parent.getExternalId() + "/root." + volUuid + ".qcow2";
Review Comment:
`composeParentBackupPath` (and `decideChain`) only computes a parent qcow2
path for the *root* volume. However `nasbackup.sh` exports one qcow2 per disk
(root + datadisks), and each disk’s incremental qcow2 needs to reference its
own parent qcow2 file. As-is, incrementals for VMs with data disks cannot be
rebased/flattened correctly. Consider passing a per-disk parent mapping
(volUuid->path) or passing the parent backup directory and deriving
`root.<uuid>.qcow2` / `datadisk.<uuid>.qcow2` for each disk.
```suggestion
* Compose the on-NAS path of a parent backup. Relative to the NAS
mount, this is the backup
* directory written by {@code nasbackup.sh} (for example
* {@code <vmBackupDir>/<timestamp>}).
*
* The backup layout contains one qcow2 per disk ({@code
root.<volUuid>.qcow2} and
* {@code datadisk.<volUuid>.qcow2}). Returning the parent backup
directory instead of a
* root-disk-specific qcow2 path allows downstream code to derive the
correct parent qcow2
* for each disk during incremental export/rebase/flatten operations.
*/
private String composeParentBackupPath(Backup parent) {
// backupPath is stored as externalId by createBackupObject — e.g.
"i-2-1234-VM/2026.04.27.13.45.00".
// Return the parent backup directory so each disk can resolve its
own parent qcow2 file.
return parent.getExternalId();
```
##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -191,18 +307,30 @@ backup_running_vm() {
virsh -c qemu:///system domblklist "$VM" --details 2>/dev/null | awk
'$2=="disk"{print $3, $4}'
)
- rm -f $dest/backup.xml
+ rm -f $dest/backup.xml $dest/checkpoint.xml
sync
# Print statistics
virsh -c qemu:///system domjobinfo $VM --completed
du -sb $dest | cut -f1
+ if [[ -n "$BITMAP_NEW" ]]; then
+ # Echo the bitmap name on its own line so the Java caller can capture it
for backup_details.
+ echo "BITMAP_CREATED=$BITMAP_NEW"
+ fi
umount $mount_point
rmdir $mount_point
}
backup_stopped_vm() {
+ # Stopped VMs cannot use libvirt's backup-begin (no QEMU process). Take a
full
+ # backup via qemu-img convert. If the caller asked for incremental, fall back
+ # to full and signal the fallback so the orchestrator can record it as a full
+ # in the chain.
+ if [[ "$MODE" == "incremental" ]]; then
+ echo "INCREMENTAL_FALLBACK=full (VM stopped — incremental requires running
VM)" >&2
Review Comment:
`INCREMENTAL_FALLBACK=...` is printed to stderr (`>&2`), but
`Script.executePipedCommands(...)` only captures stdout. As a result, the
management server will never see this marker and can't reliably record a
stopped-VM backup as FULL / start a new chain. Emit the fallback marker on
stdout (or change the Java wrapper to capture stderr too).
```suggestion
echo "INCREMENTAL_FALLBACK=full (VM stopped — incremental requires
running VM)"
```
##########
docs/rfcs/incremental-nas-backup.md:
##########
@@ -0,0 +1,406 @@
+# RFC: Incremental NAS Backup Support for KVM Hypervisor
+
+| Field | Value |
+|---------------|--------------------------------------------|
+| **Author** | James Peru, Xcobean Systems Limited |
+| **Status** | Draft |
+| **Created** | 2026-03-27 |
+| **Target** | Apache CloudStack 4.23+ |
+| **Component** | Backup & Recovery (NAS Backup Provider) |
+
+---
+
+## Summary
+
+This RFC proposes adding incremental backup support to CloudStack's NAS backup
provider for KVM hypervisors. By leveraging QEMU dirty bitmaps and libvirt's
`backup-begin` API, CloudStack can track changed disk blocks between backups
and export only the delta, reducing daily backup storage consumption by 80--95%
and shortening backup windows from hours to minutes for large VMs. The feature
is opt-in at the zone level, backward-compatible with existing full-backup
behavior, and gracefully degrades on older QEMU/libvirt versions.
+
+---
+
+## Motivation
+
+CloudStack's NAS backup provider currently performs a full disk copy every
time a backup is taken. For a 500 GB VM with five daily backups retained, that
amounts to 2.5 TB of storage consumed. At scale -- tens or hundreds of VMs --
this becomes a serious operational and financial burden.
+
+**Problems with the current approach:**
+
+1. **Storage waste.** Every backup is a full copy of the entire virtual disk,
regardless of how little data actually changed since the last backup.
+2. **Long backup windows.** Copying hundreds of gigabytes over NFS or SMB
takes hours, increasing the risk of I/O contention on production workloads.
+3. **Network bandwidth pressure.** Full-disk transfers saturate the storage
network during backup windows, impacting other VMs on the same host.
+4. **Uncompetitive feature set.** VMware (Changed Block Tracking / VADP),
Proxmox Backup Server, and Veeam all support incremental backups natively.
CloudStack's lack of incremental backup is a common complaint on the users@
mailing list and a blocker for adoption in environments with large VMs.
+
+**What incremental backup achieves:**
+
+- Only changed blocks are transferred and stored after the initial full backup.
+- A typical daily incremental for a 500 GB VM with moderate write activity is
5--15 GB, a reduction of 97--99% compared to a full copy.
+- Backup completes in minutes rather than hours.
+- Retention of 30+ daily restore points becomes economically feasible.
+
+---
+
+## Proposed Design
+
+### Backup Chain Model
+
+Incremental backups form a chain anchored by a periodic full backup:
+
+```
+Full (Day 0) -> Inc 1 (Day 1) -> Inc 2 (Day 2) -> ... -> Inc 6 (Day 6) -> Full
(Day 7) -> ...
+```
+
+Restoring to any point in time requires the full backup plus every incremental
up to the desired restore point. To bound restore complexity and protect
against chain corruption, a new full backup is forced at a configurable
interval.
+
+**Global settings (zone scope):**
+
+| Setting | Type | Default | Description
|
+|--------------------------------------|---------|---------|------------------------------------------------------|
+| `nas.backup.incremental.enabled` | Boolean | `false` | Enable
incremental backup for the zone |
+| `nas.backup.full.interval` | Integer | `7` | Days between full
backups |
+| `nas.backup.incremental.max.chain` | Integer | `6` | Max incremental
backups before forcing a new full |
+
+When `nas.backup.incremental.enabled` is `false` (the default), behavior is
identical to today -- every backup is a full copy. Existing deployments are
unaffected.
+
+---
+
+### Technical Approach
+
+#### 1. Dirty Bitmap Tracking (QEMU Layer)
+
+QEMU 4.0 introduced persistent dirty bitmaps: per-disk bitmaps that record
which blocks have been written since the bitmap was created. These bitmaps
survive QEMU restarts (they are stored in the qcow2 image header) and are the
foundation for incremental backup.
+
+**Lifecycle:**
+
+1. When incremental backup is enabled for a VM, the agent creates a persistent
dirty bitmap on each virtual disk via QMP:
+ ```json
+ {
+ "execute": "block-dirty-bitmap-add",
+ "arguments": {
+ "node": "drive-virtio-disk0",
+ "name": "backup-20260327",
+ "persistent": true
+ }
+ }
+ ```
+2. QEMU automatically sets bits in this bitmap whenever the guest writes to a
block.
+3. At backup time, the bitmap tells the backup process exactly which blocks to
read.
+4. After a successful backup, a new bitmap is created for the next cycle and
the old bitmap is optionally removed.
+
+#### 2. Backup Flow
+
+**Full backup (Day 0 or every `nas.backup.full.interval` days):**
+
+```bash
+# 1. Export the entire disk to the NAS mount
+qemu-img convert -f qcow2 -O qcow2 \
+ /var/lib/libvirt/images/vm-disk.qcow2 \
+ /mnt/nas/backups/vm-uuid/backup-full-20260327.qcow2
+
+# 2. Create a new dirty bitmap to track changes from this point
+virsh qemu-monitor-command $DOMAIN --hmp \
+ 'block-dirty-bitmap-add drive-virtio-disk0 backup-20260327 persistent=true'
+```
+
+**Incremental backup (Day 1 through Day N):**
+
+```bash
+# 1. Use libvirt backup-begin with incremental mode
+# This exports only blocks dirty since bitmap "backup-20260327"
+cat > /tmp/backup.xml <<'XML'
+<domainbackup mode="push">
+ <disks>
+ <disk name="vda" backup="yes" type="file">
+ <target file="/mnt/nas/backups/vm-uuid/backup-inc-20260328.qcow2"
+ type="qcow2"/>
+ <driver type="qcow2"/>
+ </disk>
+ </disks>
+ <incremental>backup-20260327</incremental>
+</domainbackup>
+XML
+
+virsh backup-begin $DOMAIN /tmp/backup.xml
+
+# 2. Wait for completion
+virsh domjobinfo $DOMAIN --completed
+
+# 3. Rotate bitmaps: remove old, create new
+virsh qemu-monitor-command $DOMAIN --hmp \
+ 'block-dirty-bitmap-remove drive-virtio-disk0 backup-20260327'
+virsh qemu-monitor-command $DOMAIN --hmp \
+ 'block-dirty-bitmap-add drive-virtio-disk0 backup-20260328 persistent=true'
+```
+
+**New full backup cycle (Day 7):**
+
+```bash
+# Remove all existing bitmaps
+virsh qemu-monitor-command $DOMAIN --hmp \
+ 'block-dirty-bitmap-remove drive-virtio-disk0 backup-20260327'
+
+# Take a full backup (same as Day 0)
+# Optionally prune expired chains from NAS
+```
+
+#### 3. Restore Flow
+
+Restoring from an incremental chain requires replaying the full backup plus
all incrementals up to the target restore point. This is handled entirely
within `nasbackup.sh` and is transparent to the management server and the end
user.
+
+**Example: Restore to Day 3 (full + 3 incrementals):**
+
+```bash
+# 1. Create a working copy from the full backup
+cp /mnt/nas/backups/vm-uuid/backup-full-20260327.qcow2 /tmp/restored.qcow2
+
+# 2. Apply each incremental in order using qemu-img rebase
+# Each incremental is a thin qcow2 containing only changed blocks.
+# Rebasing merges the incremental's blocks into the chain.
+qemu-img rebase -u -b /tmp/restored.qcow2 \
+ /mnt/nas/backups/vm-uuid/backup-inc-20260328.qcow2
+
+qemu-img rebase -u -b /mnt/nas/backups/vm-uuid/backup-inc-20260328.qcow2 \
+ /mnt/nas/backups/vm-uuid/backup-inc-20260329.qcow2
+
+qemu-img rebase -u -b /mnt/nas/backups/vm-uuid/backup-inc-20260329.qcow2 \
+ /mnt/nas/backups/vm-uuid/backup-inc-20260330.qcow2
+
+# 3. Flatten the chain into a single image
+qemu-img convert -f qcow2 -O qcow2 \
+ /mnt/nas/backups/vm-uuid/backup-inc-20260330.qcow2 \
+ /tmp/vm-restored-final.qcow2
+
+# 4. Return the flattened image for CloudStack to import
+```
+
+An alternative approach uses `qemu-img commit` to merge each layer down. The
implementation will benchmark both methods and choose the faster one for large
images.
+
+#### 4. Database Schema Changes
+
+**Modified table: `backups`**
+
+| Column | Type | Description
|
+|--------------------|--------------|------------------------------------------------|
+| `backup_type` | VARCHAR(16) | `FULL` or `INCREMENTAL`
|
+| `parent_backup_id` | BIGINT (FK) | For incremental: ID of the previous
backup |
+| `bitmap_name` | VARCHAR(128) | QEMU dirty bitmap identifier for this
backup |
+| `chain_id` | BIGINT (FK) | Links to the backup chain this backup
belongs to |
+
+**New table: `backup_chains`**
+
+| Column | Type | Description
|
+|-------------------|-------------|------------------------------------------------|
+| `id` | BIGINT (PK) | Auto-increment primary key
|
+| `vm_instance_id` | BIGINT (FK) | The VM this chain belongs to
|
+| `full_backup_id` | BIGINT (FK) | The full backup anchoring this chain
|
+| `state` | VARCHAR(16) | `ACTIVE`, `SEALED`, `EXPIRED`
|
+| `created` | DATETIME | When the chain was started
|
+
+**Schema migration** will be provided as a Liquibase changeset, consistent
with CloudStack's existing migration framework. The new columns are nullable to
maintain backward compatibility with existing backup records.
+
+#### 5. Management Server Changes
+
+**`BackupManagerImpl` (orchestration):**
+
+- Before taking a backup, query the active chain for the VM.
+- If no active chain exists, or the chain has reached
`nas.backup.incremental.max.chain` incrementals, or `nas.backup.full.interval`
days have elapsed since the last full backup: schedule a full backup and start
a new chain.
+- Otherwise: schedule an incremental backup linked to the previous backup in
the chain.
+- On backup failure: if the bitmap is suspected corrupt, mark the chain as
`SEALED` and force a full backup on the next run.
Review Comment:
This RFC section is now inconsistent with the implemented approach in this
PR: it describes new `backups` columns / a `backup_chains` table and refers to
`nas.backup.full.interval` / `nas.backup.incremental.max.chain`, but the code
stores chain metadata in `backup_details` (`NASBackupChainKeys`) and uses
`nas.backup.full.every`. Please update the RFC to reflect the current design to
avoid misleading readers.
```suggestion
Instead of adding new chain-tracking columns to `backups` or introducing a
separate `backup_chains` table, the implementation stores incremental-chain
metadata in `backup_details`, keyed by `NASBackupChainKeys`. This keeps the
schema aligned with the existing backup metadata model while allowing each
backup record to carry the information needed to identify its chain, parent
backup, bitmap, and chain state.
**Schema migration** will be provided as a Liquibase changeset, consistent
with CloudStack's existing migration framework. The migration adds the required
`backup_details` entries for chain metadata and remains backward-compatible
with existing backup records.
#### 5. Management Server Changes
**`BackupManagerImpl` (orchestration):**
- Before taking a backup, query the current chain metadata for the VM from
`backup_details` (`NASBackupChainKeys`).
- If no usable active chain exists, or `nas.backup.full.every` requires a
new full backup based on the most recent full backup in the chain: schedule a
full backup and start a new chain.
- Otherwise: schedule an incremental backup linked to the previous backup in
the chain.
- On backup failure: if the bitmap is suspected corrupt, mark the chain
metadata as sealed/invalid in `backup_details` and force a full backup on the
next run.
```
##########
test/integration/smoke/test_backup_recovery_nas.py:
##########
@@ -265,3 +265,222 @@ def
test_vm_backup_create_vm_from_backup_in_another_zone(self):
self.assertEqual(backup_repository.crosszoneinstancecreation, True,
"Cross-Zone Instance Creation could not be enabled on the backup repository")
self.vm_backup_create_vm_from_backup_int(template.id, [network.id])
+
+ # ------------------------------------------------------------------
+ # Incremental backup tests (RFC #12899 / PR #13074)
+ # ------------------------------------------------------------------
+ # These tests exercise the incremental NAS backup chain semantics:
+ # full -> incN cadence, restore-from-incremental, delete-middle chain
+ # repair, refuse-delete-full-with-children, and stopped-VM fallback.
+ #
+ # All tests set nas.backup.full.every to a small value (3) so a chain
+ # forms quickly without needing many backup iterations. They restore
+ # the original value at teardown.
+
+ def _set_full_every(self, value):
+ Configurations.update(self.apiclient, name='nas.backup.full.every',
+ value=str(value), zoneid=self.zone.id)
+
Review Comment:
The incremental tests always reset `nas.backup.full.every` to `10` in
`finally`, but they never read/preserve the original zone value. If a test
environment has a non-default value configured, these tests will leave the zone
config changed. Consider capturing the current value once (e.g., via
`Configurations.list(...)` in setup) and restoring that exact value in each
`finally` (or in `tearDown`).
##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -172,9 +260,37 @@ backup_running_vm() {
sleep 5
done
- # Use qemu-img convert to sparsify linstor backups which get bloated due to
virsh backup-begin.
+ # Sparsify behavior:
+ # - For LINSTOR backups (existing): qemu-img convert sparsifies the bloated
output.
+ # - For INCREMENTAL: rebase the resulting thin qcow2 onto its parent so the
chain is self-describing
+ # (so a future restore can flatten without external chain metadata).
name="root"
while read -r disk fullpath; do
+ if [[ "$effective_mode" == "incremental" ]]; then
+ volUuid="${fullpath##*/}"
+ if [[ "$fullpath" == /dev/drbd/by-res/* ]]; then
+ volUuid=$(get_linstor_uuid_from_path "$fullpath")
+ fi
+ # PARENT_PATH from the orchestrator is the parent backup's path relative
to the
+ # NAS mount root (e.g. "i-2-X/2026.04.27.12.00.00/root.UUID.qcow2").
Convert it to
+ # a path relative to THIS new qcow2's directory so the backing reference
resolves
+ # correctly the next time the NAS is mounted (mount points are
ephemeral).
+ local parent_abs="$mount_point/$PARENT_PATH"
+ if [[ ! -f "$parent_abs" ]]; then
+ echo "Parent backup file does not exist on NAS: $parent_abs"
+ cleanup
+ exit 1
+ fi
+ local parent_rel
+ parent_rel=$(realpath --relative-to="$dest" "$parent_abs")
+ if ! qemu-img rebase -u -b "$parent_rel" -F qcow2
"$dest/$name.$volUuid.qcow2" >> "$logFile" 2> >(cat >&2); then
+ echo "qemu-img rebase failed for $dest/$name.$volUuid.qcow2 onto
$parent_rel"
Review Comment:
In incremental mode, each exported qcow2 (`root.*` and `datadisk.*`) is
rebased onto the same `PARENT_PATH`. For VMs with multiple volumes this will
rebase data-disk incrementals onto the root-disk parent file, corrupting the
chain for non-root volumes. `PARENT_PATH` needs to be per-disk (or pass the
parent backup directory and derive the correct parent filename for each
`volUuid`/disk role).
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtTakeBackupCommandWrapper.java:
##########
@@ -94,21 +113,52 @@ public Answer execute(TakeBackupCommand command,
LibvirtComputingResource libvir
return answer;
}
+ // Strip out our incremental marker lines before parsing size, so the
legacy
+ // numeric-suffix parser keeps working.
+ String stdout = result.second().trim();
+ String bitmapCreated = null;
+ String bitmapRecreated = null;
+ boolean incrementalFallback = false;
+ StringBuilder filtered = new StringBuilder();
+ for (String line : stdout.split("\n")) {
+ String trimmed = line.trim();
+ if (trimmed.startsWith("BITMAP_CREATED=")) {
+ bitmapCreated = trimmed.substring("BITMAP_CREATED=".length());
+ continue;
+ }
+ if (trimmed.startsWith("BITMAP_RECREATED=")) {
+ bitmapRecreated =
trimmed.substring("BITMAP_RECREATED=".length());
+ continue;
+ }
+ if (trimmed.startsWith("INCREMENTAL_FALLBACK=")) {
+ incrementalFallback = true;
+ continue;
+ }
Review Comment:
This wrapper tries to detect `INCREMENTAL_FALLBACK=` from `result.second()`,
but `Script.executePipedCommands(...)` only returns stdout from the process
pipeline (it does not capture stderr). Since `nasbackup.sh` currently prints
the fallback marker to stderr, `incrementalFallback` will always remain false
here. Align the script/wrapper so the marker is emitted on stdout (or update
the execution helper to merge/capture stderr).
##########
plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java:
##########
@@ -205,12 +381,20 @@ public Pair<Boolean, Backup> takeBackup(final
VirtualMachine vm, Boolean quiesce
final String backupPath = String.format("%s/%s", vm.getInstanceName(),
new
SimpleDateFormat("yyyy.MM.dd.HH.mm.ss").format(creationDate));
- BackupVO backupVO = createBackupObject(vm, backupPath);
+ // Decide full vs incremental for this backup. Stopped VMs are always
full
+ // (libvirt backup-begin requires a running QEMU process).
+ ChainDecision decision = decideChain(vm);
+
+ BackupVO backupVO = createBackupObject(vm, backupPath,
decision.isIncremental() ? "INCREMENTAL" : "FULL");
TakeBackupCommand command = new
TakeBackupCommand(vm.getInstanceName(), backupPath);
command.setBackupRepoType(backupRepository.getType());
command.setBackupRepoAddress(backupRepository.getAddress());
command.setMountOptions(backupRepository.getMountOptions());
command.setQuiesce(quiesceVM);
+ command.setMode(decision.mode);
+ command.setBitmapNew(decision.bitmapNew);
+ command.setBitmapParent(decision.bitmapParent);
+ command.setParentPath(decision.parentPath);
if (VirtualMachine.State.Stopped.equals(vm.getState())) {
Review Comment:
For stopped VMs `decideChain` returns `fullStart(newBitmap)` and
`takeBackup` always sets `command.mode="full"` / `bitmapNew`. But
`nasbackup.sh`'s stopped-VM path doesn’t create a checkpoint/bitmap (no
`BITMAP_CREATED=`), so persisting `nas.bitmap_name` from the requested bitmap
can cause the next backup to attempt an incremental against a bitmap that was
never created. Consider clearing `mode/bitmapNew` (legacy full) for stopped VMs
and/or only persisting `nas.bitmap_name` when the agent confirms it via
`BITMAP_CREATED=`.
```suggestion
if (VirtualMachine.State.Stopped.equals(vm.getState())) {
// Stopped-VM backups use the offline path and do not create
checkpoints/bitmaps.
// Clear chain metadata so a full backup does not imply a bitmap
was created.
command.setMode(null);
command.setBitmapNew(null);
command.setBitmapParent(null);
command.setParentPath(null);
```
##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -278,6 +451,13 @@ cleanup() {
function usage {
echo ""
echo "Usage: $0 -o <operation> -v|--vm <domain name> -t <storage type> -s
<storage address> -m <mount options> -p <backup path> -d <disks path>
-q|--quiesce <true|false>"
+ echo " [-M|--mode <full|incremental>] [--bitmap-new <name>]
[--bitmap-parent <name>] [--parent-path <path>]"
+ echo ""
+ echo "Incremental backup options (running VMs only; requires QEMU >= 4.2 and
libvirt >= 7.2):"
+ echo " -M|--mode full Take a full backup AND create a checkpoint
(--bitmap-new required) for future incrementals."
+ echo " -M|--mode incremental Take an incremental backup since
--bitmap-parent and create new checkpoint --bitmap-new."
+ echo " Requires --bitmap-parent, --bitmap-new, and
--parent-path (parent backup file for rebase)."
+ echo " Without -M, behaves as legacy full-only backup with no checkpoint
creation."
Review Comment:
The usage text says “Without -M, behaves as legacy full-only backup with no
checkpoint creation”, but the script still runs `sanity_checks` unconditionally
(QEMU>=4.2/libvirt>=7.2) even for legacy callers and non-backup ops
(delete/stats/rebase). To preserve the documented legacy behavior/backward
compatibility, gate the version checks to only the code paths that actually
require `backup-begin`/incremental features (or only when `MODE` is set).
##########
plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java:
##########
@@ -495,24 +693,244 @@ public boolean deleteBackup(Backup backup, boolean
forced) {
throw new CloudRuntimeException(String.format("Unable to find a
running KVM host in zone %d to delete backup %s", backup.getZoneId(),
backup.getUuid()));
}
- DeleteBackupCommand command = new
DeleteBackupCommand(backup.getExternalId(), backupRepository.getType(),
- backupRepository.getAddress(),
backupRepository.getMountOptions());
+ // Repair the chain (if any) before removing the backup file. For
chained backups,
+ // children that point at this backup must be re-pointed at this
backup's parent
+ // (with their blocks merged via qemu-img rebase). For a full at the
head of a chain
+ // with surviving children, refuse unless forced — `forced=true` then
deletes the
+ // full plus every descendant.
+ ChainRepairPlan plan = computeChainRepair(backup, forced);
+ if (!plan.proceed) {
+ throw new CloudRuntimeException(plan.reason);
+ }
- BackupAnswer answer;
- try {
- answer = (BackupAnswer) agentManager.send(host.getId(), command);
- } catch (AgentUnavailableException e) {
- throw new CloudRuntimeException("Unable to contact backend control
plane to initiate backup");
- } catch (OperationTimedoutException e) {
- throw new CloudRuntimeException("Operation to delete backup timed
out, please try again");
+ // Issue rebase commands for each child that needs re-pointing
(ordered so each rebase
+ // operates on a chain that still resolves: children first if there
are nested ones).
+ for (RebaseStep step : plan.rebaseSteps) {
+ RebaseBackupCommand rebase = new
RebaseBackupCommand(step.targetMountRelativePath,
+ step.newBackingMountRelativePath,
backupRepository.getType(),
+ backupRepository.getAddress(),
backupRepository.getMountOptions());
+ BackupAnswer rebaseAnswer;
+ try {
+ rebaseAnswer = (BackupAnswer) agentManager.send(host.getId(),
rebase);
+ } catch (AgentUnavailableException e) {
+ throw new CloudRuntimeException("Unable to contact backend
control plane to repair backup chain");
+ } catch (OperationTimedoutException e) {
+ throw new CloudRuntimeException("Backup chain repair (rebase)
timed out, please try again");
+ }
+ if (rebaseAnswer == null || !rebaseAnswer.getResult()) {
+ throw new CloudRuntimeException(String.format(
+ "Backup chain repair failed: rebase of %s onto %s
returned %s",
+ step.targetMountRelativePath,
step.newBackingMountRelativePath,
+ rebaseAnswer == null ? "no answer" :
rebaseAnswer.getDetails()));
+ }
+ // Update the rebased child's parent reference + position in
backup_details.
+ BackupDetailVO parentDetail =
backupDetailsDao.findDetail(step.childBackupId,
NASBackupChainKeys.PARENT_BACKUP_ID);
+ if (parentDetail != null) {
+ parentDetail.setValue(step.newParentUuid == null ? "" :
step.newParentUuid);
+ backupDetailsDao.update(parentDetail.getId(), parentDetail);
+ } else if (step.newParentUuid != null) {
+ backupDetailsDao.persist(new BackupDetailVO(step.childBackupId,
+ NASBackupChainKeys.PARENT_BACKUP_ID,
step.newParentUuid, true));
+ }
+ BackupDetailVO posDetail =
backupDetailsDao.findDetail(step.childBackupId,
NASBackupChainKeys.CHAIN_POSITION);
+ if (posDetail != null) {
+ posDetail.setValue(String.valueOf(step.newChainPosition));
+ backupDetailsDao.update(posDetail.getId(), posDetail);
+ }
}
- if (answer != null && answer.getResult()) {
- return backupDao.remove(backup.getId());
+ // Now delete this backup's files. For a forced delete of a full with
descendants we
+ // also delete all descendants' files (newest first so each rm targets
a leaf).
+ for (Backup victim : plan.toDelete) {
+ DeleteBackupCommand command = new
DeleteBackupCommand(victim.getExternalId(), backupRepository.getType(),
+ backupRepository.getAddress(),
backupRepository.getMountOptions());
+ BackupAnswer answer;
+ try {
+ answer = (BackupAnswer) agentManager.send(host.getId(),
command);
+ } catch (AgentUnavailableException e) {
+ throw new CloudRuntimeException("Unable to contact backend
control plane to initiate backup");
+ } catch (OperationTimedoutException e) {
+ throw new CloudRuntimeException("Operation to delete backup
timed out, please try again");
+ }
+ if (answer == null || !answer.getResult()) {
+ logger.warn("Failed to delete backup file for {} ({}); leaving
DB row intact", victim.getUuid(), victim.getExternalId());
+ return false;
+ }
+ backupDao.remove(victim.getId());
}
- logger.debug("There was an error removing the backup with id {}",
backup.getId());
- return false;
+ // Shift chain_position down by 1 for any survivors deeper in the
chain than the
+ // backup we just removed (their direct parent reference is unchanged,
but their
+ // numeric position needs to stay consistent so future full-every
cadence math works).
+ if (plan.shiftPositionsBelow != null) {
+ for (Backup b : backupDao.listByVmId(null, backup.getVmId())) {
+ if (!plan.shiftPositionsBelow.chainId.equals(readDetail(b,
NASBackupChainKeys.CHAIN_ID))) {
+ continue;
+ }
+ int pos = chainPosition(b);
+ if (pos > plan.shiftPositionsBelow.afterPosition && pos !=
Integer.MAX_VALUE) {
+ BackupDetailVO posDetail =
backupDetailsDao.findDetail(b.getId(), NASBackupChainKeys.CHAIN_POSITION);
+ if (posDetail != null) {
+ posDetail.setValue(String.valueOf(pos - 1));
+ backupDetailsDao.update(posDetail.getId(), posDetail);
+ }
+ }
+ }
+ }
+
+ return true;
+ }
+
+ private static final class PositionShift {
+ final String chainId;
+ final int afterPosition; // shift positions strictly greater than this
by -1
+ PositionShift(String chainId, int afterPosition) {
+ this.chainId = chainId;
+ this.afterPosition = afterPosition;
+ }
+ }
+
+ /**
+ * Result of {@link #computeChainRepair}: whether to proceed, what to
rebase, what to delete.
+ */
+ private static final class ChainRepairPlan {
+ final boolean proceed;
+ final String reason;
+ final List<RebaseStep> rebaseSteps;
+ final List<Backup> toDelete;
+ final PositionShift shiftPositionsBelow;
+
+ private ChainRepairPlan(boolean proceed, String reason,
List<RebaseStep> rebaseSteps, List<Backup> toDelete,
+ PositionShift shiftPositionsBelow) {
+ this.proceed = proceed;
+ this.reason = reason;
+ this.rebaseSteps = rebaseSteps;
+ this.toDelete = toDelete;
+ this.shiftPositionsBelow = shiftPositionsBelow;
+ }
+
+ static ChainRepairPlan refuse(String reason) {
+ return new ChainRepairPlan(false, reason, Collections.emptyList(),
Collections.emptyList(), null);
+ }
+
+ static ChainRepairPlan proceed(List<RebaseStep> rebaseSteps,
List<Backup> toDelete) {
+ return new ChainRepairPlan(true, null, rebaseSteps, toDelete,
null);
+ }
+
+ static ChainRepairPlan proceed(List<RebaseStep> rebaseSteps,
List<Backup> toDelete, PositionShift shift) {
+ return new ChainRepairPlan(true, null, rebaseSteps, toDelete,
shift);
+ }
+ }
+
+ private static final class RebaseStep {
+ final long childBackupId;
+ final String targetMountRelativePath;
+ final String newBackingMountRelativePath;
+ final String newParentUuid; // null when re-pointed onto an
existing full's UUID is desired but unavailable
+ final int newChainPosition;
+
+ RebaseStep(long childBackupId, String targetMountRelativePath, String
newBackingMountRelativePath,
+ String newParentUuid, int newChainPosition) {
+ this.childBackupId = childBackupId;
+ this.targetMountRelativePath = targetMountRelativePath;
+ this.newBackingMountRelativePath = newBackingMountRelativePath;
+ this.newParentUuid = newParentUuid;
+ this.newChainPosition = newChainPosition;
+ }
+ }
+
+ /**
+ * Compute the chain-repair plan for deleting {@code backup}. Conservative
semantics:
+ * - Backups outside any tracked chain (no NAS chain metadata) are
deleted as-is.
+ * - A standalone backup with no children is deleted as-is.
+ * - A middle incremental: rebase its immediate child onto its own
parent, then delete it.
+ * Descendants of that child are unaffected (their backing chain still
resolves).
+ * - A full with surviving descendants: refuse unless {@code
forced=true}; then delete
+ * full + every descendant (newest first).
+ */
+ private ChainRepairPlan computeChainRepair(Backup backup, boolean forced) {
+ String chainId = readDetail(backup, NASBackupChainKeys.CHAIN_ID);
+ if (chainId == null) {
+ // Pre-incremental backups (or callers that never wrote chain
metadata) — single delete.
+ return ChainRepairPlan.proceed(Collections.emptyList(),
Collections.singletonList(backup));
+ }
+
+ // Gather every backup in the same chain for this VM.
+ List<Backup> chain = new ArrayList<>();
+ for (Backup b : backupDao.listByVmId(null, backup.getVmId())) {
+ if (chainId.equals(readDetail(b, NASBackupChainKeys.CHAIN_ID))) {
+ chain.add(b);
+ }
+ }
+ chain.sort(Comparator.comparingInt(b -> chainPosition(b)));
+
+ int targetPos = chainPosition(backup);
+ boolean isFull = targetPos == 0;
+ List<Backup> descendants = chain.stream()
+ .filter(b -> chainPosition(b) > targetPos)
+ .collect(Collectors.toList());
+
+ if (isFull) {
+ if (descendants.isEmpty()) {
+ return ChainRepairPlan.proceed(Collections.emptyList(),
Collections.singletonList(backup));
+ }
+ if (!forced) {
+ return ChainRepairPlan.refuse(String.format(
+ "Backup %s is the full anchor of a chain with %d
incremental(s). Delete the incrementals first, " +
+ "or pass forced=true to remove the entire
chain.",
+ backup.getUuid(), descendants.size()));
+ }
+ // Forced delete: remove descendants newest first, then the full.
+ List<Backup> victims = new ArrayList<>(descendants);
+ victims.sort(Comparator.comparingInt((Backup b) ->
chainPosition(b)).reversed());
+ victims.add(backup);
+ return ChainRepairPlan.proceed(Collections.emptyList(), victims);
+ }
+
+ // Middle (or tail) incremental.
+ if (descendants.isEmpty()) {
+ // Tail: nothing to rebase, just delete.
+ return ChainRepairPlan.proceed(Collections.emptyList(),
Collections.singletonList(backup));
+ }
+
+ // Middle: only the immediate child needs to absorb our blocks and
rebase onto our parent.
+ Backup immediateChild = descendants.stream()
+ .min(Comparator.comparingInt(b -> chainPosition(b)))
+ .orElseThrow(() -> new CloudRuntimeException("Internal error:
no immediate child found for chain repair"));
+ Backup ourParent = chain.stream()
+ .filter(b -> chainPosition(b) == targetPos - 1)
+ .findFirst()
+ .orElseThrow(() -> new CloudRuntimeException(String.format(
+ "Cannot delete %s: its parent (chain_position=%d) is
missing from the chain",
+ backup.getUuid(), targetPos - 1)));
+
+ VolumeVO rootVolume =
volumeDao.getInstanceRootVolume(backup.getVmId());
+ String volUuid = rootVolume == null ? "root" : rootVolume.getUuid();
+ String childPath = immediateChild.getExternalId() + "/root." + volUuid
+ ".qcow2";
+ String parentPath = ourParent.getExternalId() + "/root." + volUuid +
".qcow2";
+
+ RebaseStep step = new RebaseStep(immediateChild.getId(), childPath,
parentPath,
+ ourParent.getUuid(), chainPosition(immediateChild) - 1);
+
+ // After we delete the middle backup, every descendant's numeric
chain_position
+ // becomes stale (off by one). Their backing-file pointers don't need
re-writing
+ // (only the immediate child changed parents) but their position
metadata does.
+ return ChainRepairPlan.proceed(
+ Collections.singletonList(step),
Review Comment:
Chain repair on delete-middle currently rebases only the root qcow2
(`.../root.<volUuid>.qcow2`). For VMs with additional data disks, the
corresponding `datadisk.<uuid>.qcow2` layers will still reference the deleted
backup, breaking restores. The repair plan needs to generate `RebaseStep`s per
backed-up disk (and delete the per-disk files consistently).
```suggestion
List<RebaseStep> rebaseSteps = new ArrayList<>();
List<VolumeVO> volumes = volumeDao.findByInstance(backup.getVmId());
if (CollectionUtils.isNotEmpty(volumes)) {
for (VolumeVO volume : volumes) {
String diskPrefix =
Volume.Type.ROOT.equals(volume.getVolumeType()) ? "root." : "datadisk.";
String volumeUuid = volume.getUuid();
if (volumeUuid == null) {
LOG.warn("Skipping rebase step for volume id={} in
backup chain {} because UUID is null",
volume.getId(), chainId);
continue;
}
String childPath = immediateChild.getExternalId() + "/" +
diskPrefix + volumeUuid + ".qcow2";
String parentPath = ourParent.getExternalId() + "/" +
diskPrefix + volumeUuid + ".qcow2";
rebaseSteps.add(new RebaseStep(immediateChild.getId(),
childPath, parentPath,
ourParent.getUuid(), chainPosition(immediateChild) -
1));
}
}
if (rebaseSteps.isEmpty()) {
VolumeVO rootVolume =
volumeDao.getInstanceRootVolume(backup.getVmId());
String volUuid = rootVolume == null ? "root" :
rootVolume.getUuid();
String childPath = immediateChild.getExternalId() + "/root." +
volUuid + ".qcow2";
String parentPath = ourParent.getExternalId() + "/root." +
volUuid + ".qcow2";
rebaseSteps.add(new RebaseStep(immediateChild.getId(),
childPath, parentPath,
ourParent.getUuid(), chainPosition(immediateChild) - 1));
}
// After we delete the middle backup, every descendant's numeric
chain_position
// becomes stale (off by one). Their backing-file pointers don't
need re-writing
// (only the immediate child changed parents) but their position
metadata does.
return ChainRepairPlan.proceed(
rebaseSteps,
```
--
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]
