abh1sar commented on code in PR #13074:
URL: https://github.com/apache/cloudstack/pull/13074#discussion_r3216411026
##########
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:
Add the config `nas.backup.incremental.enabled` to enable/disable
incremental backups feature.
Also please make sure the transition is seamless with existing backups when
someone enables or disables the feature. I think the code should handle that
already, but please double check.
##########
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,
Review Comment:
The delete backup logic seems a bit complicated due to rebasing and chain
manipulation on deleting a backup with children.
The logic CloudStack uses for deleting incremental snapshots in cloudstack
will apply to backups as follows:
* Delete comes for backup B1
* If B1 has child backups:
* Mark B1 as `delete-pending` in the database
* Do not delete the backup file
* Do not modify the backup chain
* If B1 has no child backups in the database:
* call DeleteBackup(B1)
DeleteBackup(B1)
* Delete B1's backup record from the database
* Delete B1's backup file from storage
* If B1's parent (P1) is marked with `delete-pending`
* call DeleteBackup(P1)
Please consider using this logic as it is seems less risky, maintainable and
semantically consistent with the already present incremental snapshots logic.
##########
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());
Review Comment:
This logic breaks after restore. We can't use the last backup taken as
parent in that case.
May I suggest a way to tackle this:
1. Keep an `active_checkpoint_id` in vm_details
2. Take Backup will update VM's `active_checkpoint_id` and the backup's
`checkpoint_id` to the same value.
4. Restore will set the VM's `active_checkpoint_id` to null
5. If VM's `active_checkpoint_id` is null, next backup is full.
6. If VM's `active_checkpoint_id` is not null, it is an incremental backup.
* 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 a
full backup.
* I don't think we need the loop written on line 257. Just checking
the latest backup should be enough.
##########
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:
The backup_files for different volumes have different uuids. They all don't
share root volume's uuid.
So, we need to pass separate parent backup path's for each volume.
The parent backup path consists of two parts.
1. The directory
2. The backup file name
The directory is `<nas root>/<instance-name>/<external-id>`
the backup file name is `[root|datadisk].<volume path uuid>.qcow2`
RestoreBackupCommand passes the directory and backup files for the volumes
to be restored.
We have to do the same thing and pass the directory and backup files
belonging to the parent to the TakeBackupCommand. So you can reuse that code.
```
List<Backup.VolumeInfo> backedVolumes = backup.getBackedUpVolumes();
restoreCommand.setBackupPath(backup.getExternalId());
restoreCommand.setBackupFiles(getBackupFiles(backedVolumes));
```
In similar terms we can write
```
List<Backup.VolumeInfo> parentBackedVolumes =
parentBackup.getBackedUpVolumes();
TakeBackupCommand command = new
TakeBackupCommand(vm.getInstanceName(), backupPath);
command.setBackupPath(backup.getExternalId());
command.setBackupFiles(getBackupFiles(backedVolumes));
```
##########
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:
We should set the new bitmap in the qcow2 files for stopped VMs backup also
so that the next backup when the vm is started is incremental.
`qemu-img bitmap -add` can be used to do it.
--
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]
