----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/11479/#review21347 -----------------------------------------------------------
api/src/com/cloud/offering/DiskOffering.java <https://reviews.apache.org/r/11479/#comment44377> Is the notion of burst IOPS standard across QoS storage systems or specific to a product? api/src/com/cloud/storage/Storage.java <https://reviews.apache.org/r/11479/#comment44252> Coding standards are that enumeration values are all caps. core/src/com/cloud/agent/api/AttachVolumeCommand.java <https://reviews.apache.org/r/11479/#comment44253> Strip leading spaces on blank lines and remove tab characters. engine/storage/volume/src/org/apache/cloudstack/storage/volume/VolumeObject.java <https://reviews.apache.org/r/11479/#comment45019> As discussed on the mailing list, Burst IOPS are implementation specific. Defer implementation to a later release where we have a more robust mechanism for capturing extended data. plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java <https://reviews.apache.org/r/11479/#comment45020> Why did this method change from taking a domain object to a taking discrete values? Is this information not available on the StorageFilerTO? plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java <https://reviews.apache.org/r/11479/#comment44260> Is it valid to configure CHAP with just a username? While odd, just curious if there is a scenario where it is done. If so, the Also, check that the strings are not blank which covers both null, empty strings, and strings containing only spaces. (StringUtils#isNotBlank). plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java <https://reviews.apache.org/r/11479/#comment44279> Why not use a for-each loop to reduce/simplify the code? plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java <https://reviews.apache.org/r/11479/#comment44261> Fix indentation in this block to match coding standards -- 4 spaces per level. plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java <https://reviews.apache.org/r/11479/#comment45022> As discussed on the list, the intention is to remove the StoragePoolType.Dynamic in favor of a VolumeTO#isManaged check. When this method becomes available, extracting the allocation of the underlying storage to a private method would make things a good bit more readable. plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java <https://reviews.apache.org/r/11479/#comment44262> To reduce the conditional nesting, check if (!created[0]), perform the associated work, and return -- then proceed with the true case. plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java <https://reviews.apache.org/r/11479/#comment45024> 1. Extract the 100000 value to a constant 2. Should this slack value be a global configuration setting? plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/driver/SolidfirePrimaryDataStoreDriver.java <https://reviews.apache.org/r/11479/#comment45025> Why is the UUID being prepended with the provider name? Smells of magic value ... plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/driver/SolidfirePrimaryDataStoreDriver.java <https://reviews.apache.org/r/11479/#comment45029> 1. I would implement two constructors -- a noarg and this one. The noarg calls this constructor with the default values where this constructor applies the values passed. This approach is more explicit, and more clearly conveys to the calls the expected behavior. Otherwise, to get default values, the caller has to know to call with magic 0 values. 2. Hard-coded like these seem a bit inflexible. Can they be made configurable as a global configuration option or the DataStore configuration? plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/driver/SolidfirePrimaryDataStoreDriver.java <https://reviews.apache.org/r/11479/#comment45031> Why is the exception being logged to debug and not error? plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/driver/SolidfirePrimaryDataStoreDriver.java <https://reviews.apache.org/r/11479/#comment45032> Why is this exception being logged to debug and not error? plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/driver/SolidfirePrimaryDataStoreDriver.java <https://reviews.apache.org/r/11479/#comment45030> Readability: check this condition at the top of the method and bail out early to avoid large nested blocks. plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/driver/SolidfirePrimaryDataStoreDriver.java <https://reviews.apache.org/r/11479/#comment45033> What's the resolution of this TODO? plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/driver/SolidfirePrimaryDataStoreDriver.java <https://reviews.apache.org/r/11479/#comment45034> What's the resolution of this TODO? plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/driver/SolidfirePrimaryDataStoreDriver.java <https://reviews.apache.org/r/11479/#comment45035> What's the resolution of this TODO? plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/lifecycle/SolidFirePrimaryDataStoreLifeCycle.java <https://reviews.apache.org/r/11479/#comment45037> Why is the UUID being prepended with the provider name? Smells of magic value. plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/lifecycle/SolidFirePrimaryDataStoreLifeCycle.java <https://reviews.apache.org/r/11479/#comment45038> Prefer IllegalStateException for this condition plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/lifecycle/SolidFirePrimaryDataStoreLifeCycle.java <https://reviews.apache.org/r/11479/#comment45040> 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. plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/lifecycle/SolidFirePrimaryDataStoreLifeCycle.java <https://reviews.apache.org/r/11479/#comment45041> Consider using Guava's Splitter. It will make this code much more concise ... plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/lifecycle/SolidFirePrimaryDataStoreLifeCycle.java <https://reviews.apache.org/r/11479/#comment45042> Consider using Guava's Splitter to grab out the value. plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/lifecycle/SolidFirePrimaryDataStoreLifeCycle.java <https://reviews.apache.org/r/11479/#comment45043> Prefer IllegalStateException in this situation plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/lifecycle/SolidFirePrimaryDataStoreLifeCycle.java <https://reviews.apache.org/r/11479/#comment45044> See previous comments about using java.net.URI. plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/lifecycle/SolidFirePrimaryDataStoreLifeCycle.java <https://reviews.apache.org/r/11479/#comment45045> Prefer an explicit throws list to be more declarative plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/provider/SolidfirePrimaryDataStoreProvider.java <https://reviews.apache.org/r/11479/#comment45046> Why aren't these values being injected via @Inject? plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java <https://reviews.apache.org/r/11479/#comment45047> Implement equals, hashCode, and toString plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java <https://reviews.apache.org/r/11479/#comment45050> Make these attributes as final since they can not be modified plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java <https://reviews.apache.org/r/11479/#comment45048> Implement equals, hashCode, and toString plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java <https://reviews.apache.org/r/11479/#comment45049> Mark attributes as final since they can not be modified plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java <https://reviews.apache.org/r/11479/#comment45051> 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. plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java <https://reviews.apache.org/r/11479/#comment45052> 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. plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java <https://reviews.apache.org/r/11479/#comment45053> 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. plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java <https://reviews.apache.org/r/11479/#comment45054> 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. plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java <https://reviews.apache.org/r/11479/#comment45055> 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. plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java <https://reviews.apache.org/r/11479/#comment45056> 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. plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java <https://reviews.apache.org/r/11479/#comment45057> 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. plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java <https://reviews.apache.org/r/11479/#comment45058> What is the purpose of this class? All of the data is private and no methods are provided to access or modify it. plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java <https://reviews.apache.org/r/11479/#comment45059> What is the purpose of this class? All of the data is private and no methods are provided to access or modify it. plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java <https://reviews.apache.org/r/11479/#comment45060> What is the purpose of this class? All of the data is private and no methods are provided to access or modify it. plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java <https://reviews.apache.org/r/11479/#comment45061> What is the purpose of this class? All of the data is private and no methods are provided to access or modify it. plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java <https://reviews.apache.org/r/11479/#comment45062> What is the purpose of this class? All of the data is private and no methods are provided to access or modify it. plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java <https://reviews.apache.org/r/11479/#comment45063> What is the purpose of this class? All of the data is private and no methods are provided to access or modify it. plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java <https://reviews.apache.org/r/11479/#comment45066> Explicitly list the exceptions thrown plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java <https://reviews.apache.org/r/11479/#comment45064> Extract 515-525 to a private factory method plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java <https://reviews.apache.org/r/11479/#comment45065> By spec, any 20x status code represents success. Why is success being constrainted to 200? By spec, we should check >= 200 && < 300. I recommend creating a general utility method, isSuccess, to implement this check. plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java <https://reviews.apache.org/r/11479/#comment45068> 1) This resource management block should be in a finally block to ensure execution. Also, we want to ignore any exceptions from it. 2) We should be setting up an HttpConnection pool through HttpConnectionManager. The shutdown method kills the pool. This class should be fitted to create an HttpConnectionManager at startup and simply release the HttpConnections back to pool. plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java <https://reviews.apache.org/r/11479/#comment45069> Prefer an IllegalStateException here plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java <https://reviews.apache.org/r/11479/#comment45071> This method looks like it will also throw an exception. Is that correct? server/src/com/cloud/configuration/ConfigurationManagerImpl.java <https://reviews.apache.org/r/11479/#comment45072> Is it permissable for minIOPS to be equal to maxIOPS? server/src/com/cloud/storage/VolumeManagerImpl.java <https://reviews.apache.org/r/11479/#comment45074> Don't use the Long constructor because it avoid the constant pool resulting in memory bloat and garbage collector stress. Use Long.valueOf or, even better, just use 0L and let the compiler autobox the value. server/src/com/cloud/storage/VolumeManagerImpl.java <https://reviews.apache.org/r/11479/#comment45075> Check >= 0 to be completely defensive. server/src/com/cloud/storage/VolumeManagerImpl.java <https://reviews.apache.org/r/11479/#comment45076> Check >= 0 to be completely defensive. - John Burwell 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 > >
