abh1sar commented on code in PR #13074:
URL: https://github.com/apache/cloudstack/pull/13074#discussion_r3521026199
##########
plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java:
##########
@@ -168,6 +205,290 @@ 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. Each volume needs its own parent file because
backup files
+ // are named after each volume's own UUID (root.<uuid>.qcow2 /
datadisk.<uuid>.qcow2).
+ 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);
+ }
+
+ /**
+ * Decision used when the incremental feature is disabled: a plain
full backup that
+ * creates no bitmap and carries no chain identity, so nothing
chain/checkpoint-related
+ * is sent to the agent or persisted. Keeps the feature-off path
byte-for-byte legacy.
+ */
+ static ChainDecision legacyFull() {
+ return new ChainDecision(NASBackupChainKeys.TYPE_LEGACY_FULL,
null, null, null, null, 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);
+ }
+
+ boolean isLegacyFull() {
+ return NASBackupChainKeys.TYPE_LEGACY_FULL.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. The decision
deliberately avoids
+ * relying on "last backup taken", because that row is misleading after a
restore.</p>
+ */
+ protected ChainDecision decideChain(VirtualMachine vm) {
+ // Master switch — when the operator disables incrementals at the zone
level the backup
+ // behaves exactly like the pre-incremental full-only path: no bitmap
is generated and no
+ // chain/checkpoint metadata is created, sent to the agent, or
persisted (legacy-full).
+ Boolean incrementalEnabled =
NASBackupIncrementalEnabled.valueIn(vm.getDataCenterId());
+ if (incrementalEnabled == null || !incrementalEnabled) {
+ return ChainDecision.legacyFull();
+ }
+
Review Comment:
Can we add the condition that if any of the volumes in the VM is not one of
NFS/SharedMountPoint/LocalStorage then return legacyFull?
```
i.e, !(ScopeType.HOST.equals(storagePool.getScope()) ||
Storage.StoragePoolType.SharedMountPoint.equals(storagePool.getPoolType() ||
Storage.StoragePoolType.NetworkFilesystem.equals(storagePool.getPoolType())
```
I think this is require as checkpoints will anyway not be possible on
LInstor and Ceph-RBD and it helps avoid any regression on those storages.
--
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]
