Re: Review Request: CS-16104: Improve restart network behaviour for basic network

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