Copilot commented on code in PR #14484:
URL: https://github.com/apache/cloudstack/pull/14484#discussion_r4437462649
##########
server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java:
##########
@@ -1681,7 +1682,7 @@ public SnapshotInfo takeSnapshot(VolumeInfo volume)
throws ResourceAllocationExc
SnapshotInfo snapshotOnPrimary =
snapshotStrategy.takeSnapshot(snapshot);
if (archiveSnapshotToSecondary) {
- if (!isKvmAndFileBasedStorage) {
+ if (!isKvmAndFileBasedStorage &&
!isClvmNgIncrementalCandidate) {
Review Comment:
With `asyncbackup=true`, CLVM-NG incrementals now remove the primary
snapshot-store reference, but the condition at line 1718 still selects
`Primary`. The lookup then fails, skipping the snapshot-create usage event and
storage-quota correction and leaving the incremental charged at the full volume
size. Add `!isClvmNgIncrementalCandidate` to that downstream condition so
accounting uses the ready image-store reference.
##########
server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java:
##########
@@ -1681,7 +1682,7 @@ public SnapshotInfo takeSnapshot(VolumeInfo volume)
throws ResourceAllocationExc
SnapshotInfo snapshotOnPrimary =
snapshotStrategy.takeSnapshot(snapshot);
if (archiveSnapshotToSecondary) {
- if (!isKvmAndFileBasedStorage) {
+ if (!isKvmAndFileBasedStorage &&
!isClvmNgIncrementalCandidate) {
backupSnapshotToSecondary(payload.getAsyncBackup(),
snapshotStrategy, snapshotOnPrimary, payload.getZoneIds(),
payload.getStoragePoolIds());
Review Comment:
For CLVM-NG incrementals with `asyncbackup=true`, this bypasses
`BackupSnapshotTask`, which handles requested destination zones and pools. The
remaining destination-copy calls run only when `asyncbackup=false`, so requests
can report success without creating the requested replicas. Schedule
asynchronous destination copies after the direct-to-secondary snapshot is
finalized, without rerunning backup from the removed primary reference.
##########
server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java:
##########
@@ -1926,7 +1927,7 @@ private void updateSnapshotPayload(long storagePoolId,
CreateSnapshotPayload pay
payload.setLocationType(null);
}
- if (isKvmAndFileBasedStorage &&
kvmIncrementalSnapshot.valueIn(clusterId)) {
+ if ((isKvmAndFileBasedStorage || StoragePoolType.CLVM_NG == poolType)
&& kvmIncrementalSnapshot.valueIn(clusterId)) {
Review Comment:
When incremental snapshots are enabled but
`snapshot.backup.to.secondary=false` or the volume is in an edge zone,
`takeSnapshot` does not attach an image store. This condition still enables
CLVM-NG incremental mode, so the agent rejects snapshots that previously used
the LVM snapshot path. Gate CLVM-NG incremental mode on secondary archival
being enabled to preserve primary-only snapshots.
--
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]
