Merge pull request #1560 from nvazquez/gcbug

CLOUDSTACK-9386: DS template copies dont get deleted in VMware ESXi with multiple clusters and zone wide storageJIRA TICKET: https://issues.apache.org/jira/browse/CLOUDSTACK-9386

### Introduction
In some production environments with multiple clusters it was noticed that unused templates were consuming too much storage. It was discovered that template cleanup was not deleting marked templates on ESXi.

### Description of the problem
Suppose we have multiple clusters `(c1, c2,...,cN)` on a data center and template `T` from which we deploy vms on `c1.`
Suppose now that we expunge those vms, and there's no other vm instance from template `T,` so this was the actual workflow:

1. CloudStack marks template for cleanup after `storage.cleanup.interval` seconds, by setting `marked_for_gc = 1` on `template_spool_ref` table, for that template.

2. After another `storage.cleanup.interval` seconds a `DestroyCommand` will be sent, to delete template from primary storage

3. On `VmwareResource`, command is processed, and it first picks up a random cluster, say `ci != c1` to look for vm template (using volume's path) and destroy it. But, as template was on `c1` it cannot be found, so it won't be deleted. Entry on `template_spool_ref` is deleted but not the actual template on hypervisor side.

### Proposed solution
We propose a way to attack problem shown in point 3, by not picking up a random cluster to look for vm but using vSphere data center. This way we make sure vm template will be deleted in every case, and not depending on random cluster selection

* pr/1560:
  CLOUDSTACK-9386: Find vm on datacenter instead of randomly choosing a cluster

Signed-off-by: Koushik Das <koushik@apache.org>

plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java

@@ -5535,17 +5535,8 @@ public class VmwareResource implements StoragePoolResource, ServerResource, Vmwa
             VmwareHypervisorHost hyperHost = getHyperHost(context, null);
             VolumeTO vol = cmd.getVolume();
 
-            ManagedObjectReference morDs = HypervisorHostHelper.findDatastoreWithBackwardsCompatibility(hyperHost, vol.getPoolUuid());
-            if (morDs == null) {
-                String msg = "Unable to find datastore based on volume mount point " + vol.getMountPoint();
-                s_logger.error(msg);
-                throw new Exception(msg);
-            }
+            VirtualMachineMO vmMo = findVmOnDatacenter(context, hyperHost, vol);
 
-            ManagedObjectReference morCluster = hyperHost.getHyperHostCluster();
-            ClusterMO clusterMo = new ClusterMO(context, morCluster);
-
-            VirtualMachineMO vmMo = clusterMo.findVmOnHyperHost(vol.getPath());
             if (vmMo != null && vmMo.isTemplate()) {
                 if (s_logger.isInfoEnabled()) {
                     s_logger.info("Destroy template volume " + vol.getPath());
@@ -5570,6 +5561,26 @@ public class VmwareResource implements StoragePoolResource, ServerResource, Vmwa
         }
     }
 
+    /**
+     * Use data center to look for vm, instead of randomly picking up a cluster<br/>
+     * (in multiple cluster environments vm could not be found if wrong cluster was chosen)
+     * @param context vmware context
+     * @param hyperHost vmware hv host
+     * @param vol volume
+     * @return a virtualmachinemo if could be found on datacenter.
+     * @throws Exception if there is an error while finding vm
+     * @throws CloudRuntimeException if datacenter cannot be found
+     */
+    protected VirtualMachineMO findVmOnDatacenter(VmwareContext context, VmwareHypervisorHost hyperHost, VolumeTO vol) throws Exception {
+        DatacenterMO dcMo = new DatacenterMO(context, hyperHost.getHyperHostDatacenter());
+        if (dcMo.getMor() == null) {
+            String msg = "Unable to find VMware DC";
+            s_logger.error(msg);
+            throw new CloudRuntimeException(msg);
+        }
+        return dcMo.findVm(vol.getPath());
+    }
+
     private String getAbsoluteVmdkFile(VirtualDisk disk) {
         String vmdkAbsFile = null;
         VirtualDeviceBackingInfo backingInfo = disk.getBacking();

plugins/hypervisors/vmware/test/com/cloud/hypervisor/vmware/resource/VmwareResourceTest.java

@@ -27,6 +27,8 @@ import static org.mockito.Mockito.never;
 import static org.mockito.Matchers.eq;
 
 import java.util.ArrayList;
+import static org.powermock.api.mockito.PowerMockito.whenNew;
+
 import java.util.Arrays;
 import java.util.HashMap;
 import java.util.Map;
@@ -45,6 +47,7 @@ import org.powermock.api.mockito.PowerMockito;
 import org.powermock.core.classloader.annotations.PrepareForTest;
 import org.powermock.modules.junit4.PowerMockRunner;
 
+import com.vmware.vim25.ManagedObjectReference;
 import com.vmware.vim25.VirtualDevice;
 import com.vmware.vim25.VirtualDeviceConfigSpec;
 import com.vmware.vim25.VirtualMachineConfigSpec;
@@ -55,16 +58,22 @@ import com.cloud.agent.api.ScaleVmCommand;
 import com.cloud.agent.api.to.DataTO;
 import com.cloud.agent.api.to.NfsTO;
 import com.cloud.agent.api.to.VirtualMachineTO;
+import com.cloud.agent.api.to.VolumeTO;
+import com.cloud.hypervisor.vmware.mo.DatacenterMO;
 import com.cloud.hypervisor.vmware.mo.VirtualMachineMO;
 import com.cloud.hypervisor.vmware.mo.VmwareHypervisorHost;
 import com.cloud.hypervisor.vmware.util.VmwareContext;
 import com.cloud.storage.resource.VmwareStorageProcessor;
 import com.cloud.storage.resource.VmwareStorageSubsystemCommandHandler;
 
+import com.cloud.utils.exception.CloudRuntimeException;
+
 @RunWith(PowerMockRunner.class)
-@PrepareForTest(CopyCommand.class)
+@PrepareForTest({CopyCommand.class, DatacenterMO.class, VmwareResource.class})
 public class VmwareResourceTest {
 
+    private static final String VOLUME_PATH = "XXXXXXXXXXXX";
+
     @Mock
     VmwareStorageProcessor storageProcessor;
     @Mock
@@ -110,6 +119,12 @@ public class VmwareResourceTest {
     DataTO srcDataTO;
     @Mock
     NfsTO srcDataNfsTO;
+    @Mock
+    VolumeTO volume;
+    @Mock
+    ManagedObjectReference mor;
+    @Mock
+    DatacenterMO datacenter;
 
     CopyCommand storageCmd;
 
@@ -128,6 +143,7 @@ public class VmwareResourceTest {
         when(srcDataTO.getDataStore()).thenReturn(srcDataNfsTO);
         when(srcDataNfsTO.getNfsVersion()).thenReturn(NFS_VERSION);
         when(videoCard.getVideoRamSizeInKB()).thenReturn(VIDEO_CARD_MEMORY_SIZE);
+        when(volume.getPath()).thenReturn(VOLUME_PATH);
     }
 
     //Test successful scaling up the vm
@@ -258,4 +274,19 @@ public class VmwareResourceTest {
         verify(_resource, never()).examineStorageSubSystemCommandNfsVersion(storageCmd);
     }
 
-}
+    @Test(expected=CloudRuntimeException.class)
+    public void testFindVmOnDatacenterNullHyperHostReference() throws Exception {
+        when(hyperHost.getMor()).thenReturn(null);
+        _resource.findVmOnDatacenter(context, hyperHost, volume);
+    }
+
+    @Test
+    public void testFindVmOnDatacenter() throws Exception {
+        when(hyperHost.getHyperHostDatacenter()).thenReturn(mor);
+        when(datacenter.getMor()).thenReturn(mor);
+        when(datacenter.findVm(VOLUME_PATH)).thenReturn(vmMo);
+        whenNew(DatacenterMO.class).withArguments(context, mor).thenReturn(datacenter);
+        VirtualMachineMO result = _resource.findVmOnDatacenter(context, hyperHost, volume);
+        assertEquals(vmMo, result);
+    }
+}