DaanHoogland commented on code in PR #6331:
URL: https://github.com/apache/cloudstack/pull/6331#discussion_r863501602
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java:
##########
@@ -102,6 +102,7 @@ public Answer execute(final MigrateCommand command, final
LibvirtComputingResour
final Map<String, Boolean> vlanToPersistenceMap =
command.getVlanToPersistenceMap();
final String destinationUri =
createMigrationURI(command.getDestinationIp(), libvirtComputingResource);
final List<MigrateDiskInfo> migrateDiskInfoList =
command.getMigrateDiskInfoList();
+ s_logger.debug(String.format("Trying to migrate VM [%s] to destination
host: [%s].", vmName, destinationUri));
Review Comment:
```suggestion
if (s_logger.isDebugEnabled()) {
s_logger.debug(String.format("Trying to migrate VM [%s] to
destination host: [%s].", vmName, destinationUri));
}
```
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -4500,12 +4500,15 @@ public List<Ternary<String, Boolean, String>>
cleanVMSnapshotMetadata(Domain dm)
s_logger.debug("Cleaning the metadata of vm snapshots of vm " +
dm.getName());
List<Ternary<String, Boolean, String>> vmsnapshots = new
ArrayList<Ternary<String, Boolean, String>>();
if (dm.snapshotNum() == 0) {
+ s_logger.debug(String.format("VM [%s] does not have any snapshots.
Skipping cleanup of snapshots for this VM.", dm.getName()));
Review Comment:
```suggestion
if (s_logger.isDebugEnabled()) {
s_logger.debug(String.format("VM [%s] does not have any
snapshots. Skipping cleanup of snapshots for this VM.", dm.getName()));
}
```
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java:
##########
@@ -148,13 +150,16 @@ Use VIR_DOMAIN_XML_SECURE (value = 1) prior to v1.0.0.
final String target = command.getDestinationIp();
xmlDesc = dm.getXMLDesc(xmlFlag);
- xmlDesc = replaceIpForVNCInDescFile(xmlDesc, target);
+ s_logger.debug(String.format("VM [%s] with XML configuration [%s]
will be migrated to host [%s].", vmName, xmlDesc, target));
Review Comment:
```suggestion
if (s_logger.isDebugEnabled()) {
s_logger.debug(String.format("VM [%s] with XML configuration
[%s] will be migrated to host [%s].", vmName, xmlDesc, target));
}
```
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -4517,25 +4520,23 @@ public List<Ternary<String, Boolean, String>>
cleanVMSnapshotMetadata(Domain dm)
Element rootElement = doc.getDocumentElement();
currentSnapshotName = getTagValue("name", rootElement);
- } catch (ParserConfigurationException e) {
- s_logger.debug(e.toString());
- } catch (SAXException e) {
- s_logger.debug(e.toString());
- } catch (IOException e) {
- s_logger.debug(e.toString());
+ } catch (ParserConfigurationException | SAXException | IOException
e) {
+ s_logger.error(String.format("Failed to parse snapshot
configuration [%s] of VM [%s] due to: [%s].", snapshotXML, dm.getName(),
e.getMessage()), e);
}
} catch (LibvirtException e) {
- s_logger.debug("Fail to get the current vm snapshot for vm: " +
dm.getName() + ", continue");
+ s_logger.error(String.format("Failed to get the current snapshot
of VM [%s] due to: [%s]. Continuing the migration process.", dm.getName(),
e.getMessage()), e);
}
int flags = 2; // VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY = 2
String[] snapshotNames = dm.snapshotListNames();
Arrays.sort(snapshotNames);
+ s_logger.debug(String.format("Found [%s] snapshots in VM [%s] to
clean.", snapshotNames.length, dm.getName()));
for (String snapshotName: snapshotNames) {
DomainSnapshot snapshot = dm.snapshotLookupByName(snapshotName);
Boolean isCurrent = (currentSnapshotName != null &&
currentSnapshotName.equals(snapshotName)) ? true: false;
vmsnapshots.add(new Ternary<String, Boolean, String>(snapshotName,
isCurrent, snapshot.getXMLDesc()));
}
for (String snapshotName: snapshotNames) {
+ s_logger.debug(String.format("Cleaning snapshot [%s] of VM [%s]
metadata.", snapshotNames, dm.getName()));
Review Comment:
```suggestion
if (s_logger.isDebugEnabled()) {
s_logger.debug(String.format("Cleaning snapshot [%s] of VM
[%s] metadata.", snapshotNames, dm.getName()));
}
```
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java:
##########
@@ -451,15 +460,17 @@ protected MigrateDiskInfo
searchDiskDefOnMigrateDiskInfoList(List<MigrateDiskInf
* @param target the ip address to migrate to
* @return the new xmlDesc
*/
- String replaceIpForVNCInDescFile(String xmlDesc, final String target) {
+ String replaceIpForVNCInDescFile(String xmlDesc, final String target,
String vmName) {
final int begin = xmlDesc.indexOf(GRAPHICS_ELEM_START);
if (begin >= 0) {
final int end = xmlDesc.lastIndexOf(GRAPHICS_ELEM_END) +
GRAPHICS_ELEM_END.length();
if (end > begin) {
+ String originalGraphElem = xmlDesc.substring(begin, end);
String graphElem = xmlDesc.substring(begin, end);
graphElem = graphElem.replaceAll("listen='[a-zA-Z0-9\\.]*'",
"listen='" + target + "'");
graphElem = graphElem.replaceAll("address='[a-zA-Z0-9\\.]*'",
"address='" + target + "'");
xmlDesc = xmlDesc.replaceAll(GRAPHICS_ELEM_START +
CONTENTS_WILDCARD + GRAPHICS_ELEM_END, graphElem);
+ s_logger.debug(String.format("Replaced the VNC IP address [%s]
with [%s] in VM [%s].", originalGraphElem, graphElem, vmName));
Review Comment:
```suggestion
if (s_logger.isDebugEnabled()) {
s_logger.debug(String.format("Replaced the VNC IP address
[%s] with [%s] in VM [%s].", originalGraphElem, graphElem, vmName));
}
```
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java:
##########
@@ -174,12 +179,16 @@ Use VIR_DOMAIN_XML_SECURE (value = 1) prior to v1.0.0.
final boolean migrateStorageManaged =
command.isMigrateStorageManaged();
if (migrateStorage) {
+ s_logger.debug(String.format("Changing VM [%s] volumes during
migration to host: [%s].", vmName, target));
xmlDesc = replaceStorage(xmlDesc, mapMigrateStorage,
migrateStorageManaged);
+ s_logger.debug(String.format("Changed VM [%s] XML
configuration of used storage. New XML configuration is [%s].", vmName,
xmlDesc));
}
Map<String, DpdkTO> dpdkPortsMapping =
command.getDpdkInterfaceMapping();
if (MapUtils.isNotEmpty(dpdkPortsMapping)) {
+ s_logger.debug(String.format("Changing VM [%s] DPDK interfaces
during migration to host: [%s].", vmName, target));
xmlDesc = replaceDpdkInterfaces(xmlDesc, dpdkPortsMapping);
+ s_logger.debug(String.format("Changed VM [%s] XML
configuration of DPDK interfaces. New XML configuration is [%s].", vmName,
xmlDesc));
Review Comment:
```suggestion
if (s_logger.isDebugEnabled()) {
s_logger.debug(String.format("Changed VM [%s] XML
configuration of DPDK interfaces. New XML configuration is [%s].", vmName,
xmlDesc));
}
```
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java:
##########
@@ -148,13 +150,16 @@ Use VIR_DOMAIN_XML_SECURE (value = 1) prior to v1.0.0.
final String target = command.getDestinationIp();
xmlDesc = dm.getXMLDesc(xmlFlag);
- xmlDesc = replaceIpForVNCInDescFile(xmlDesc, target);
+ s_logger.debug(String.format("VM [%s] with XML configuration [%s]
will be migrated to host [%s].", vmName, xmlDesc, target));
+
+ xmlDesc = replaceIpForVNCInDescFile(xmlDesc, target, vmName);
String oldIsoVolumePath = getOldVolumePath(disks, vmName);
String newIsoVolumePath =
getNewVolumePathIfDatastoreHasChanged(libvirtComputingResource, conn, to);
if (newIsoVolumePath != null &&
!newIsoVolumePath.equals(oldIsoVolumePath)) {
s_logger.debug(String.format("Editing mount path of iso from
%s to %s", oldIsoVolumePath, newIsoVolumePath));
xmlDesc = replaceDiskSourceFile(xmlDesc, newIsoVolumePath,
vmName);
+ s_logger.debug(String.format("Replaced disk mount point [%s]
with [%s] in VM [%s] XML configuration. New XML configuration is [%s].",
oldIsoVolumePath, newIsoVolumePath, vmName, xmlDesc));
Review Comment:
```suggestion
if (s_logger.isDebugEnabled()) {
s_logger.debug(String.format("Replaced disk mount point [%s]
with [%s] in VM [%s] XML configuration. New XML configuration is [%s].",
oldIsoVolumePath, newIsoVolumePath, vmName, xmlDesc));
}
```
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java:
##########
@@ -313,8 +323,7 @@ Use VIR_DOMAIN_XML_SECURE (value = 1) prior to v1.0.0.
}
}
- if (result != null) {
- } else {
+ if (result == null) {
Review Comment:
:+1:
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java:
##########
@@ -122,6 +123,7 @@ public Answer execute(final MigrateCommand command, final
LibvirtComputingResour
ifaces = libvirtComputingResource.getInterfaces(conn, vmName);
disks = libvirtComputingResource.getDisks(conn, vmName);
+ s_logger.debug(String.format("Found domain with name [%s].
Starting VM migration to host [%s].", vmName, destinationUri));
Review Comment:
```suggestion
if (s_logger.isDebugEnabled()) {
s_logger.debug(String.format("Found domain with name [%s].
Starting VM migration to host [%s].", vmName, destinationUri));
}
```
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java:
##########
@@ -174,12 +179,16 @@ Use VIR_DOMAIN_XML_SECURE (value = 1) prior to v1.0.0.
final boolean migrateStorageManaged =
command.isMigrateStorageManaged();
if (migrateStorage) {
+ s_logger.debug(String.format("Changing VM [%s] volumes during
migration to host: [%s].", vmName, target));
Review Comment:
```suggestion
if (s_logger.isTraceEnabled()) {
s_logger.trace(String.format("Changing VM [%s] volumes
during migration to host: [%s].", vmName, target));
}
```
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java:
##########
@@ -174,12 +179,16 @@ Use VIR_DOMAIN_XML_SECURE (value = 1) prior to v1.0.0.
final boolean migrateStorageManaged =
command.isMigrateStorageManaged();
if (migrateStorage) {
+ s_logger.debug(String.format("Changing VM [%s] volumes during
migration to host: [%s].", vmName, target));
xmlDesc = replaceStorage(xmlDesc, mapMigrateStorage,
migrateStorageManaged);
+ s_logger.debug(String.format("Changed VM [%s] XML
configuration of used storage. New XML configuration is [%s].", vmName,
xmlDesc));
}
Map<String, DpdkTO> dpdkPortsMapping =
command.getDpdkInterfaceMapping();
if (MapUtils.isNotEmpty(dpdkPortsMapping)) {
+ s_logger.debug(String.format("Changing VM [%s] DPDK interfaces
during migration to host: [%s].", vmName, target));
Review Comment:
```suggestion
if (s_logger.isTraceEnabled()) {
s_logger.trace(String.format("Changing VM [%s] DPDK
interfaces during migration to host: [%s].", vmName, target));
}
```
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java:
##########
@@ -174,12 +179,16 @@ Use VIR_DOMAIN_XML_SECURE (value = 1) prior to v1.0.0.
final boolean migrateStorageManaged =
command.isMigrateStorageManaged();
if (migrateStorage) {
+ s_logger.debug(String.format("Changing VM [%s] volumes during
migration to host: [%s].", vmName, target));
xmlDesc = replaceStorage(xmlDesc, mapMigrateStorage,
migrateStorageManaged);
+ s_logger.debug(String.format("Changed VM [%s] XML
configuration of used storage. New XML configuration is [%s].", vmName,
xmlDesc));
Review Comment:
```suggestion
if (s_logger.isDebugEnabled()) {
s_logger.debug(String.format("Changed VM [%s] XML
configuration of used storage. New XML configuration is [%s].", vmName,
xmlDesc));
}
```
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java:
##########
@@ -262,16 +271,17 @@ Use VIR_DOMAIN_XML_SECURE (value = 1) prior to v1.0.0.
}
}
}
- s_logger.info("Migration thread for " + vmName + " is done");
+ s_logger.info(String.format("Migration thread of VM [%s]
finished.", vmName));
destDomain =
migrateThread.get(AgentPropertiesFileHandler.getPropertyValue(AgentProperties.VM_MIGRATE_DOMAIN_RETRIEVE_TIMEOUT),
TimeUnit.SECONDS);
if (destDomain != null) {
+ s_logger.debug(String.format("Cleaning the disks of VM [%s] in
the source pool after VM migration finished.", vmName));
Review Comment:
```suggestion
if (s_logger.isDebugEnabled()) {
s_logger.debug(String.format("Cleaning the disks of VM [%s]
in the source pool after VM migration finished.", vmName));
}
```
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -4500,12 +4500,15 @@ public List<Ternary<String, Boolean, String>>
cleanVMSnapshotMetadata(Domain dm)
s_logger.debug("Cleaning the metadata of vm snapshots of vm " +
dm.getName());
List<Ternary<String, Boolean, String>> vmsnapshots = new
ArrayList<Ternary<String, Boolean, String>>();
if (dm.snapshotNum() == 0) {
+ s_logger.debug(String.format("VM [%s] does not have any snapshots.
Skipping cleanup of snapshots for this VM.", dm.getName()));
return vmsnapshots;
}
String currentSnapshotName = null;
try {
DomainSnapshot snapshotCurrent = dm.snapshotCurrent();
String snapshotXML = snapshotCurrent.getXMLDesc();
+ s_logger.debug(String.format("Current snapshot of VM [%s] has the
following XML: [%s].", dm.getName(), snapshotXML));
Review Comment:
```suggestion
if (s_logger.isDebugEnabled()) {
s_logger.debug(String.format("Current snapshot of VM [%s]
has the following XML: [%s].", dm.getName(), snapshotXML));
}
```
--
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]
