You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Rohit Yadav <ro...@citrix.com> on 2012/08/27 13:10:11 UTC
Review Request: CS-16104: Improve restart network behaviour for basic network
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6781/
-----------------------------------------------------------
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
Re: Review Request: CS-16104: Improve restart network behaviour for basic
network
Posted by Alena Prokharchyk <al...@citrix.com>.
> On Sept. 14, 2012, 11:43 p.m., Alena Prokharchyk wrote:
> > 1) long dcId = dest.getDataCenter().getId();
> > List<HostPodVO> pods = _podDao.listByDataCenterIdVMTypeAndState(dcId, VirtualMachine.Type.User, VirtualMachine.State.Running);
> > pods.addAll(_podDao.listByDataCenterIdVMTypeAndState(dcId, VirtualMachine.Type.User, VirtualMachine.State.Starting));
> >
> >
> > Instead of calling method 2 times, you can add State... to the istByDataCenterIdVMTypeAndState method and pass both states - Starting and Running - separated by coma.
> >
> > Please return list of PodIds in the response; we don't need the objects. The dao method has to be placed
> >
> >
> > 2) The above applies to listUserVms as well:
> >
> > List<DomainRouterVO> virtualRouters = _routerDao.listByPodIdAndState(podId, VirtualMachine.State.Running);
> > virtualRouters.addAll(_routerDao.listByPodIdAndState(podId, VirtualMachine.State.Starting));
> >
> >
> >
> > Call one dao method, and only get the count().
> >
> >
> >
> > 3) Move this method to vmInstanceDaoImpl - it doesn't make any sense to keep it in podDaoImpl as all we have to return - podIds, and the original search builder is done against vmInstanceDao. And vm_instance table has pod_id/data_center_id fields, there is no reason to make a joins with host_pod_ref.I
> >
> >
> > Also pass zoneId, vmState(s) - make it accept multiple state by defining State... in the method signature, vmType parameters to the call:
> >
> > @Override
> > public List<HostPodVO> listByDataCenterIdVMTypeAndState(long id, VirtualMachine.Type type, VirtualMachine.State state) {
> > final VMInstanceDaoImpl _vmDao = ComponentLocator.inject(VMInstanceDaoImpl.class);
> > SearchBuilder<VMInstanceVO> vmInstanceSearch = _vmDao.createSearchBuilder();
> > vmInstanceSearch.and("type", vmInstanceSearch.entity().getType(), SearchCriteria.Op.EQ);
> > vmInstanceSearch.and("state", vmInstanceSearch.entity().getState(), SearchCriteria.Op.EQ);
> >
> > SearchBuilder<HostPodVO> podIdSearch = createSearchBuilder();
> > podIdSearch.and("dc", podIdSearch.entity().getDataCenterId(), SearchCriteria.Op.EQ);
> > podIdSearch.select(null, SearchCriteria.Func.DISTINCT, podIdSearch.entity().getId());
> > podIdSearch.join("vmInstanceSearch", vmInstanceSearch, podIdSearch.entity().getId(),
> > vmInstanceSearch.entity().getPodIdToDeployIn(), JoinBuilder.JoinType.INNER);
> > podIdSearch.done();
> >
> > SearchCriteria<HostPodVO> sc = podIdSearch.create();
> > sc.setParameters("dc", id);
> > sc.setJoinParameters("vmInstanceSearch", "type", type);
> > sc.setJoinParameters("vmInstanceSearch", "state", state);
> > return listBy(sc);
> > }
> >
> >
> >
> >
> > 4) Remove this method from domainRouterDaoImpl:
> >
> > @Override
> > + public List<DomainRouterVO> listByPodId(Long podId) {
> > + SearchCriteria<DomainRouterVO> sc = AllFieldsSearch.create();
> > + sc.setParameters("podId", podId);
> > + return listBy(sc);
> > + }
> >
> >
> >
> > This method can cover both cases - search by pod and search by podId - if State is changed to State... in the method signature:
> >
> >
> > @Override
> > + public List<DomainRouterVO> listByPodIdAndState(Long podId, State state) {
> > + SearchCriteria<DomainRouterVO> sc = AllFieldsSearch.create();
> > + sc.setParameters("podId", podId);
> > + sc.setParameters("state", state);
> > + return listBy(sc);
> > + }
> >
> > State... can accept either none, or one, or multiple values for state.
>
> Rohit Yadav wrote:
> For 1, we need pod object as they are used to create new deploydestinations:
> destinations.add(new DeployDestination(dest.getDataCenter(), pod, null, null));
>
> For 2, we add the list of routers (which we found or deployed) to the routers list which is returned. In case of restartNetwork in basic zone, if all the routers are skipped, we return null, the caller method to findOrDeployVirtualRouter fails and users gets message that the command failed. That's why I used list and not a count method over the dao.
>
> Please clarify so I can send another patch for review. Thanks.
1) Ok, you can continue adding POD objects, but implement calling the DAO method just once, by passing both Starting and Running state.
2) Again, you can continue returning the objects, but retrieve them by using just 1 query (pass both states there)
- Alena
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6781/#review11575
-----------------------------------------------------------
On Sept. 12, 2012, 11:32 a.m., Rohit Yadav wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6781/
> -----------------------------------------------------------
>
> (Updated Sept. 12, 2012, 11:32 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
> CLOUDSTACK-70 on issues.apache.org
>
> Download original patch: http://patchbin.baagi.org/p?id=yfawv5
> git am <patch> # to apply retaining commit message
>
>
> This addresses bug CS-16104.
>
>
> Diffs
> -----
>
> server/src/com/cloud/dc/dao/HostPodDao.java f031ac9
> server/src/com/cloud/dc/dao/HostPodDaoImpl.java bec8c51
> server/src/com/cloud/network/NetworkManagerImpl.java 292a259
> server/src/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java 6618fdf
> 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 d0f65c4
>
> Diff: https://reviews.apache.org/r/6781/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Rohit Yadav
>
>
Re: Review Request: CS-16104: Improve restart network behaviour for basic
network
Posted by Rohit Yadav <ro...@citrix.com>.
> On Sept. 14, 2012, 11:43 p.m., Alena Prokharchyk wrote:
> > 1) long dcId = dest.getDataCenter().getId();
> > List<HostPodVO> pods = _podDao.listByDataCenterIdVMTypeAndState(dcId, VirtualMachine.Type.User, VirtualMachine.State.Running);
> > pods.addAll(_podDao.listByDataCenterIdVMTypeAndState(dcId, VirtualMachine.Type.User, VirtualMachine.State.Starting));
> >
> >
> > Instead of calling method 2 times, you can add State... to the istByDataCenterIdVMTypeAndState method and pass both states - Starting and Running - separated by coma.
> >
> > Please return list of PodIds in the response; we don't need the objects. The dao method has to be placed
> >
> >
> > 2) The above applies to listUserVms as well:
> >
> > List<DomainRouterVO> virtualRouters = _routerDao.listByPodIdAndState(podId, VirtualMachine.State.Running);
> > virtualRouters.addAll(_routerDao.listByPodIdAndState(podId, VirtualMachine.State.Starting));
> >
> >
> >
> > Call one dao method, and only get the count().
> >
> >
> >
> > 3) Move this method to vmInstanceDaoImpl - it doesn't make any sense to keep it in podDaoImpl as all we have to return - podIds, and the original search builder is done against vmInstanceDao. And vm_instance table has pod_id/data_center_id fields, there is no reason to make a joins with host_pod_ref.I
> >
> >
> > Also pass zoneId, vmState(s) - make it accept multiple state by defining State... in the method signature, vmType parameters to the call:
> >
> > @Override
> > public List<HostPodVO> listByDataCenterIdVMTypeAndState(long id, VirtualMachine.Type type, VirtualMachine.State state) {
> > final VMInstanceDaoImpl _vmDao = ComponentLocator.inject(VMInstanceDaoImpl.class);
> > SearchBuilder<VMInstanceVO> vmInstanceSearch = _vmDao.createSearchBuilder();
> > vmInstanceSearch.and("type", vmInstanceSearch.entity().getType(), SearchCriteria.Op.EQ);
> > vmInstanceSearch.and("state", vmInstanceSearch.entity().getState(), SearchCriteria.Op.EQ);
> >
> > SearchBuilder<HostPodVO> podIdSearch = createSearchBuilder();
> > podIdSearch.and("dc", podIdSearch.entity().getDataCenterId(), SearchCriteria.Op.EQ);
> > podIdSearch.select(null, SearchCriteria.Func.DISTINCT, podIdSearch.entity().getId());
> > podIdSearch.join("vmInstanceSearch", vmInstanceSearch, podIdSearch.entity().getId(),
> > vmInstanceSearch.entity().getPodIdToDeployIn(), JoinBuilder.JoinType.INNER);
> > podIdSearch.done();
> >
> > SearchCriteria<HostPodVO> sc = podIdSearch.create();
> > sc.setParameters("dc", id);
> > sc.setJoinParameters("vmInstanceSearch", "type", type);
> > sc.setJoinParameters("vmInstanceSearch", "state", state);
> > return listBy(sc);
> > }
> >
> >
> >
> >
> > 4) Remove this method from domainRouterDaoImpl:
> >
> > @Override
> > + public List<DomainRouterVO> listByPodId(Long podId) {
> > + SearchCriteria<DomainRouterVO> sc = AllFieldsSearch.create();
> > + sc.setParameters("podId", podId);
> > + return listBy(sc);
> > + }
> >
> >
> >
> > This method can cover both cases - search by pod and search by podId - if State is changed to State... in the method signature:
> >
> >
> > @Override
> > + public List<DomainRouterVO> listByPodIdAndState(Long podId, State state) {
> > + SearchCriteria<DomainRouterVO> sc = AllFieldsSearch.create();
> > + sc.setParameters("podId", podId);
> > + sc.setParameters("state", state);
> > + return listBy(sc);
> > + }
> >
> > State... can accept either none, or one, or multiple values for state.
For 1, we need pod object as they are used to create new deploydestinations:
destinations.add(new DeployDestination(dest.getDataCenter(), pod, null, null));
For 2, we add the list of routers (which we found or deployed) to the routers list which is returned. In case of restartNetwork in basic zone, if all the routers are skipped, we return null, the caller method to findOrDeployVirtualRouter fails and users gets message that the command failed. That's why I used list and not a count method over the dao.
Please clarify so I can send another patch for review. Thanks.
- Rohit
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6781/#review11575
-----------------------------------------------------------
On Sept. 12, 2012, 11:32 a.m., Rohit Yadav wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6781/
> -----------------------------------------------------------
>
> (Updated Sept. 12, 2012, 11:32 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
> CLOUDSTACK-70 on issues.apache.org
>
> Download original patch: http://patchbin.baagi.org/p?id=yfawv5
> git am <patch> # to apply retaining commit message
>
>
> This addresses bug CS-16104.
>
>
> Diffs
> -----
>
> server/src/com/cloud/dc/dao/HostPodDao.java f031ac9
> server/src/com/cloud/dc/dao/HostPodDaoImpl.java bec8c51
> server/src/com/cloud/network/NetworkManagerImpl.java 292a259
> server/src/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java 6618fdf
> 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 d0f65c4
>
> Diff: https://reviews.apache.org/r/6781/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Rohit Yadav
>
>
Re: Review Request: CS-16104: Improve restart network behaviour for basic
network
Posted by Alena Prokharchyk <al...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6781/#review11575
-----------------------------------------------------------
1) long dcId = dest.getDataCenter().getId();
List<HostPodVO> pods = _podDao.listByDataCenterIdVMTypeAndState(dcId, VirtualMachine.Type.User, VirtualMachine.State.Running);
pods.addAll(_podDao.listByDataCenterIdVMTypeAndState(dcId, VirtualMachine.Type.User, VirtualMachine.State.Starting));
Instead of calling method 2 times, you can add State... to the istByDataCenterIdVMTypeAndState method and pass both states - Starting and Running - separated by coma.
Please return list of PodIds in the response; we don't need the objects. The dao method has to be placed
2) The above applies to listUserVms as well:
List<DomainRouterVO> virtualRouters = _routerDao.listByPodIdAndState(podId, VirtualMachine.State.Running);
virtualRouters.addAll(_routerDao.listByPodIdAndState(podId, VirtualMachine.State.Starting));
Call one dao method, and only get the count().
3) Move this method to vmInstanceDaoImpl - it doesn't make any sense to keep it in podDaoImpl as all we have to return - podIds, and the original search builder is done against vmInstanceDao. And vm_instance table has pod_id/data_center_id fields, there is no reason to make a joins with host_pod_ref.I
Also pass zoneId, vmState(s) - make it accept multiple state by defining State... in the method signature, vmType parameters to the call:
@Override
public List<HostPodVO> listByDataCenterIdVMTypeAndState(long id, VirtualMachine.Type type, VirtualMachine.State state) {
final VMInstanceDaoImpl _vmDao = ComponentLocator.inject(VMInstanceDaoImpl.class);
SearchBuilder<VMInstanceVO> vmInstanceSearch = _vmDao.createSearchBuilder();
vmInstanceSearch.and("type", vmInstanceSearch.entity().getType(), SearchCriteria.Op.EQ);
vmInstanceSearch.and("state", vmInstanceSearch.entity().getState(), SearchCriteria.Op.EQ);
SearchBuilder<HostPodVO> podIdSearch = createSearchBuilder();
podIdSearch.and("dc", podIdSearch.entity().getDataCenterId(), SearchCriteria.Op.EQ);
podIdSearch.select(null, SearchCriteria.Func.DISTINCT, podIdSearch.entity().getId());
podIdSearch.join("vmInstanceSearch", vmInstanceSearch, podIdSearch.entity().getId(),
vmInstanceSearch.entity().getPodIdToDeployIn(), JoinBuilder.JoinType.INNER);
podIdSearch.done();
SearchCriteria<HostPodVO> sc = podIdSearch.create();
sc.setParameters("dc", id);
sc.setJoinParameters("vmInstanceSearch", "type", type);
sc.setJoinParameters("vmInstanceSearch", "state", state);
return listBy(sc);
}
4) Remove this method from domainRouterDaoImpl:
@Override
+ public List<DomainRouterVO> listByPodId(Long podId) {
+ SearchCriteria<DomainRouterVO> sc = AllFieldsSearch.create();
+ sc.setParameters("podId", podId);
+ return listBy(sc);
+ }
This method can cover both cases - search by pod and search by podId - if State is changed to State... in the method signature:
@Override
+ public List<DomainRouterVO> listByPodIdAndState(Long podId, State state) {
+ SearchCriteria<DomainRouterVO> sc = AllFieldsSearch.create();
+ sc.setParameters("podId", podId);
+ sc.setParameters("state", state);
+ return listBy(sc);
+ }
State... can accept either none, or one, or multiple values for state.
- Alena Prokharchyk
On Sept. 12, 2012, 11:32 a.m., Rohit Yadav wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6781/
> -----------------------------------------------------------
>
> (Updated Sept. 12, 2012, 11:32 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
> CLOUDSTACK-70 on issues.apache.org
>
> Download original patch: http://patchbin.baagi.org/p?id=yfawv5
> git am <patch> # to apply retaining commit message
>
>
> This addresses bug CS-16104.
>
>
> Diffs
> -----
>
> server/src/com/cloud/dc/dao/HostPodDao.java f031ac9
> server/src/com/cloud/dc/dao/HostPodDaoImpl.java bec8c51
> server/src/com/cloud/network/NetworkManagerImpl.java 292a259
> server/src/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java 6618fdf
> 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 d0f65c4
>
> Diff: https://reviews.apache.org/r/6781/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Rohit Yadav
>
>
Re: Review Request: CS-16104: Improve restart network behaviour for basic
network
Posted by Rohit Yadav <ro...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6781/
-----------------------------------------------------------
(Updated Oct. 11, 2012, 1:17 p.m.)
Review request for cloudstack, Abhinandan Prateek, Alena Prokharchyk, and Chiradeep Vittal.
Changes
-------
Update patch as per Alena's suggestions.
Pl. review the new patch.
Description
-------
Patch as per discussion on ML and http://bugs.cloudstack.org/browse/CS-16104
CLOUDSTACK-70 on issues.apache.org
Download original patch and git am <patch>:
http://bhaisaab.org/patches/cloudstack/0001-CLOUSTACK-70-Improve-restart-network-behaviour-for-b.patch
This addresses bug CS-16104.
Diffs (updated)
-----
server/src/com/cloud/dc/dao/HostPodDao.java f031ac9
server/src/com/cloud/dc/dao/HostPodDaoImpl.java bec8c51
server/src/com/cloud/network/NetworkManagerImpl.java 7b38c83
server/src/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java 4400a9b
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 65adf66
Diff: https://reviews.apache.org/r/6781/diff/
Testing
-------
Thanks,
Rohit Yadav
Re: Review Request: CS-16104: Improve restart network behaviour for basic
network
Posted by Rohit Yadav <ro...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6781/
-----------------------------------------------------------
(Updated Oct. 11, 2012, 1:15 p.m.)
Review request for cloudstack, Abhinandan Prateek, Alena Prokharchyk, and Chiradeep Vittal.
Description (updated)
-------
Patch as per discussion on ML and http://bugs.cloudstack.org/browse/CS-16104
CLOUDSTACK-70 on issues.apache.org
Download original patch and git am <patch>:
http://bhaisaab.org/patches/cloudstack/0001-CLOUSTACK-70-Improve-restart-network-behaviour-for-b.patch
This addresses bug CS-16104.
Diffs
-----
server/src/com/cloud/dc/dao/HostPodDao.java f031ac9
server/src/com/cloud/dc/dao/HostPodDaoImpl.java bec8c51
server/src/com/cloud/network/NetworkManagerImpl.java 292a259
server/src/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java 6618fdf
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 d0f65c4
Diff: https://reviews.apache.org/r/6781/diff/
Testing
-------
Thanks,
Rohit Yadav
Re: Review Request: CS-16104: Improve restart network behaviour for basic
network
Posted by Rohit Yadav <ro...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6781/
-----------------------------------------------------------
(Updated Sept. 12, 2012, 11:32 a.m.)
Review request for cloudstack, Abhinandan Prateek, Alena Prokharchyk, and Chiradeep Vittal.
Description (updated)
-------
Patch as per discussion on ML and http://bugs.cloudstack.org/browse/CS-16104
CLOUDSTACK-70 on issues.apache.org
Download original patch: http://patchbin.baagi.org/p?id=yfawv5
git am <patch> # to apply retaining commit message
This addresses bug CS-16104.
Diffs
-----
server/src/com/cloud/dc/dao/HostPodDao.java f031ac9
server/src/com/cloud/dc/dao/HostPodDaoImpl.java bec8c51
server/src/com/cloud/network/NetworkManagerImpl.java 292a259
server/src/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java 6618fdf
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 d0f65c4
Diff: https://reviews.apache.org/r/6781/diff/
Testing
-------
Thanks,
Rohit Yadav
Re: Review Request: CS-16104: Improve restart network behaviour for basic
network
Posted by Rohit Yadav <ro...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6781/
-----------------------------------------------------------
(Updated Sept. 12, 2012, 11:28 a.m.)
Review request for cloudstack, Abhinandan Prateek, Alena Prokharchyk, and Chiradeep Vittal.
Changes
-------
New patch. Please review. For ASF master.
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 (updated)
-----
server/src/com/cloud/dc/dao/HostPodDao.java f031ac9
server/src/com/cloud/dc/dao/HostPodDaoImpl.java bec8c51
server/src/com/cloud/network/NetworkManagerImpl.java 292a259
server/src/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java 6618fdf
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 d0f65c4
Diff: https://reviews.apache.org/r/6781/diff/
Testing
-------
Thanks,
Rohit Yadav
Re: Review Request: CS-16104: Improve restart network behaviour for basic
network
Posted by Rohit Yadav <ro...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6781/#review11323
-----------------------------------------------------------
I'm splitting the fix. Will send DAO related fix separately.
- Rohit Yadav
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
>
>
Re: Review Request: CS-16104: Improve restart network behaviour for basic
network
Posted by Rohit Yadav <ro...@citrix.com>.
> On Sept. 11, 2012, 12:47 a.m., Alena Prokharchyk wrote:
> > Please reopen the review ticket
Reopened as per your comment:
https://issues.apache.org/jira/browse/CLOUDSTACK-70
http://bugs.cloudstack.org/browse/CS-16104
- Rohit
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6781/#review11299
-----------------------------------------------------------
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
>
>
Re: Review Request: CS-16104: Improve restart network behaviour for basic
network
Posted by Alena Prokharchyk <al...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6781/#review11299
-----------------------------------------------------------
Please reopen the review ticket
- 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
>
>
Re: Review Request: CS-16104: Improve restart network behaviour for basic
network
Posted by Murali Reddy <mu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6781/#review11238
-----------------------------------------------------------
Ship it!
Ship It!
- Murali Reddy
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
>
>
Re: Review Request: CS-16104: Improve restart network behaviour for
basic network
Posted by Alena Prokharchyk <Al...@citrix.com>.
On 9/11/12 7:42 AM, "Rohit Yadav" <ro...@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/>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
Re: Review Request: CS-16104: Improve restart network behaviour for basic
network
Posted by Rohit Yadav <ro...@citrix.com>.
> 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
>
>
Re: Review Request: CS-16104: Improve restart network behaviour for basic
network
Posted by Alena Prokharchyk <al...@citrix.com>.
-----------------------------------------------------------
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
>
>