> On June 12, 2013, 11:15 p.m., John Burwell wrote: > > plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java, > > line 6494 > > <https://reviews.apache.org/r/11479/diff/1/?file=297882#file297882line6494> > > > > To reduce the conditional nesting, check if (!created[0]), perform the > > associated work, and return -- then proceed with the true case.
I did some refactoring before the code review was available on Review Board. I think this issue has been addressed. > On June 12, 2013, 11:15 p.m., John Burwell wrote: > > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java, > > line 232 > > <https://reviews.apache.org/r/11479/diff/1/?file=297887#file297887line232> > > > > Cruft Prevention: If this class is unused (infered from the > > @SupressWarnings annotation) why it is in the codebase? Delete it if we > > don't need it. This class is used by the Gson library for JSON purposes. Since Eclipse cannot see certain variables being used, it marks them as not in use. They actually are needed by Gson, so I put in the @SuppressWarnings. > On June 12, 2013, 11:15 p.m., John Burwell wrote: > > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java, > > line 280 > > <https://reviews.apache.org/r/11479/diff/1/?file=297887#file297887line280> > > > > Cruft Prevention: If this class is unused (infered from the > > @SupressWarnings annotation) why it is in the codebase? Delete it if we > > don't need it. This class is used by the Gson library for JSON purposes. Since Eclipse cannot see certain variables being used, it marks them as not in use. They actually are needed by Gson, so I put in the @SuppressWarnings. > On June 12, 2013, 11:15 p.m., John Burwell wrote: > > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java, > > line 325 > > <https://reviews.apache.org/r/11479/diff/1/?file=297887#file297887line325> > > > > Cruft Prevention: If this class is unused (infered from the > > @SupressWarnings annotation) why it is in the codebase? Delete it if we > > don't need it. This class is used by the Gson library for JSON purposes. Since Eclipse cannot see certain variables being used, it marks them as not in use. They actually are needed by Gson, so I put in the @SuppressWarnings. > On June 12, 2013, 11:15 p.m., John Burwell wrote: > > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java, > > line 347 > > <https://reviews.apache.org/r/11479/diff/1/?file=297887#file297887line347> > > > > Cruft Prevention: If this class is unused (infered from the > > @SupressWarnings annotation) why it is in the codebase? Delete it if we > > don't need it. This class is used by the Gson library for JSON purposes. Since Eclipse cannot see certain variables being used, it marks them as not in use. They actually are needed by Gson, so I put in the @SuppressWarnings. > On June 12, 2013, 11:15 p.m., John Burwell wrote: > > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java, > > line 369 > > <https://reviews.apache.org/r/11479/diff/1/?file=297887#file297887line369> > > > > Cruft Prevention: If this class is unused (infered from the > > @SupressWarnings annotation) why it is in the codebase? Delete it if we > > don't need it. This class is used by the Gson library for JSON purposes. Since Eclipse cannot see certain variables being used, it marks them as not in use. They actually are needed by Gson, so I put in the @SuppressWarnings. > On June 12, 2013, 11:15 p.m., John Burwell wrote: > > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java, > > line 391 > > <https://reviews.apache.org/r/11479/diff/1/?file=297887#file297887line391> > > > > Cruft Prevention: If this class is unused (infered from the > > @SupressWarnings annotation) why it is in the codebase? Delete it if we > > don't need it. This class is used by the Gson library for JSON purposes. Since Eclipse cannot see certain variables being used, it marks them as not in use. They actually are needed by Gson, so I put in the @SuppressWarnings. > On June 12, 2013, 11:15 p.m., John Burwell wrote: > > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java, > > line 413 > > <https://reviews.apache.org/r/11479/diff/1/?file=297887#file297887line413> > > > > Cruft Prevention: If this class is unused (infered from the > > @SupressWarnings annotation) why it is in the codebase? Delete it if we > > don't need it. This class is used by the Gson library for JSON purposes. Since Eclipse cannot see certain variables being used, it marks them as not in use. They actually are needed by Gson, so I put in the @SuppressWarnings. - 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 > >
