Damans227 commented on code in PR #14502:
URL: https://github.com/apache/cloudstack/pull/14502#discussion_r4388380373
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtDomainXMLParser.java:
##########
@@ -405,6 +406,21 @@ public boolean parseDomainXML(String domXML) {
return false;
}
+ private void parseBackingFiles(Element disk, DiskDef def) {
+ NodeList backingStoreNodeList =
disk.getElementsByTagName("backingStore");
+ List<String> backingStoreList = new ArrayList<>();
+ while (backingStoreNodeList.getLength() > 0) {
+ Element backingStore = (Element)backingStoreNodeList.item(0);
+ String path = getAttrValue("source", "file", backingStore);
+ if (StringUtils.isEmpty(path)) {
+ break;
+ }
+
backingStoreList.add(path.substring(path.lastIndexOf(File.separator))+1);
Review Comment:
@JoaoJandre is the +1 meant to be inside the brackets? right now every name
in the list ends up with a 1 on the end
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMergeDiskOnlyVMSnapshotCommandWrapper.java:
##########
@@ -113,38 +124,83 @@ protected Answer
mergeDiskOnlySnapshotsForRunningVM(MergeDiskOnlyVmSnapshotComma
Domain domain = resource.getDomain(conn, vmName);
List<LibvirtVMDef.DiskDef> disks = resource.getDisks(conn, vmName);
KVMStoragePoolManager storageManager = resource.getStoragePoolMgr();
- QemuImg qemuImg = new QemuImg(resource.getCmdsTimeout());
+ long timeout = cmd.getWait() * 1000L;
logger.debug("Merging disk-only snapshots for running VM [{}] using
the following Snapshot Merge Trees [{}].", vmName, snapshotMergeTreeTOList);
for (SnapshotMergeTreeTO mergeTreeTO : snapshotMergeTreeTOList) {
+ if (timeout <= 0) {
+ logger.warn("Disk-only VM snapshot for VM [{}] timed out.
Consider increasing the timeout and try again later.", vmName);
+ return new Answer(cmd, false, null);
+ }
+ long startTime = System.currentTimeMillis();
DataTO childTO = mergeTreeTO.getChild();
SnapshotObjectTO parentSnapshotTO = (SnapshotObjectTO)
mergeTreeTO.getParent();
VolumeObjectTO volumeObjectTO = parentSnapshotTO.getVolume();
KVMStoragePool storagePool =
libvirtUtilitiesHelper.getPrimaryPoolFromDataTo(volumeObjectTO, storageManager);
boolean active =
DataObjectType.VOLUME.equals(childTO.getObjectType());
- String label = resource.getDiskWithPathOfVolumeObjectTO(disks,
volumeObjectTO).getDiskLabel();
- String parentSnapshotLocalPath =
storagePool.getLocalPathFor(parentSnapshotTO.getPath());
- String childDeltaPath =
storagePool.getLocalPathFor(childTO.getPath());
- logger.debug("Found label [{}] for [{}]. Will merge delta at [{}]
into delta at [{}].", label, volumeObjectTO, parentSnapshotLocalPath,
childDeltaPath);
+ LibvirtVMDef.DiskDef diskDef = getDiskDef(disks, volumeObjectTO,
childTO, parentSnapshotTO);
+ if (diskDef == null) {
+ logger.warn("Volume [{}] does not match any record we have.
This must be manually normalized.", volumeObjectTO.getUuid());
+ return new Answer(cmd, false, null);
+ }
+ String label = diskDef.getDiskLabel();
+
+ DomainBlockJobInfo jobInfo = domain.getBlockJobInfo(label, 0);
+ if (jobInfo == null || jobInfo.type == 0 && jobInfo.end == 0 &&
jobInfo.cur == 0) {
+ logger.debug("No block job found for disk [{}] of VM [{}].
Continuing the merge process.", label, vmName);
+ } else {
+ logger.debug("Found a block job for disk [{}] of VM [{}]. We
will abort it to continue with the VM snapshot merge process.", label, vmName);
+ domain.blockJobAbort(label, 0);
Review Comment:
@JoaoJandre can we only stop the job when its a commit job? right now any
job on that disk gets stopped, like a disk move or a backup
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -6363,14 +6363,15 @@ public static String
generateSecretUUIDFromString(String seed) {
* @param snapshotName Name of the snapshot;
* @param volume VolumeObjectTO of the corresponding volume;
* @param conn Libvirt connection;
+ * @param timeout the timeout in milisseconds
* @throws LibvirtException
*/
public void mergeSnapshotIntoBaseFile(Domain vm, String diskLabel, String
baseFilePath, String topFilePath, boolean active, String snapshotName,
VolumeObjectTO volume,
- Connect conn) throws LibvirtException {
+ Connect conn, long timeout) throws LibvirtException {
if
(AgentPropertiesFileHandler.getPropertyValue(AgentProperties.LIBVIRT_EVENTS_ENABLED))
{
mergeSnapshotIntoBaseFileWithEventsAndConfigurableTimeout(vm,
diskLabel, baseFilePath, topFilePath, active, snapshotName, volume, conn);
Review Comment:
@JoaoJandre should the new timeout be passed here too? with libvirt events
on, the merge still uses the old agent setting
##########
engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategy.java:
##########
@@ -442,6 +443,7 @@ private List<SnapshotVO> mergeSnapshots(VMSnapshotVO
vmSnapshotVO, VMSnapshotVO
}
MergeDiskOnlyVmSnapshotCommand mergeDiskOnlyVMSnapshotCommand = new
MergeDiskOnlyVmSnapshotCommand(snapshotMergeTreeToList, userVm.getState(),
userVm.getName());
+ mergeDiskOnlyVMSnapshotCommand.setWait(DEFAULT_MERGE_TIMEOUT);
Answer answer = agentMgr.easySend(hostId,
mergeDiskOnlyVMSnapshotCommand);
if (answer == null || !answer.getResult()) {
throw new CloudRuntimeException(String.format("Failed to merge VM
snapshot [%s] due to %s.", vmSnapshotVO.getUuid(), answer != null ?
answer.getDetails() : "Communication failure"));
Review Comment:
@JoaoJandre should this error tell the user to retry the delete? if the vm
gets stopped before that, it still wont start
##########
engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategy.java:
##########
@@ -86,6 +86,7 @@ public class KvmFileBasedStorageVmSnapshotStrategy extends
StorageVMSnapshotStra
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";
+ private static final int DEFAULT_MERGE_TIMEOUT = 10800;
Review Comment:
@JoaoJandre any reason to fix this at 3 hours instead of making it a setting?
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMergeDiskOnlyVMSnapshotCommandWrapper.java:
##########
@@ -113,38 +124,83 @@ protected Answer
mergeDiskOnlySnapshotsForRunningVM(MergeDiskOnlyVmSnapshotComma
Domain domain = resource.getDomain(conn, vmName);
List<LibvirtVMDef.DiskDef> disks = resource.getDisks(conn, vmName);
KVMStoragePoolManager storageManager = resource.getStoragePoolMgr();
- QemuImg qemuImg = new QemuImg(resource.getCmdsTimeout());
+ long timeout = cmd.getWait() * 1000L;
logger.debug("Merging disk-only snapshots for running VM [{}] using
the following Snapshot Merge Trees [{}].", vmName, snapshotMergeTreeTOList);
for (SnapshotMergeTreeTO mergeTreeTO : snapshotMergeTreeTOList) {
+ if (timeout <= 0) {
+ logger.warn("Disk-only VM snapshot for VM [{}] timed out.
Consider increasing the timeout and try again later.", vmName);
+ return new Answer(cmd, false, null);
+ }
+ long startTime = System.currentTimeMillis();
DataTO childTO = mergeTreeTO.getChild();
SnapshotObjectTO parentSnapshotTO = (SnapshotObjectTO)
mergeTreeTO.getParent();
VolumeObjectTO volumeObjectTO = parentSnapshotTO.getVolume();
KVMStoragePool storagePool =
libvirtUtilitiesHelper.getPrimaryPoolFromDataTo(volumeObjectTO, storageManager);
boolean active =
DataObjectType.VOLUME.equals(childTO.getObjectType());
- String label = resource.getDiskWithPathOfVolumeObjectTO(disks,
volumeObjectTO).getDiskLabel();
- String parentSnapshotLocalPath =
storagePool.getLocalPathFor(parentSnapshotTO.getPath());
- String childDeltaPath =
storagePool.getLocalPathFor(childTO.getPath());
- logger.debug("Found label [{}] for [{}]. Will merge delta at [{}]
into delta at [{}].", label, volumeObjectTO, parentSnapshotLocalPath,
childDeltaPath);
+ LibvirtVMDef.DiskDef diskDef = getDiskDef(disks, volumeObjectTO,
childTO, parentSnapshotTO);
+ if (diskDef == null) {
+ logger.warn("Volume [{}] does not match any record we have.
This must be manually normalized.", volumeObjectTO.getUuid());
+ return new Answer(cmd, false, null);
+ }
+ String label = diskDef.getDiskLabel();
+
+ DomainBlockJobInfo jobInfo = domain.getBlockJobInfo(label, 0);
+ if (jobInfo == null || jobInfo.type == 0 && jobInfo.end == 0 &&
jobInfo.cur == 0) {
+ logger.debug("No block job found for disk [{}] of VM [{}].
Continuing the merge process.", label, vmName);
+ } else {
+ logger.debug("Found a block job for disk [{}] of VM [{}]. We
will abort it to continue with the VM snapshot merge process.", label, vmName);
+ domain.blockJobAbort(label, 0);
+ }
+
+ boolean volumeAlreadyMerged = isVolumeAlreadyMerged(diskDef,
childTO);
+ String parentSnapshotLocalPath =
storagePool.getLocalPathFor(parentSnapshotTO.getPath());
- resource.mergeSnapshotIntoBaseFile(domain, label,
parentSnapshotLocalPath, childDeltaPath, active, childTO.getPath(),
- volumeObjectTO, conn);
+ if (volumeAlreadyMerged) {
+ logger.info("Volume [{}] was already merged in a previous
attempt. Skipping merge for it.", volumeObjectTO.getUuid());
+ } else {
+ String childDeltaPath =
storagePool.getLocalPathFor(childTO.getPath());
+ logger.debug("Found label [{}] for [{}]. Will merge delta at
[{}] into delta at [{}].", label, volumeObjectTO, parentSnapshotLocalPath,
childDeltaPath);
+ resource.mergeSnapshotIntoBaseFile(domain, label,
parentSnapshotLocalPath, childDeltaPath, active, childTO.getPath(),
volumeObjectTO, conn, timeout);
+ }
QemuImgFile parent = new QemuImgFile(parentSnapshotLocalPath,
QemuImg.PhysicalDiskFormat.QCOW2);
-
List<QemuImgFile> grandChildren =
mergeTreeTO.getGrandChildren().stream()
.map(snapshotTo -> new
QemuImgFile(storagePool.getLocalPathFor(snapshotTo.getPath()),
QemuImg.PhysicalDiskFormat.QCOW2))
.collect(Collectors.toList());
logger.debug("Rebasing grandChildren [{}] into parent at [{}].",
grandChildren, parentSnapshotLocalPath);
for (QemuImgFile grandChild : grandChildren) {
+ QemuImg qemuImg = new QemuImg(timeout);
qemuImg.rebase(grandChild, parent,
parent.getFormat().toString(), false);
}
+ timeout = timeout - (System.currentTimeMillis() - startTime);
}
return new Answer(cmd, true, null);
}
+
+ protected boolean isVolumeAlreadyMerged(LibvirtVMDef.DiskDef diskDef,
DataTO childTO) {
Review Comment:
@JoaoJandre can we add tests for this check and the backing file parsing? if
it gets it wrong a real merge is skipped, and codecov shows most new lines
arent covered
--
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]
