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: Support for local data disk feature. (CS-14277)
Date Sun, 12 Aug 2012 20:46:06 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6431/#review10179
-----------------------------------------------------------


Its best to get another pair of eyes to review the change as well.
I have one more question which was asked in another thread but let me reask - "if the end
user chooses HA service offering and local storage disk offering would that deployment go
through ?" in fact should it go through ?


api/src/com/cloud/api/ApiConstants.java
<https://reviews.apache.org/r/6431/#comment21554>

    Can you please correct the LOCAL_STORAGE_EANBLED here though it doesnt affect outside



api/src/com/cloud/api/commands/CreateDiskOfferingCmd.java
<https://reviews.apache.org/r/6431/#comment21555>

    Can we use ServiceOffering.StorageType.shared.toString() here  instead of hard coding
?



api/src/com/cloud/api/commands/UpdateZoneCmd.java
<https://reviews.apache.org/r/6431/#comment21565>

    it should return null here



server/src/com/cloud/configuration/ConfigurationManagerImpl.java
<https://reviews.apache.org/r/6431/#comment21564>

    Will this be ever null since the getter function never returns null



server/src/com/cloud/configuration/ConfigurationManagerImpl.java
<https://reviews.apache.org/r/6431/#comment21558>

    Seems cryptic to me. I would have preferred throwing exception in the last clause and
marking the flag as false in the else if clause what say ?



server/src/com/cloud/storage/allocator/FirstFitStoragePoolAllocator.java
<https://reviews.apache.org/r/6431/#comment21556>

    If volume is local then it wouldnt enter this allocator correct ?



server/src/com/cloud/storage/allocator/FirstFitStoragePoolAllocator.java
<https://reviews.apache.org/r/6431/#comment21557>

    Again this allocator is not for local storage.



server/src/com/cloud/vm/UserVmManagerImpl.java
<https://reviews.apache.org/r/6431/#comment21560>

    Get this directly from the disk offering obtained above



server/src/com/cloud/vm/UserVmManagerImpl.java
<https://reviews.apache.org/r/6431/#comment21561>

    An english line comment would be appreciated :)



server/src/com/cloud/vm/UserVmManagerImpl.java
<https://reviews.apache.org/r/6431/#comment21562>

    Can you please throw a better exception like "cant attach the local volume - vol_uuid
 to the vm - vm_uuid since the migration of local volume is not allowed"



setup/db/db/schema-303to40.sql
<https://reviews.apache.org/r/6431/#comment21559>

    We should also remove the global config for local storage isn't it ?



ui/index.jsp
<https://reviews.apache.org/r/6431/#comment21563>

    Can you please get the UI reviewed by someone else as I dont have any expertise here


- Nitin Mehta


On Aug. 9, 2012, 5:45 p.m., Koushik Das wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6431/
> -----------------------------------------------------------
> 
> (Updated Aug. 9, 2012, 5:45 p.m.)
> 
> 
> Review request for cloudstack, Abhinandan Prateek and Nitin Mehta.
> 
> 
> Description
> -------
> 
> Support for local data disk. Currently enable/disable config is at zone level, in subsequent
checkins it can be made more granular.
>     Following changes are made:
>     - Create disk offering API now takes an extra parameter to denote storage type (local
or shared). This is similar to storage type in service offering.
>     - Create/delete of data volume on local storage
>     - Attach/detach for local data volumes. Re-attach is allowed as long as vm host and
data volume storage pool host is same.
>     - Migration of VM instance is not supported if it uses local root or data volumes.
>     - Migrate is not supported for local volumes.
>     - Zone level config to enable/disable local storage usage for service and disk offerings.
>     - Local storage gets discovered when a host is added/reconnected if zone level config
is enabled. When disabled existing local storages are not removed but any new local storage
is not added.
>     - Deploy VM command validates service and disk offerings based on local storage config.
>     - Upgrade uses the global config 'use.local.storage' to set the zone level config
for local storage.
> 
> 
> Diffs
> -----
> 
>   api/src/com/cloud/api/ApiConstants.java 08b9465 
>   api/src/com/cloud/api/commands/CreateDiskOfferingCmd.java b3d9962 
>   api/src/com/cloud/api/commands/CreateZoneCmd.java b36c721 
>   api/src/com/cloud/api/commands/DeployVMCmd.java 9e2bc24 
>   api/src/com/cloud/api/commands/UpdateZoneCmd.java c22bff7 
>   api/src/com/cloud/api/response/DiskOfferingResponse.java 9b4f891 
>   api/src/com/cloud/api/response/ZoneResponse.java f591d70 
>   api/src/com/cloud/dc/DataCenter.java 2d3064f 
>   client/WEB-INF/classes/resources/messages.properties 4e734e5 
>   server/src/com/cloud/api/ApiResponseHelper.java 399a816 
>   server/src/com/cloud/configuration/ConfigurationManager.java 3504204 
>   server/src/com/cloud/configuration/ConfigurationManagerImpl.java 4373bb3 
>   server/src/com/cloud/dc/DataCenterVO.java f5beda3 
>   server/src/com/cloud/storage/LocalStoragePoolListener.java 1be7a55 
>   server/src/com/cloud/storage/StorageManagerImpl.java c7dda00 
>   server/src/com/cloud/storage/allocator/FirstFitStoragePoolAllocator.java 006931d 
>   server/src/com/cloud/vm/UserVmManagerImpl.java c514a98 
>   setup/db/create-schema.sql b327106 
>   setup/db/db/schema-303to40.sql 39b5265 
>   ui/index.jsp 460215e 
>   ui/scripts/configuration.js 766dcf3 
>   ui/scripts/storage.js e75244f 
>   ui/scripts/system.js de0ce4d 
> 
> Diff: https://reviews.apache.org/r/6431/diff/
> 
> 
> Testing
> -------
> 
> Tested on XS.
> 
> 
> Thanks,
> 
> Koushik Das
> 
>


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