Merge pull request #1251 from koushik-das/CLOUDSTACK-9180

CLOUDSTACK-9180: Optimize concurrent VM deployment operation on same network

Check if VR needs to be allocated for a given network and only acquire lock if required

Refer to the bug for details.

* pr/1251:
  CLOUDSTACK-9180: Optimize concurrent VM deployment operation on same network Check if VR needs to be allocated for a given network and only acquire lock if required

Signed-off-by: Will Stevens <williamstevens@gmail.com>

server/src/org/cloud/network/router/deployment/RouterDeploymentDefinition.java

@@ -193,28 +193,45 @@ public class RouterDeploymentDefinition {
     }
 
     public List<DomainRouterVO> deployVirtualRouter() throws InsufficientCapacityException, ConcurrentOperationException, ResourceUnavailableException {
-
         findOrDeployVirtualRouter();
-
         return nwHelper.startRouters(this);
     }
 
+    private boolean isRouterDeployed() throws ResourceUnavailableException {
+        boolean isDeployed = true;
+        checkPreconditions();
+        final List<DeployDestination> destinations = findDestinations();
+        for (final DeployDestination destination : destinations) {
+            dest = destination;
+            generateDeploymentPlan();
+            planDeploymentRouters();
+            if (getNumberOfRoutersToDeploy() > 0 && prepareDeployment()) {
+                isDeployed = false;
+                break;
+            }
+        }
+        return isDeployed;
+    }
+
     @DB
     protected void findOrDeployVirtualRouter() throws ConcurrentOperationException, InsufficientCapacityException, ResourceUnavailableException {
-        try {
-            lock();
-            checkPreconditions();
+        if (!isRouterDeployed()) {
+            try {
+                lock();
+                // reset router list that got populated during isRouterDeployed()
+                routers.clear();
+                checkPreconditions();
 
-            // dest has pod=null, for Basic Zone findOrDeployVRs for all Pods
-            final List<DeployDestination> destinations = findDestinations();
-
-            for (final DeployDestination destination : destinations) {
-                dest = destination;
-                generateDeploymentPlan();
-                executeDeployment();
+                // dest has pod=null, for Basic Zone findOrDeployVRs for all Pods
+                final List<DeployDestination> destinations = findDestinations();
+                for (final DeployDestination destination : destinations) {
+                    dest = destination;
+                    generateDeploymentPlan();
+                    executeDeployment();
+                }
+            } finally {
+                unlock();
             }
-        } finally {
-            unlock();
         }
     }
 
@@ -378,7 +395,7 @@ public class RouterDeploymentDefinition {
         final PhysicalNetworkServiceProvider provider = physicalProviderDao.findByServiceProvider(physicalNetworkId, type.toString());
 
         if (provider == null) {
-            throw new CloudRuntimeException(String.format("Cannot find service provider %s  in physical network %s", type.toString(), physicalNetworkId));
+            throw new CloudRuntimeException(String.format("Cannot find service provider %s in physical network %s", type.toString(), physicalNetworkId));
         }
 
         vrProvider = vrProviderDao.findByNspIdAndType(provider.getId(), type);

server/test/org/cloud/network/router/deployment/RouterDeploymentDefinitionTest.java

@@ -513,9 +513,9 @@ public class RouterDeploymentDefinitionTest extends RouterDeploymentDefinitionTe
         } finally {
             // Assert
             verify(deploymentUT, times(1)).lock();
-            verify(deploymentUT, times(1)).checkPreconditions();
-            verify(deploymentUT, times(1)).findDestinations();
-            verify(deploymentUT, times(2)).generateDeploymentPlan();
+            verify(deploymentUT, times(2)).checkPreconditions();
+            verify(deploymentUT, times(2)).findDestinations();
+            verify(deploymentUT, times(3)).generateDeploymentPlan();
             verify(deploymentUT, times(2)).executeDeployment();
             //verify(deploymentUT, times(2)).planDeploymentRouters();
             verify(deploymentUT, times(1)).unlock();
@@ -524,6 +524,36 @@ public class RouterDeploymentDefinitionTest extends RouterDeploymentDefinitionTe
         fail();
     }
 
+    @Test
+    public void testDeployVirtualRouterSkip() throws ConcurrentOperationException,
+    InsufficientCapacityException, ResourceUnavailableException {
+
+        // Prepare
+        final List<DeployDestination> mockDestinations = new ArrayList<>();
+        mockDestinations.add(mock(DeployDestination.class));
+        mockDestinations.add(mock(DeployDestination.class));
+
+        final RouterDeploymentDefinition deploymentUT = spy(deployment);
+        doNothing().when(deploymentUT).checkPreconditions();
+        doReturn(mockDestinations).when(deploymentUT).findDestinations();
+        doNothing().when(deploymentUT).planDeploymentRouters();
+        doNothing().when(deploymentUT).generateDeploymentPlan();
+        doReturn(0).when(deploymentUT).getNumberOfRoutersToDeploy();
+
+        // Execute
+        try {
+            deploymentUT.findOrDeployVirtualRouter();
+        } finally {
+            // Assert
+            verify(deploymentUT, times(0)).lock(); // lock shouldn't be acquired when VR already present
+            verify(deploymentUT, times(1)).checkPreconditions();
+            verify(deploymentUT, times(1)).findDestinations();
+            verify(deploymentUT, times(2)).generateDeploymentPlan();
+            verify(deploymentUT, times(0)).executeDeployment(); // no need to deploy VR as already present
+            verify(deploymentUT, times(0)).unlock(); // same as lock
+        }
+    }
+
     @Test
     public void testGetNumberOfRoutersToDeploy() {
         // Prepare