----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/11479/#review22508 -----------------------------------------------------------
api/src/com/cloud/offering/DiskOffering.java <https://reviews.apache.org/r/11479/#comment46044> Strip out additional tab charcters api/src/com/cloud/offering/DiskOffering.java <https://reviews.apache.org/r/11479/#comment46045> When would it be valid for the value of this property to be null? Seems like it should be boolean, not Boolean. api/src/com/cloud/storage/Volume.java <https://reviews.apache.org/r/11479/#comment46046> Strip out tab charcaters api/src/org/apache/cloudstack/api/command/admin/offering/CreateDiskOfferingCmd.java <https://reviews.apache.org/r/11479/#comment46047> Strip out tab characters api/src/org/apache/cloudstack/api/command/admin/offering/CreateDiskOfferingCmd.java <https://reviews.apache.org/r/11479/#comment46048> Strip out tab characters api/src/org/apache/cloudstack/api/command/admin/storage/CreateStoragePoolCmd.java <https://reviews.apache.org/r/11479/#comment46049> Strip out tab characters api/src/org/apache/cloudstack/api/command/user/volume/CreateVolumeCmd.java <https://reviews.apache.org/r/11479/#comment46050> Strip out tab characters api/src/org/apache/cloudstack/api/command/user/volume/CreateVolumeCmd.java <https://reviews.apache.org/r/11479/#comment46051> Strip out tab characters api/src/org/apache/cloudstack/api/response/DiskOfferingResponse.java <https://reviews.apache.org/r/11479/#comment46052> Strip out tab characters api/src/org/apache/cloudstack/api/response/DiskOfferingResponse.java <https://reviews.apache.org/r/11479/#comment46053> Strip out tab characters api/src/org/apache/cloudstack/api/response/StoragePoolResponse.java <https://reviews.apache.org/r/11479/#comment46054> Strip out tab characters api/src/org/apache/cloudstack/api/response/VolumeResponse.java <https://reviews.apache.org/r/11479/#comment46055> Strip out tab characters api/src/org/apache/cloudstack/api/response/VolumeResponse.java <https://reviews.apache.org/r/11479/#comment46056> Strip out tab characters core/src/com/cloud/agent/api/AttachVolumeAnswer.java <https://reviews.apache.org/r/11479/#comment46057> Strip out tab characters core/src/com/cloud/agent/api/AttachVolumeCommand.java <https://reviews.apache.org/r/11479/#comment46058> Fix formatting/continuation from the previous line core/src/com/cloud/agent/api/AttachVolumeCommand.java <https://reviews.apache.org/r/11479/#comment46059> Fix indentation core/test/org/apache/cloudstack/api/agent/test/AttachVolumeAnswerTest.java <https://reviews.apache.org/r/11479/#comment46060> Create a new AttachVolumeAnswer constructor that only takes two parameters and encapsulates the default value. Leaky abstraction that negatively impacts maintenance. engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/PrimaryDataStoreParameters.java <https://reviews.apache.org/r/11479/#comment46032> Strip out tab characters engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/PrimaryDataStoreParameters.java <https://reviews.apache.org/r/11479/#comment46031> Strip out tab characters engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/PrimaryDataStoreParameters.java <https://reviews.apache.org/r/11479/#comment46033> Strip out tab characters engine/api/src/org/apache/cloudstack/storage/datastore/db/StoragePoolVO.java <https://reviews.apache.org/r/11479/#comment46034> Strip out tab characters engine/schema/src/com/cloud/storage/DiskOfferingVO.java <https://reviews.apache.org/r/11479/#comment46035> 1) The minIops and maxIops attributes should be private. 2) Strip out tab characters engine/schema/src/com/cloud/storage/DiskOfferingVO.java <https://reviews.apache.org/r/11479/#comment46036> Strip out tab characters engine/schema/src/com/cloud/storage/DiskOfferingVO.java <https://reviews.apache.org/r/11479/#comment46037> 1) Fix identation 2) Strip out tab characters engine/schema/src/com/cloud/storage/VolumeVO.java <https://reviews.apache.org/r/11479/#comment46038> 1) Strip out tab characters 2) Make minIops and maxIops attributes private engine/schema/src/com/cloud/storage/VolumeVO.java <https://reviews.apache.org/r/11479/#comment46039> Strip out tab characters engine/schema/src/com/cloud/storage/VolumeVO.java <https://reviews.apache.org/r/11479/#comment46040> 1) Strip out tab characters 2) Fix formatting from the previous line engine/schema/src/com/cloud/storage/VolumeVO.java <https://reviews.apache.org/r/11479/#comment46041> Strip out additional tab characters engine/storage/src/org/apache/cloudstack/storage/datastore/PrimaryDataStoreEntityImpl.java <https://reviews.apache.org/r/11479/#comment46042> What is the resolution of this TODO? It appears that this getter is broken as it doesn't return a parameter value. engine/storage/src/org/apache/cloudstack/storage/volume/datastore/PrimaryDataStoreHelper.java <https://reviews.apache.org/r/11479/#comment46043> Strip out tab characters engine/storage/volume/src/org/apache/cloudstack/storage/datastore/lifecycle/DefaultPrimaryDataStoreLifeCycleImpl.java <https://reviews.apache.org/r/11479/#comment46061> I know it is out of the scope of this patch, but please correct the name of this method on the interface and all implementations to conform to JavaBean property standard. Accessors for boolean properties should be named is<PropertyName>. Failure to conform to this convention will interfere with any reflection tools we elect to use. engine/storage/volume/src/org/apache/cloudstack/storage/volume/VolumeObject.java <https://reviews.apache.org/r/11479/#comment46062> 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. plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java <https://reviews.apache.org/r/11479/#comment46066> 1) Don't throw Exception generically. Throw out the specific exceptions that can occur. 2) What is the error handling here? This method has no recovery provisions. A failure could leave dangling resources. Seems that we should be catching Exception, and attempting to undo the operations performed, and then re-throwing the exception as a CloudRuntimeException. plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java <https://reviews.apache.org/r/11479/#comment46063> 1) Strip out tab characters 2) Fix indentation plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java <https://reviews.apache.org/r/11479/#comment46064> Exception is too broad. Throw a CloudRuntimeException. plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java <https://reviews.apache.org/r/11479/#comment46065> Casting down from a long to int seems dangerous. Why are casting this value down? plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java <https://reviews.apache.org/r/11479/#comment46067> String out tab characters plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java <https://reviews.apache.org/r/11479/#comment46068> 1) Strip out tab characters 2) Fix indentation 3) Don't throw Exception -- it's too generic. Seems that we should be catching Exception, and attempting to undo any operations performed in order to ensure that we don't leave the hypervisor in an inconsistent state then rethrowing the exception as a CloudRuntimeException. plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java <https://reviews.apache.org/r/11479/#comment46070> Use an ExceutorService. Don't implement your own thread pool. Also, why are we multi-threading? Are there performance numbers to justify this complexity? plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java <https://reviews.apache.org/r/11479/#comment46069> Why are we locking a variable with local method scope? There should be *no* need to synchronize here, and it is very expensive. plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java <https://reviews.apache.org/r/11479/#comment46071> Strip out tab characters plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java <https://reviews.apache.org/r/11479/#comment46072> Strip out tab characters plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java <https://reviews.apache.org/r/11479/#comment46073> Strip out tab characters plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java <https://reviews.apache.org/r/11479/#comment46074> Strip out tab characters plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java <https://reviews.apache.org/r/11479/#comment46076> Do not generically throw Exception -- it is too broad. Implement error handling, and ensure the hypervisor is returned to a consistent state. plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java <https://reviews.apache.org/r/11479/#comment46075> Don't throw exception -- throw a runtime exception. In CloudStack, use CloudRuntimeException. plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java <https://reviews.apache.org/r/11479/#comment46077> Strip out tab characters plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/driver/SolidfirePrimaryDataStoreDriver.java <https://reviews.apache.org/r/11479/#comment46078> Strip out tab characters plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/driver/SolidfirePrimaryDataStoreDriver.java <https://reviews.apache.org/r/11479/#comment46079> 1) Strip out tab characters 2) Fix indentation 3) Don't throw Exception -- it is too broad. Implement error handling to undo operations and attempt to return the hypervisor to a consistent state. plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/driver/SolidfirePrimaryDataStoreDriver.java <https://reviews.apache.org/r/11479/#comment46080> Seems like these default values should be configurable by device. At minimum they should be constants. Also, if they are constants, push them down to the Iops class, and encapsulate the values and tests for setting them. plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/driver/SolidfirePrimaryDataStoreDriver.java <https://reviews.apache.org/r/11479/#comment46081> Set defaults here. Also, as noted earlier, don't throw Exception ... plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/driver/SolidfirePrimaryDataStoreDriver.java <https://reviews.apache.org/r/11479/#comment46082> Throw only the checked exceptions expected to be thrown. Throwing Exception causes us to miss error handling later introduced by dependencies that we should account for in this method. The compiler will catch it, and cause us to realize and address it. plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/driver/SolidfirePrimaryDataStoreDriver.java <https://reviews.apache.org/r/11479/#comment46098> Lines 190-208 look very similar to lines 72-90 createSolidFireVolume. Can this work be extracted to a common method used by both methods? plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/driver/SolidfirePrimaryDataStoreDriver.java <https://reviews.apache.org/r/11479/#comment46083> Strip out tab characters plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/driver/SolidfirePrimaryDataStoreDriver.java <https://reviews.apache.org/r/11479/#comment46084> I would collapse lines 242-247 into something such as the following to make things a little more readable: storagePool.setAvailableBytes(availableBytes >= 0 : availableBytes ? 0); plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/driver/SolidfirePrimaryDataStoreDriver.java <https://reviews.apache.org/r/11479/#comment46090> Shouldn't this exception be passed up to higher layers? Seems like no storage available should be a stopper for clients of this method. plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/driver/SolidfirePrimaryDataStoreDriver.java <https://reviews.apache.org/r/11479/#comment46085> Too broad a catch statement as it will swallow RuntimeExceptions that should percolate up to higher layers. plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/driver/SolidfirePrimaryDataStoreDriver.java <https://reviews.apache.org/r/11479/#comment46086> Strip out tab characters plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/driver/SolidfirePrimaryDataStoreDriver.java <https://reviews.apache.org/r/11479/#comment46087> I would change the structure of this block to check if the type is *not* a VOLUME, handle it, and return. The remainder of the method will be the success path -- reducing an indentation level and making things much more readable. plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/driver/SolidfirePrimaryDataStoreDriver.java <https://reviews.apache.org/r/11479/#comment46089> Shouldn't this exception be passed up to higher layers? Seems like no storage available should be a stopper for clients of this method. plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/driver/SolidfirePrimaryDataStoreDriver.java <https://reviews.apache.org/r/11479/#comment46088> As mentioned above, don't catch exception. plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/driver/SolidfirePrimaryDataStoreDriver.java <https://reviews.apache.org/r/11479/#comment46092> Why is there no implementation? If this method is not supported, throw an UnsupportedOperationException. If this methods are used, the client will under the mistaken impression that the copy was performed. plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/driver/SolidfirePrimaryDataStoreDriver.java <https://reviews.apache.org/r/11479/#comment46091> Why is there no implementation? If this method is not supported, throw an UnsupportedOperationException. If this methods are used, the client will under the mistaken impression that a snapshot was taken. plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/driver/SolidfirePrimaryDataStoreDriver.java <https://reviews.apache.org/r/11479/#comment46093> Why is there no implementation? If this method is not supported, throw an UnsupportedOperationException. If this methods are used, the client will under the mistaken impression that a snapshot was reverted. plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/driver/SolidfirePrimaryDataStoreDriver.java <https://reviews.apache.org/r/11479/#comment46094> Why is there no implementation? If this method is not supported, throw an UnsupportedOperationException. If this methods are used, the client will under the mistaken impression that a resize was performed. plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/lifecycle/SolidFirePrimaryDataStoreLifeCycle.java <https://reviews.apache.org/r/11479/#comment46095> Strip out tab characters plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/lifecycle/SolidFirePrimaryDataStoreLifeCycle.java <https://reviews.apache.org/r/11479/#comment46097> This a very broad catch to discover an illegal argument. Why can't the underlying runtime exception just bubble up, and this method proactively check the argument before attempting to operate? plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/lifecycle/SolidFirePrimaryDataStoreLifeCycle.java <https://reviews.apache.org/r/11479/#comment46096> Why are we parsing a URL here? Use the java.net.URI class. It will perform this operation, and handle a number of corner cases this method handle such as escaped characters. plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/lifecycle/SolidFirePrimaryDataStoreLifeCycle.java <https://reviews.apache.org/r/11479/#comment46099> Consider using Guava's Splitter class because it much more expressive, and will greatly shorten this method. It also a tested utility ... plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/lifecycle/SolidFirePrimaryDataStoreLifeCycle.java <https://reviews.apache.org/r/11479/#comment46100> Consider using Guava's Splitter class because it much more expressive, and will greatly shorten this method. It also a tested utility ... plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/lifecycle/SolidFirePrimaryDataStoreLifeCycle.java <https://reviews.apache.org/r/11479/#comment46101> Why is URL parsing logic being implemented? Use java.net.URI ... plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/lifecycle/SolidFirePrimaryDataStoreLifeCycle.java <https://reviews.apache.org/r/11479/#comment46102> It seems like this method is lying to the client. The host was not attached, but we are telling them that it occurred. Why are we lying here? plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/lifecycle/SolidFirePrimaryDataStoreLifeCycle.java <https://reviews.apache.org/r/11479/#comment46103> It seems like this method is lying to the client. The cluster was not attached, but we are telling them that it occurred. Why are we lying here? plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/lifecycle/SolidFirePrimaryDataStoreLifeCycle.java <https://reviews.apache.org/r/11479/#comment46105> because? plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/lifecycle/SolidFirePrimaryDataStoreLifeCycle.java <https://reviews.apache.org/r/11479/#comment46104> While it is out of the scope of this patch, fix the spelling error, and change the name to conform with the JavaBean property specification. plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/lifecycle/SolidFirePrimaryDataStoreLifeCycle.java <https://reviews.apache.org/r/11479/#comment46106> Lines 313-331 seems very similar to code in create and delete. Consolidate into a common private method. plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/lifecycle/SolidFirePrimaryDataStoreLifeCycle.java <https://reviews.apache.org/r/11479/#comment46107> Too broad a catch. Only catch the checked exceptions expected. plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/provider/SolidfirePrimaryDataStoreProvider.java <https://reviews.apache.org/r/11479/#comment46109> Strip out tab characters plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/provider/SolidfirePrimaryDataStoreProvider.java <https://reviews.apache.org/r/11479/#comment46108> Why are these attributes protected and not private? plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/provider/SolidfirePrimaryDataStoreProvider.java <https://reviews.apache.org/r/11479/#comment46110> Consider using Guava's ImmutableSet .. this method can be reduced to ImmutableSet.of(DataStoreProviderType.PRIMARY). Also, why prevents someone from using a SolidFire for secondary storage? plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java <https://reviews.apache.org/r/11479/#comment46111> Strip out tab characters plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java <https://reviews.apache.org/r/11479/#comment46117> 1) Strip out tab characters 2) Fix indentation plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java <https://reviews.apache.org/r/11479/#comment46112> As mentioned elsewhere, too broad a throws. Refine and add error handling as necessary to ensure the system is in a consistent state. plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java <https://reviews.apache.org/r/11479/#comment46113> As mentioned elsewhere, too broad a throws. Refine and add error handling as necessary to ensure the system is in a consistent state. plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java <https://reviews.apache.org/r/11479/#comment46114> As mentioned elsewhere, too broad a throws. Refine and add error handling as necessary to ensure the system is in a consistent state. plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java <https://reviews.apache.org/r/11479/#comment46115> Inspecific hashCode. All attributes must be calculated for proper determinism. plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java <https://reviews.apache.org/r/11479/#comment46116> 1) Check that obj is not null 2) Check getClass().equals rather than instanceof to avoid class version mismatch issues cause by Classloader hinkiness 3) Check all attributes to ensure determinism plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java <https://reviews.apache.org/r/11479/#comment46118> Inspecific hashCode. All attributes must be calculated for proper determinism. plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java <https://reviews.apache.org/r/11479/#comment46119> 1) Check that obj is not null 2) Check getClass().equals rather than instanceof to avoid class version mismatch issues cause by Classloader hinkiness 3) Check all attributes to ensure determinism plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java <https://reviews.apache.org/r/11479/#comment46120> If this class is unused, why not remove the cruft rather ignore the warning? plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java <https://reviews.apache.org/r/11479/#comment46121> If this class is unused, why not remove the cruft rather ignore the warning? plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java <https://reviews.apache.org/r/11479/#comment46122> If this class is unused, why not remove the cruft rather ignore the warning? plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java <https://reviews.apache.org/r/11479/#comment46123> If this class is unused, why not remove the cruft rather ignore the warning? plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java <https://reviews.apache.org/r/11479/#comment46124> If this class is unused, why not remove the cruft rather ignore the warning? plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java <https://reviews.apache.org/r/11479/#comment46125> If this class is unused, why not remove the cruft rather ignore the warning? plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java <https://reviews.apache.org/r/11479/#comment46126> If this class is unused, why not remove the cruft rather ignore the warning? plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java <https://reviews.apache.org/r/11479/#comment46127> If this class is unused, why not remove the cruft rather ignore the warning? plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java <https://reviews.apache.org/r/11479/#comment46128> Clients of this method can not recover from these exceptions. Catch these exception and rethrow them as CloudRuntimeException. plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java <https://reviews.apache.org/r/11479/#comment46129> Seems like we should be using the pooled connection manager. However, I don't know how it would be properly managed in the static context. Likely needs to be a ticket opened to be addressed later. plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java <https://reviews.apache.org/r/11479/#comment46131> Use CloudRuntimeException plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java <https://reviews.apache.org/r/11479/#comment46130> Don't catch throwable as it can swallow Errors that should abend the JVM. Only catch the checked exception expected. Also, log the ignored exception to WARN. plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java <https://reviews.apache.org/r/11479/#comment46132> This method seems like it should be in a common utility class ... plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java <https://reviews.apache.org/r/11479/#comment46133> Throw CloudRuntimeException not Exception. plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java <https://reviews.apache.org/r/11479/#comment46134> Throw CloudRuntimeException not Exception. server/src/com/cloud/api/query/dao/VolumeJoinDaoImpl.java <https://reviews.apache.org/r/11479/#comment46135> Strip out tab characters server/src/com/cloud/api/query/vo/DiskOfferingJoinVO.java <https://reviews.apache.org/r/11479/#comment46136> Strip out tab characters server/src/com/cloud/api/query/vo/DiskOfferingJoinVO.java <https://reviews.apache.org/r/11479/#comment46137> Strip out tab characters server/src/com/cloud/api/query/vo/VolumeJoinVO.java <https://reviews.apache.org/r/11479/#comment46138> Strip out tab characters server/src/com/cloud/api/query/vo/VolumeJoinVO.java <https://reviews.apache.org/r/11479/#comment46139> Strip out tab characters server/src/com/cloud/configuration/ConfigurationManager.java <https://reviews.apache.org/r/11479/#comment46140> Strip out tab characters server/src/com/cloud/configuration/ConfigurationManagerImpl.java <https://reviews.apache.org/r/11479/#comment46142> Fix formatting/indentation overflow server/src/com/cloud/configuration/ConfigurationManagerImpl.java <https://reviews.apache.org/r/11479/#comment46141> Strip out tab characters server/src/com/cloud/configuration/ConfigurationManagerImpl.java <https://reviews.apache.org/r/11479/#comment46143> Strip out tab characters server/src/com/cloud/storage/StorageManager.java <https://reviews.apache.org/r/11479/#comment46146> Strip out tab characters server/src/com/cloud/storage/StorageManagerImpl.java <https://reviews.apache.org/r/11479/#comment46148> Coding convention: Surround wit curly braces server/src/com/cloud/storage/StorageManagerImpl.java <https://reviews.apache.org/r/11479/#comment46147> Strip out tab characters server/src/com/cloud/storage/VolumeManager.java <https://reviews.apache.org/r/11479/#comment46149> Strip out tab characters server/src/com/cloud/storage/VolumeManagerImpl.java <https://reviews.apache.org/r/11479/#comment46150> Strip out tab characters server/src/com/cloud/storage/VolumeManagerImpl.java <https://reviews.apache.org/r/11479/#comment46151> Strip out tab characters server/src/com/cloud/storage/VolumeManagerImpl.java <https://reviews.apache.org/r/11479/#comment46152> Strip out tab characters server/src/com/cloud/storage/VolumeManagerImpl.java <https://reviews.apache.org/r/11479/#comment46153> Strip out tab characters server/src/com/cloud/storage/VolumeManagerImpl.java <https://reviews.apache.org/r/11479/#comment46154> Strip out tab characters server/src/com/cloud/storage/VolumeManagerImpl.java <https://reviews.apache.org/r/11479/#comment46155> Strip out tab characters server/src/com/cloud/storage/VolumeManagerImpl.java <https://reviews.apache.org/r/11479/#comment46156> Strip out tab characters server/src/com/cloud/storage/VolumeManagerImpl.java <https://reviews.apache.org/r/11479/#comment46157> Strip out tab characters server/src/com/cloud/storage/VolumeManagerImpl.java <https://reviews.apache.org/r/11479/#comment46158> Strip out tab characters server/src/com/cloud/storage/VolumeManagerImpl.java <https://reviews.apache.org/r/11479/#comment46159> Strip out tab characters server/src/com/cloud/test/DatabaseConfig.java <https://reviews.apache.org/r/11479/#comment46160> Leaky abstraction: Create a construction on DiskOfferingVO that support passing the six parameters and encapsulates this default behavior. server/test/com/cloud/vpc/MockConfigurationManagerImpl.java <https://reviews.apache.org/r/11479/#comment46161> Strip out tab characters ui/scripts/configuration.js <https://reviews.apache.org/r/11479/#comment46162> Strip out tab characters ui/scripts/configuration.js <https://reviews.apache.org/r/11479/#comment46163> Strip out tab characters ui/scripts/configuration.js <https://reviews.apache.org/r/11479/#comment46164> Strip out tab characters ui/scripts/configuration.js <https://reviews.apache.org/r/11479/#comment46165> Strip out tab characters ui/scripts/configuration.js <https://reviews.apache.org/r/11479/#comment46166> Strip out tab characters ui/scripts/configuration.js <https://reviews.apache.org/r/11479/#comment46167> Strip out tab characters ui/scripts/configuration.js <https://reviews.apache.org/r/11479/#comment46168> Strip out tab characters ui/scripts/configuration.js <https://reviews.apache.org/r/11479/#comment46169> Strip out tab characters ui/scripts/configuration.js <https://reviews.apache.org/r/11479/#comment46170> Strip out tab characters ui/scripts/storage.js <https://reviews.apache.org/r/11479/#comment46171> Strip out tab characters ui/scripts/storage.js <https://reviews.apache.org/r/11479/#comment46172> Strip out tab characters utils/src/com/cloud/utils/StringUtils.java <https://reviews.apache.org/r/11479/#comment46173> Strip out tab characters vmware-base/src/com/cloud/hypervisor/vmware/mo/HostDatastoreSystemMO.java <https://reviews.apache.org/r/11479/#comment46174> Strip out tab characters vmware-base/src/com/cloud/hypervisor/vmware/mo/HostMO.java <https://reviews.apache.org/r/11479/#comment46175> Strip out tab characters vmware-base/src/com/cloud/hypervisor/vmware/mo/HostStorageSystemMO.java <https://reviews.apache.org/r/11479/#comment46176> Strip out tab characters vmware-base/src/com/cloud/hypervisor/vmware/mo/HostStorageSystemMO.java <https://reviews.apache.org/r/11479/#comment46177> Throwing Exception is too broad. Can the client recover from the checked exception encountered by this method? If not, catch and re-throw as CloudRuntimeException. Otherwise, declare as throwing only the expected checked exceptions. vmware-base/src/com/cloud/hypervisor/vmware/mo/HostStorageSystemMO.java <https://reviews.apache.org/r/11479/#comment46178> Throwing Exception is too broad. Can the client recover from the checked exception encountered by this method? If not, catch and re-throw as CloudRuntimeException. Otherwise, declare as throwing only the expected checked exceptions. vmware-base/src/com/cloud/hypervisor/vmware/mo/HostStorageSystemMO.java <https://reviews.apache.org/r/11479/#comment46179> Throwing Exception is too broad. Can the client recover from the checked exception encountered by this method? If not, catch and re-throw as CloudRuntimeException. Otherwise, declare as throwing only the expected checked exceptions. - John Burwell 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 > >
