Applygin fix from commit ID aaeadc5c44e3fe16a1deea5348b085b08b5f4f4d Sheng Yang changed 2 classes, ut only one was related to the bug CLOUDSTACK-7605. I applied the changed on the routerslist, used during the deployment of the virtual routers.
Tested Advanced Zone against the simulator. 69 happy tests in place Project: http://git-wip-us.apache.org/repos/asf/cloudstack/repo Commit: http://git-wip-us.apache.org/repos/asf/cloudstack/commit/a8cb7c14 Tree: http://git-wip-us.apache.org/repos/asf/cloudstack/tree/a8cb7c14 Diff: http://git-wip-us.apache.org/repos/asf/cloudstack/diff/a8cb7c14 Branch: refs/heads/vpc-refactor Commit: a8cb7c1480499b1c901ff4f69ebf7ccb3587c541 Parents: f79e7cd Author: Wilder Rodrigues <wrodrig...@schubergphilis.com> Authored: Tue Sep 30 14:18:52 2014 +0200 Committer: Wilder Rodrigues <wrodrig...@schubergphilis.com> Committed: Fri Oct 3 06:21:29 2014 +0200 ---------------------------------------------------------------------- .../deployment/RouterDeploymentDefinition.java | 217 +++++++++---------- .../VpcRouterDeploymentDefinition.java | 45 ++-- .../RouterDeploymentDefinitionTest.java | 2 +- 3 files changed, 126 insertions(+), 138 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/cloudstack/blob/a8cb7c14/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 7728f61..89dde3e 100644 --- a/server/src/org/cloud/network/router/deployment/RouterDeploymentDefinition.java +++ b/server/src/org/cloud/network/router/deployment/RouterDeploymentDefinition.java @@ -100,8 +100,7 @@ public class RouterDeploymentDefinition { protected boolean isPublicNetwork; protected PublicIp sourceNatIp; - protected RouterDeploymentDefinition(final Network guestNetwork, final DeployDestination dest, - final Account owner, final Map<Param, Object> params, final boolean isRedundant) { + protected RouterDeploymentDefinition(final Network guestNetwork, final DeployDestination dest, final Account owner, final Map<Param, Object> params, final boolean isRedundant) { this.guestNetwork = guestNetwork; this.dest = dest; @@ -111,98 +110,103 @@ public class RouterDeploymentDefinition { } public Long getOfferingId() { - return this.offeringId; + return offeringId; } public Vpc getVpc() { return null; } + public Network getGuestNetwork() { return guestNetwork; } + public DeployDestination getDest() { return dest; } + public Account getOwner() { return owner; } + public Map<Param, Object> getParams() { return params; } + public boolean isRedundant() { return isRedundant; } + public DeploymentPlan getPlan() { return plan; } + public boolean isVpcRouter() { return false; } + public Pod getPod() { return dest.getPod(); } + public Long getPodId() { return dest.getPod() == null ? null : dest.getPod().getId(); } + public List<DomainRouterVO> getRouters() { return routers; } public VirtualRouterProvider getVirtualProvider() { - return this.vrProvider; + return vrProvider; } public boolean isBasic() { - return this.dest.getDataCenter().getNetworkType() == NetworkType.Basic; + return dest.getDataCenter().getNetworkType() == NetworkType.Basic; } public boolean isPublicNetwork() { - return this.isPublicNetwork; + return isPublicNetwork; } public PublicIp getSourceNatIP() { - return this.sourceNatIp; + return sourceNatIp; } protected void generateDeploymentPlan() { - final long dcId = this.dest.getDataCenter().getId(); + final long dcId = dest.getDataCenter().getId(); Long podId = null; - if (this.isBasic()) { - if (this.dest.getPod() == null) { + if (isBasic()) { + if (dest.getPod() == null) { throw new CloudRuntimeException("Pod id is expected in deployment destination"); } - podId = this.dest.getPod().getId(); + podId = dest.getPod().getId(); } - this.plan = new DataCenterDeployment(dcId, podId, null, null, null, null); + plan = new DataCenterDeployment(dcId, podId, null, null, null, null); } - public List<DomainRouterVO> deployVirtualRouter() - throws InsufficientCapacityException, - ConcurrentOperationException, ResourceUnavailableException { + public List<DomainRouterVO> deployVirtualRouter() throws InsufficientCapacityException, ConcurrentOperationException, ResourceUnavailableException { - this.findOrDeployVirtualRouter(); + findOrDeployVirtualRouter(); return nwHelper.startRouters(this); } @DB - protected void findOrDeployVirtualRouter() - throws ConcurrentOperationException, InsufficientCapacityException, ResourceUnavailableException { - + protected void findOrDeployVirtualRouter() throws ConcurrentOperationException, InsufficientCapacityException, ResourceUnavailableException { try { - this.lock(); - this.checkPreconditions(); + lock(); + checkPreconditions(); // dest has pod=null, for Basic Zone findOrDeployVRs for all Pods final List<DeployDestination> destinations = findDestinations(); for (final DeployDestination destination : destinations) { - this.dest = destination; - this.planDeploymentRouters(); - this.generateDeploymentPlan(); - this.executeDeployment(); + dest = destination; + generateDeploymentPlan(); + executeDeployment(); } } finally { - this.unlock(); + unlock(); } } @@ -211,30 +215,25 @@ public class RouterDeploymentDefinition { if (lock == null) { throw new ConcurrentOperationException("Unable to lock network " + guestNetwork.getId()); } - this.tableLockId = lock.getId(); + tableLockId = lock.getId(); } protected void unlock() { - if (this.tableLockId != null) { - networkDao.releaseFromLockTable(this.tableLockId); + if (tableLockId != null) { + networkDao.releaseFromLockTable(tableLockId); if (logger.isDebugEnabled()) { - logger.debug("Lock is released for network id " + this.tableLockId - + " as a part of router startup in " + dest); + logger.debug("Lock is released for network id " + tableLockId + " as a part of router startup in " + dest); } } } protected void checkPreconditions() throws ResourceUnavailableException { - if (guestNetwork.getState() != Network.State.Implemented && - guestNetwork.getState() != Network.State.Setup && - guestNetwork.getState() != Network.State.Implementing) { - throw new ResourceUnavailableException("Network is not yet fully implemented: " + guestNetwork, - Network.class, this.guestNetwork.getId()); + if (guestNetwork.getState() != Network.State.Implemented && guestNetwork.getState() != Network.State.Setup && guestNetwork.getState() != Network.State.Implementing) { + throw new ResourceUnavailableException("Network is not yet fully implemented: " + guestNetwork, Network.class, guestNetwork.getId()); } if (guestNetwork.getTrafficType() != TrafficType.Guest) { - throw new ResourceUnavailableException("Network is not type Guest as expected: " + guestNetwork, - Network.class, this.guestNetwork.getId()); + throw new ResourceUnavailableException("Network is not type Guest as expected: " + guestNetwork, Network.class, guestNetwork.getId()); } } @@ -242,8 +241,9 @@ public class RouterDeploymentDefinition { // dest has pod=null, for Basic Zone findOrDeployVRs for all Pods final List<DeployDestination> destinations = new ArrayList<DeployDestination>(); - // for basic zone, if 'dest' has pod set to null then this is network restart scenario otherwise it is a vm deployment scenario - if (this.isBasic() && dest.getPod() == null) { + // for basic zone, if 'dest' has pod set to null then this is network + // restart scenario otherwise it is a vm deployment scenario + if (isBasic() && dest.getPod() == null) { // Find all pods in the data center with running or starting user vms final long dcId = dest.getDataCenter().getId(); final List<HostPodVO> pods = listByDataCenterIdVMTypeAndStates(dcId, VirtualMachine.Type.User, VirtualMachine.State.Starting, VirtualMachine.State.Running); @@ -261,7 +261,7 @@ public class RouterDeploymentDefinition { // Add virtualRouters to the routers, this avoids the situation when // all routers are skipped and VirtualRouterElement throws exception - this.routers.addAll(virtualRouters); + routers.addAll(virtualRouters); // If List size is one, we already have a starting or running VR, skip deployment if (virtualRouters.size() == 1) { @@ -280,43 +280,42 @@ public class RouterDeploymentDefinition { protected int getNumberOfRoutersToDeploy() { // TODO Are we sure this makes sense? Somebody said 5 was too many? - if (this.routers.size() >= 5) { + if (routers.size() >= 5) { logger.error("Too many redundant routers!"); } - // If old network is redundant but new is single router, then routers.size() = 2 but routerCount = 1 + // If old network is redundant but new is single router, then + // routers.size() = 2 but routerCount = 1 int routersExpected = 1; - if (this.isRedundant) { + if (isRedundant) { routersExpected = 2; } - return routersExpected < this.routers.size() ? - 0 : routersExpected - this.routers.size(); + return routersExpected < routers.size() ? 0 : routersExpected - routers.size(); } protected void setupAccountOwner() { if (networkModel.isNetworkSystem(guestNetwork) || guestNetwork.getGuestType() == Network.GuestType.Shared) { - this.owner = accountMgr.getAccount(Account.ACCOUNT_ID_SYSTEM); + owner = accountMgr.getAccount(Account.ACCOUNT_ID_SYSTEM); } } /** - * It executes last pending tasks to prepare the deployment and checks the deployment - * can proceed. If it can't it return false + * 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() { - this.setupAccountOwner(); + setupAccountOwner(); // Check if public network has to be set on VR - this.isPublicNetwork = networkModel.isProviderSupportServiceInNetwork( - guestNetwork.getId(), Service.SourceNat, Provider.VirtualRouter); + isPublicNetwork = networkModel.isProviderSupportServiceInNetwork(guestNetwork.getId(), Service.SourceNat, Provider.VirtualRouter); boolean canProceed = true; - if (this.isRedundant && !this.isPublicNetwork) { + if (isRedundant && !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<>(); + routers = new ArrayList<DomainRouterVO>(); canProceed = false; } @@ -324,39 +323,39 @@ public class RouterDeploymentDefinition { } /** - * 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. + * 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. * * @throws ConcurrentOperationException * @throws InsufficientCapacityException * @throws ResourceUnavailableException */ - protected void executeDeployment() - throws ConcurrentOperationException, InsufficientCapacityException, ResourceUnavailableException { - - //Check current redundant routers, if possible(all routers are stopped), reset the priority - this.setupPriorityOfRedundantRouter(); - - if (this.getNumberOfRoutersToDeploy() > 0 && this.prepareDeployment()) { - this.findVirtualProvider(); - this.findOfferingId(); - this.findSourceNatIP(); - this.deployAllVirtualRouters(); + protected void executeDeployment() throws ConcurrentOperationException, InsufficientCapacityException, ResourceUnavailableException { + // Check current redundant routers, if possible(all routers are + // stopped), reset the priority + planDeploymentRouters(); + setupPriorityOfRedundantRouter(); + + if (getNumberOfRoutersToDeploy() > 0 && prepareDeployment()) { + findVirtualProvider(); + findOfferingId(); + findSourceNatIP(); + deployAllVirtualRouters(); } } protected void findSourceNatIP() throws InsufficientAddressCapacityException, ConcurrentOperationException { - this.sourceNatIp = null; - if (this.isPublicNetwork) { - this.sourceNatIp = this.ipAddrMgr.assignSourceNatIpAddressToGuestNetwork( - this.owner,this.guestNetwork); + sourceNatIp = null; + if (isPublicNetwork) { + sourceNatIp = ipAddrMgr.assignSourceNatIpAddressToGuestNetwork(owner, guestNetwork); } } protected void findOfferingId() { Long networkOfferingId = networkOfferingDao.findById(guestNetwork.getNetworkOfferingId()).getServiceOfferingId(); if (networkOfferingId != null) { - this.offeringId = networkOfferingId; + offeringId = networkOfferingId; } } @@ -364,42 +363,36 @@ public class RouterDeploymentDefinition { // Check if providers are supported in the physical networks final Type type = Type.VirtualRouter; final Long physicalNetworkId = networkModel.getPhysicalNetworkId(guestNetwork); - final PhysicalNetworkServiceProvider provider = - physicalProviderDao.findByServiceProvider(physicalNetworkId, type.toString()); + 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)); } - this.vrProvider = vrProviderDao.findByNspIdAndType(provider.getId(), type); - if (this.vrProvider == null) { - throw new CloudRuntimeException( - String.format("Cannot find virtual router provider %s as service provider %s", - type.toString(), provider.getId())); + vrProvider = vrProviderDao.findByNspIdAndType(provider.getId(), type); + if (vrProvider == null) { + throw new CloudRuntimeException(String.format("Cannot find virtual router provider %s as service provider %s", type.toString(), provider.getId())); } } - protected void deployAllVirtualRouters() - throws ConcurrentOperationException, InsufficientCapacityException, ResourceUnavailableException { - - int routersToDeploy = this.getNumberOfRoutersToDeploy(); - for(int i = 0; i < routersToDeploy; i++) { - // Don't start the router as we are holding the network lock that needs to be released at the end of router allocation - DomainRouterVO router = this.nwHelper.deployRouter(this, false); + protected void deployAllVirtualRouters() throws ConcurrentOperationException, InsufficientCapacityException, ResourceUnavailableException { + int routersToDeploy = getNumberOfRoutersToDeploy(); + for (int i = 0; i < routersToDeploy; i++) { + // Don't start the router as we are holding the network lock that + // needs to be released at the end of router allocation + DomainRouterVO router = nwHelper.deployRouter(this, false); if (router != null) { - this.routerDao.addRouterToGuestNetwork(router, this.guestNetwork); - this.routers.add(router); + routerDao.addRouterToGuestNetwork(router, guestNetwork); + //Fix according to changes by Sheng Yang in commit ID cb4513379996b262ae378daf00c6388c6b7313cf + routers.add(router); } } } /** - * Lists all pods given a Data Center Id, a {@link VirtualMachine.Type} and a list of - * {@link VirtualMachine.State} - * + * Lists all pods given a Data Center Id, a {@link VirtualMachine.Type} and + * a list of {@link VirtualMachine.State} * @param id * @param type * @param states @@ -419,28 +412,25 @@ public class RouterDeploymentDefinition { final SearchCriteria<HostPodVO> sc = podIdSearch.create(); sc.setParameters("dc", id); sc.setJoinParameters("vmInstanceSearch", "type", type); - sc.setJoinParameters("vmInstanceSearch", "states", (Object[])states); + sc.setJoinParameters("vmInstanceSearch", "states", (Object[]) states); return podDao.search(sc, null); } protected void planDeploymentRouters() { - if (this.isBasic()) { - this.routers = routerDao.listByNetworkAndPodAndRole(this.guestNetwork.getId(), - this.getPodId(), Role.VIRTUAL_ROUTER); + if (isBasic()) { + routers.addAll(routerDao.listByNetworkAndPodAndRole(guestNetwork.getId(), getPodId(), Role.VIRTUAL_ROUTER)); } else { - this.routers = routerDao.listByNetworkAndRole(this.guestNetwork.getId(), - Role.VIRTUAL_ROUTER); + routers.addAll(routerDao.listByNetworkAndRole(guestNetwork.getId(), Role.VIRTUAL_ROUTER)); } } /** - * Routers need reset if at least one of the routers is not redundant or stopped. - * - * @return + * Routers need reset if at least one of the routers is not redundant or + * stopped. */ protected boolean routersNeedReset() { boolean needReset = true; - for (final DomainRouterVO router : this.routers) { + for (final DomainRouterVO router : routers) { if (!router.getIsRedundantRouter() || router.getState() != VirtualMachine.State.Stopped) { needReset = false; break; @@ -451,18 +441,17 @@ public class RouterDeploymentDefinition { } /** - * Only for redundant deployment and if any routers needed reset, we shall reset all - * routers priorities + * Only for redundant deployment and if any routers needed reset, we shall + * reset all routers priorities */ protected void setupPriorityOfRedundantRouter() { - if (this.isRedundant && this.routersNeedReset()) { - for (final DomainRouterVO router : this.routers) { - // getUpdatedPriority() would update the value later - router.setPriority(0); - router.setIsPriorityBumpUp(false); - routerDao.update(router.getId(), router); - } + if (isRedundant && routersNeedReset()) { + for (final DomainRouterVO router : routers) { + // getUpdatedPriority() would update the value later + router.setPriority(0); + router.setIsPriorityBumpUp(false); + routerDao.update(router.getId(), router); } + } } - } \ No newline at end of file http://git-wip-us.apache.org/repos/asf/cloudstack/blob/a8cb7c14/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 b72c69d..f8fb26e 100644 --- a/server/src/org/cloud/network/router/deployment/VpcRouterDeploymentDefinition.java +++ b/server/src/org/cloud/network/router/deployment/VpcRouterDeploymentDefinition.java @@ -53,9 +53,7 @@ public class VpcRouterDeploymentDefinition extends RouterDeploymentDefinition { protected Vpc vpc; - - protected VpcRouterDeploymentDefinition(final Vpc vpc, final DeployDestination dest, final Account owner, - final Map<Param, Object> params, final boolean isRedundant) { + protected VpcRouterDeploymentDefinition(final Vpc vpc, final DeployDestination dest, final Account owner, final Map<Param, Object> params, final boolean isRedundant) { super(null, dest, owner, params, isRedundant); @@ -64,7 +62,7 @@ public class VpcRouterDeploymentDefinition extends RouterDeploymentDefinition { @Override public Vpc getVpc() { - return this.vpc; + return vpc; } @Override @@ -83,16 +81,15 @@ public class VpcRouterDeploymentDefinition extends RouterDeploymentDefinition { if (vpcLock == null) { throw new ConcurrentOperationException("Unable to lock vpc " + vpc.getId()); } - this.tableLockId = vpcLock.getId(); + tableLockId = vpcLock.getId(); } @Override protected void unlock() { - if (this.tableLockId != null) { - vpcDao.releaseFromLockTable(this.tableLockId); + if (tableLockId != null) { + vpcDao.releaseFromLockTable(tableLockId); if (logger.isDebugEnabled()) { - logger.debug("Lock is released for vpc id " + this.tableLockId - + " as a part of router startup in " + dest); + logger.debug("Lock is released for vpc id " + tableLockId + " as a part of router startup in " + dest); } } } @@ -105,14 +102,15 @@ public class VpcRouterDeploymentDefinition extends RouterDeploymentDefinition { @Override protected List<DeployDestination> findDestinations() { final List<DeployDestination> destinations = new ArrayList<>(); - destinations.add(this.dest); + destinations.add(dest); return destinations; } @Override protected int getNumberOfRoutersToDeploy() { - // TODO Should we make our changes here in order to enable Redundant Router for VPC? - return this.routers.isEmpty() ? 1 : 0; + // TODO Should we make our changes here in order to enable Redundant + // Router for VPC? + return routers.isEmpty() ? 1 : 0; } /** @@ -128,12 +126,13 @@ public class VpcRouterDeploymentDefinition extends RouterDeploymentDefinition { @Override protected void setupPriorityOfRedundantRouter() { // Nothing to do for now - // TODO Shouldn't we add this behavior once Redundant Router works for Vpc too + // TODO Shouldn't we add this behavior once Redundant Router works for + // Vpc too } @Override protected void findSourceNatIP() throws InsufficientAddressCapacityException, ConcurrentOperationException { - this.sourceNatIp = vpcMgr.assignSourceNatIpAddressToVpc(this.owner, vpc); + sourceNatIp = vpcMgr.assignSourceNatIpAddressToVpc(owner, vpc); } @Override @@ -145,8 +144,8 @@ public class VpcRouterDeploymentDefinition extends RouterDeploymentDefinition { if (provider == null) { throw new CloudRuntimeException("Cannot find service provider " + Type.VPCVirtualRouter.toString() + " in physical network " + pNtwk.getId()); } - this.vrProvider = vrProviderDao.findByNspIdAndType(provider.getId(), Type.VPCVirtualRouter); - if (this.vrProvider != null) { + vrProvider = vrProviderDao.findByNspIdAndType(provider.getId(), Type.VPCVirtualRouter); + if (vrProvider != null) { break; } } @@ -156,28 +155,28 @@ public class VpcRouterDeploymentDefinition extends RouterDeploymentDefinition { protected void findOfferingId() { Long vpcOfferingId = vpcOffDao.findById(vpc.getVpcOfferingId()).getServiceOfferingId(); if (vpcOfferingId != null) { - this.offeringId = vpcOfferingId; + offeringId = vpcOfferingId; } } @Override - protected void deployAllVirtualRouters() - throws ConcurrentOperationException, InsufficientCapacityException, ResourceUnavailableException { + protected void deployAllVirtualRouters() throws ConcurrentOperationException, InsufficientCapacityException, + ResourceUnavailableException { - DomainRouterVO router = this.nwHelper.deployRouter(this, true); + DomainRouterVO router = nwHelper.deployRouter(this, true); if (router != null) { - this.routers.add(router); + routers.add(router); } } @Override protected void planDeploymentRouters() { - this.routers = this.routerDao.listByVpcId(this.vpc.getId()); + routers = routerDao.listByVpcId(vpc.getId()); } @Override protected void generateDeploymentPlan() { - this.plan = new DataCenterDeployment(this.dest.getDataCenter().getId()); + plan = new DataCenterDeployment(dest.getDataCenter().getId()); } } http://git-wip-us.apache.org/repos/asf/cloudstack/blob/a8cb7c14/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 021e8d6..c12ffb8 100644 --- a/server/test/org/cloud/network/router/deployment/RouterDeploymentDefinitionTest.java +++ b/server/test/org/cloud/network/router/deployment/RouterDeploymentDefinitionTest.java @@ -525,9 +525,9 @@ public class RouterDeploymentDefinitionTest extends RouterDeploymentDefinitionTe verify(deploymentUT, times(1)).lock(); verify(deploymentUT, times(1)).checkPreconditions(); verify(deploymentUT, times(1)).findDestinations(); - verify(deploymentUT, times(2)).planDeploymentRouters(); verify(deploymentUT, times(2)).generateDeploymentPlan(); verify(deploymentUT, times(2)).executeDeployment(); + //verify(deploymentUT, times(2)).planDeploymentRouters(); verify(deploymentUT, times(1)).unlock(); }
