> On Sept. 10, 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::findOrDeployVirtualRouterInGuestNetwork
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
New Logic:
+ if (dc.getNetworkType() == NetworkType.Basic) {
+ for (HostPodVO pod: _podDao.listByDataCenterId(dc.getId())) {
+
+ List<DomainRouterVO> virtualRouters =
_domainRouterDao.listByPodId(pod.getId());
+ 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
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6781/#review11288
-----------------------------------------------------------
On Aug. 27, 2012, 11:10 a.m., Rohit Yadav wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6781/
> -----------------------------------------------------------
>
> (Updated Aug. 27, 2012, 11:10 a.m.)
>
>
> Review request for cloudstack, Abhinandan Prateek, Alena Prokharchyk, and
> Chiradeep Vittal.
>
>
> 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
>
>
> This addresses bug 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
>
> Diff: https://reviews.apache.org/r/6781/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Rohit Yadav
>
>
