> On Aug. 12, 2012, 8:46 p.m., Nitin Mehta wrote: > > 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 ?
Currently if service offering is chosen with both HA and local storage then deployment is allowed. Same behavior holds for disk offering as well. > On Aug. 12, 2012, 8:46 p.m., Nitin Mehta wrote: > > server/src/com/cloud/storage/allocator/FirstFitStoragePoolAllocator.java, > > line 87 > > <https://reviews.apache.org/r/6431/diff/2/?file=136203#file136203line87> > > > > If volume is local then it wouldnt enter this allocator correct ? Thats correct. LocalStoragePoolAllocator is used for local storage. > On Aug. 12, 2012, 8:46 p.m., Nitin Mehta wrote: > > server/src/com/cloud/storage/allocator/FirstFitStoragePoolAllocator.java, > > line 102 > > <https://reviews.apache.org/r/6431/diff/2/?file=136203#file136203line102> > > > > Again this allocator is not for local storage. LocalStoragePoolAllocator extends FirstFit, and allocateToPool calls the 'super' implementation. So it really depends on the context. > On Aug. 12, 2012, 8:46 p.m., Nitin Mehta wrote: > > server/src/com/cloud/vm/UserVmManagerImpl.java, line 693 > > <https://reviews.apache.org/r/6431/diff/2/?file=136204#file136204line693> > > > > Get this directly from the disk offering obtained above The current implementation will hold, but anyways modified it to use the setting from disk offering. > On Aug. 12, 2012, 8:46 p.m., Nitin Mehta wrote: > > setup/db/db/schema-303to40.sql, line 84 > > <https://reviews.apache.org/r/6431/diff/2/?file=136206#file136206line84> > > > > We should also remove the global config for local storage isn't it ? Currently the value is not used so won't have any impact. Will do it as part of cleanup later. > On Aug. 12, 2012, 8:46 p.m., Nitin Mehta wrote: > > ui/index.jsp, line 2457 > > <https://reviews.apache.org/r/6431/diff/2/?file=136207#file136207line2457> > > > > Can you please get the UI reviewed by someone else as I dont have any > > expertise here Already verified by QA team, so closing > On Aug. 12, 2012, 8:46 p.m., Nitin Mehta wrote: > > server/src/com/cloud/configuration/ConfigurationManagerImpl.java, line 1909 > > <https://reviews.apache.org/r/6431/diff/2/?file=136199#file136199line1909> > > > > 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 ? Logic is as follows - if storage type is neither of 'local' or 'shared' throw validation exception. - Koushik ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6431/#review10179 ----------------------------------------------------------- On Aug. 20, 2012, 11:54 a.m., Koushik Das wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/6431/ > ----------------------------------------------------------- > > (Updated Aug. 20, 2012, 11:54 a.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 825e276 > 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 13517ab > server/src/com/cloud/api/ApiResponseHelper.java 0340a94 > server/src/com/cloud/configuration/ConfigurationManager.java df28251 > server/src/com/cloud/configuration/ConfigurationManagerImpl.java 3b07707 > server/src/com/cloud/dc/DataCenterVO.java a2b7d5f > server/src/com/cloud/storage/LocalStoragePoolListener.java 1be7a55 > server/src/com/cloud/storage/StorageManagerImpl.java 5a256dc > server/src/com/cloud/storage/allocator/FirstFitStoragePoolAllocator.java > 006931d > server/src/com/cloud/vm/UserVmManagerImpl.java a9474ba > setup/db/create-schema.sql fa933e3 > setup/db/db/schema-303to40.sql 9f24966 > ui/index.jsp 2d8b8d6 > ui/scripts/configuration.js 85a169b > ui/scripts/storage.js e75244f > ui/scripts/system.js 015f491 > > Diff: https://reviews.apache.org/r/6431/diff/ > > > Testing > ------- > > Tested on XS. > > > Thanks, > > Koushik Das > >
