Import Remote KVM VM logging improvements (#9284)

sureshanaparti · web-flow · commit c17aa0d9ada2 · 2024-06-24T11:34:37.000+05:30
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
@@ -203,8 +203,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));
@@ -221,8 +221,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
@@ -3771,29 +3771,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) {
-            s_logger.warn("Failed to list Defined domains", e);
+            s_logger.warn("Failed to list defined domains", e);
         }
 
         int[] ids = null;
         try {
             ids = conn.listDomains();
         } catch (final LibvirtException e) {
             s_logger.warn("Failed to list domains", e);
-            return la;
+            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) {
                 s_logger.warn("Unable to get vms", e);
             } finally {
@@ -3807,7 +3807,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
@@ -50,36 +50,37 @@ public final class LibvirtGetRemoteVmsCommandWrapper extends CommandWrapper<GetR
 
     @Override
     public Answer execute(final GetRemoteVmsCommand command, final LibvirtComputingResource libvirtComputingResource) {
-        String hypervisorURI = "qemu+tcp://" + 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);
-                s_logger.debug("VM " + domain.getName() + " - powerstate: " + ps + ", state: " + state.toString());
+                s_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) {
-                        s_logger.error("Couldn't fetch VM " + domain.getName() + " details, due to: " + e.getMessage(), e);
+                        s_logger.error("Couldn't fetch remote VM " + domain.getName() + " details, due to: " + e.getMessage(), e);
                     }
                 }
                 domain.free();
             }
-            s_logger.debug("Found " + unmanagedInstances.size() + " stopped VMs on host " + command.getRemoteIp());
+            s_logger.debug("Found " + unmanagedInstances.size() + " stopped VMs on remote host " + remoteIp);
             return new GetRemoteVmsAnswer(command, "", unmanagedInstances);
         } catch (final LibvirtException e) {
-            s_logger.error("Failed to list stopped VMs on remote host " + command.getRemoteIp() + ", due to: " + e.getMessage(), e);
+            s_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());
         }
     }
 
@@ -105,8 +106,8 @@ private UnmanagedInstanceTO getUnmanagedInstance(LibvirtComputingResource libvir
 
             return instance;
         } catch (Exception e) {
-            s_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());
+            s_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
@@ -48,7 +48,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
@@ -800,6 +800,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());
@@ -2434,7 +2435,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) {
@@ -2474,8 +2474,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
`@@ -3771,29 +3771,29 @@ public Pair<String, String> getSourceHostPath(String diskPath) {`
`3771`	`3771`	`}`
`3772`	`3772`
`3773`	`3773`	`public List<String> getAllVmNames(final Connect conn) {`
`3774`		`- final ArrayList<String> la = new ArrayList<String>();`
	`3774`	`+ final ArrayList<String> domainNames = new ArrayList<String>();`
`3775`	`3775`	`try {`
`3776`	`3776`	`final String names[] = conn.listDefinedDomains();`
`3777`	`3777`	`for (int i = 0; i < names.length; i++) {`
`3778`		`- la.add(names[i]);`
	`3778`	`+ domainNames.add(names[i]);`
`3779`	`3779`	`}`
`3780`	`3780`	`} catch (final LibvirtException e) {`
`3781`		`- s_logger.warn("Failed to list Defined domains", e);`
	`3781`	`+ s_logger.warn("Failed to list defined domains", e);`
`3782`	`3782`	`}`
`3783`	`3783`
`3784`	`3784`	`int[] ids = null;`
`3785`	`3785`	`try {`
`3786`	`3786`	`ids = conn.listDomains();`
`3787`	`3787`	`} catch (final LibvirtException e) {`
`3788`	`3788`	`s_logger.warn("Failed to list domains", e);`
`3789`		`- return la;`
	`3789`	`+ return domainNames;`
`3790`	`3790`	`}`
`3791`	`3791`
`3792`	`3792`	`Domain dm = null;`
`3793`	`3793`	`for (int i = 0; i < ids.length; i++) {`
`3794`	`3794`	`try {`
`3795`	`3795`	`dm = conn.domainLookupByID(ids[i]);`
`3796`		`- la.add(dm.getName());`
	`3796`	`+ domainNames.add(dm.getName());`
`3797`	`3797`	`} catch (final LibvirtException e) {`
`3798`	`3798`	`s_logger.warn("Unable to get vms", e);`
`3799`	`3799`	`} finally {`
`@@ -3807,7 +3807,7 @@ public List<String> getAllVmNames(final Connect conn) {`
`3807`	`3807`	`}`
`3808`	`3808`	`}`
`3809`	`3809`
`3810`		`- return la;`
	`3810`	`+ return domainNames;`
`3811`	`3811`	`}`
`3812`	`3812`
`3813`	`3813`	`private HashMap<String, HostVmStateReportEntry> getHostVmStateReport() {`