You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by John Burwell <jb...@basho.com> on 2013/06/28 17:42:23 UTC
Re: Review Request 11479: SolidFire storage plug-in and enhancements to the
storage framework and GUI
-----------------------------------------------------------
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
>
>
Re: Review Request 11479: SolidFire storage plug-in and enhancements to the
storage framework and GUI
Posted by Mike Tutkowski <mi...@solidfire.com>.
> On June 28, 2013, 3:42 p.m., John Burwell wrote:
> > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/lifecycle/SolidFirePrimaryDataStoreLifeCycle.java, line 194
> > <https://reviews.apache.org/r/11479/diff/4/?file=310131#file310131line194>
> >
> > Consider using Guava's Splitter class because it much more expressive, and will greatly shorten this method. It also a tested utility ...
Noted in my to-do document as a potential future activity to implement.
> On June 28, 2013, 3:42 p.m., John Burwell wrote:
> > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/lifecycle/SolidFirePrimaryDataStoreLifeCycle.java, line 210
> > <https://reviews.apache.org/r/11479/diff/4/?file=310131#file310131line210>
> >
> > Consider using Guava's Splitter class because it much more expressive, and will greatly shorten this method. It also a tested utility ...
Noted in my to-do document as a potential future activity to implement.
> On June 28, 2013, 3:42 p.m., John Burwell wrote:
> > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/lifecycle/SolidFirePrimaryDataStoreLifeCycle.java, line 237
> > <https://reviews.apache.org/r/11479/diff/4/?file=310131#file310131line237>
> >
> > Why is URL parsing logic being implemented? Use java.net.URI ...
Noted in my to-do document as a potential future activity to implement.
> On June 28, 2013, 3:42 p.m., John Burwell wrote:
> > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/lifecycle/SolidFirePrimaryDataStoreLifeCycle.java, line 145
> > <https://reviews.apache.org/r/11479/diff/4/?file=310131#file310131line145>
> >
> > 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.
Noted in my to-do document as a potential future activity to implement.
> On June 28, 2013, 3:42 p.m., John Burwell wrote:
> > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/provider/SolidfirePrimaryDataStoreProvider.java, line 83
> > <https://reviews.apache.org/r/11479/diff/4/?file=310132#file310132line83>
> >
> > 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?
Noted in my to-do document as a potential future activity to implement.
Users can make use of SolidFire storage for secondary storage, but they would not do so via the SolidFire plug-in (they would just use the default plug-in).
- 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
>
>
Re: Review Request 11479: SolidFire storage plug-in and enhancements to the
storage framework and GUI
Posted by Mike Tutkowski <mi...@solidfire.com>.
> On June 28, 2013, 9:42 a.m., John Burwell wrote:
> > plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java, line 3927
> > <https://reviews.apache.org/r/11479/diff/4/?file=310126#file310126line3927>
> >
> > Use an ExceutorService. Don't implement your own thread pool. Also, why are we multi-threading? Are there performance numbers to justify this complexity?
It often takes on the order of seconds to rescan HBAs and VMFS datastores and - due to certain VMware issues - these kinds of scans need to happen several times while I'm attaching and detaching volumes. Instead of taking, say, 3 seconds per scan times the number of hosts and making the user wait that long, I perform these scans in parallel. This can be the difference between the user waiting only 10 - 15 seconds or having to wait over a minute.
I have converted the custom threading "pool" to use the ExecutorService.
- Mike
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11479/#review22508
-----------------------------------------------------------
On June 21, 2013, 10:35 a.m., Mike Tutkowski wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11479/
> -----------------------------------------------------------
>
> (Updated June 21, 2013, 10:35 a.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
>
>
Re: Review Request 11479: SolidFire storage plug-in and enhancements to the
storage framework and GUI
Posted by Mike Tutkowski <mi...@solidfire.com>.
> On June 28, 2013, 3:42 p.m., John Burwell wrote:
> > core/src/com/cloud/agent/api/AttachVolumeCommand.java, line 61
> > <https://reviews.apache.org/r/11479/diff/4/?file=310095#file310095line61>
> >
> > Fix formatting/continuation from the previous line
I think I fixed all formatting issues in this file.
> On June 28, 2013, 3:42 p.m., John Burwell wrote:
> > engine/storage/src/org/apache/cloudstack/storage/datastore/PrimaryDataStoreEntityImpl.java, line 181
> > <https://reviews.apache.org/r/11479/diff/4/?file=310117#file310117line181>
> >
> > What is the resolution of this TODO? It appears that this getter is broken as it doesn't return a parameter value.
It appears this class has been removed from the codebase.
> On June 28, 2013, 3:42 p.m., John Burwell wrote:
> > engine/storage/volume/src/org/apache/cloudstack/storage/datastore/lifecycle/DefaultPrimaryDataStoreLifeCycleImpl.java, line 111
> > <https://reviews.apache.org/r/11479/diff/4/?file=310121#file310121line111>
> >
> > 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.
This method no longer seems to be present in the codebase.
> On June 28, 2013, 3:42 p.m., John Burwell wrote:
> > plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java, line 3789
> > <https://reviews.apache.org/r/11479/diff/4/?file=310126#file310126line3789>
> >
> > 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.
I agree exceptions should be more specific. The problem here is that this VMware code (the VmwareResource class itself) was written with generic exception handling all over the place. I think if we want to correct this, it will take a larger effort (therefore I'm thinking this change is out of scope for this release).
> On June 28, 2013, 3:42 p.m., John Burwell wrote:
> > plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java, line 3813
> > <https://reviews.apache.org/r/11479/diff/4/?file=310126#file310126line3813>
> >
> > Exception is too broad. Throw a CloudRuntimeException.
I agree exceptions should be more specific. The problem here is that this VMware code (the VmwareResource class itself) was written with generic exception handling all over the place. I think if we want to correct this, it will take a larger effort (therefore I'm thinking this change is out of scope for this release).
> On June 28, 2013, 3:42 p.m., John Burwell wrote:
> > plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java, line 3816
> > <https://reviews.apache.org/r/11479/diff/4/?file=310126#file310126line3816>
> >
> > Casting down from a long to int seems dangerous. Why are casting this value down?
The VMware API is expecting an int referencing the size in MBs. This code here is copied/pasted from another location in the VmwareResource file where the same conversion happens.
I have made a utility function for this.
> On June 28, 2013, 3:42 p.m., John Burwell wrote:
> > plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java, line 3891
> > <https://reviews.apache.org/r/11479/diff/4/?file=310126#file310126line3891>
> >
> > 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.
I agree exceptions should be more specific. The problem here is that this VMware code (the VmwareResource class itself) was written with generic exception handling all over the place. I think if we want to correct this, it will take a larger effort (therefore I'm thinking this change is out of scope for this release).
> On June 28, 2013, 3:42 p.m., John Burwell wrote:
> > plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java, line 3955
> > <https://reviews.apache.org/r/11479/diff/4/?file=310126#file310126line3955>
> >
> > Why are we locking a variable with local method scope? There should be *no* need to synchronize here, and it is very expensive.
It is actually a local variable that has been passed into multiple threads to record any exceptions that might happen. Since more than one thread can call the "add" method at the same time, I have protected the data structure with synchronization.
> On June 28, 2013, 3:42 p.m., John Burwell wrote:
> > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/driver/SolidfirePrimaryDataStoreDriver.java, line 126
> > <https://reviews.apache.org/r/11479/diff/4/?file=310130#file310130line126>
> >
> > Set defaults here. Also, as noted earlier, don't throw Exception ...
I now throw an IllegalArgumentException if the values are not correct.
> On June 28, 2013, 3:42 p.m., John Burwell wrote:
> > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/driver/SolidfirePrimaryDataStoreDriver.java, line 193
> > <https://reviews.apache.org/r/11479/diff/4/?file=310130#file310130line193>
> >
> > Lines 190-208 look very similar to lines 72-90 createSolidFireVolume. Can this work be extracted to a common method used by both methods?
I believe this has been fixed since the review came out (when I performed a merge and made other updates).
> On June 28, 2013, 3:42 p.m., John Burwell wrote:
> > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/driver/SolidfirePrimaryDataStoreDriver.java, line 246
> > <https://reviews.apache.org/r/11479/diff/4/?file=310130#file310130line246>
> >
> > 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);
Implemented with ternary operator now...originally I didn't because people have told me they find ternary operators less readable. :)
> On June 28, 2013, 3:42 p.m., John Burwell wrote:
> > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/driver/SolidfirePrimaryDataStoreDriver.java, line 253
> > <https://reviews.apache.org/r/11479/diff/4/?file=310130#file310130line253>
> >
> > Shouldn't this exception be passed up to higher layers? Seems like no storage available should be a stopper for clients of this method.
I believe the exception is passed up to higher layers this way:
result.setResult(errMsg);
> On June 28, 2013, 3:42 p.m., John Burwell wrote:
> > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/driver/SolidfirePrimaryDataStoreDriver.java, line 301
> > <https://reviews.apache.org/r/11479/diff/4/?file=310130#file310130line301>
> >
> > Shouldn't this exception be passed up to higher layers? Seems like no storage available should be a stopper for clients of this method.
I believe the exception is passed up to higher layers this way:
result.setResult(errMsg);
> On June 28, 2013, 3:42 p.m., John Burwell wrote:
> > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/lifecycle/SolidFirePrimaryDataStoreLifeCycle.java, line 263
> > <https://reviews.apache.org/r/11479/diff/4/?file=310131#file310131line263>
> >
> > 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?
Method no longer exists.
> On June 28, 2013, 3:42 p.m., John Burwell wrote:
> > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/lifecycle/SolidFirePrimaryDataStoreLifeCycle.java, line 269
> > <https://reviews.apache.org/r/11479/diff/4/?file=310131#file310131line269>
> >
> > 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?
Method no longer exists.
> On June 28, 2013, 3:42 p.m., John Burwell wrote:
> > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/lifecycle/SolidFirePrimaryDataStoreLifeCycle.java, line 280
> > <https://reviews.apache.org/r/11479/diff/4/?file=310131#file310131line280>
> >
> > because?
Method no longer exists.
> On June 28, 2013, 3:42 p.m., John Burwell wrote:
> > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/lifecycle/SolidFirePrimaryDataStoreLifeCycle.java, line 282
> > <https://reviews.apache.org/r/11479/diff/4/?file=310131#file310131line282>
> >
> > 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.
Method no longer exists.
> On June 28, 2013, 3:42 p.m., John Burwell wrote:
> > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/provider/SolidfirePrimaryDataStoreProvider.java, line 36
> > <https://reviews.apache.org/r/11479/diff/4/?file=310132#file310132line36>
> >
> > Why are these attributes protected and not private?
It was a copy/paste/modify from a "template." I have changed them to be private.
> 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 646
> > <https://reviews.apache.org/r/11479/diff/4/?file=310133#file310133line646>
> >
> > This method seems like it should be in a common utility class ...
Something to consider perhaps going forward.
> On June 28, 2013, 3:42 p.m., John Burwell wrote:
> > vmware-base/src/com/cloud/hypervisor/vmware/mo/HostStorageSystemMO.java, line 36
> > <https://reviews.apache.org/r/11479/diff/4/?file=310160#file310160line36>
> >
> > 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.
This is a general-purpose problem with VMware-related code. It would be nice if we fixed this in a comprehensive manner at some point.
> On June 28, 2013, 3:42 p.m., John Burwell wrote:
> > vmware-base/src/com/cloud/hypervisor/vmware/mo/HostStorageSystemMO.java, line 40
> > <https://reviews.apache.org/r/11479/diff/4/?file=310160#file310160line40>
> >
> > 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.
This is a general-purpose problem with VMware-related code. It would be nice if we fixed this in a comprehensive manner at some point.
> On June 28, 2013, 3:42 p.m., John Burwell wrote:
> > vmware-base/src/com/cloud/hypervisor/vmware/mo/HostStorageSystemMO.java, line 44
> > <https://reviews.apache.org/r/11479/diff/4/?file=310160#file310160line44>
> >
> > 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.
This is a general-purpose problem with VMware-related code. It would be nice if we fixed this in a comprehensive manner at some point.
- 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
>
>
Re: Review Request 11479: SolidFire storage plug-in and enhancements to the
storage framework and GUI
Posted by Mike Tutkowski <mi...@solidfire.com>.
> On June 28, 2013, 9:42 a.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.
>
> Mike Tutkowski wrote:
> null is used if this feature is not in use.
>
> John Burwell wrote:
> What is the difference between false and null?
null means storage QoS is not in use.
false means storage QoS is in use and the admin has selected the min and max IOPS values.
true means storage QoS is in use and the admin has not selected the min and max IOPS values (the end user can select them).
- Mike
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11479/#review22508
-----------------------------------------------------------
On June 21, 2013, 10:35 a.m., Mike Tutkowski wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11479/
> -----------------------------------------------------------
>
> (Updated June 21, 2013, 10:35 a.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
>
>
Re: Review Request 11479: SolidFire storage plug-in and enhancements to the
storage framework and GUI
Posted by John Burwell <jb...@basho.com>.
> 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.
>
> Mike Tutkowski wrote:
> null is used if this feature is not in use.
What is the difference between false and null?
- John
-----------------------------------------------------------
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
>
>
Re: Review Request 11479: SolidFire storage plug-in and enhancements to the
storage framework and GUI
Posted by Mike Tutkowski <mi...@solidfire.com>.
> 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
>
>