incubator-cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Nitin Mehta" <nitin.me...@citrix.com>
Subject Re: Review Request: Cloudstack-711 Cpu and ram overcommit at cluster level.
Date Thu, 31 Jan 2013 08:49:07 GMT

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


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