Import Remote KVM VM logging improvements (apache#9284)

sureshanaparti · dhslove · commit 4bea15a28677 · 2024-06-28T13:58:16.000+09:00
diff --git a/api/src/main/java/org/apache/cloudstack/api/command/admin/vm/ImportUnmanagedInstanceCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/admin/vm/ImportUnmanagedInstanceCmd.java
@@ -201,8 +201,8 @@ public Map<String, Long> getNicNetworkList() {
             for (Map<String, String> entry : (Collection<Map<String, String>>)nicNetworkList.values()) {
                 String nic = entry.get(VmDetailConstants.NIC);
                 String networkUuid = entry.get(VmDetailConstants.NETWORK);
-                if (logger.isTraceEnabled()) {
-                    logger.trace(String.format("nic, '%s', goes on net, '%s'", nic, networkUuid));
+                if (logger.isDebugEnabled()) {
+                    logger.debug(String.format("nic, '%s', goes on net, '%s'", nic, networkUuid));
                 }
                 if (StringUtils.isAnyEmpty(nic, networkUuid) || _entityMgr.findByUuid(Network.class, networkUuid) == null) {
                     throw new InvalidParameterValueException(String.format("Network ID: %s for NIC ID: %s is invalid", networkUuid, nic));
@@ -219,8 +219,8 @@ public Map<String, Network.IpAddresses> getNicIpAddressList() {
             for (Map<String, String> entry : (Collection<Map<String, String>>)nicIpAddressList.values()) {
                 String nic = entry.get(VmDetailConstants.NIC);
                 String ipAddress = StringUtils.defaultIfEmpty(entry.get(VmDetailConstants.IP4_ADDRESS), null);
-                if (logger.isTraceEnabled()) {
-                    logger.trace(String.format("nic, '%s', gets ip, '%s'", nic, ipAddress));
+                if (logger.isDebugEnabled()) {
+                    logger.debug(String.format("nic, '%s', gets ip, '%s'", nic, ipAddress));
                 }
                 if (StringUtils.isEmpty(nic)) {
                     throw new InvalidParameterValueException(String.format("NIC ID: '%s' is invalid for IP address mapping", nic));
diff --git a/core/src/main/java/com/cloud/agent/api/CheckVolumeAnswer.java b/core/src/main/java/com/cloud/agent/api/CheckVolumeAnswer.java
@@ -17,7 +17,6 @@
 
 package com.cloud.agent.api;
 
-@LogLevel(LogLevel.Log4jLevel.Trace)
 public class CheckVolumeAnswer extends Answer {
 
     private long size;
diff --git a/core/src/main/java/com/cloud/agent/api/CheckVolumeCommand.java b/core/src/main/java/com/cloud/agent/api/CheckVolumeCommand.java
@@ -21,7 +21,6 @@
 
 import com.cloud.agent.api.to.StorageFilerTO;
 
-@LogLevel(LogLevel.Log4jLevel.Trace)
 public class CheckVolumeCommand extends Command {
 
     String srcFile;
diff --git a/core/src/main/java/com/cloud/agent/api/CopyRemoteVolumeAnswer.java b/core/src/main/java/com/cloud/agent/api/CopyRemoteVolumeAnswer.java
@@ -17,7 +17,6 @@
 
 package com.cloud.agent.api;
 
-@LogLevel(LogLevel.Log4jLevel.Trace)
 public class CopyRemoteVolumeAnswer extends Answer {
 
     private String remoteIp;
diff --git a/core/src/main/java/com/cloud/agent/api/CopyRemoteVolumeCommand.java b/core/src/main/java/com/cloud/agent/api/CopyRemoteVolumeCommand.java
@@ -21,10 +21,10 @@
 
 import com.cloud.agent.api.to.StorageFilerTO;
 
-@LogLevel(LogLevel.Log4jLevel.Trace)
 public class CopyRemoteVolumeCommand extends Command {
     String remoteIp;
     String username;
+    @LogLevel(LogLevel.Log4jLevel.Off)
     String password;
     String srcFile;
     String tmpPath;
diff --git a/core/src/main/java/com/cloud/agent/api/GetRemoteVmsAnswer.java b/core/src/main/java/com/cloud/agent/api/GetRemoteVmsAnswer.java
@@ -22,10 +22,10 @@
 import java.util.HashMap;
 import java.util.List;
 
-@LogLevel(LogLevel.Log4jLevel.Trace)
 public class GetRemoteVmsAnswer extends Answer {
 
     private String remoteIp;
+    @LogLevel(LogLevel.Log4jLevel.Trace)
     private HashMap<String, UnmanagedInstanceTO> unmanagedInstances;
 
     List<String> vmNames;
diff --git a/core/src/main/java/com/cloud/agent/api/GetRemoteVmsCommand.java b/core/src/main/java/com/cloud/agent/api/GetRemoteVmsCommand.java
@@ -19,11 +19,11 @@
 
 package com.cloud.agent.api;
 
-@LogLevel(LogLevel.Log4jLevel.Trace)
 public class GetRemoteVmsCommand extends Command {
 
     String remoteIp;
     String username;
+    @LogLevel(LogLevel.Log4jLevel.Off)
     String password;
 
     public GetRemoteVmsCommand(String remoteIp, String username, String password) {
diff --git a/core/src/main/java/com/cloud/agent/api/GetUnmanagedInstancesAnswer.java b/core/src/main/java/com/cloud/agent/api/GetUnmanagedInstancesAnswer.java
@@ -21,10 +21,10 @@
 
 import org.apache.cloudstack.vm.UnmanagedInstanceTO;
 
-@LogLevel(LogLevel.Log4jLevel.Trace)
 public class GetUnmanagedInstancesAnswer extends Answer {
 
     private String instanceName;
+    @LogLevel(LogLevel.Log4jLevel.Trace)
     private HashMap<String, UnmanagedInstanceTO> unmanagedInstances;
 
     GetUnmanagedInstancesAnswer() {
diff --git a/core/src/main/java/com/cloud/agent/api/GetUnmanagedInstancesCommand.java b/core/src/main/java/com/cloud/agent/api/GetUnmanagedInstancesCommand.java
@@ -28,10 +28,10 @@
  * All managed instances will be filtered while trying to find unmanaged instances.
  */
 
-@LogLevel(LogLevel.Log4jLevel.Trace)
 public class GetUnmanagedInstancesCommand extends Command {
 
     String instanceName;
+    @LogLevel(LogLevel.Log4jLevel.Trace)
     List<String> managedInstancesNames;
 
     public GetUnmanagedInstancesCommand() {
diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java
@@ -4061,29 +4061,29 @@ public Pair<String, String> getSourceHostPath(String diskPath) {
     }
 
     public List<String> getAllVmNames(final Connect conn) {
-        final ArrayList<String> la = new ArrayList<String>();
+        final ArrayList<String> domainNames = new ArrayList<String>();
         try {
             final String names[] = conn.listDefinedDomains();
             for (int i = 0; i < names.length; i++) {
-                la.add(names[i]);
+                domainNames.add(names[i]);
             }
         } catch (final LibvirtException e) {
-            LOGGER.warn("Failed to list Defined domains", e);
+            logger.warn("Failed to list defined domains", e);
         }
 
         int[] ids = null;
         try {
             ids = conn.listDomains();
         } catch (final LibvirtException e) {
-            LOGGER.warn("Failed to list domains", e);
-            return la;
+            logger.warn("Failed to list domains", e);
+            return domainNames;
         }
 
         Domain dm = null;
         for (int i = 0; i < ids.length; i++) {
             try {
                 dm = conn.domainLookupByID(ids[i]);
-                la.add(dm.getName());
+                domainNames.add(dm.getName());
             } catch (final LibvirtException e) {
                 LOGGER.warn("Unable to get vms", e);
             } finally {
@@ -4097,7 +4097,7 @@ public List<String> getAllVmNames(final Connect conn) {
             }
         }
 
-        return la;
+        return domainNames;
     }
 
     private HashMap<String, HostVmStateReportEntry> getHostVmStateReport() {
diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtGetRemoteVmsCommandWrapper.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtGetRemoteVmsCommandWrapper.java
@@ -47,38 +47,37 @@ public final class LibvirtGetRemoteVmsCommandWrapper extends CommandWrapper<GetR
 
     @Override
     public Answer execute(final GetRemoteVmsCommand command, final LibvirtComputingResource libvirtComputingResource) {
-        String result = null;
-        String hypervisorURI = "qemu+ssh://" + command.getRemoteIp() + "/system";
+        String remoteIp = command.getRemoteIp();
+        String hypervisorURI = "qemu+tcp://" + remoteIp + "/system";
         HashMap<String, UnmanagedInstanceTO> unmanagedInstances = new HashMap<>();
         try {
             Connect conn = LibvirtConnection.getConnection(hypervisorURI);
             final List<String> allVmNames = libvirtComputingResource.getAllVmNames(conn);
+            s_logger.info(String.format("Found %d VMs on the remote host %s", allVmNames.size(), remoteIp));
             for (String name : allVmNames) {
                 final Domain domain = libvirtComputingResource.getDomain(conn, name);
-
                 final DomainInfo.DomainState ps = domain.getInfo().state;
                 final VirtualMachine.PowerState state = libvirtComputingResource.convertToPowerState(ps);
-
-                logger.debug("VM " + domain.getName() + " - powerstate: " + ps + ", state: " + state.toString());
+                logger.debug(String.format("Remote VM %s - powerstate: %s, state: %s", domain.getName(), ps.toString(), state.toString()));
 
                 if (state == VirtualMachine.PowerState.PowerOff) {
                     try {
                         UnmanagedInstanceTO instance = getUnmanagedInstance(libvirtComputingResource, domain, conn);
                         unmanagedInstances.put(instance.getName(), instance);
                     } catch (Exception e) {
-                        logger.error("Couldn't fetch VM " + domain.getName() + " details, due to: " + e.getMessage(), e);
+                        logger.error("Couldn't fetch remote VM " + domain.getName() + " details, due to: " + e.getMessage(), e);
                     }
                 }
                 domain.free();
             }
-            logger.debug("Found " + unmanagedInstances.size() + " stopped VMs on host " + command.getRemoteIp());
+            logger.debug("Found " + unmanagedInstances.size() + " stopped VMs on remote host " + remoteIp);
             return new GetRemoteVmsAnswer(command, "", unmanagedInstances);
         } catch (final LibvirtException e) {
-            logger.error("Failed to list stopped VMs on remote host " + command.getRemoteIp() + ", due to: " + e.getMessage(), e);
+            logger.error("Failed to list stopped VMs on remote host " + remoteIp + ", due to: " + e.getMessage(), e);
             if (e.getMessage().toLowerCase().contains("connection refused")) {
-                return new Answer(command, false, "Unable to connect to remote host " + command.getRemoteIp() + ", please check the libvirtd tcp connectivity and retry");
+                return new Answer(command, false, "Unable to connect to remote host " + remoteIp + ", please check the libvirtd tcp connectivity and retry");
             }
-            return new Answer(command, false, "Unable to list stopped VMs on remote host " + command.getRemoteIp() + ", due to: " + e.getMessage());
+            return new Answer(command, false, "Unable to list stopped VMs on remote host " + remoteIp + ", due to: " + e.getMessage());
         }
     }
 
@@ -104,8 +103,8 @@ private UnmanagedInstanceTO getUnmanagedInstance(LibvirtComputingResource libvir
 
             return instance;
         } catch (Exception e) {
-            logger.debug("Unable to retrieve unmanaged instance info,  due to: " + e.getMessage(), e);
-            throw new CloudRuntimeException("Unable to retrieve unmanaged instance info, due to: " + e.getMessage());
+            logger.debug("Unable to retrieve remote unmanaged instance info,  due to: " + e.getMessage(), e);
+            throw new CloudRuntimeException("Unable to retrieve remote unmanaged instance info, due to: " + e.getMessage());
         }
     }
 
diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtGetUnmanagedInstancesCommandWrapper.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtGetUnmanagedInstancesCommandWrapper.java
@@ -46,7 +46,7 @@ public final class LibvirtGetUnmanagedInstancesCommandWrapper extends CommandWra
 
     @Override
     public GetUnmanagedInstancesAnswer execute(GetUnmanagedInstancesCommand command, LibvirtComputingResource libvirtComputingResource) {
-        logger.info("Fetching unmanaged instance on host");
+        logger.info("Fetching unmanaged instances on host");
 
         HashMap<String, UnmanagedInstanceTO> unmanagedInstances = new HashMap<>();
         try {
diff --git a/server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java b/server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java
@@ -802,6 +802,7 @@ private Pair<DiskProfile, StoragePool> importExternalDisk(UnmanagedInstanceTO.Di
         }
         int copyTimeoutInSecs = copyTimeout * 60;
         copyRemoteVolumeCommand.setWait(copyTimeoutInSecs);
+        LOGGER.error(String.format("Initiating copy remote volume %s from %s, timeout %d secs", path, remoteUrl, copyTimeoutInSecs));
         Answer answer = agentManager.easySend(dest.getHost().getId(), copyRemoteVolumeCommand);
         if (!(answer instanceof CopyRemoteVolumeAnswer)) {
             throw new CloudRuntimeException("Error while copying volume of remote instance: " + answer.getDetails());
@@ -2467,7 +2468,6 @@ private NetworkVO createDefaultNetworkForAccount(DataCenter zone, Account owner,
         return defaultNetwork;
     }
 
-    //generate unit test
     public ListResponse<UnmanagedInstanceResponse> listVmsForImport(ListVmsForImportCmd cmd) {
         final Account caller = CallContext.current().getCallingAccount();
         if (caller.getType() != Account.Type.ADMIN) {
@@ -2507,8 +2507,8 @@ public ListResponse<UnmanagedInstanceResponse> listVmsForImport(ListVmsForImport
     private HashMap<String, UnmanagedInstanceTO> getRemoteVmsOnKVMHost(long zoneId, String remoteHostUrl, String username, String password) {
         //ToDo: add option to list one Vm by name
         List<HostVO> hosts = resourceManager.listAllUpAndEnabledHostsInOneZoneByHypervisor(Hypervisor.HypervisorType.KVM, zoneId);
-        if(hosts.size() < 1) {
-            throw new CloudRuntimeException("No hosts available for VM import");
+        if (hosts.size() < 1) {
+            throw new CloudRuntimeException("No hosts available to list VMs on remote host " + remoteHostUrl);
         }
         HostVO host = hosts.get(0);
         GetRemoteVmsCommand getRemoteVmsCommand = new GetRemoteVmsCommand(remoteHostUrl, username, password);

Original file line number	Diff line number	Diff line change
`@@ -4061,29 +4061,29 @@ public Pair<String, String> getSourceHostPath(String diskPath) {`
`4061`	`4061`	`}`
`4062`	`4062`
`4063`	`4063`	`public List<String> getAllVmNames(final Connect conn) {`
`4064`		`- final ArrayList<String> la = new ArrayList<String>();`
	`4064`	`+ final ArrayList<String> domainNames = new ArrayList<String>();`
`4065`	`4065`	`try {`
`4066`	`4066`	`final String names[] = conn.listDefinedDomains();`
`4067`	`4067`	`for (int i = 0; i < names.length; i++) {`
`4068`		`- la.add(names[i]);`
	`4068`	`+ domainNames.add(names[i]);`
`4069`	`4069`	`}`
`4070`	`4070`	`} catch (final LibvirtException e) {`
`4071`		`- LOGGER.warn("Failed to list Defined domains", e);`
	`4071`	`+ logger.warn("Failed to list defined domains", e);`
`4072`	`4072`	`}`
`4073`	`4073`
`4074`	`4074`	`int[] ids = null;`
`4075`	`4075`	`try {`
`4076`	`4076`	`ids = conn.listDomains();`
`4077`	`4077`	`} catch (final LibvirtException e) {`
`4078`		`- LOGGER.warn("Failed to list domains", e);`
`4079`		`- return la;`
	`4078`	`+ logger.warn("Failed to list domains", e);`
	`4079`	`+ return domainNames;`
`4080`	`4080`	`}`
`4081`	`4081`
`4082`	`4082`	`Domain dm = null;`
`4083`	`4083`	`for (int i = 0; i < ids.length; i++) {`
`4084`	`4084`	`try {`
`4085`	`4085`	`dm = conn.domainLookupByID(ids[i]);`
`4086`		`- la.add(dm.getName());`
	`4086`	`+ domainNames.add(dm.getName());`
`4087`	`4087`	`} catch (final LibvirtException e) {`
`4088`	`4088`	`LOGGER.warn("Unable to get vms", e);`
`4089`	`4089`	`} finally {`
`@@ -4097,7 +4097,7 @@ public List<String> getAllVmNames(final Connect conn) {`
`4097`	`4097`	`}`
`4098`	`4098`	`}`
`4099`	`4099`
`4100`		`- return la;`
	`4100`	`+ return domainNames;`
`4101`	`4101`	`}`
`4102`	`4102`
`4103`	`4103`	`private HashMap<String, HostVmStateReportEntry> getHostVmStateReport() {`