Skip to content

Fix instance snapshots with KvmFileBasedStorageVmSnapshotStrategy #14515

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
sureshanaparti wants to merge 2 commits into
base: 4.22
Choose a base branch
from
Draft

Fix instance snapshots with KvmFileBasedStorageVmSnapshotStrategy #14515

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a768e89
Fix instance snapshot with KvmFileBasedStorageVmSnapshotStrategy
sureshanaparti Oct 28, 2026
c8da86e
review changes
sureshanaparti Oct 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 43 additions & 6 deletions .../java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,8 @@
public class KvmFileBasedStorageVmSnapshotStrategy extends StorageVMSnapshotStrategy {

private static final List<Storage.StoragePoolType> supportedStoragePoolTypes = List.of(Storage.StoragePoolType.Filesystem, Storage.StoragePoolType.NetworkFilesystem, Storage.StoragePoolType.SharedMountPoint);
private static final String KVM_FILE_BASED_STORAGE_SNAPSHOT_NVRAM = "kvmFileBasedStorageSnapshotNvram";
protected static final String KVM_FILE_BASED_STORAGE_SNAPSHOT_NVRAM = "kvmFileBasedStorageSnapshotNvram";
protected static final String KVM_FILE_BASED_STORAGE_SNAPSHOT_STRATEGY = "kvmFileBasedStorageSnapshotStrategy";

@Inject
protected SnapshotDataStoreDao snapshotDataStoreDao;
Expand Down Expand Up @@ -133,8 +134,12 @@ public VMSnapshot takeVMSnapshot(VMSnapshot vmSnapshot) {
@Override
public boolean deleteVMSnapshot(VMSnapshot vmSnapshot) {
logger.info("Starting VM snapshot delete process for snapshot [{}].", vmSnapshot.getUuid());
UserVmVO userVm = userVmDao.findById(vmSnapshot.getVmId());
VMSnapshotVO vmSnapshotBeingDeleted = (VMSnapshotVO) vmSnapshot;
if (!hasRelevantVmSnapshotDetails(vmSnapshotBeingDeleted)) {
return deleteVMSnapshotMetadataOnly(vmSnapshotBeingDeleted);
}

UserVmVO userVm = userVmDao.findById(vmSnapshot.getVmId());
Long hostId = pickHostForNvramSidecarCleanup(vmSnapshotBeingDeleted, userVm, "delete");
validateHostSupportsNvramSidecarCleanup(vmSnapshotBeingDeleted, hostId, "delete");
Comment on lines 135 to 144
long virtualSize = 0;
Expand Down Expand Up @@ -322,16 +327,47 @@ private void mergeOldSiblingWithOldParentIfOldParentIsDead(VMSnapshotVO oldParen
transitStateWithoutThrow(oldParent, VMSnapshot.Event.OperationSucceeded);
}

protected boolean deleteVMSnapshotMetadataOnly(VMSnapshotVO vmSnapshot) {
logger.info("VM snapshot [{}] has no volume snapshots or NVRAM sidecar on primary storage; removing only its metadata.", vmSnapshot.getUuid());

transitStateWithoutThrow(vmSnapshot, VMSnapshot.Event.ExpungeRequested);
transitStateWithoutThrow(vmSnapshot, VMSnapshot.Event.OperationSucceeded);

vmSnapshotDetailsDao.removeDetails(vmSnapshot.getId());

vmSnapshot.setRemoved(DateUtil.now());
vmSnapshotDao.update(vmSnapshot.getId(), vmSnapshot);

return true;
}

protected boolean hasKVMFileBasedStorageSnapshotDetailInVmSnapshotDetails(VMSnapshot vmSnapshot) {
return CollectionUtils.isNotEmpty(vmSnapshotDetailsDao.findDetails(vmSnapshot.getId(), KVM_FILE_BASED_STORAGE_SNAPSHOT));
}

protected boolean hasRelevantVmSnapshotDetails(VMSnapshotVO vmSnapshot) {
return hasKVMFileBasedStorageSnapshotDetailInVmSnapshotDetails(vmSnapshot) || StringUtils.isNotBlank(getNvramSnapshotPath(vmSnapshot));
}

protected boolean isCreatedByThisStrategy(VMSnapshot vmSnapshot) {
return vmSnapshotDetailsDao.findDetail(vmSnapshot.getId(), KVM_FILE_BASED_STORAGE_SNAPSHOT_STRATEGY) != null;
}

@Override
public StrategyPriority canHandle(VMSnapshot vmSnapshot) {
if (!VMSnapshot.State.Allocated.equals(vmSnapshot.getState())) {
List<VMSnapshotDetailsVO> vmSnapshotDetails = vmSnapshotDetailsDao.findDetails(vmSnapshot.getId(), KVM_FILE_BASED_STORAGE_SNAPSHOT);
if (CollectionUtils.isEmpty(vmSnapshotDetails)) {
VMSnapshot.State state = vmSnapshot.getState();
if (!VMSnapshot.State.Allocated.equals(state)) {
if (hasKVMFileBasedStorageSnapshotDetailInVmSnapshotDetails(vmSnapshot) || isCreatedByThisStrategy(vmSnapshot)) {
return StrategyPriority.HIGHEST;
}
if (!VMSnapshot.State.Error.equals(state)) {
logger.debug("KVM file based storage VM snapshot strategy cannot handle [{}] as it is not a KVM file based storage VM snapshot.",
vmSnapshot.getUuid());
return StrategyPriority.CANT_HANDLE;
}
return StrategyPriority.HIGHEST;
// Error rows created before the strategy marker existed carry no detail at all; fall back to the creation eligibility check.
logger.debug("VM snapshot [{}] is in Error state without any KVM file based storage details; checking whether this strategy would have created it.",
vmSnapshot.getUuid());
Comment on lines +363 to +370

@JoaoJandre JoaoJandre Oct 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this fallback is good approach. Aren't the other changes enough?
My fear is that we eventually try to operate a VM snapshot which is not from this strategy.

}
Comment on lines 357 to 371

long vmId = vmSnapshot.getVmId();
Expand Down Expand Up @@ -523,6 +559,7 @@ protected VMSnapshot takeVmSnapshotInternal(VMSnapshot vmSnapshot, Map<VolumeInf
logger.info("Starting disk-only VM snapshot process for VM [{}].", userVm.getUuid());

transitStateWithoutThrow(vmSnapshot, VMSnapshot.Event.CreateRequested);
vmSnapshotDetailsDao.addDetail(vmSnapshot.getId(), KVM_FILE_BASED_STORAGE_SNAPSHOT_STRATEGY, Boolean.TRUE.toString(), false);

Long hostId = pickHostForUefiNvramAwareDiskOnlySnapshot(userVm, "create");
validateHostSupportsUefiNvramAwareDiskOnlySnapshots(hostId, userVm, "create");
Expand Down
Loading
Loading