----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9156/#review15926 -----------------------------------------------------------
Overall....good work. The code volume is too high for me to understand all the changes at first glance. I would suggest creating a branch as well so that you do not loose the changes and make progressive changes api/src/com/cloud/vm/VirtualMachineProfile.java <https://reviews.apache.org/r/9156/#comment34217> You forgot to make changes for the upgrade api/src/com/cloud/vm/VirtualMachineProfile.java <https://reviews.apache.org/r/9156/#comment34218> You need to change the alert manager as well api/src/org/apache/cloudstack/api/ApiConstants.java <https://reviews.apache.org/r/9156/#comment34209> lets call it memory than RAM because thats the usage everywhere api/src/org/apache/cloudstack/api/command/admin/cluster/AddClusterCmd.java <https://reviews.apache.org/r/9156/#comment34210> Why String and not float ? api/src/org/apache/cloudstack/api/command/admin/cluster/AddClusterCmd.java <https://reviews.apache.org/r/9156/#comment34211> Can you put this validations with all the other ones ? api/src/org/apache/cloudstack/api/command/admin/cluster/AddClusterCmd.java <https://reviews.apache.org/r/9156/#comment34212> If its over commit then it should be >=1 ? plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java <https://reviews.apache.org/r/9156/#comment34215> Should it be changing the following values vm.setMemoryStaticMin(conn, memsize); vm.setMemoryDynamicMin(conn, memsize); vm.setMemoryDynamicMax(conn, memsize); vm.setMemoryStaticMax(conn, memsize); server/src/com/cloud/agent/manager/allocator/impl/FirstFitAllocator.java <https://reviews.apache.org/r/9156/#comment34213> Doing this for every cluster means a lot of DB queries...can we cache it ? or store it cluster table itself ? server/src/com/cloud/agent/manager/allocator/impl/FirstFitAllocator.java <https://reviews.apache.org/r/9156/#comment34216> check tabs check it across the board server/src/com/cloud/agent/manager/allocator/impl/FirstFitAllocator.java <https://reviews.apache.org/r/9156/#comment34214> please maintain spacing b/n commas server/src/com/cloud/capacity/CapacityManagerImpl.java <https://reviews.apache.org/r/9156/#comment34219> this makes the capacity check extremely expensive. Try keeping the ratio cached somewhr server/src/com/cloud/capacity/CapacityManagerImpl.java <https://reviews.apache.org/r/9156/#comment34220> add ram over commit ratio as well server/src/com/cloud/vm/VirtualMachineManagerImpl.java <https://reviews.apache.org/r/9156/#comment34221> expensive - Nitin Mehta On Jan. 30, 2013, 3:05 p.m., bharat kumar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/9156/ > ----------------------------------------------------------- > > (Updated Jan. 30, 2013, 3:05 p.m.) > > > Review request for cloudstack, Abhinandan Prateek, Prachi Damle, and Nitin > Mehta. > > > Description > ------- > > This patch enables cpu and ram overcommit on a per cluster basis(only for > xenserver.). > The dashboard will show actual values instead of the overcommit values. > > > This addresses bug CLOUDSTACK-711. > > > Diffs > ----- > > api/src/com/cloud/vm/VirtualMachineProfile.java 0fab443 > api/src/org/apache/cloudstack/api/ApiConstants.java 58a7831 > api/src/org/apache/cloudstack/api/command/admin/cluster/AddClusterCmd.java > c64883c > api/src/org/apache/cloudstack/api/response/ClusterResponse.java e436c99 > > plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java > 065d3be > > plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/XcpServerResource.java > 0ce91bc > > plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/XenServer56FP1Resource.java > 58b8a03 > server/src/com/cloud/agent/manager/allocator/impl/FirstFitAllocator.java > 2a5258c > server/src/com/cloud/api/ApiDBUtils.java 0b08b26 > server/src/com/cloud/api/ApiResponseHelper.java 1c8849a > server/src/com/cloud/capacity/CapacityManager.java fffb41f > server/src/com/cloud/capacity/CapacityManagerImpl.java c410109 > server/src/com/cloud/capacity/dao/CapacityDao.java 0c0723b > server/src/com/cloud/capacity/dao/CapacityDaoImpl.java a2df475 > server/src/com/cloud/deploy/BareMetalPlanner.java 7616a38 > server/src/com/cloud/deploy/FirstFitPlanner.java 06e46fb > server/src/com/cloud/hypervisor/HypervisorGuruBase.java 2efe6d3 > server/src/com/cloud/resource/ResourceManagerImpl.java f82424a > server/src/com/cloud/vm/VirtualMachineManagerImpl.java e0647bb > server/src/com/cloud/vm/VirtualMachineProfileImpl.java e83d6a0 > > Diff: https://reviews.apache.org/r/9156/diff/ > > > Testing > ------- > > Did manual testing. > > > Thanks, > > bharat kumar > >
