On 9/11/12 7:42 AM, "Rohit Yadav" <rohit.ya...@citrix.com> wrote:
> > > > > > > > > > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/6781/ > > > > > > On September 10th, 2012, 9:31 p.m., Alena Prokharchyk wrote: > > 1) Have to hange the logic below; we have to reduce the number of >iteration. > >* start with getting the PODs having user vms. >* Only for those pods check if there are any VRs running. > >Now you are doing it the other way around, and it increases the number of >iterations. Plus see all the comments that I've made, inline: > > >+ DataCenter dc = _dcDao.findById(network.getDataCenterId()); >+ //Pod based network restart for basic network, one VR per pod >+ if (dc.getNetworkType() == NetworkType.Basic) { >+ //Loop through all pods with running user vms and >restart network >+ for (HostPodVO pod: >_podDao.listByDataCenterId(dc.getId())) { >+ s_logger.debug("Trying to restart network for Pod: " >+ pod.getName() + ", id=" + pod.getId()); >+ //If cleanup is false, don't implement network on >running VRs >+ List<DomainRouterVO> virtualRouters = >_domainRouterDao.listByPodId(pod.getId()); >+ Boolean podHasSingleVR = (virtualRouters.size() == >1); > >+ if (!podHasSingleVR) { - COMMENT: wrong assumptions >to make. Pod might not have any VRs if there are no user vms exist. >+ s_logger.warn("Pod should have only one VR in >Basic Zone, please check!"); - COMMENT: put assert statement for >developers here >+ } >+ if (!cleanup && virtualRouters != null && >podHasSingleVR >+ && virtualRouters.get(0).getState() == >VirtualMachine.State.Running) { - COMMENT: check for Starting state as >well. If the router is starting, no need to start a new one. Also remove >(virtualRouters != null) as DAO never returns NULL, it always returns an >empty set. >+ s_logger.debug("Cleanup=false: Found a running >VR, skipping network implementation for the pod"); >+ continue; >+ } > > > >+ //Implement network only if there are running user >vms in 'pod' >+ List<VMInstanceVO> vms = >_vmDao.listByPodId(pod.getId()); COMMENT: instead of pulling all vms, and >then iterating through them one by one and checking state and type, call >Dao method to return you only user vms, and only in Running state. >Besides, never call list, call COUNT function inside the DAO as we just >need to know how many vms are running, and don't need to know the details > >+ for (VMInstanceVO vm: vms) { - COMMENT - incorrect >kind of logic. We shouldn't call implementNetworkElementsAndResources per >each pod as this call actually does netowrk rules implementation for the >ENTIRE network, and you manage to call it per each Pod. And the provider >for Basic network is not always the Virtual router, it can be Netscaler >as well. Placing VR implementation details in Network manager, and do the >determination based on the fact that VR is the only one provider, is >incorrect. See my comment #2 showing how this logic needs to be changed. >All the changes affecting Virtual Router behavior, should be placed in >the Virtual Router manager. > > >+ // implement the network elements and rules again >+ if (vm.getType() == Type.User && vm.getState() >== VirtualMachine.State.Running) { >+ DeployDestination dest = new >DeployDestination(dc, pod, null, null); >+ implementNetworkElementsAndResources(dest, >context, network, offering); >+ break; >+ } >+ } >+ } >+ } else { >+ // implement the network elements and rules again >+ DeployDestination dest = new DeployDestination(dc, null, >null, null); >+ implementNetworkElementsAndResources(dest, context, >network, offering); >+ } > setRestartRequired(network, true); > > >2) The logic shouldn't be a part of NetworkManagerImpl. It's wrong to >call implementNetworkElementsAndResources() per each POD as this call >don't just triggers VR restart, but also does network rules re-implement. >For each and every pod in the network, it's going to re-implement ALL the >rules in each iteration. Plus network manager should never know how > > >All the logic should be done in the VirtualRouterManager instead: > >* From the networkManagerImpl, don't distinguish what Zone the restart is >called for. Use the same logic as we used to use before: > > try { > implementNetworkElementsAndResources(dest, context, network, >offering); > setRestartRequired(network, true); > } catch (Exception ex) { > s_logger.warn("Failed to implement network " + network + " >elements and resources as a part of network restart due to ", ex); > return false; > } > >* In the Virtual Router manager, check the zone/dest info. If the zone is >Basic, and dest is missing pod information, restart-recreate the VR in >all the pods that have user vms running. > > > > >> List<VMInstanceVO> vms = >>_vmDao.listByPodId(pod.getId()); COMMENT: instead of pulling all vms, >>and then iterating through them one by one and checking state and type, >>call Dao method to return you only user vms, and only in Running state. >>Besides, never call list, call COUNT function inside the DAO as we just >>need to know how many vms are running, and don't need to know the details > >I think we should also consider the case when user vms are starting. > >>All the logic should be done in the VirtualRouterManager instead: > >No class by that name. Okay I understand the issue. >Which would be a better place to implement this logic; >in VirtualRouterElement::implement (Don't know the effect on VPC) >or in >VirtualNetworkApplianceManagerImpl::deployVirtualRouterInGuestNetwork or >in >VirtualNetworkApplianceManagerImpl::findOrDeployVirtualRouterInGuestNetwor >k VirtualNetworkApplianceManagerImpl, in findOrDeployVirtualRouterInGuestNetwork as this method returns list of routers. In this method you find out how many routers you have to deploy, and then for each deployment plan call deployRouter. > > >Or we change the behaviour how we find and deploy the routers, the >getDeploymentPlanAndRouters to return a list of pair of deployment plan, >list of routers; > >From Pair<DeploymentPlan, List<DomainRouterVO>> to: >List<Pair<DeploymentPlan, List<DomainRouterVO>>> >getDeploymentPlanAndRouters Yes, you can do this to figure out the deployment plan and routers. > >New Logic: > >+ if (dc.getNetworkType() == NetworkType.Basic) { >+ for (HostPodVO pod: >_podDao.listByDataCenterId(dc.getId())) { >+ >+ List<DomainRouterVO> virtualRouters = >_domainRouterDao.listByPodId(pod.getId()); Again, list by states (Running, Starting) here please. And change the logic to first find pods having user vms, and then iterate through these pods. Grab VR information only for the pods having user vms running. Change the logic below to support it. >+ assert (virtualRouters.size() <= 1) : "Pod can have >utmost one VR in Basic Zone, please check!"; >+ >+ if (!cleanup && (virtualRouters.size() > 0) >+ && (virtualRouters.get(0).getState() == >VirtualMachine.State.Running >+ || virtualRouters.get(0).getState() == >VirtualMachine.State.Starting)) { >+ s_logger.debug("Cleanup=false: Found a running >VR, skipping network implementation for the pod"); >+ continue; >+ } >+ >+ Long totalRunningOrStartingUserVMs = >_vmDao.countByPodIdVMTypeAndState(pod.getId(), Type.User, >VirtualMachine.State.Running) >+ + >_vmDao.countByPodIdVMTypeAndState(pod.getId(), Type.User, >VirtualMachine.State.Starting); >+ >+ if (totalRunningOrStartingUserVMs > 0L) { >+ s_logger.warn("Found total " + >totalRunningOrStartingUserVMs + " running or starting User VMs"); >+ // implement the network elements and rules again >+ // PUT HERE CODE TO find or deploy router >+ } >+ } >+ } >+ else {//HANDLE OTHER CASE} >- Rohit > >On August 27th, 2012, 11:10 a.m., Rohit Yadav wrote: > > Review request for cloudstack, Abhinandan Prateek, Alena Prokharchyk, >and Chiradeep Vittal. >By Rohit Yadav. >Updated Aug. 27, 2012, 11:10 a.m. >Description > > > > Patch as per discussion on ML and >http://bugs.cloudstack.org/browse/CS-16104 > >For git am: >Download original patch: http://patchbin.baagi.org/p?id=yfawv5 > > > Bugs: > >CS-16104 > > >Diffs > > >* server/src/com/cloud/network/NetworkManagerImpl.java (817075e) > > >* server/src/com/cloud/vm/dao/DomainRouterDao.java (01e3258) > > >* server/src/com/cloud/vm/dao/DomainRouterDaoImpl.java (175d3f2) > > >* server/src/com/cloud/vm/dao/VMInstanceDao.java (2cf3d75) > > >* server/src/com/cloud/vm/dao/VMInstanceDaoImpl.java (571b5d1) > > >* ui/scripts/network.js (ec32c49) > >View Diff <https://reviews.apache.org/r/6781/diff/> > > > > > > > > > > > > > > > > > > >
