jmsperu commented on code in PR #13074:
URL: https://github.com/apache/cloudstack/pull/13074#discussion_r3291231916
##########
plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java:
##########
@@ -85,6 +86,16 @@ public class NASBackupProvider extends AdapterBase
implements BackupProvider, Co
true,
BackupFrameworkEnabled.key());
Review Comment:
Done in `691931de23` — added `nas.backup.incremental.enabled` ConfigKey
(zone-scoped, dynamic, default true). `decideChain()` checks the master switch
first so flipping it off forces every backup to be a fresh full while leaving
existing chains restorable; flipping it back on starts a new chain on the next
backup. Added `decideChainReturnsFullWhenIncrementalDisabled` unit test
covering the disabled path.
##########
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.
Review Comment:
Done in `5be1910ff0` — `TakeBackupCommand.parentPath: String` is now
`parentPaths: List<String>`, one entry per VM volume in deviceId order.
`composeParentBackupPaths(parent, vmId)` reads the parent's `backedUpVolumes`
and emits the correctly-named file per position (`root.<uuid>.qcow2` for the
first, `datadisk.<uuid>.qcow2` for the rest). `nasbackup.sh` parses
`--parent-paths` into a bash array and indexes by disk position in the rebase
loop. When the current VM's volume count doesn't match the parent (volume
added/removed since), compose returns null and `decideChain` falls back to a
fresh full so we never risk misaligning a rebase. Mirrors the per-volume
pattern RestoreBackupCommand already uses.
##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -324,6 +504,36 @@ while [[ $# -gt 0 ]]; do
shift
shift
;;
+ -M|--mode)
+ MODE="$2"
+ shift
+ shift
+ ;;
+ --bitmap-new)
+ BITMAP_NEW="$2"
Review Comment:
Done in `0bdcdb1484` — `backup_stopped_vm()` now runs `qemu-img bitmap
--add` per file-backed disk after the `qemu-img convert` succeeds (persistent +
enabled is the qemu-img default since QEMU 5.0). RBD/LINSTOR sources are
skipped — `qemu-img bitmap` isn't the right primitive for those. On success the
function emits `BITMAP_CREATED=` so the orchestrator pins it on the VM's
`active_checkpoint_id` just like an online backup would; `decideChain` then
picks this backup as the parent on the next attempt. Also taught the
online-side `recreate-checkpoint` path to tolerate a pre-seeded bitmap (libvirt
7.2+ reuses an existing bitmap of the same name; older libvirt fails the create
— in that case we remove the orphan and retry). Either path ends with libvirt's
view in sync with what's on the qcow2.
--
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]
