You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Mike Tutkowski <mi...@solidfire.com> on 2013/07/08 22:29:53 UTC
Re: Review Request 11479: SolidFire storage plug-in and enhancements to the
storage framework and GUI
> 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
>
>