Copilot commented on code in PR #14484:
URL: https://github.com/apache/cloudstack/pull/14484#discussion_r4360293054
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java:
##########
@@ -2109,13 +2117,7 @@ public Answer createSnapshot(final CreateObjectCommand
cmd) {
newSnapshot.setPhysicalSize(snapshotSize);
}
} else if (primaryPool.getType() == StoragePoolType.CLVM ||
primaryPool.getType() == StoragePoolType.CLVM_NG) {
- if (primaryPool.getType() == StoragePoolType.CLVM_NG &&
snapshotTO.isKvmIncrementalSnapshot()) {
- if (secondaryPool == null) {
- String errorMsg = String.format("Incremental
snapshots for CLVM_NG require secondary storage. " +
- "Please configure secondary storage or
disable incremental snapshots for volume [%s].", volume.getName());
- logger.error(errorMsg);
- return new CreateObjectAnswer(errorMsg);
- }
+ if (isClvmNgIncremental) {
logger.info("Taking incremental snapshot of CLVM_NG
volume [{}] using QCOW2 backup to secondary storage.", volume.getName());
newSnapshot =
takeIncrementalVolumeSnapshotOfStoppedVm(snapshotTO, primaryPool, secondaryPool,
imageStoreTo.getUrl(), snapshotName, volume,
conn, cmd.getWait());
Review Comment:
`imageStoreTo.getUrl()` can throw a NullPointerException if `imageStoreTo`
is null. The earlier guard only checks `secondaryPool == null`, but does not
guarantee `imageStoreTo` is non-null; either validate `imageStoreTo` for
CLVM-NG incremental snapshots (and return a clear error) or pass a URL derived
from `secondaryPool` when `imageStoreTo` is absent.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java:
##########
@@ -2095,7 +2095,15 @@ public Answer createSnapshot(final CreateObjectCommand
cmd) {
String diskPath = disk.getPath();
String snapshotPath = diskPath + File.separator + snapshotName;
SnapshotObjectTO newSnapshot = new SnapshotObjectTO();
- if (DomainInfo.DomainState.VIR_DOMAIN_RUNNING.equals(state) &&
!primaryPool.isExternalSnapshot()) {
+ boolean isClvmNgIncremental = primaryPool.getType() ==
StoragePoolType.CLVM_NG && snapshotTO.isKvmIncrementalSnapshot();
+ if (isClvmNgIncremental && secondaryPool == null) {
+ String errorMsg = String.format("Incremental snapshots for
CLVM_NG require secondary storage. " +
+ "Please configure secondary storage or disable
incremental snapshots for volume [%s].", volume.getName());
+ logger.error(errorMsg);
+ return new CreateObjectAnswer(errorMsg);
+ }
+ boolean isLiveClvmNgIncremental = isClvmNgIncremental &&
(DomainInfo.DomainState.VIR_DOMAIN_RUNNING.equals(state) ||
DomainInfo.DomainState.VIR_DOMAIN_PAUSED.equals(state));
+ if ((DomainInfo.DomainState.VIR_DOMAIN_RUNNING.equals(state) &&
!primaryPool.isExternalSnapshot()) || isLiveClvmNgIncremental) {
if (snapshotTO.isKvmIncrementalSnapshot()) {
Review Comment:
`takeIncrementalVolumeSnapshotOfRunningVm(...)` is now used for the
`VIR_DOMAIN_PAUSED` state as well. To keep the code self-explanatory, consider
either renaming the method (or extracting a more general “live” method) or
adding an inline comment explaining that the implementation supports both
RUNNING and PAUSED domains.
##########
server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java:
##########
@@ -1638,8 +1638,9 @@ public SnapshotInfo takeSnapshot(VolumeInfo volume)
throws ResourceAllocationExc
boolean backupSnapToSecondary =
isBackupSnapshotToSecondaryForZone(volume.getDataCenterId());
StoragePoolType poolType = volume.getStoragePoolType();
+ boolean isClvmNgIncrementalCandidate = StoragePoolType.CLVM_NG ==
poolType && kvmIncrementalSnapshot.valueIn(clusterId);
Review Comment:
CLVM-NG incremental snapshots appear to require secondary storage (enforced
later in `KVMStorageProcessor`), but this method only allocates/targets
secondary storage when `backupSnapToSecondary` is true. Consider failing fast
here with a targeted error when `isClvmNgIncrementalCandidate` is true but
`backupSnapToSecondary` is false, to avoid late failures and make the
configuration dependency explicit.
--
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]
