> On June 12, 2013, 11:15 p.m., John Burwell wrote: > > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/lifecycle/SolidFirePrimaryDataStoreLifeCycle.java, > > line 131 > > <https://reviews.apache.org/r/11479/diff/1/?file=297885#file297885line131> > > > > Why not use the URI class to parse out the parameters? This code could > > be broken by a malformed URL/URI. I recommend parsing into a java.net.URI > > on line 60.
Yeah, it's called a URL, but it's really not a URL. The parameter where you pass in vendor-specific info to your plug-in from the API is called url and its contents are what I'm parsing here. Its contents are really just name/value pairs. > On June 12, 2013, 11:15 p.m., John Burwell wrote: > > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/lifecycle/SolidFirePrimaryDataStoreLifeCycle.java, > > line 184 > > <https://reviews.apache.org/r/11479/diff/1/?file=297885#file297885line184> > > > > Consider using Guava's Splitter. It will make this code much more > > concise ... Noted to investigate > On June 12, 2013, 11:15 p.m., John Burwell wrote: > > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/lifecycle/SolidFirePrimaryDataStoreLifeCycle.java, > > line 196 > > <https://reviews.apache.org/r/11479/diff/1/?file=297885#file297885line196> > > > > Consider using Guava's Splitter to grab out the value. Noted to investigate > On June 12, 2013, 11:15 p.m., John Burwell wrote: > > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/lifecycle/SolidFirePrimaryDataStoreLifeCycle.java, > > line 218 > > <https://reviews.apache.org/r/11479/diff/1/?file=297885#file297885line218> > > > > See previous comments about using java.net.URI. Really just key/value pairs. The name url comes from the API where the plug-in-specific info that's passed to the driver is called this. - Mike ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/11479/#review21347 ----------------------------------------------------------- On May 31, 2013, 9:18 p.m., Mike Tutkowski wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/11479/ > ----------------------------------------------------------- > > (Updated May 31, 2013, 9:18 p.m.) > > > Review request for cloudstack, edison su and John Burwell. > > > Description > ------- > > This patch implements a storage plug-in for SolidFire. The plug-in is based > on the new storage framework that went in with 4.2, as well. > > In addition, there are GUI (and related) changes to enable admins and end > users to specify a Min, Max, and Burst number of IOPS for a Disk Offering. > These fields (although optional) tend to follow the pattern previously > established for the Disk Size field. > > Also, the storage framework itself has been enhanced. For example, it now > supports creating and deleting storage repositories as is necessary for a > dynamic type of zone-wide primary storage (such as the SolidFire plug-in is). > > The desired behavior of the software is as such: > > * Allow an admin to invoke the CloudStack API to add Primary Storage based on > the SolidFire plug-in. > > * Allow an admin to create a Disk Offering that specifies a Min, Max, and > Burst number of IOPS or allows the admin to pass this ability on to the end > user. > > * Allow an end user to execute such a Disk Offering. As is the case for any > Disk Offering, this leads to the creation of a row in the volumes table. > > * Allow an end user to attach the resultant volume (noted in the DB) to a VM. > The storage framework invokes logic in the plug-in and the plug-in creates a > volume on its SAN with the correct size and IOPS values. The agent software > for XenServer detects that such an attach is being requested and creates a > Storage Repository (and single VDI within the SR) based on the storage IP > address of the SAN and the IQN of the volume. The VDI is then hooked up to > the VM. > > * Allow an end user to detach the volume. This leads to the destruction of > the SR, but the SAN volume remains intact and can be reattached later to any > VM running in a XenServer resource pool in the zone. > > * Allow an end user to delete the volume. This leads to the volume being > deleted on the SAN and being marked in the CloudStack DB as deleted. > > > Diffs > ----- > > api/src/com/cloud/offering/DiskOffering.java dd77c70 > api/src/com/cloud/storage/Storage.java c130fe2 > api/src/com/cloud/storage/Volume.java 4903594 > api/src/org/apache/cloudstack/api/ApiConstants.java 1e9435f > > api/src/org/apache/cloudstack/api/command/admin/offering/CreateDiskOfferingCmd.java > aa11599 > api/src/org/apache/cloudstack/api/command/user/volume/CreateVolumeCmd.java > 86a494b > api/src/org/apache/cloudstack/api/response/DiskOfferingResponse.java > 377e66e > api/src/org/apache/cloudstack/api/response/VolumeResponse.java 21d7d1a > client/WEB-INF/classes/resources/messages.properties 1638be1 > client/pom.xml 0c38ecb > client/tomcatconf/applicationContext.xml.in edf83a9 > core/src/com/cloud/agent/api/AttachVolumeCommand.java 302b8f8 > core/test/org/apache/cloudstack/api/agent/test/AttachVolumeAnswerTest.java > 251a6cb > core/test/org/apache/cloudstack/api/agent/test/AttachVolumeCommandTest.java > 1ec416a > > engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/PrimaryDataStoreParameters.java > b2b787c > engine/schema/src/com/cloud/storage/DiskOfferingVO.java 909d7fe > engine/schema/src/com/cloud/storage/VolumeVO.java 1699afd > > engine/storage/integration-test/test/org/apache/cloudstack/storage/allocator/StorageAllocatorTest.java > 9444fa5 > > engine/storage/src/org/apache/cloudstack/storage/allocator/ZoneWideStoragePoolAllocator.java > e976980 > > engine/storage/src/org/apache/cloudstack/storage/volume/datastore/PrimaryDataStoreHelper.java > 5f8daf4 > > engine/storage/volume/src/org/apache/cloudstack/storage/volume/VolumeObject.java > ea31be3 > > plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java > 4680fde > plugins/storage/volume/solidfire/pom.xml 9db0685 > > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/driver/SolidfirePrimaryDataStoreDriver.java > f31126c > > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/lifecycle/SolidFirePrimaryDataStoreLifeCycle.java > PRE-CREATION > > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/provider/SolidfirePrimaryDataStoreProvider.java > 650cac8 > > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java > PRE-CREATION > server/src/com/cloud/api/query/dao/DiskOfferingJoinDaoImpl.java 283181f > server/src/com/cloud/api/query/dao/VolumeJoinDaoImpl.java e27e2d9 > server/src/com/cloud/api/query/vo/DiskOfferingJoinVO.java 6d3cdcb > server/src/com/cloud/api/query/vo/VolumeJoinVO.java 6ef8c91 > server/src/com/cloud/configuration/ConfigurationManager.java 8db037b > server/src/com/cloud/configuration/ConfigurationManagerImpl.java 8baf1fb > server/src/com/cloud/server/ConfigurationServerImpl.java bc52e9a > server/src/com/cloud/storage/VolumeManager.java d198e5d > server/src/com/cloud/storage/VolumeManagerImpl.java 4b654eb > server/src/com/cloud/test/DatabaseConfig.java 70c8178 > server/test/com/cloud/vpc/MockConfigurationManagerImpl.java 21b3590 > setup/db/db/schema-410to420.sql cf4e98d > tools/marvin/marvin/cloudstackConnection.py b092ef0 > ui/dictionary.jsp ded9ea0 > ui/scripts/configuration.js 211d7b7 > ui/scripts/docs.js 7c1aaf8 > ui/scripts/storage.js e816334 > ui/scripts/system.js 8b9a81f > > Diff: https://reviews.apache.org/r/11479/diff/ > > > Testing > ------- > > Manual testing has been performed: > > A plug-in can be added to CloudStack via an API call. > > Create a Disk Offering where the admin specifies both Disk Size and Disk > IOPS. Disk IOPS are left blank. > Create a Disk Offering where the admin allows the end user to customize both > Disk Size and Disk IOPS. Disk IOPS are left blank. > Create a Disk Offering where the admin specifies Disk Size, but allows the > end user to customize Disk IOPS. Disk IOPS are left blank. > Create a Disk Offering where the admin specifies Disk IOPS, but allows the > end user to customize Disk Size. Disk IOPS are left blank. > > Create a Disk Offering where the admin specifies both Disk Size and Disk > IOPS. Disk IOPS are all filled in. > Create a Disk Offering where the admin allows the end user to customize both > Disk Size and Disk IOPS. Disk IOPS are all filled in. > Create a Disk Offering where the admin specifies Disk Size, but allows the > end user to customize Disk IOPS. Disk IOPS are all filled in. > Create a Disk Offering where the admin specifies Disk IOPS, but allows the > end user to customize Disk Size. Disk IOPS are all filled in. > > A newly created volume is attached to a VM for the first time and an SR and > VDI are created. > This volume is detached and the VDI and SR are deleted. > The volume is reattached and is properly introduced into the SR and VDI > ("introduced" in the sense that the data that was previously on the volume is > not destroyed upon reattach). > This volume is detached and the VDI and SR are deleted. > > > Thanks, > > Mike Tutkowski > >
