From 58b57ca5dbe514c94469a28d2ebaf0474550ecec Mon Sep 17 00:00:00 2001 From: Sateesh Chodapuneedi Date: Thu, 6 Jun 2013 15:33:10 +0530 Subject: [PATCH] CLOUDSTACK-2805 [VMware] addVmwareDc fails with NPE when non-existent DC name is passed to the API Made vcenter as required parameter to addVmwareDc API. Removed stale parameter url from addVmwareDc API. Improved Exception handling, logging remote exception details. --- .../apache/cloudstack/api/ApiConstants.java | 1 + .../vmware/dao/VmwareDatacenterDaoImpl.java | 4 +- .../vmware/manager/VmwareManagerImpl.java | 102 ++++++------------ .../command/admin/zone/AddVmwareDcCmd.java | 8 +- .../vmware/VmwareDatacenterApiUnitTest.java | 4 +- 5 files changed, 42 insertions(+), 77 deletions(-) diff --git a/api/src/org/apache/cloudstack/api/ApiConstants.java b/api/src/org/apache/cloudstack/api/ApiConstants.java index a0cd0f5a600..ab1402ccde9 100755 --- a/api/src/org/apache/cloudstack/api/ApiConstants.java +++ b/api/src/org/apache/cloudstack/api/ApiConstants.java @@ -405,6 +405,7 @@ public class ApiConstants { public static final String VSM_CONFIG_MODE = "vsmconfigmode"; public static final String VSM_CONFIG_STATE = "vsmconfigstate"; public static final String VSM_DEVICE_STATE = "vsmdevicestate"; + public static final String VCENTER = "vcenter"; public static final String ADD_VSM_FLAG = "addvsmflag"; public static final String END_POINT = "endpoint"; public static final String REGION_ID = "regionid"; diff --git a/plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/dao/VmwareDatacenterDaoImpl.java b/plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/dao/VmwareDatacenterDaoImpl.java index 8324e93409a..9f5796a073a 100644 --- a/plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/dao/VmwareDatacenterDaoImpl.java +++ b/plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/dao/VmwareDatacenterDaoImpl.java @@ -75,7 +75,7 @@ public class VmwareDatacenterDaoImpl extends GenericDaoBase getVmwareDatacenterByNameAndVcenter(String name, String vCenterHost) { - SearchCriteria sc = guidSearch.create(); + SearchCriteria sc = nameVcSearch.create(); sc.setParameters("name", name); sc.setParameters("vCenterHost", vCenterHost); return search(sc, null); @@ -83,7 +83,7 @@ public class VmwareDatacenterDaoImpl extends GenericDaoBase listVmwareDatacenterByName(String name) { - SearchCriteria sc = guidSearch.create(); + SearchCriteria sc = nameSearch.create(); sc.setParameters("name", name); return search(sc, null); } diff --git a/plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/manager/VmwareManagerImpl.java b/plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/manager/VmwareManagerImpl.java index 664651e1fff..a604392f7a9 100755 --- a/plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/manager/VmwareManagerImpl.java +++ b/plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/manager/VmwareManagerImpl.java @@ -21,7 +21,7 @@ import java.io.IOException; import java.net.URI; import java.net.URISyntaxException; import java.net.URL; -import java.net.URLDecoder; +import java.rmi.RemoteException; import java.util.ArrayList; import java.util.HashMap; import java.util.List; @@ -38,8 +38,6 @@ import javax.naming.ConfigurationException; import org.apache.cloudstack.api.command.admin.zone.AddVmwareDcCmd; import org.apache.cloudstack.api.command.admin.zone.RemoveVmwareDcCmd; -import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreManager; -import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreRole; import org.apache.log4j.Logger; import com.cloud.agent.AgentManager; @@ -50,8 +48,6 @@ import com.cloud.agent.api.Answer; import com.cloud.agent.api.Command; import com.cloud.agent.api.StartupCommand; import com.cloud.agent.api.StartupRoutingCommand; -import com.cloud.agent.api.storage.MigrateVolumeAnswer; -import com.cloud.agent.api.to.VolumeTO; import com.cloud.cluster.ClusterManager; import com.cloud.configuration.Config; import com.cloud.configuration.dao.ConfigurationDao; @@ -80,7 +76,6 @@ import com.cloud.hypervisor.vmware.dao.LegacyZoneDao; import com.cloud.hypervisor.vmware.dao.VmwareDatacenterDao; import com.cloud.hypervisor.vmware.dao.VmwareDatacenterZoneMapDao; import com.cloud.hypervisor.vmware.mo.CustomFieldConstants; -import com.cloud.hypervisor.vmware.mo.CustomFieldsManagerMO; import com.cloud.hypervisor.vmware.mo.DatacenterMO; import com.cloud.hypervisor.vmware.mo.DiskControllerType; import com.cloud.hypervisor.vmware.mo.HostFirewallSystemMO; @@ -89,7 +84,6 @@ import com.cloud.hypervisor.vmware.mo.HypervisorHostHelper; import com.cloud.hypervisor.vmware.mo.TaskMO; import com.cloud.hypervisor.vmware.mo.VirtualEthernetCardType; import com.cloud.hypervisor.vmware.mo.VmwareHostType; -import com.cloud.utils.ssh.SshHelper; import com.cloud.hypervisor.vmware.resource.VmwareContextFactory; import com.cloud.hypervisor.vmware.util.VmwareClient; import com.cloud.hypervisor.vmware.util.VmwareContext; @@ -103,15 +97,10 @@ import com.cloud.serializer.GsonHelper; import com.cloud.server.ConfigurationServer; import com.cloud.storage.JavaStorageLayer; import com.cloud.storage.StorageLayer; -import com.cloud.storage.StoragePool; -import com.cloud.storage.VolumeVO; -import com.cloud.storage.dao.VolumeDao; import com.cloud.storage.secondary.SecondaryStorageVmManager; import com.cloud.utils.FileUtil; import com.cloud.utils.NumbersUtil; import com.cloud.utils.Pair; -import com.cloud.utils.UriUtils; -import com.cloud.utils.component.Manager; import com.cloud.utils.component.ManagerBase; import com.cloud.utils.concurrency.NamedThreadFactory; import com.cloud.utils.db.DB; @@ -121,8 +110,6 @@ import com.cloud.utils.exception.CloudRuntimeException; import com.cloud.utils.script.Script; import com.cloud.utils.ssh.SshHelper; import com.cloud.vm.DomainRouterVO; -import com.cloud.vm.VMInstanceVO; -import com.cloud.vm.dao.VMInstanceDao; import com.google.gson.Gson; import com.vmware.vim25.AboutInfo; import com.vmware.vim25.HostConnectSpec; @@ -530,7 +517,7 @@ public class VmwareManagerImpl extends ManagerBase implements VmwareManager, Vmw _configServer.updateKeyPairs(); s_logger.info("Copy System VM patch ISO file to secondary storage. source ISO: " + srcIso.getAbsolutePath() + - ", destination: " + destIso.getAbsolutePath()); + ", destination: " + destIso.getAbsolutePath()); try { FileUtil.copyfile(srcIso, destIso); } catch(IOException e) { @@ -579,7 +566,7 @@ public class VmwareManagerImpl extends ManagerBase implements VmwareManager, Vmw assert(isoFile != null); if(!isoFile.exists()) { - s_logger.error("Unable to locate systemvm.iso in your setup at " + isoFile.toString()); + s_logger.error("Unable to locate systemvm.iso in your setup at " + isoFile.toString()); } return isoFile; } @@ -596,7 +583,7 @@ public class VmwareManagerImpl extends ManagerBase implements VmwareManager, Vmw } assert(keyFile != null); if(!keyFile.exists()) { - s_logger.error("Unable to locate id_rsa.cloud in your setup at " + keyFile.toString()); + s_logger.error("Unable to locate id_rsa.cloud in your setup at " + keyFile.toString()); } return keyFile; } @@ -914,9 +901,9 @@ public class VmwareManagerImpl extends ManagerBase implements VmwareManager, Vmw public VmwareDatacenterVO addVmwareDatacenter(AddVmwareDcCmd cmd) throws ResourceInUseException { VmwareDatacenterVO vmwareDc = null; Long zoneId = cmd.getZoneId(); - String url = cmd.getUrl(); String userName = cmd.getUsername(); String password = cmd.getPassword(); + String vCenterHost = cmd.getVcenter(); String vmwareDcName = cmd.getName(); // Zone validation @@ -928,25 +915,26 @@ public class VmwareManagerImpl extends ManagerBase implements VmwareManager, Vmw throw new CloudRuntimeException("Zone " + zoneId + " is already associated with a VMware datacenter."); } - // Validate url and get uri - URI uri = getUri(url); - - // Validate username and password and DC name + // Validate username, password, VMware DC name and vCenter if (userName == null) { - throw new InvalidParameterValueException("Invalid parameter username."); + throw new InvalidParameterValueException("Missing or invalid parameter username."); } if (password == null) { - throw new InvalidParameterValueException("Invalid parameter password."); + throw new InvalidParameterValueException("Missing or invalid parameter username."); } if (vmwareDcName == null) { - throw new InvalidParameterValueException("Invalid parameter name. Please provide valid VMware datacenter name."); + throw new InvalidParameterValueException("Missing or invalid parameter name. Please provide valid VMware datacenter name."); + } + + if (vCenterHost == null) { + throw new InvalidParameterValueException("Missing or invalid parameter name. " + + "Please provide valid VMware vCenter server's IP address or fully qualified domain name."); } // Check if DC is already part of zone // In that case vmware_data_center table should have the DC - String vCenterHost = uri.getHost(); vmwareDc = _vmwareDcDao.getVmwareDatacenterByGuid(vmwareDcName + "@" + vCenterHost); if (vmwareDc != null) { throw new ResourceInUseException("This DC is already part of other CloudStack zone(s). Cannot add this DC to more zones."); @@ -963,14 +951,13 @@ public class VmwareManagerImpl extends ManagerBase implements VmwareManager, Vmw context = VmwareContextFactory.create(vCenterHost, userName, password); // Check if DC exists on vCenter - try { - dcMo = new DatacenterMO(context, vmwareDcName); - } catch(Throwable t) { - String msg = "Unable to find DC " + vmwareDcName + " in vCenter " + vCenterHost; + dcMo = new DatacenterMO(context, vmwareDcName); + dcMor = dcMo.getMor(); + if (dcMor == null) { + String msg = "Unable to find VMware DC " + vmwareDcName + " in vCenter " + vCenterHost + ". "; s_logger.error(msg); - throw new DiscoveryException(msg); + throw new InvalidParameterValueException(msg); } - assert (dcMo != null); // Check if DC is already associated with another cloudstack deployment // Get custom field property cloud.zone over this DC @@ -1020,9 +1007,13 @@ public class VmwareManagerImpl extends ManagerBase implements VmwareManager, Vmw } dcMo.setCustomFieldValue(CustomFieldConstants.CLOUD_ZONE, "true"); - } catch (Exception e) { - String msg = "VMware DC discovery failed due to : " + VmwareHelper.getExceptionMessage(e); - s_logger.error(msg); + } catch (Throwable e) { + String msg = "Failed to add VMware DC to zone "; + if (e instanceof RemoteException) { + msg = "Encountered remote exception at vCenter. " + VmwareHelper.getExceptionMessage(e); + } else { + msg += "due to : " + e.getMessage(); + } throw new CloudRuntimeException(msg); } finally { if (context != null) @@ -1032,6 +1023,7 @@ public class VmwareManagerImpl extends ManagerBase implements VmwareManager, Vmw return vmwareDc; } + @Override public boolean removeVmwareDatacenter(RemoveVmwareDcCmd cmd) throws ResourceInUseException { Long zoneId = cmd.getZoneId(); @@ -1070,8 +1062,8 @@ public class VmwareManagerImpl extends ManagerBase implements VmwareManager, Vmw _vmwareDcZoneMapDao.remove(vmwareDcZoneMap.getId()); txn.commit(); } catch (Exception e) { - s_logger.info("Caught exception when trying to delete VMware datacenter record.." + e.getMessage()); - throw new CloudRuntimeException("Failed to delete VMware datacenter "); + s_logger.info("Caught exception when trying to delete VMware datacenter record." + e.getMessage()); + throw new CloudRuntimeException("Failed to delete VMware datacenter."); } // Construct context @@ -1083,7 +1075,7 @@ public class VmwareManagerImpl extends ManagerBase implements VmwareManager, Vmw try { dcMo = new DatacenterMO(context, vmwareDcName); } catch(Throwable t) { - String msg = "able to find DC " + vmwareDcName + " in vCenter " + vCenterHost; + String msg = "Unable to find DC " + vmwareDcName + " in vCenter " + vCenterHost; s_logger.error(msg); throw new DiscoveryException(msg); } @@ -1095,7 +1087,7 @@ public class VmwareManagerImpl extends ManagerBase implements VmwareManager, Vmw s_logger.info("Sucessfully reset custom field property cloud.zone over DC " + vmwareDcName); } catch (Exception e) { String msg = "Unable to reset custom field property cloud.zone over DC " + vmwareDcName - + " due to : " + VmwareHelper.getExceptionMessage(e); + + " due to : " + VmwareHelper.getExceptionMessage(e); s_logger.error(msg); throw new CloudRuntimeException(msg); } finally { @@ -1111,7 +1103,7 @@ public class VmwareManagerImpl extends ManagerBase implements VmwareManager, Vmw DataCenterVO zone = _dcDao.findById(zoneId); if (zone == null) { InvalidParameterValueException ex = new InvalidParameterValueException( - "Can't find zone by the id specified"); + "Can't find zone by the id specified."); throw ex; } @@ -1122,40 +1114,12 @@ public class VmwareManagerImpl extends ManagerBase implements VmwareManager, Vmw for (ClusterVO cluster : clusters) { if (cluster.getHypervisorType().equals(HypervisorType.VMware)) { throw new ResourceInUseException("Zone has one or more clusters." - + " Can't " + errStr + " which already has clusters."); + + " Can't " + errStr + " which already has clusters."); } } } } - private URI getUri(String url) throws InvalidParameterValueException { - if (url == null) { - throw new InvalidParameterValueException("Invalid url."); - } - - URI uri = null; - try { - uri = new URI(UriUtils.encodeURIComponent(url)); - if (uri.getScheme() == null) { - throw new InvalidParameterValueException( - "uri.scheme is null " + url - + ", add http:// as a prefix"); - } else if (uri.getScheme().equalsIgnoreCase("http")) { - if (uri.getHost() == null - || uri.getHost().equalsIgnoreCase("") - || uri.getPath() == null - || uri.getPath().equalsIgnoreCase("")) { - throw new InvalidParameterValueException( - "Your host and/or path is wrong. Make sure it's of the format http://hostname/path"); - } - } - } catch (URISyntaxException e) { - throw new InvalidParameterValueException(url - + " is not a valid uri"); - } - return uri; - } - @Override public boolean isLegacyZone(long dcId) { boolean isLegacyZone = false; diff --git a/plugins/hypervisors/vmware/src/org/apache/cloudstack/api/command/admin/zone/AddVmwareDcCmd.java b/plugins/hypervisors/vmware/src/org/apache/cloudstack/api/command/admin/zone/AddVmwareDcCmd.java index fde96c81570..317452b7b7f 100644 --- a/plugins/hypervisors/vmware/src/org/apache/cloudstack/api/command/admin/zone/AddVmwareDcCmd.java +++ b/plugins/hypervisors/vmware/src/org/apache/cloudstack/api/command/admin/zone/AddVmwareDcCmd.java @@ -48,8 +48,8 @@ public class AddVmwareDcCmd extends BaseCmd { @Parameter(name=ApiConstants.NAME, type=CommandType.STRING, required=true, description="Name of VMware datacenter to be added to specified zone.") private String name; - @Parameter(name=ApiConstants.URL, type=CommandType.STRING, required=false, description="The URL of vCenter.") - private String url; + @Parameter(name=ApiConstants.VCENTER, type=CommandType.STRING, required=true, description="The name/ip of vCenter. Make sure it is IP address or full qualified domain name for host running vCenter server.") + private String vCenter; @Parameter(name=ApiConstants.USERNAME, type=CommandType.STRING, required=false, description="The Username required to connect to resource.") private String username; @@ -64,8 +64,8 @@ public class AddVmwareDcCmd extends BaseCmd { return name; } - public String getUrl() { - return url; + public String getVcenter() { + return vCenter; } public String getUsername() { diff --git a/plugins/hypervisors/vmware/test/com/cloud/hypervisor/vmware/VmwareDatacenterApiUnitTest.java b/plugins/hypervisors/vmware/test/com/cloud/hypervisor/vmware/VmwareDatacenterApiUnitTest.java index 688b93cfccc..de08c93c78a 100644 --- a/plugins/hypervisors/vmware/test/com/cloud/hypervisor/vmware/VmwareDatacenterApiUnitTest.java +++ b/plugins/hypervisors/vmware/test/com/cloud/hypervisor/vmware/VmwareDatacenterApiUnitTest.java @@ -214,7 +214,7 @@ public class VmwareDatacenterApiUnitTest { Mockito.when(_vmwareDcZoneMapDao.findByZoneId(1L)).thenReturn(null); Mockito.when(_vmwareDcZoneMapDao.expunge(1L)).thenReturn(true); Mockito.when(addCmd.getZoneId()).thenReturn(1L); - Mockito.when(addCmd.getUrl()).thenReturn(url); + Mockito.when(addCmd.getVcenter()).thenReturn(vCenterHost); Mockito.when(addCmd.getUsername()).thenReturn(user); Mockito.when(addCmd.getPassword()).thenReturn(password); Mockito.when(addCmd.getName()).thenReturn(vmwareDcName); @@ -265,7 +265,7 @@ public class VmwareDatacenterApiUnitTest { //@Test(expected = InvalidParameterValueException.class) public void testAddVmwareDcWithNullUrl() throws ResourceInUseException, IllegalArgumentException, DiscoveryException, Exception { - Mockito.when(addCmd.getUrl()).thenReturn(null); + Mockito.when(addCmd.getVcenter()).thenReturn(null); _vmwareDatacenterService.addVmwareDatacenter(addCmd); }