Copilot commented on code in PR #13746:
URL: https://github.com/apache/cloudstack/pull/13746#discussion_r3682833042
##########
engine/schema/src/main/java/org/apache/cloudstack/storage/datastore/db/SnapshotDataStoreDaoImpl.java:
##########
@@ -352,8 +361,15 @@ public SnapshotDataStoreVO findParent(DataStoreRole role,
Long storeId, Long zon
return null;
}
+ boolean contentBasedChain = kvmIncrementalSnapshot &&
Hypervisor.HypervisorType.KVM.equals(hypervisorType) &&
usesContentBasedChain(volumeId);
+ if (contentBasedChain && (role == null || !role.isImageStore())) {
+ logger.trace("Content-based snapshot chains only exist on the
image store. Returning null as parent for volume [{}] and role [{}].",
volumeId, role);
+ return null;
+ }
+ boolean checkpointBasedChain = kvmIncrementalSnapshot &&
Hypervisor.HypervisorType.KVM.equals(hypervisorType) && !contentBasedChain;
Review Comment:
`volumeId` is a `Long` in this method signature, but it’s passed to
`usesContentBasedChain(...)` which takes a primitive `long` (auto-unboxing). If
`volumeId` is ever null, this will throw a `NullPointerException` before any
query logic runs. Guard `volumeId` before calling `usesContentBasedChain`
(e.g., treat null as `contentBasedChain = false`), or change
`usesContentBasedChain` to accept `Long` and handle null internally.
##########
plugins/storage/volume/linstor/src/main/java/org/apache/cloudstack/storage/datastore/driver/LinstorPrimaryDataStoreDriverImpl.java:
##########
@@ -1071,15 +1074,30 @@ protected Answer copySnapshot(DataObject srcData,
DataObject destData) {
value,
Integer.parseInt(Config.BackupSnapshotWait.getDefaultValue()));
SnapshotObject snapshotObject = (SnapshotObject)srcData;
- Boolean snapshotFullBackup = snapshotObject.getFullBackup();
final StoragePoolVO pool =
_storagePoolDao.findById(srcData.getDataStore().getId());
final DevelopersApi api = getLinstorAPI(pool);
- boolean fullSnapshot = true;
- if (snapshotFullBackup != null) {
- fullSnapshot = snapshotFullBackup;
- }
+
+ // For encrypted volumes Linstor adds a LUKS layer (DRBD -> LUKS ->
STORAGE). The storage
+ // layer snapshot device (getSnapshotPath) therefore only exposes the
raw LUKS ciphertext,
+ // while restore writes onto the decrypted DRBD device
(/dev/drbd/by-res/.../0). Backing up
+ // the ciphertext and writing it back to the decrypted layer corrupts
the volume (and the
+ // shrink to the net volume size would even truncate the ciphertext).
So for encrypted
+ // volumes we never read the storage snapshot directly: restore the
snapshot into a temporary
+ // resource and back up its decrypted DRBD device instead, symmetric
to the restore path.
+ final boolean encrypted =
snapshotObject.getBaseVolume().getPassphraseId() != null;
+
+ SnapshotDataStoreVO destRef = _snapshotStoreDao.findByStoreSnapshot(
+ destData.getDataStore().getRole(),
destData.getDataStore().getId(), destData.getId());
+ // encrypted volumes are always backed up as full copies: an
incremental rebase would need
+ // the LUKS secret for both the delta and the backing file
+ String parentPath = encrypted ? null :
getIncrementalParentPath(destRef);
+ boolean fullSnapshot = parentPath == null;
Review Comment:
`snapshotObject.getFullBackup()` is no longer consulted when deciding
between full vs incremental backups. If `fullBackup=true` is a
supported/expected override (e.g., force full even when an incremental parent
exists), this change silently ignores the caller’s intent. Consider
reintroducing the `fullBackup` override so that an explicitly requested full
backup forces `parentPath=null` / `fullSnapshot=true`, and ensures chain parent
links are cleared accordingly.
##########
plugins/storage/volume/linstor/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LinstorBackupSnapshotCommandWrapper.java:
##########
@@ -176,12 +219,33 @@ public CopyCmdAnswer execute(LinstorBackupSnapshotCommand
cmd, LibvirtComputingR
final byte[] passphrase = src.getVolume() != null ?
src.getVolume().getPassphrase() : null;
final boolean encrypted = passphrase != null && passphrase.length
> 0;
- String dstPath = convertImageToQCow2(srcPath, dst, secondaryPool,
passphrase, cmd.getWaitInMillSeconds());
+ final Map<String, String> options = cmd.getOptions();
+ final String parentInstallPath = options != null ?
+ options.get(LinstorBackupSnapshotCommand.OPTION_PARENT_PATH) :
null;
+
+ boolean incremental = false;
+ String dstPath = null;
+ if (!encrypted && parentInstallPath != null && src.getVolume() !=
null) {
+ final File parentFile = new File(secondaryPool.getLocalPath()
+ File.separator + parentInstallPath);
+ if (parentFile.isFile()) {
+ dstPath = createIncrementalQCow2(
+ srcPath, dst, secondaryPool, parentFile,
src.getVolume().getSize(), cmd.getWaitInMillSeconds());
+ incremental = true;
+ } else {
+ LOGGER.warn("Parent snapshot file '{}' missing on
secondary storage, taking a full backup instead",
+ parentFile.getAbsolutePath());
+ }
+ }
Review Comment:
`parentInstallPath` is used to build a filesystem path via string
concatenation. Even if this value typically comes from internal state, it’s
safer to prevent path traversal and ensure the resolved path stays within
`secondaryPool.getLocalPath()`. Consider resolving via canonical paths and
verifying the canonical parent file path is under the secondary pool root
before using it (otherwise fall back to full backup).
##########
plugins/storage/volume/linstor/CHANGELOG.md:
##########
@@ -24,6 +24,12 @@ All notable changes to Linstor CloudStack plugin will be
documented in this file
The format is based on [Keep a
Changelog](https://keepachangelog.com/en/1.0.0/),
and this project adheres to [Semantic
Versioning](https://semver.org/spec/v2.0.0.html).
+## [2026-07-30]
+
+### Added
+
+- Support for incremental snapshots on secondary storage backuped snapshots
Review Comment:
Correct the wording: 'backuped' is not standard; use 'backed up' (or
rephrase the sentence).
##########
engine/schema/src/main/java/org/apache/cloudstack/storage/datastore/db/SnapshotDataStoreDaoImpl.java:
##########
@@ -379,13 +395,29 @@ public SnapshotDataStoreVO findParent(DataStoreRole role,
Long storeId, Long zon
SnapshotDataStoreVO parent = snapshotList.get(0);
- if (kvmIncrementalSnapshot && parent.getKvmCheckpointPath() == null &&
Hypervisor.HypervisorType.KVM.equals(hypervisorType)) {
+ if (checkpointBasedChain && parent.getKvmCheckpointPath() == null) {
return null;
}
return parent;
}
+ /**
+ * Volumes on Linstor primary storage chain incremental snapshots on
secondary storage through a
+ * content diff (qemu-img rebase) against the parent snapshot file instead
of qemu checkpoints, so
+ * parent selection must not require a checkpoint path. Encrypted volumes
are excluded as they are
+ * always backed up as full copies (a rebase would need the LUKS secret
for delta and backing file).
+ */
+ @Override
+ public boolean usesContentBasedChain(long volumeId) {
+ VolumeVO volume = volumeDao.findByIdIncludingRemoved(volumeId);
+ if (volume == null || volume.getPoolId() == null ||
volume.getPassphraseId() != null) {
+ return false;
+ }
+ StoragePoolVO pool = storagePoolDao.findById(volume.getPoolId());
+ return pool != null &&
Storage.StoragePoolType.Linstor.equals(pool.getPoolType());
+ }
Review Comment:
`usesContentBasedChain` performs (at least) two DB lookups (volume + pool).
Because it’s called from `findParent(...)`, this could add noticeable overhead
in snapshot-heavy workflows. If possible, consider reducing per-call DB work
(e.g., a single query/join, caching the result per volume/pool for the duration
of the operation, or passing pool type / encryption info from a layer that
already loaded the volume/pool).
--
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]
