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