Factor out prepareDeployment and update tests
Project: http://git-wip-us.apache.org/repos/asf/cloudstack/repo Commit: http://git-wip-us.apache.org/repos/asf/cloudstack/commit/240a539d Tree: http://git-wip-us.apache.org/repos/asf/cloudstack/tree/240a539d Diff: http://git-wip-us.apache.org/repos/asf/cloudstack/diff/240a539d Branch: refs/heads/master Commit: 240a539d438f1c857cb5f5bba5ec9181dfde4aea Parents: d2d9157 Author: Antonio Fornie <afor...@schubergphilis.com> Authored: Tue Jul 29 06:47:50 2014 -0500 Committer: wilderrodrigues <wrodrig...@schubergphilis.com> Committed: Tue Oct 14 15:01:16 2014 +0200 ---------------------------------------------------------------------- .../deployment/RouterDeploymentDefinition.java | 53 +++++---- .../VpcRouterDeploymentDefinition.java | 37 +++--- .../RouterDeploymentDefinitionTest.java | 118 +++++++++++++------ .../VpcRouterDeploymentDefinitionTest.java | 7 +- 4 files changed, 137 insertions(+), 78 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/cloudstack/blob/240a539d/server/src/org/cloud/network/router/deployment/RouterDeploymentDefinition.java ---------------------------------------------------------------------- diff --git a/server/src/org/cloud/network/router/deployment/RouterDeploymentDefinition.java b/server/src/org/cloud/network/router/deployment/RouterDeploymentDefinition.java index ea22faf..09192d3 100644 --- a/server/src/org/cloud/network/router/deployment/RouterDeploymentDefinition.java +++ b/server/src/org/cloud/network/router/deployment/RouterDeploymentDefinition.java @@ -302,6 +302,32 @@ public class RouterDeploymentDefinition { } /** + * It executes last pending tasks to prepare the deployment and checks the deployment + * can proceed. If it can't it return false + * + * @return if the deployment can proceed + */ + protected boolean prepareDeployment() { + boolean canProceed = true; + if (networkModel.isNetworkSystem(guestNetwork) || guestNetwork.getGuestType() == Network.GuestType.Shared) { + this.owner = accountMgr.getAccount(Account.ACCOUNT_ID_SYSTEM); + } + + // Check if public network has to be set on VR + this.isPublicNetwork = networkModel.isProviderSupportServiceInNetwork( + guestNetwork.getId(), Service.SourceNat, Provider.VirtualRouter); + + if (this.isRedundant && !this.isPublicNetwork) { + // TODO Shouldn't be this throw an exception instead of log error and empty list of routers + logger.error("Didn't support redundant virtual router without public network!"); + this.routers = new ArrayList<>(); + canProceed = false; + } + + return canProceed; + } + + /** * Executes preparation and deployment of the routers. After this method ends, {@link this#routers} * should have all of the deployed routers ready for start, and no more. * @@ -313,28 +339,13 @@ public class RouterDeploymentDefinition { throws ConcurrentOperationException, InsufficientCapacityException, ResourceUnavailableException { //Check current redundant routers, if possible(all routers are stopped), reset the priority - // TODO Why shouldn't we call this method also for VPC - setupPriorityOfRedundantRouter(); + this.setupPriorityOfRedundantRouter(); - if (this.getNumberOfRoutersToDeploy() > 0) { - if (networkModel.isNetworkSystem(guestNetwork) || guestNetwork.getGuestType() == Network.GuestType.Shared) { - this.owner = accountMgr.getAccount(Account.ACCOUNT_ID_SYSTEM); - } - - // Check if public network has to be set on VR - this.isPublicNetwork = networkModel.isProviderSupportServiceInNetwork( - guestNetwork.getId(), Service.SourceNat, Provider.VirtualRouter); - - if (this.isRedundant && !this.isPublicNetwork) { - // TODO Shouldn't be this throw an exception instead of log error and empty list of routers - logger.error("Didn't support redundant virtual router without public network!"); - this.routers = new ArrayList<>(); - } else { - this.findVirtualProvider(); - this.findOfferingId(); - this.findSourceNatIP(); - this.deployAllVirtualRouters(); - } + if (this.getNumberOfRoutersToDeploy() > 0 && this.prepareDeployment()) { + this.findVirtualProvider(); + this.findOfferingId(); + this.findSourceNatIP(); + this.deployAllVirtualRouters(); } } http://git-wip-us.apache.org/repos/asf/cloudstack/blob/240a539d/server/src/org/cloud/network/router/deployment/VpcRouterDeploymentDefinition.java ---------------------------------------------------------------------- diff --git a/server/src/org/cloud/network/router/deployment/VpcRouterDeploymentDefinition.java b/server/src/org/cloud/network/router/deployment/VpcRouterDeploymentDefinition.java index 3baa4e7..a1e1bc4 100644 --- a/server/src/org/cloud/network/router/deployment/VpcRouterDeploymentDefinition.java +++ b/server/src/org/cloud/network/router/deployment/VpcRouterDeploymentDefinition.java @@ -31,14 +31,11 @@ import com.cloud.deploy.DeployDestination; import com.cloud.exception.ConcurrentOperationException; import com.cloud.exception.InsufficientAddressCapacityException; import com.cloud.exception.InsufficientCapacityException; -import com.cloud.exception.InsufficientServerCapacityException; import com.cloud.exception.ResourceUnavailableException; -import com.cloud.exception.StorageUnavailableException; import com.cloud.network.Network; import com.cloud.network.PhysicalNetwork; import com.cloud.network.PhysicalNetworkServiceProvider; import com.cloud.network.VirtualRouterProvider.Type; -import com.cloud.network.addr.PublicIp; import com.cloud.network.dao.PhysicalNetworkDao; import com.cloud.network.router.VpcVirtualNetworkHelperImpl; import com.cloud.network.vpc.Vpc; @@ -123,21 +120,19 @@ public class VpcRouterDeploymentDefinition extends RouterDeploymentDefinition { } /** - * @see RouterDeploymentDefinition#executeDeployment() + * @see RouterDeploymentDefinition#prepareDeployment() + * + * @return if the deployment can proceed */ @Override - protected void executeDeployment() - throws ConcurrentOperationException, InsufficientCapacityException, ResourceUnavailableException { - //2) Return routers if exist, otherwise... - if (getNumberOfRoutersToDeploy() > 0) { - this.findVirtualProvider(); - this.findOfferingId(); - this.findSourceNatIP(); - - //3) Deploy Virtual Router - DomainRouterVO router = deployVpcRouter(sourceNatIp); - this.routers.add(router); - } + protected boolean prepareDeployment() { + return true; + } + + @Override + protected void setupPriorityOfRedundantRouter() { + // Nothing to do for now + // TODO Shouldn't we add this behavior once Redundant Router works for Vpc too } @Override @@ -171,16 +166,18 @@ public class VpcRouterDeploymentDefinition extends RouterDeploymentDefinition { } } - protected DomainRouterVO deployVpcRouter(final PublicIp sourceNatIp) - throws ConcurrentOperationException, InsufficientAddressCapacityException, - InsufficientServerCapacityException, InsufficientCapacityException, StorageUnavailableException, ResourceUnavailableException { + @Override + protected void deployAllVirtualRouters() + throws ConcurrentOperationException, InsufficientCapacityException, ResourceUnavailableException { LinkedHashMap<Network, List<? extends NicProfile>> networks = this.nwHelper.createVpcRouterNetworks(this); DomainRouterVO router = nwHelper.deployRouter(this, networks, true, vpcMgr.getSupportedVpcHypervisors()); - return router; + if (router != null) { + this.routers.add(router); + } } @Override http://git-wip-us.apache.org/repos/asf/cloudstack/blob/240a539d/server/test/org/cloud/network/router/deployment/RouterDeploymentDefinitionTest.java ---------------------------------------------------------------------- diff --git a/server/test/org/cloud/network/router/deployment/RouterDeploymentDefinitionTest.java b/server/test/org/cloud/network/router/deployment/RouterDeploymentDefinitionTest.java index 026996e..92d2546 100644 --- a/server/test/org/cloud/network/router/deployment/RouterDeploymentDefinitionTest.java +++ b/server/test/org/cloud/network/router/deployment/RouterDeploymentDefinitionTest.java @@ -42,7 +42,6 @@ import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.Mock; -import org.mockito.Mockito; import org.mockito.runners.MockitoJUnitRunner; import com.cloud.dc.DataCenter.NetworkType; @@ -391,7 +390,7 @@ public class RouterDeploymentDefinitionTest extends RouterDeploymentDefinitionTe this.mockPods.add(this.mockHostPodVO1); this.mockPods.add(this.mockHostPodVO2); // Deployment under test is a Mockito spy - RouterDeploymentDefinition deploymentUT = Mockito.spy(this.deployment); + RouterDeploymentDefinition deploymentUT = spy(this.deployment); doReturn(mockPods).when(deploymentUT).listByDataCenterIdVMTypeAndStates( DATA_CENTER_ID, VirtualMachine.Type.User, VirtualMachine.State.Starting, VirtualMachine.State.Running); @@ -797,7 +796,7 @@ public class RouterDeploymentDefinitionTest extends RouterDeploymentDefinitionTe this.deployment.routers = new ArrayList<>(); this.deployment.isRedundant = true; //this.deployment.routers.add(routerVO1); - RouterDeploymentDefinition deploymentUT = Mockito.spy(this.deployment); + RouterDeploymentDefinition deploymentUT = spy(this.deployment); doReturn(2).when(deploymentUT).getNumberOfRoutersToDeploy(); doReturn(null).when(this.mockNetworkGeneralHelper).createRouterNetworks(deploymentUT); @@ -818,18 +817,20 @@ public class RouterDeploymentDefinitionTest extends RouterDeploymentDefinitionTe routerVO2, this.deployment.routers.get(1)); } + + + + + + + + + + @Test - public void testExecuteDeploymentPublicNw() - throws ConcurrentOperationException, InsufficientCapacityException, ResourceUnavailableException { + public void testPrepareDeploymentPublicNw() { // Prepare this.deployment.isRedundant = true; - RouterDeploymentDefinition deploymentUT = Mockito.spy(this.deployment); - doNothing().when(deploymentUT).setupPriorityOfRedundantRouter(); - doReturn(2).when(deploymentUT).getNumberOfRoutersToDeploy(); - doNothing().when(deploymentUT).findVirtualProvider(); - doNothing().when(deploymentUT).findOfferingId(); - doNothing().when(deploymentUT).findSourceNatIP(); - doNothing().when(deploymentUT).deployAllVirtualRouters(); when(this.mockNetworkModel.isNetworkSystem(this.mockNw)).thenReturn(true); Account newAccountOwner = mock(Account.class); @@ -839,10 +840,72 @@ public class RouterDeploymentDefinitionTest extends RouterDeploymentDefinitionTe NW_ID, Service.SourceNat, Provider.VirtualRouter)).thenReturn(true); // Execute + boolean canProceedDeployment = this.deployment.prepareDeployment(); + + // Assert + assertEquals("New account owner not properly set", newAccountOwner, this.deployment.owner); + } + + @Test + public void testPrepareDeploymentNonRedundant() { + // Prepare + this.deployment.isRedundant = false; + + when(this.mockNetworkModel.isNetworkSystem(this.mockNw)).thenReturn(true); + Account newAccountOwner = mock(Account.class); + when(this.mockAccountMgr.getAccount(Account.ACCOUNT_ID_SYSTEM)).thenReturn(newAccountOwner); + + when(this.mockNetworkModel.isProviderSupportServiceInNetwork( + NW_ID, Service.SourceNat, Provider.VirtualRouter)).thenReturn(false); + + // Execute + boolean canProceedDeployment = this.deployment.prepareDeployment(); + + // Assert + assertEquals("New account owner not properly set", newAccountOwner, deployment.owner); + } + + @Test + public void testPrepareDeploymentRedundantNonPublicNw() { + // Prepare + this.deployment.isRedundant = false; + + when(this.mockNetworkModel.isNetworkSystem(this.mockNw)).thenReturn(true); + Account newAccountOwner = mock(Account.class); + when(this.mockAccountMgr.getAccount(Account.ACCOUNT_ID_SYSTEM)).thenReturn(newAccountOwner); + + when(this.mockNetworkModel.isProviderSupportServiceInNetwork( + NW_ID, Service.SourceNat, Provider.VirtualRouter)).thenReturn(false); + + // Execute + boolean canProceedDeployment = this.deployment.prepareDeployment(); + + // Assert + assertEquals("New account owner not properly set", newAccountOwner, this.deployment.owner); + assertEquals("Since is redundant deployment in non public nw there should be 0 routers to start", + 0, this.deployment.routers.size()); + verify(this.mockNetworkModel, times(1)).isNetworkSystem(this.mockNw); + } + + @Test + public void testExecuteDeploymentPublicNw() + throws ConcurrentOperationException, InsufficientCapacityException, ResourceUnavailableException { + // Prepare + this.deployment.isRedundant = true; + RouterDeploymentDefinition deploymentUT = spy(this.deployment); + doNothing().when(deploymentUT).setupPriorityOfRedundantRouter(); + doReturn(1).when(deploymentUT).getNumberOfRoutersToDeploy(); + doReturn(true).when(deploymentUT).prepareDeployment(); + doNothing().when(deploymentUT).findVirtualProvider(); + doNothing().when(deploymentUT).findOfferingId(); + doNothing().when(deploymentUT).findSourceNatIP(); + doNothing().when(deploymentUT).deployAllVirtualRouters(); + + // Execute deploymentUT.executeDeployment(); // Assert - assertEquals("New account owner not properly set", newAccountOwner, deploymentUT.owner); + verify(deploymentUT, times(1)).prepareDeployment(); verify(deploymentUT, times(1)).findVirtualProvider(); verify(deploymentUT, times(1)).findOfferingId(); verify(deploymentUT, times(1)).findSourceNatIP(); @@ -854,26 +917,20 @@ public class RouterDeploymentDefinitionTest extends RouterDeploymentDefinitionTe throws ConcurrentOperationException, InsufficientCapacityException, ResourceUnavailableException { // Prepare this.deployment.isRedundant = false; - RouterDeploymentDefinition deploymentUT = Mockito.spy(this.deployment); + RouterDeploymentDefinition deploymentUT = spy(this.deployment); doNothing().when(deploymentUT).setupPriorityOfRedundantRouter(); - doReturn(2).when(deploymentUT).getNumberOfRoutersToDeploy(); + doReturn(1).when(deploymentUT).getNumberOfRoutersToDeploy(); + doReturn(true).when(deploymentUT).prepareDeployment(); doNothing().when(deploymentUT).findVirtualProvider(); doNothing().when(deploymentUT).findOfferingId(); doNothing().when(deploymentUT).findSourceNatIP(); doNothing().when(deploymentUT).deployAllVirtualRouters(); - when(this.mockNetworkModel.isNetworkSystem(this.mockNw)).thenReturn(true); - Account newAccountOwner = mock(Account.class); - when(this.mockAccountMgr.getAccount(Account.ACCOUNT_ID_SYSTEM)).thenReturn(newAccountOwner); - - when(this.mockNetworkModel.isProviderSupportServiceInNetwork( - NW_ID, Service.SourceNat, Provider.VirtualRouter)).thenReturn(true); - // Execute deploymentUT.executeDeployment(); // Assert - assertEquals("New account owner not properly set", newAccountOwner, deploymentUT.owner); + verify(deploymentUT, times(0)).prepareDeployment(); verify(deploymentUT, times(1)).findVirtualProvider(); verify(deploymentUT, times(1)).findOfferingId(); verify(deploymentUT, times(1)).findSourceNatIP(); @@ -885,7 +942,7 @@ public class RouterDeploymentDefinitionTest extends RouterDeploymentDefinitionTe throws ConcurrentOperationException, InsufficientCapacityException, ResourceUnavailableException { // Prepare this.deployment.isRedundant = true; - RouterDeploymentDefinition deploymentUT = Mockito.spy(this.deployment); + RouterDeploymentDefinition deploymentUT = spy(this.deployment); doNothing().when(deploymentUT).setupPriorityOfRedundantRouter(); doReturn(2).when(deploymentUT).getNumberOfRoutersToDeploy(); doNothing().when(deploymentUT).findVirtualProvider(); @@ -893,21 +950,10 @@ public class RouterDeploymentDefinitionTest extends RouterDeploymentDefinitionTe doNothing().when(deploymentUT).findSourceNatIP(); doNothing().when(deploymentUT).deployAllVirtualRouters(); - when(this.mockNetworkModel.isNetworkSystem(this.mockNw)).thenReturn(true); - Account newAccountOwner = mock(Account.class); - when(this.mockAccountMgr.getAccount(Account.ACCOUNT_ID_SYSTEM)).thenReturn(newAccountOwner); - - when(this.mockNetworkModel.isProviderSupportServiceInNetwork( - NW_ID, Service.SourceNat, Provider.VirtualRouter)).thenReturn(false); - // Execute deploymentUT.executeDeployment(); // Assert - assertEquals("New account owner not properly set", newAccountOwner, deploymentUT.owner); - assertEquals("Since is redundant deployment in non public nw there should be 0 routers to start", - 0, this.deployment.routers.size()); - verify(this.mockNetworkModel, times(1)).isNetworkSystem(this.mockNw); verify(deploymentUT, times(0)).findVirtualProvider(); verify(deploymentUT, times(0)).findOfferingId(); verify(deploymentUT, times(0)).findSourceNatIP(); @@ -920,7 +966,7 @@ public class RouterDeploymentDefinitionTest extends RouterDeploymentDefinitionTe // Prepare this.deployment.isRedundant = true; this.deployment.isPublicNetwork = false; - RouterDeploymentDefinition deploymentUT = Mockito.spy(this.deployment); + RouterDeploymentDefinition deploymentUT = spy(this.deployment); doNothing().when(deploymentUT).setupPriorityOfRedundantRouter(); doReturn(0).when(deploymentUT).getNumberOfRoutersToDeploy(); doNothing().when(deploymentUT).findVirtualProvider(); http://git-wip-us.apache.org/repos/asf/cloudstack/blob/240a539d/server/test/org/cloud/network/router/deployment/VpcRouterDeploymentDefinitionTest.java ---------------------------------------------------------------------- diff --git a/server/test/org/cloud/network/router/deployment/VpcRouterDeploymentDefinitionTest.java b/server/test/org/cloud/network/router/deployment/VpcRouterDeploymentDefinitionTest.java index 829acf7..2260e2c 100644 --- a/server/test/org/cloud/network/router/deployment/VpcRouterDeploymentDefinitionTest.java +++ b/server/test/org/cloud/network/router/deployment/VpcRouterDeploymentDefinitionTest.java @@ -64,7 +64,12 @@ public class VpcRouterDeploymentDefinitionTest extends RouterDeploymentDefinitio @Test public void testConstructionFieldsAndFlags() { - assertTrue("", this.deployment instanceof VpcRouterDeploymentDefinition); + assertTrue("Not really a VpcRouterDeploymentDefinition what the builder created", + this.deployment instanceof VpcRouterDeploymentDefinition); + assertTrue("A VpcRouterDeploymentDefinition should declare it is", + this.deployment.isVpcRouter()); + assertEquals("A VpcRouterDeploymentDefinition should have a Vpc", + this.mockVpc, this.deployment.getVpc()); } @Test
