incubator-cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alena Prokharchyk" <alena.prokharc...@citrix.com>
Subject Re: Review Request: CS-16104: Improve restart network behaviour for basic network
Date Mon, 10 Sep 2012 21:31:23 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6781/#review11288
-----------------------------------------------------------


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.

- Alena Prokharchyk


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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message