> On June 28, 2013, 3:42 p.m., John Burwell wrote: > > api/src/com/cloud/offering/DiskOffering.java, line 60 > > <https://reviews.apache.org/r/11479/diff/4/?file=310081#file310081line60> > > > > When would it be valid for the value of this property to be null? > > Seems like it should be boolean, not Boolean.
null is used if this feature is not in use. > On June 28, 2013, 3:42 p.m., John Burwell wrote: > > engine/storage/volume/src/org/apache/cloudstack/storage/volume/VolumeObject.java, > > line 79 > > <https://reviews.apache.org/r/11479/diff/4/?file=310122#file310122line79> > > > > Change of this method to conform with JavaBean property specifications > > -- get<PropertyName>. In this case, getISCIName would likely be most > > appropriate. As mentioned previously, these conventions are critical for > > proper reflection operation. Member variable is named "_iScsiName", so getter is named get_iScsiName() (which is kind of nice because it allows us to have more standard casing for "iSCSI", where the "i" is small and the "S" is capital) > On June 28, 2013, 3:42 p.m., John Burwell wrote: > > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java, > > line 291 > > <https://reviews.apache.org/r/11479/diff/4/?file=310133#file310133line291> > > > > If this class is unused, why not remove the cruft rather ignore the > > warning? It is required for JSON parsing (using the Gson library), but the code-checking logic believes one or more properties are not in use, so it would generate one or more unnecessary warnings. > On June 28, 2013, 3:42 p.m., John Burwell wrote: > > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java, > > line 339 > > <https://reviews.apache.org/r/11479/diff/4/?file=310133#file310133line339> > > > > If this class is unused, why not remove the cruft rather ignore the > > warning? It is required for JSON parsing (using the Gson library), but the code-checking logic believes one or more properties are not in use, so it would generate one or more unnecessary warnings. > On June 28, 2013, 3:42 p.m., John Burwell wrote: > > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java, > > line 361 > > <https://reviews.apache.org/r/11479/diff/4/?file=310133#file310133line361> > > > > If this class is unused, why not remove the cruft rather ignore the > > warning? It is required for JSON parsing (using the Gson library), but the code-checking logic believes one or more properties are not in use, so it would generate one or more unnecessary warnings. > On June 28, 2013, 3:42 p.m., John Burwell wrote: > > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java, > > line 384 > > <https://reviews.apache.org/r/11479/diff/4/?file=310133#file310133line384> > > > > If this class is unused, why not remove the cruft rather ignore the > > warning? It is required for JSON parsing (using the Gson library), but the code-checking logic believes one or more properties are not in use, so it would generate one or more unnecessary warnings. > On June 28, 2013, 3:42 p.m., John Burwell wrote: > > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java, > > line 406 > > <https://reviews.apache.org/r/11479/diff/4/?file=310133#file310133line406> > > > > If this class is unused, why not remove the cruft rather ignore the > > warning? It is required for JSON parsing (using the Gson library), but the code-checking logic believes one or more properties are not in use, so it would generate one or more unnecessary warnings. > On June 28, 2013, 3:42 p.m., John Burwell wrote: > > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java, > > line 428 > > <https://reviews.apache.org/r/11479/diff/4/?file=310133#file310133line428> > > > > If this class is unused, why not remove the cruft rather ignore the > > warning? It is required for JSON parsing (using the Gson library), but the code-checking logic believes one or more properties are not in use, so it would generate one or more unnecessary warnings. > On June 28, 2013, 3:42 p.m., John Burwell wrote: > > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java, > > line 450 > > <https://reviews.apache.org/r/11479/diff/4/?file=310133#file310133line450> > > > > If this class is unused, why not remove the cruft rather ignore the > > warning? It is required for JSON parsing (using the Gson library), but the code-checking logic believes one or more properties are not in use, so it would generate one or more unnecessary warnings. > On June 28, 2013, 3:42 p.m., John Burwell wrote: > > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java, > > line 472 > > <https://reviews.apache.org/r/11479/diff/4/?file=310133#file310133line472> > > > > If this class is unused, why not remove the cruft rather ignore the > > warning? It is required for JSON parsing (using the Gson library), but the code-checking logic believes one or more properties are not in use, so it would generate one or more unnecessary warnings. - Mike ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/11479/#review22508 ----------------------------------------------------------- On June 21, 2013, 4:35 p.m., Mike Tutkowski wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/11479/ > ----------------------------------------------------------- > > (Updated June 21, 2013, 4:35 p.m.) > > > Review request for cloudstack, edison su and John Burwell. > > > Repository: cloudstack-git > > > 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 ae4528c > api/src/com/cloud/storage/StoragePool.java 8b95383 > api/src/com/cloud/storage/Volume.java 4903594 > api/src/org/apache/cloudstack/api/ApiConstants.java 1704ca3 > > api/src/org/apache/cloudstack/api/command/admin/offering/CreateDiskOfferingCmd.java > a2c5f77 > > api/src/org/apache/cloudstack/api/command/admin/storage/CreateStoragePoolCmd.java > 74eb2b9 > api/src/org/apache/cloudstack/api/command/user/volume/CreateVolumeCmd.java > 86a494b > api/src/org/apache/cloudstack/api/response/DiskOfferingResponse.java > 35cf21a > api/src/org/apache/cloudstack/api/response/StoragePoolResponse.java 965407d > api/src/org/apache/cloudstack/api/response/VolumeResponse.java e3463bd > client/WEB-INF/classes/resources/messages.properties a0a36c8 > client/pom.xml ab758eb > client/tomcatconf/applicationContext.xml.in 049e483 > core/src/com/cloud/agent/api/AttachVolumeAnswer.java b377b7c > core/src/com/cloud/agent/api/AttachVolumeCommand.java 2658262 > core/test/org/apache/cloudstack/api/agent/test/AttachVolumeAnswerTest.java > 251a6cb > core/test/org/apache/cloudstack/api/agent/test/AttachVolumeCommandTest.java > 1ec416a > > core/test/org/apache/cloudstack/api/agent/test/BackupSnapshotCommandTest.java > 44d53aa > core/test/org/apache/cloudstack/api/agent/test/SnapshotCommandTest.java > c2d69c0 > core/test/src/com/cloud/agent/api/test/ResizeVolumeCommandTest.java 02085f5 > > engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/DataStoreDriver.java > cf5759b > > engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/PrimaryDataStoreParameters.java > b2b787c > > engine/api/src/org/apache/cloudstack/storage/datastore/db/PrimaryDataStoreDaoImpl.java > d461d58 > > engine/api/src/org/apache/cloudstack/storage/datastore/db/StoragePoolVO.java > 0262f65 > engine/schema/src/com/cloud/storage/DiskOfferingVO.java 44f9e8f > engine/schema/src/com/cloud/storage/VolumeVO.java 1699afd > engine/schema/src/com/cloud/storage/dao/VolumeDao.java 2513181 > engine/schema/src/com/cloud/storage/dao/VolumeDaoImpl.java 12ca3c7 > > engine/storage/image/src/org/apache/cloudstack/storage/image/ImageServiceImpl.java > 99b1013 > > engine/storage/image/src/org/apache/cloudstack/storage/image/driver/AncientImageDataStoreDriverImpl.java > 4c16f2f > > engine/storage/image/src/org/apache/cloudstack/storage/image/driver/DefaultImageDataStoreDriverImpl.java > 3d46c73 > > engine/storage/integration-test/test/org/apache/cloudstack/storage/allocator/StorageAllocatorTest.java > 9444fa5 > > engine/storage/snapshot/src/org/apache/cloudstack/storage/snapshot/strategy/AncientSnapshotStrategy.java > 4aba3d9 > > engine/storage/src/org/apache/cloudstack/storage/allocator/AbstractStoragePoolAllocator.java > 5326701 > > engine/storage/src/org/apache/cloudstack/storage/allocator/ZoneWideStoragePoolAllocator.java > d8d4132 > > engine/storage/src/org/apache/cloudstack/storage/datastore/DataObjectManagerImpl.java > 9d1afbe > > engine/storage/src/org/apache/cloudstack/storage/datastore/PrimaryDataStoreEntityImpl.java > 2dc3e25 > > engine/storage/src/org/apache/cloudstack/storage/volume/datastore/PrimaryDataStoreHelper.java > 349f6ba > > engine/storage/volume/src/org/apache/cloudstack/storage/datastore/DefaultPrimaryDataStore.java > 31e6908 > > engine/storage/volume/src/org/apache/cloudstack/storage/datastore/driver/DefaultPrimaryDataStoreDriverImpl.java > e5ee742 > > engine/storage/volume/src/org/apache/cloudstack/storage/datastore/lifecycle/DefaultPrimaryDataStoreLifeCycleImpl.java > cffa1ce > > engine/storage/volume/src/org/apache/cloudstack/storage/volume/VolumeObject.java > ea31be3 > > engine/storage/volume/src/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java > 54dcbd2 > > plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java > 7d90f6a > > plugins/hypervisors/simulator/src/com/cloud/agent/manager/MockStorageManagerImpl.java > a50dff6 > > plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java > 1af4239 > > plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java > 5e8283a > > plugins/storage/volume/default/src/org/apache/cloudstack/storage/datastore/driver/CloudStackPrimaryDataStoreDriverImpl.java > 0486902 > 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 7022ee6 > server/src/com/cloud/api/query/dao/StoragePoolJoinDaoImpl.java f2b9525 > server/src/com/cloud/api/query/dao/VolumeJoinDaoImpl.java 453e82e > server/src/com/cloud/api/query/vo/DiskOfferingJoinVO.java 2336a48 > server/src/com/cloud/api/query/vo/StoragePoolJoinVO.java 29e97f4 > server/src/com/cloud/api/query/vo/VolumeJoinVO.java 735cf9a > server/src/com/cloud/configuration/ConfigurationManager.java 93cadfa > server/src/com/cloud/configuration/ConfigurationManagerImpl.java e7e3f74 > server/src/com/cloud/server/ConfigurationServerImpl.java 510455b > server/src/com/cloud/storage/StorageManager.java 29c7ebc > server/src/com/cloud/storage/StorageManagerImpl.java 20b435c > server/src/com/cloud/storage/VolumeManager.java 56de408 > server/src/com/cloud/storage/VolumeManagerImpl.java e5868d3 > server/src/com/cloud/test/DatabaseConfig.java ef0259d > server/test/com/cloud/vpc/MockConfigurationManagerImpl.java 6e3d187 > setup/db/db/schema-410to420.sql bcdf2d9 > tools/marvin/marvin/cloudstackConnection.py b092ef0 > ui/dictionary.jsp 7809cdb > ui/scripts/configuration.js 150f244 > ui/scripts/docs.js 5aa352a > ui/scripts/sharedFunctions.js d87f0dc > ui/scripts/storage.js 2c03d39 > ui/scripts/system.js df37f31 > utils/src/com/cloud/utils/StringUtils.java 359b169 > vmware-base/src/com/cloud/hypervisor/vmware/mo/HostDatastoreSystemMO.java > 3dcd724 > vmware-base/src/com/cloud/hypervisor/vmware/mo/HostMO.java a866fdc > vmware-base/src/com/cloud/hypervisor/vmware/mo/HostStorageSystemMO.java > PRE-CREATION > > 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 > >
