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/13 01:15:52 UTC
Re: Review Request: 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/#review21347
-----------------------------------------------------------
api/src/com/cloud/offering/DiskOffering.java
<https://reviews.apache.org/r/11479/#comment44377>
Is the notion of burst IOPS standard across QoS storage systems or specific to a product?
api/src/com/cloud/storage/Storage.java
<https://reviews.apache.org/r/11479/#comment44252>
Coding standards are that enumeration values are all caps.
core/src/com/cloud/agent/api/AttachVolumeCommand.java
<https://reviews.apache.org/r/11479/#comment44253>
Strip leading spaces on blank lines and remove tab characters.
engine/storage/volume/src/org/apache/cloudstack/storage/volume/VolumeObject.java
<https://reviews.apache.org/r/11479/#comment45019>
As discussed on the mailing list, Burst IOPS are implementation specific. Defer implementation to a later release where we have a more robust mechanism for capturing extended data.
plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java
<https://reviews.apache.org/r/11479/#comment45020>
Why did this method change from taking a domain object to a taking discrete values? Is this information not available on the StorageFilerTO?
plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java
<https://reviews.apache.org/r/11479/#comment44260>
Is it valid to configure CHAP with just a username? While odd, just curious if there is a scenario where it is done. If so, the
Also, check that the strings are not blank which covers both null, empty strings, and strings containing only spaces. (StringUtils#isNotBlank).
plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java
<https://reviews.apache.org/r/11479/#comment44279>
Why not use a for-each loop to reduce/simplify the code?
plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java
<https://reviews.apache.org/r/11479/#comment44261>
Fix indentation in this block to match coding standards -- 4 spaces per level.
plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java
<https://reviews.apache.org/r/11479/#comment45022>
As discussed on the list, the intention is to remove the StoragePoolType.Dynamic in favor of a VolumeTO#isManaged check. When this method becomes available, extracting the allocation of the underlying storage to a private method would make things a good bit more readable.
plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java
<https://reviews.apache.org/r/11479/#comment44262>
To reduce the conditional nesting, check if (!created[0]), perform the associated work, and return -- then proceed with the true case.
plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java
<https://reviews.apache.org/r/11479/#comment45024>
1. Extract the 100000 value to a constant
2. Should this slack value be a global configuration setting?
plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/driver/SolidfirePrimaryDataStoreDriver.java
<https://reviews.apache.org/r/11479/#comment45025>
Why is the UUID being prepended with the provider name? Smells of magic value ...
plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/driver/SolidfirePrimaryDataStoreDriver.java
<https://reviews.apache.org/r/11479/#comment45029>
1. I would implement two constructors -- a noarg and this one. The noarg calls this constructor with the default values where this constructor applies the values passed. This approach is more explicit, and more clearly conveys to the calls the expected behavior. Otherwise, to get default values, the caller has to know to call with magic 0 values.
2. Hard-coded like these seem a bit inflexible. Can they be made configurable as a global configuration option or the DataStore configuration?
plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/driver/SolidfirePrimaryDataStoreDriver.java
<https://reviews.apache.org/r/11479/#comment45031>
Why is the exception being logged to debug and not error?
plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/driver/SolidfirePrimaryDataStoreDriver.java
<https://reviews.apache.org/r/11479/#comment45032>
Why is this exception being logged to debug and not error?
plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/driver/SolidfirePrimaryDataStoreDriver.java
<https://reviews.apache.org/r/11479/#comment45030>
Readability: check this condition at the top of the method and bail out early to avoid large nested blocks.
plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/driver/SolidfirePrimaryDataStoreDriver.java
<https://reviews.apache.org/r/11479/#comment45033>
What's the resolution of this TODO?
plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/driver/SolidfirePrimaryDataStoreDriver.java
<https://reviews.apache.org/r/11479/#comment45034>
What's the resolution of this TODO?
plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/driver/SolidfirePrimaryDataStoreDriver.java
<https://reviews.apache.org/r/11479/#comment45035>
What's the resolution of this TODO?
plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/lifecycle/SolidFirePrimaryDataStoreLifeCycle.java
<https://reviews.apache.org/r/11479/#comment45037>
Why is the UUID being prepended with the provider name? Smells of magic value.
plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/lifecycle/SolidFirePrimaryDataStoreLifeCycle.java
<https://reviews.apache.org/r/11479/#comment45038>
Prefer IllegalStateException for this condition
plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/lifecycle/SolidFirePrimaryDataStoreLifeCycle.java
<https://reviews.apache.org/r/11479/#comment45040>
Why not use the URI class to parse out the parameters? This code could be broken by a malformed URL/URI. I recommend parsing into a java.net.URI on line 60.
plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/lifecycle/SolidFirePrimaryDataStoreLifeCycle.java
<https://reviews.apache.org/r/11479/#comment45041>
Consider using Guava's Splitter. It will make this code much more concise ...
plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/lifecycle/SolidFirePrimaryDataStoreLifeCycle.java
<https://reviews.apache.org/r/11479/#comment45042>
Consider using Guava's Splitter to grab out the value.
plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/lifecycle/SolidFirePrimaryDataStoreLifeCycle.java
<https://reviews.apache.org/r/11479/#comment45043>
Prefer IllegalStateException in this situation
plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/lifecycle/SolidFirePrimaryDataStoreLifeCycle.java
<https://reviews.apache.org/r/11479/#comment45044>
See previous comments about using java.net.URI.
plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/lifecycle/SolidFirePrimaryDataStoreLifeCycle.java
<https://reviews.apache.org/r/11479/#comment45045>
Prefer an explicit throws list to be more declarative
plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/provider/SolidfirePrimaryDataStoreProvider.java
<https://reviews.apache.org/r/11479/#comment45046>
Why aren't these values being injected via @Inject?
plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java
<https://reviews.apache.org/r/11479/#comment45047>
Implement equals, hashCode, and toString
plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java
<https://reviews.apache.org/r/11479/#comment45050>
Make these attributes as final since they can not be modified
plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java
<https://reviews.apache.org/r/11479/#comment45048>
Implement equals, hashCode, and toString
plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java
<https://reviews.apache.org/r/11479/#comment45049>
Mark attributes as final since they can not be modified
plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java
<https://reviews.apache.org/r/11479/#comment45051>
Cruft Prevention: If this class is unused (infered from the @SupressWarnings annotation) why it is in the codebase? Delete it if we don't need it.
plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java
<https://reviews.apache.org/r/11479/#comment45052>
Cruft Prevention: If this class is unused (infered from the @SupressWarnings annotation) why it is in the codebase? Delete it if we don't need it.
plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java
<https://reviews.apache.org/r/11479/#comment45053>
Cruft Prevention: If this class is unused (infered from the @SupressWarnings annotation) why it is in the codebase? Delete it if we don't need it.
plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java
<https://reviews.apache.org/r/11479/#comment45054>
Cruft Prevention: If this class is unused (infered from the @SupressWarnings annotation) why it is in the codebase? Delete it if we don't need it.
plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java
<https://reviews.apache.org/r/11479/#comment45055>
Cruft Prevention: If this class is unused (infered from the @SupressWarnings annotation) why it is in the codebase? Delete it if we don't need it.
plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java
<https://reviews.apache.org/r/11479/#comment45056>
Cruft Prevention: If this class is unused (infered from the @SupressWarnings annotation) why it is in the codebase? Delete it if we don't need it.
plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java
<https://reviews.apache.org/r/11479/#comment45057>
Cruft Prevention: If this class is unused (infered from the @SupressWarnings annotation) why it is in the codebase? Delete it if we don't need it.
plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java
<https://reviews.apache.org/r/11479/#comment45058>
What is the purpose of this class? All of the data is private and no methods are provided to access or modify it.
plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java
<https://reviews.apache.org/r/11479/#comment45059>
What is the purpose of this class? All of the data is private and no methods are provided to access or modify it.
plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java
<https://reviews.apache.org/r/11479/#comment45060>
What is the purpose of this class? All of the data is private and no methods are provided to access or modify it.
plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java
<https://reviews.apache.org/r/11479/#comment45061>
What is the purpose of this class? All of the data is private and no methods are provided to access or modify it.
plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java
<https://reviews.apache.org/r/11479/#comment45062>
What is the purpose of this class? All of the data is private and no methods are provided to access or modify it.
plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java
<https://reviews.apache.org/r/11479/#comment45063>
What is the purpose of this class? All of the data is private and no methods are provided to access or modify it.
plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java
<https://reviews.apache.org/r/11479/#comment45066>
Explicitly list the exceptions thrown
plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java
<https://reviews.apache.org/r/11479/#comment45064>
Extract 515-525 to a private factory method
plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java
<https://reviews.apache.org/r/11479/#comment45065>
By spec, any 20x status code represents success. Why is success being constrainted to 200? By spec, we should check >= 200 && < 300. I recommend creating a general utility method, isSuccess, to implement this check.
plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java
<https://reviews.apache.org/r/11479/#comment45068>
1) This resource management block should be in a finally block to ensure execution. Also, we want to ignore any exceptions from it.
2) We should be setting up an HttpConnection pool through HttpConnectionManager. The shutdown method kills the pool. This class should be fitted to create an HttpConnectionManager at startup and simply release the HttpConnections back to pool.
plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java
<https://reviews.apache.org/r/11479/#comment45069>
Prefer an IllegalStateException here
plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java
<https://reviews.apache.org/r/11479/#comment45071>
This method looks like it will also throw an exception. Is that correct?
server/src/com/cloud/configuration/ConfigurationManagerImpl.java
<https://reviews.apache.org/r/11479/#comment45072>
Is it permissable for minIOPS to be equal to maxIOPS?
server/src/com/cloud/storage/VolumeManagerImpl.java
<https://reviews.apache.org/r/11479/#comment45074>
Don't use the Long constructor because it avoid the constant pool resulting in memory bloat and garbage collector stress. Use Long.valueOf or, even better, just use 0L and let the compiler autobox the value.
server/src/com/cloud/storage/VolumeManagerImpl.java
<https://reviews.apache.org/r/11479/#comment45075>
Check >= 0 to be completely defensive.
server/src/com/cloud/storage/VolumeManagerImpl.java
<https://reviews.apache.org/r/11479/#comment45076>
Check >= 0 to be completely defensive.
- John Burwell
On May 31, 2013, 9:18 p.m., Mike Tutkowski wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11479/
> -----------------------------------------------------------
>
> (Updated May 31, 2013, 9:18 p.m.)
>
>
> Review request for cloudstack, edison su and John Burwell.
>
>
> Description
> -------
>
> This patch implements a storage plug-in for SolidFire. The plug-in is based on the new storage framework that went in with 4.2, as well.
>
> In addition, there are GUI (and related) changes to enable admins and end users to specify a Min, Max, and Burst number of IOPS for a Disk Offering. These fields (although optional) tend to follow the pattern previously established for the Disk Size field.
>
> Also, the storage framework itself has been enhanced. For example, it now supports creating and deleting storage repositories as is necessary for a dynamic type of zone-wide primary storage (such as the SolidFire plug-in is).
>
> The desired behavior of the software is as such:
>
> * Allow an admin to invoke the CloudStack API to add Primary Storage based on the SolidFire plug-in.
>
> * Allow an admin to create a Disk Offering that specifies a Min, Max, and Burst number of IOPS or allows the admin to pass this ability on to the end user.
>
> * Allow an end user to execute such a Disk Offering. As is the case for any Disk Offering, this leads to the creation of a row in the volumes table.
>
> * Allow an end user to attach the resultant volume (noted in the DB) to a VM. The storage framework invokes logic in the plug-in and the plug-in creates a volume on its SAN with the correct size and IOPS values. The agent software for XenServer detects that such an attach is being requested and creates a Storage Repository (and single VDI within the SR) based on the storage IP address of the SAN and the IQN of the volume. The VDI is then hooked up to the VM.
>
> * Allow an end user to detach the volume. This leads to the destruction of the SR, but the SAN volume remains intact and can be reattached later to any VM running in a XenServer resource pool in the zone.
>
> * Allow an end user to delete the volume. This leads to the volume being deleted on the SAN and being marked in the CloudStack DB as deleted.
>
>
> Diffs
> -----
>
> api/src/com/cloud/offering/DiskOffering.java dd77c70
> api/src/com/cloud/storage/Storage.java c130fe2
> api/src/com/cloud/storage/Volume.java 4903594
> api/src/org/apache/cloudstack/api/ApiConstants.java 1e9435f
> api/src/org/apache/cloudstack/api/command/admin/offering/CreateDiskOfferingCmd.java aa11599
> api/src/org/apache/cloudstack/api/command/user/volume/CreateVolumeCmd.java 86a494b
> api/src/org/apache/cloudstack/api/response/DiskOfferingResponse.java 377e66e
> api/src/org/apache/cloudstack/api/response/VolumeResponse.java 21d7d1a
> client/WEB-INF/classes/resources/messages.properties 1638be1
> client/pom.xml 0c38ecb
> client/tomcatconf/applicationContext.xml.in edf83a9
> core/src/com/cloud/agent/api/AttachVolumeCommand.java 302b8f8
> core/test/org/apache/cloudstack/api/agent/test/AttachVolumeAnswerTest.java 251a6cb
> core/test/org/apache/cloudstack/api/agent/test/AttachVolumeCommandTest.java 1ec416a
> engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/PrimaryDataStoreParameters.java b2b787c
> engine/schema/src/com/cloud/storage/DiskOfferingVO.java 909d7fe
> engine/schema/src/com/cloud/storage/VolumeVO.java 1699afd
> engine/storage/integration-test/test/org/apache/cloudstack/storage/allocator/StorageAllocatorTest.java 9444fa5
> engine/storage/src/org/apache/cloudstack/storage/allocator/ZoneWideStoragePoolAllocator.java e976980
> engine/storage/src/org/apache/cloudstack/storage/volume/datastore/PrimaryDataStoreHelper.java 5f8daf4
> engine/storage/volume/src/org/apache/cloudstack/storage/volume/VolumeObject.java ea31be3
> plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java 4680fde
> plugins/storage/volume/solidfire/pom.xml 9db0685
> plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/driver/SolidfirePrimaryDataStoreDriver.java f31126c
> plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/lifecycle/SolidFirePrimaryDataStoreLifeCycle.java PRE-CREATION
> plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/provider/SolidfirePrimaryDataStoreProvider.java 650cac8
> plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java PRE-CREATION
> server/src/com/cloud/api/query/dao/DiskOfferingJoinDaoImpl.java 283181f
> server/src/com/cloud/api/query/dao/VolumeJoinDaoImpl.java e27e2d9
> server/src/com/cloud/api/query/vo/DiskOfferingJoinVO.java 6d3cdcb
> server/src/com/cloud/api/query/vo/VolumeJoinVO.java 6ef8c91
> server/src/com/cloud/configuration/ConfigurationManager.java 8db037b
> server/src/com/cloud/configuration/ConfigurationManagerImpl.java 8baf1fb
> server/src/com/cloud/server/ConfigurationServerImpl.java bc52e9a
> server/src/com/cloud/storage/VolumeManager.java d198e5d
> server/src/com/cloud/storage/VolumeManagerImpl.java 4b654eb
> server/src/com/cloud/test/DatabaseConfig.java 70c8178
> server/test/com/cloud/vpc/MockConfigurationManagerImpl.java 21b3590
> setup/db/db/schema-410to420.sql cf4e98d
> tools/marvin/marvin/cloudstackConnection.py b092ef0
> ui/dictionary.jsp ded9ea0
> ui/scripts/configuration.js 211d7b7
> ui/scripts/docs.js 7c1aaf8
> ui/scripts/storage.js e816334
> ui/scripts/system.js 8b9a81f
>
> Diff: https://reviews.apache.org/r/11479/diff/
>
>
> Testing
> -------
>
> Manual testing has been performed:
>
> A plug-in can be added to CloudStack via an API call.
>
> Create a Disk Offering where the admin specifies both Disk Size and Disk IOPS. Disk IOPS are left blank.
> Create a Disk Offering where the admin allows the end user to customize both Disk Size and Disk IOPS. Disk IOPS are left blank.
> Create a Disk Offering where the admin specifies Disk Size, but allows the end user to customize Disk IOPS. Disk IOPS are left blank.
> Create a Disk Offering where the admin specifies Disk IOPS, but allows the end user to customize Disk Size. Disk IOPS are left blank.
>
> Create a Disk Offering where the admin specifies both Disk Size and Disk IOPS. Disk IOPS are all filled in.
> Create a Disk Offering where the admin allows the end user to customize both Disk Size and Disk IOPS. Disk IOPS are all filled in.
> Create a Disk Offering where the admin specifies Disk Size, but allows the end user to customize Disk IOPS. Disk IOPS are all filled in.
> Create a Disk Offering where the admin specifies Disk IOPS, but allows the end user to customize Disk Size. Disk IOPS are all filled in.
>
> A newly created volume is attached to a VM for the first time and an SR and VDI are created.
> This volume is detached and the VDI and SR are deleted.
> The volume is reattached and is properly introduced into the SR and VDI ("introduced" in the sense that the data that was previously on the volume is not destroyed upon reattach).
> This volume is detached and the VDI and SR are deleted.
>
>
> Thanks,
>
> Mike Tutkowski
>
>
Re: Review Request: SolidFire storage plug-in and enhancements to the
storage framework and GUI
Posted by Mike Tutkowski <mi...@solidfire.com>.
> On June 12, 2013, 11:15 p.m., John Burwell wrote:
> > api/src/com/cloud/offering/DiskOffering.java, line 70
> > <https://reviews.apache.org/r/11479/diff/1/?file=297861#file297861line70>
> >
> > Is the notion of burst IOPS standard across QoS storage systems or specific to a product?
We decided to defer Burst IOPS to CS 4.3, where a more general-purpose approach can be implemented.
> On June 12, 2013, 11:15 p.m., John Burwell wrote:
> > api/src/com/cloud/storage/Storage.java, line 107
> > <https://reviews.apache.org/r/11479/diff/1/?file=297862#file297862line107>
> >
> > Coding standards are that enumeration values are all caps.
I have removed this field from the codebase in favor of another approach.
> On June 12, 2013, 11:15 p.m., John Burwell wrote:
> > plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java, line 6182
> > <https://reviews.apache.org/r/11479/diff/1/?file=297882#file297882line6182>
> >
> > Why did this method change from taking a domain object to a taking discrete values? Is this information not available on the StorageFilerTO?
getIscsiSR was initially written with the assumption that it would create an SR based on information that is exclusively contained in a row in the storage_pool table. With the new storage plug-in framework and a 1:1 mapping between a SAN volume and an SR, information from the volumes table needed to be passed in when required.
I investigated what getIscsiSR was using from StorageFilerTO and passed those values in instead. This way I could re-use getIscsiSR with info from the volumes table when needed.
> On June 12, 2013, 11:15 p.m., John Burwell wrote:
> > plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java, line 6486
> > <https://reviews.apache.org/r/11479/diff/1/?file=297882#file297882line6486>
> >
> > As discussed on the list, the intention is to remove the StoragePoolType.Dynamic in favor of a VolumeTO#isManaged check. When this method becomes available, extracting the allocation of the underlying storage to a private method would make things a good bit more readable.
No need for StoragePoolType.Dynamic anymore
> On June 12, 2013, 11:15 p.m., John Burwell wrote:
> > plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java, line 6500
> > <https://reviews.apache.org/r/11479/diff/1/?file=297882#file297882line6500>
> >
> > 1. Extract the 100000 value to a constant
> > 2. Should this slack value be a global configuration setting?
I have been able to determine approximately how much metadata per gigabyte Xen uses for an SR. I've removed this value and replaced it with a method that performs a more general calculation.
> On June 12, 2013, 11:15 p.m., John Burwell wrote:
> > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/driver/SolidfirePrimaryDataStoreDriver.java, line 80
> > <https://reviews.apache.org/r/11479/diff/1/?file=297884#file297884line80>
> >
> > Why is the UUID being prepended with the provider name? Smells of magic value ...
Edison recommended I create any value for this UUID that - given the same inputs - would lead to the same value.
I just prepended it with "SolidFire_" to make it easier to locate such plug-ins in the storage_pool table.
After submitting this code, I did realize admins might want more than one SolidFire SAN in a CS zone, so I've changed this UUID to permit that.
> On June 12, 2013, 11:15 p.m., John Burwell wrote:
> > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/driver/SolidfirePrimaryDataStoreDriver.java, line 300
> > <https://reviews.apache.org/r/11479/diff/1/?file=297884#file297884line300>
> >
> > What's the resolution of this TODO?
Removed the line
> On June 12, 2013, 11:15 p.m., John Burwell wrote:
> > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/driver/SolidfirePrimaryDataStoreDriver.java, line 307
> > <https://reviews.apache.org/r/11479/diff/1/?file=297884#file297884line307>
> >
> > What's the resolution of this TODO?
Removed the line
> On June 12, 2013, 11:15 p.m., John Burwell wrote:
> > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/driver/SolidfirePrimaryDataStoreDriver.java, line 314
> > <https://reviews.apache.org/r/11479/diff/1/?file=297884#file297884line314>
> >
> > What's the resolution of this TODO?
Removed the line
> On June 12, 2013, 11:15 p.m., John Burwell wrote:
> > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/lifecycle/SolidFirePrimaryDataStoreLifeCycle.java, line 72
> > <https://reviews.apache.org/r/11479/diff/1/?file=297885#file297885line72>
> >
> > Why is the UUID being prepended with the provider name? Smells of magic value.
Edison recommended I create any value for this UUID that - given the same inputs - would lead to the same value.
I just prepended it with "SolidFire_" to make it easier to locate such plug-ins in the storage_pool table.
After submitting this code, I did realize admins might want more than one SolidFire SAN in a CS zone, so I've changed this UUID to permit that.
> On June 12, 2013, 11:15 p.m., John Burwell wrote:
> > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java, line 434
> > <https://reviews.apache.org/r/11479/diff/1/?file=297887#file297887line434>
> >
> > What is the purpose of this class? All of the data is private and no methods are provided to access or modify it.
It is used by Gson to parse JSON into a Java class that is set up to accept the appropriate format.
> On June 12, 2013, 11:15 p.m., John Burwell wrote:
> > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java, line 444
> > <https://reviews.apache.org/r/11479/diff/1/?file=297887#file297887line444>
> >
> > What is the purpose of this class? All of the data is private and no methods are provided to access or modify it.
It is used by Gson to parse JSON into a Java class that is set up to accept the appropriate format.
> On June 12, 2013, 11:15 p.m., John Burwell wrote:
> > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java, line 461
> > <https://reviews.apache.org/r/11479/diff/1/?file=297887#file297887line461>
> >
> > What is the purpose of this class? All of the data is private and no methods are provided to access or modify it.
It is used by Gson to parse JSON into a Java class that is set up to accept the appropriate format.
> On June 12, 2013, 11:15 p.m., John Burwell wrote:
> > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java, line 471
> > <https://reviews.apache.org/r/11479/diff/1/?file=297887#file297887line471>
> >
> > What is the purpose of this class? All of the data is private and no methods are provided to access or modify it.
It is used by Gson to parse JSON into a Java class that is set up to accept the appropriate format.
> On June 12, 2013, 11:15 p.m., John Burwell wrote:
> > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java, line 488
> > <https://reviews.apache.org/r/11479/diff/1/?file=297887#file297887line488>
> >
> > What is the purpose of this class? All of the data is private and no methods are provided to access or modify it.
It is used by Gson to parse JSON into a Java class that is set up to accept the appropriate format.
> On June 12, 2013, 11:15 p.m., John Burwell wrote:
> > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java, line 498
> > <https://reviews.apache.org/r/11479/diff/1/?file=297887#file297887line498>
> >
> > What is the purpose of this class? All of the data is private and no methods are provided to access or modify it.
It is used by Gson to parse JSON into a Java class that is set up to accept the appropriate format.
> On June 12, 2013, 11:15 p.m., John Burwell wrote:
> > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java, line 570
> > <https://reviews.apache.org/r/11479/diff/1/?file=297887#file297887line570>
> >
> > This method looks like it will also throw an exception. Is that correct?
That is correct. It has a "throws Exception" clause.
> On June 12, 2013, 11:15 p.m., John Burwell wrote:
> > server/src/com/cloud/configuration/ConfigurationManagerImpl.java, line 2216
> > <https://reviews.apache.org/r/11479/diff/1/?file=297893#file297893line2216>
> >
> > Is it permissable for minIOPS to be equal to maxIOPS?
Yes
> On June 12, 2013, 11:15 p.m., John Burwell wrote:
> > server/src/com/cloud/storage/VolumeManagerImpl.java, line 897
> > <https://reviews.apache.org/r/11479/diff/1/?file=297896#file297896line897>
> >
> > Check >= 0 to be completely defensive.
I believe you meant minIops <= 0 to be completely defensive.
Either way, this logic had been changed since I submitted the code for review and this issue is no longer applicable.
> On June 12, 2013, 11:15 p.m., John Burwell wrote:
> > server/src/com/cloud/storage/VolumeManagerImpl.java, line 905
> > <https://reviews.apache.org/r/11479/diff/1/?file=297896#file297896line905>
> >
> > Check >= 0 to be completely defensive.
I believe you meant minIops <= 0 to be completely defensive.
Either way, this logic had been changed since I submitted the code for review and this issue is no longer applicable.
- Mike
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11479/#review21347
-----------------------------------------------------------
On May 31, 2013, 9:18 p.m., Mike Tutkowski wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11479/
> -----------------------------------------------------------
>
> (Updated May 31, 2013, 9:18 p.m.)
>
>
> Review request for cloudstack, edison su and John Burwell.
>
>
> Description
> -------
>
> This patch implements a storage plug-in for SolidFire. The plug-in is based on the new storage framework that went in with 4.2, as well.
>
> In addition, there are GUI (and related) changes to enable admins and end users to specify a Min, Max, and Burst number of IOPS for a Disk Offering. These fields (although optional) tend to follow the pattern previously established for the Disk Size field.
>
> Also, the storage framework itself has been enhanced. For example, it now supports creating and deleting storage repositories as is necessary for a dynamic type of zone-wide primary storage (such as the SolidFire plug-in is).
>
> The desired behavior of the software is as such:
>
> * Allow an admin to invoke the CloudStack API to add Primary Storage based on the SolidFire plug-in.
>
> * Allow an admin to create a Disk Offering that specifies a Min, Max, and Burst number of IOPS or allows the admin to pass this ability on to the end user.
>
> * Allow an end user to execute such a Disk Offering. As is the case for any Disk Offering, this leads to the creation of a row in the volumes table.
>
> * Allow an end user to attach the resultant volume (noted in the DB) to a VM. The storage framework invokes logic in the plug-in and the plug-in creates a volume on its SAN with the correct size and IOPS values. The agent software for XenServer detects that such an attach is being requested and creates a Storage Repository (and single VDI within the SR) based on the storage IP address of the SAN and the IQN of the volume. The VDI is then hooked up to the VM.
>
> * Allow an end user to detach the volume. This leads to the destruction of the SR, but the SAN volume remains intact and can be reattached later to any VM running in a XenServer resource pool in the zone.
>
> * Allow an end user to delete the volume. This leads to the volume being deleted on the SAN and being marked in the CloudStack DB as deleted.
>
>
> Diffs
> -----
>
> api/src/com/cloud/offering/DiskOffering.java dd77c70
> api/src/com/cloud/storage/Storage.java c130fe2
> api/src/com/cloud/storage/Volume.java 4903594
> api/src/org/apache/cloudstack/api/ApiConstants.java 1e9435f
> api/src/org/apache/cloudstack/api/command/admin/offering/CreateDiskOfferingCmd.java aa11599
> api/src/org/apache/cloudstack/api/command/user/volume/CreateVolumeCmd.java 86a494b
> api/src/org/apache/cloudstack/api/response/DiskOfferingResponse.java 377e66e
> api/src/org/apache/cloudstack/api/response/VolumeResponse.java 21d7d1a
> client/WEB-INF/classes/resources/messages.properties 1638be1
> client/pom.xml 0c38ecb
> client/tomcatconf/applicationContext.xml.in edf83a9
> core/src/com/cloud/agent/api/AttachVolumeCommand.java 302b8f8
> core/test/org/apache/cloudstack/api/agent/test/AttachVolumeAnswerTest.java 251a6cb
> core/test/org/apache/cloudstack/api/agent/test/AttachVolumeCommandTest.java 1ec416a
> engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/PrimaryDataStoreParameters.java b2b787c
> engine/schema/src/com/cloud/storage/DiskOfferingVO.java 909d7fe
> engine/schema/src/com/cloud/storage/VolumeVO.java 1699afd
> engine/storage/integration-test/test/org/apache/cloudstack/storage/allocator/StorageAllocatorTest.java 9444fa5
> engine/storage/src/org/apache/cloudstack/storage/allocator/ZoneWideStoragePoolAllocator.java e976980
> engine/storage/src/org/apache/cloudstack/storage/volume/datastore/PrimaryDataStoreHelper.java 5f8daf4
> engine/storage/volume/src/org/apache/cloudstack/storage/volume/VolumeObject.java ea31be3
> plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java 4680fde
> plugins/storage/volume/solidfire/pom.xml 9db0685
> plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/driver/SolidfirePrimaryDataStoreDriver.java f31126c
> plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/lifecycle/SolidFirePrimaryDataStoreLifeCycle.java PRE-CREATION
> plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/provider/SolidfirePrimaryDataStoreProvider.java 650cac8
> plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java PRE-CREATION
> server/src/com/cloud/api/query/dao/DiskOfferingJoinDaoImpl.java 283181f
> server/src/com/cloud/api/query/dao/VolumeJoinDaoImpl.java e27e2d9
> server/src/com/cloud/api/query/vo/DiskOfferingJoinVO.java 6d3cdcb
> server/src/com/cloud/api/query/vo/VolumeJoinVO.java 6ef8c91
> server/src/com/cloud/configuration/ConfigurationManager.java 8db037b
> server/src/com/cloud/configuration/ConfigurationManagerImpl.java 8baf1fb
> server/src/com/cloud/server/ConfigurationServerImpl.java bc52e9a
> server/src/com/cloud/storage/VolumeManager.java d198e5d
> server/src/com/cloud/storage/VolumeManagerImpl.java 4b654eb
> server/src/com/cloud/test/DatabaseConfig.java 70c8178
> server/test/com/cloud/vpc/MockConfigurationManagerImpl.java 21b3590
> setup/db/db/schema-410to420.sql cf4e98d
> tools/marvin/marvin/cloudstackConnection.py b092ef0
> ui/dictionary.jsp ded9ea0
> ui/scripts/configuration.js 211d7b7
> ui/scripts/docs.js 7c1aaf8
> ui/scripts/storage.js e816334
> ui/scripts/system.js 8b9a81f
>
> Diff: https://reviews.apache.org/r/11479/diff/
>
>
> Testing
> -------
>
> Manual testing has been performed:
>
> A plug-in can be added to CloudStack via an API call.
>
> Create a Disk Offering where the admin specifies both Disk Size and Disk IOPS. Disk IOPS are left blank.
> Create a Disk Offering where the admin allows the end user to customize both Disk Size and Disk IOPS. Disk IOPS are left blank.
> Create a Disk Offering where the admin specifies Disk Size, but allows the end user to customize Disk IOPS. Disk IOPS are left blank.
> Create a Disk Offering where the admin specifies Disk IOPS, but allows the end user to customize Disk Size. Disk IOPS are left blank.
>
> Create a Disk Offering where the admin specifies both Disk Size and Disk IOPS. Disk IOPS are all filled in.
> Create a Disk Offering where the admin allows the end user to customize both Disk Size and Disk IOPS. Disk IOPS are all filled in.
> Create a Disk Offering where the admin specifies Disk Size, but allows the end user to customize Disk IOPS. Disk IOPS are all filled in.
> Create a Disk Offering where the admin specifies Disk IOPS, but allows the end user to customize Disk Size. Disk IOPS are all filled in.
>
> A newly created volume is attached to a VM for the first time and an SR and VDI are created.
> This volume is detached and the VDI and SR are deleted.
> The volume is reattached and is properly introduced into the SR and VDI ("introduced" in the sense that the data that was previously on the volume is not destroyed upon reattach).
> This volume is detached and the VDI and SR are deleted.
>
>
> Thanks,
>
> Mike Tutkowski
>
>
Re: Review Request: SolidFire storage plug-in and enhancements to the
storage framework and GUI
Posted by Mike Tutkowski <mi...@solidfire.com>.
> On June 12, 2013, 11:15 p.m., John Burwell wrote:
> > plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java, line 6494
> > <https://reviews.apache.org/r/11479/diff/1/?file=297882#file297882line6494>
> >
> > To reduce the conditional nesting, check if (!created[0]), perform the associated work, and return -- then proceed with the true case.
I did some refactoring before the code review was available on Review Board. I think this issue has been addressed.
> On June 12, 2013, 11:15 p.m., John Burwell wrote:
> > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java, line 232
> > <https://reviews.apache.org/r/11479/diff/1/?file=297887#file297887line232>
> >
> > Cruft Prevention: If this class is unused (infered from the @SupressWarnings annotation) why it is in the codebase? Delete it if we don't need it.
This class is used by the Gson library for JSON purposes.
Since Eclipse cannot see certain variables being used, it marks them as not in use. They actually are needed by Gson, so I put in the @SuppressWarnings.
> On June 12, 2013, 11:15 p.m., John Burwell wrote:
> > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java, line 280
> > <https://reviews.apache.org/r/11479/diff/1/?file=297887#file297887line280>
> >
> > Cruft Prevention: If this class is unused (infered from the @SupressWarnings annotation) why it is in the codebase? Delete it if we don't need it.
This class is used by the Gson library for JSON purposes.
Since Eclipse cannot see certain variables being used, it marks them as not in use. They actually are needed by Gson, so I put in the @SuppressWarnings.
> On June 12, 2013, 11:15 p.m., John Burwell wrote:
> > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java, line 325
> > <https://reviews.apache.org/r/11479/diff/1/?file=297887#file297887line325>
> >
> > Cruft Prevention: If this class is unused (infered from the @SupressWarnings annotation) why it is in the codebase? Delete it if we don't need it.
This class is used by the Gson library for JSON purposes.
Since Eclipse cannot see certain variables being used, it marks them as not in use. They actually are needed by Gson, so I put in the @SuppressWarnings.
> On June 12, 2013, 11:15 p.m., John Burwell wrote:
> > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java, line 347
> > <https://reviews.apache.org/r/11479/diff/1/?file=297887#file297887line347>
> >
> > Cruft Prevention: If this class is unused (infered from the @SupressWarnings annotation) why it is in the codebase? Delete it if we don't need it.
This class is used by the Gson library for JSON purposes.
Since Eclipse cannot see certain variables being used, it marks them as not in use. They actually are needed by Gson, so I put in the @SuppressWarnings.
> On June 12, 2013, 11:15 p.m., John Burwell wrote:
> > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java, line 369
> > <https://reviews.apache.org/r/11479/diff/1/?file=297887#file297887line369>
> >
> > Cruft Prevention: If this class is unused (infered from the @SupressWarnings annotation) why it is in the codebase? Delete it if we don't need it.
This class is used by the Gson library for JSON purposes.
Since Eclipse cannot see certain variables being used, it marks them as not in use. They actually are needed by Gson, so I put in the @SuppressWarnings.
> On June 12, 2013, 11:15 p.m., John Burwell wrote:
> > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java, line 391
> > <https://reviews.apache.org/r/11479/diff/1/?file=297887#file297887line391>
> >
> > Cruft Prevention: If this class is unused (infered from the @SupressWarnings annotation) why it is in the codebase? Delete it if we don't need it.
This class is used by the Gson library for JSON purposes.
Since Eclipse cannot see certain variables being used, it marks them as not in use. They actually are needed by Gson, so I put in the @SuppressWarnings.
> On June 12, 2013, 11:15 p.m., John Burwell wrote:
> > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java, line 413
> > <https://reviews.apache.org/r/11479/diff/1/?file=297887#file297887line413>
> >
> > Cruft Prevention: If this class is unused (infered from the @SupressWarnings annotation) why it is in the codebase? Delete it if we don't need it.
This class is used by the Gson library for JSON purposes.
Since Eclipse cannot see certain variables being used, it marks them as not in use. They actually are needed by Gson, so I put in the @SuppressWarnings.
- Mike
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11479/#review21347
-----------------------------------------------------------
On May 31, 2013, 9:18 p.m., Mike Tutkowski wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11479/
> -----------------------------------------------------------
>
> (Updated May 31, 2013, 9:18 p.m.)
>
>
> Review request for cloudstack, edison su and John Burwell.
>
>
> Description
> -------
>
> This patch implements a storage plug-in for SolidFire. The plug-in is based on the new storage framework that went in with 4.2, as well.
>
> In addition, there are GUI (and related) changes to enable admins and end users to specify a Min, Max, and Burst number of IOPS for a Disk Offering. These fields (although optional) tend to follow the pattern previously established for the Disk Size field.
>
> Also, the storage framework itself has been enhanced. For example, it now supports creating and deleting storage repositories as is necessary for a dynamic type of zone-wide primary storage (such as the SolidFire plug-in is).
>
> The desired behavior of the software is as such:
>
> * Allow an admin to invoke the CloudStack API to add Primary Storage based on the SolidFire plug-in.
>
> * Allow an admin to create a Disk Offering that specifies a Min, Max, and Burst number of IOPS or allows the admin to pass this ability on to the end user.
>
> * Allow an end user to execute such a Disk Offering. As is the case for any Disk Offering, this leads to the creation of a row in the volumes table.
>
> * Allow an end user to attach the resultant volume (noted in the DB) to a VM. The storage framework invokes logic in the plug-in and the plug-in creates a volume on its SAN with the correct size and IOPS values. The agent software for XenServer detects that such an attach is being requested and creates a Storage Repository (and single VDI within the SR) based on the storage IP address of the SAN and the IQN of the volume. The VDI is then hooked up to the VM.
>
> * Allow an end user to detach the volume. This leads to the destruction of the SR, but the SAN volume remains intact and can be reattached later to any VM running in a XenServer resource pool in the zone.
>
> * Allow an end user to delete the volume. This leads to the volume being deleted on the SAN and being marked in the CloudStack DB as deleted.
>
>
> Diffs
> -----
>
> api/src/com/cloud/offering/DiskOffering.java dd77c70
> api/src/com/cloud/storage/Storage.java c130fe2
> api/src/com/cloud/storage/Volume.java 4903594
> api/src/org/apache/cloudstack/api/ApiConstants.java 1e9435f
> api/src/org/apache/cloudstack/api/command/admin/offering/CreateDiskOfferingCmd.java aa11599
> api/src/org/apache/cloudstack/api/command/user/volume/CreateVolumeCmd.java 86a494b
> api/src/org/apache/cloudstack/api/response/DiskOfferingResponse.java 377e66e
> api/src/org/apache/cloudstack/api/response/VolumeResponse.java 21d7d1a
> client/WEB-INF/classes/resources/messages.properties 1638be1
> client/pom.xml 0c38ecb
> client/tomcatconf/applicationContext.xml.in edf83a9
> core/src/com/cloud/agent/api/AttachVolumeCommand.java 302b8f8
> core/test/org/apache/cloudstack/api/agent/test/AttachVolumeAnswerTest.java 251a6cb
> core/test/org/apache/cloudstack/api/agent/test/AttachVolumeCommandTest.java 1ec416a
> engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/PrimaryDataStoreParameters.java b2b787c
> engine/schema/src/com/cloud/storage/DiskOfferingVO.java 909d7fe
> engine/schema/src/com/cloud/storage/VolumeVO.java 1699afd
> engine/storage/integration-test/test/org/apache/cloudstack/storage/allocator/StorageAllocatorTest.java 9444fa5
> engine/storage/src/org/apache/cloudstack/storage/allocator/ZoneWideStoragePoolAllocator.java e976980
> engine/storage/src/org/apache/cloudstack/storage/volume/datastore/PrimaryDataStoreHelper.java 5f8daf4
> engine/storage/volume/src/org/apache/cloudstack/storage/volume/VolumeObject.java ea31be3
> plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java 4680fde
> plugins/storage/volume/solidfire/pom.xml 9db0685
> plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/driver/SolidfirePrimaryDataStoreDriver.java f31126c
> plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/lifecycle/SolidFirePrimaryDataStoreLifeCycle.java PRE-CREATION
> plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/provider/SolidfirePrimaryDataStoreProvider.java 650cac8
> plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java PRE-CREATION
> server/src/com/cloud/api/query/dao/DiskOfferingJoinDaoImpl.java 283181f
> server/src/com/cloud/api/query/dao/VolumeJoinDaoImpl.java e27e2d9
> server/src/com/cloud/api/query/vo/DiskOfferingJoinVO.java 6d3cdcb
> server/src/com/cloud/api/query/vo/VolumeJoinVO.java 6ef8c91
> server/src/com/cloud/configuration/ConfigurationManager.java 8db037b
> server/src/com/cloud/configuration/ConfigurationManagerImpl.java 8baf1fb
> server/src/com/cloud/server/ConfigurationServerImpl.java bc52e9a
> server/src/com/cloud/storage/VolumeManager.java d198e5d
> server/src/com/cloud/storage/VolumeManagerImpl.java 4b654eb
> server/src/com/cloud/test/DatabaseConfig.java 70c8178
> server/test/com/cloud/vpc/MockConfigurationManagerImpl.java 21b3590
> setup/db/db/schema-410to420.sql cf4e98d
> tools/marvin/marvin/cloudstackConnection.py b092ef0
> ui/dictionary.jsp ded9ea0
> ui/scripts/configuration.js 211d7b7
> ui/scripts/docs.js 7c1aaf8
> ui/scripts/storage.js e816334
> ui/scripts/system.js 8b9a81f
>
> Diff: https://reviews.apache.org/r/11479/diff/
>
>
> Testing
> -------
>
> Manual testing has been performed:
>
> A plug-in can be added to CloudStack via an API call.
>
> Create a Disk Offering where the admin specifies both Disk Size and Disk IOPS. Disk IOPS are left blank.
> Create a Disk Offering where the admin allows the end user to customize both Disk Size and Disk IOPS. Disk IOPS are left blank.
> Create a Disk Offering where the admin specifies Disk Size, but allows the end user to customize Disk IOPS. Disk IOPS are left blank.
> Create a Disk Offering where the admin specifies Disk IOPS, but allows the end user to customize Disk Size. Disk IOPS are left blank.
>
> Create a Disk Offering where the admin specifies both Disk Size and Disk IOPS. Disk IOPS are all filled in.
> Create a Disk Offering where the admin allows the end user to customize both Disk Size and Disk IOPS. Disk IOPS are all filled in.
> Create a Disk Offering where the admin specifies Disk Size, but allows the end user to customize Disk IOPS. Disk IOPS are all filled in.
> Create a Disk Offering where the admin specifies Disk IOPS, but allows the end user to customize Disk Size. Disk IOPS are all filled in.
>
> A newly created volume is attached to a VM for the first time and an SR and VDI are created.
> This volume is detached and the VDI and SR are deleted.
> The volume is reattached and is properly introduced into the SR and VDI ("introduced" in the sense that the data that was previously on the volume is not destroyed upon reattach).
> This volume is detached and the VDI and SR are deleted.
>
>
> Thanks,
>
> Mike Tutkowski
>
>
Re: Review Request: SolidFire storage plug-in and enhancements to the
storage framework and GUI
Posted by Mike Tutkowski <mi...@solidfire.com>.
> On June 12, 2013, 11:15 p.m., John Burwell wrote:
> > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/lifecycle/SolidFirePrimaryDataStoreLifeCycle.java, line 131
> > <https://reviews.apache.org/r/11479/diff/1/?file=297885#file297885line131>
> >
> > Why not use the URI class to parse out the parameters? This code could be broken by a malformed URL/URI. I recommend parsing into a java.net.URI on line 60.
Yeah, it's called a URL, but it's really not a URL. The parameter where you pass in vendor-specific info to your plug-in from the API is called url and its contents are what I'm parsing here. Its contents are really just name/value pairs.
> On June 12, 2013, 11:15 p.m., John Burwell wrote:
> > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/lifecycle/SolidFirePrimaryDataStoreLifeCycle.java, line 184
> > <https://reviews.apache.org/r/11479/diff/1/?file=297885#file297885line184>
> >
> > Consider using Guava's Splitter. It will make this code much more concise ...
Noted to investigate
> On June 12, 2013, 11:15 p.m., John Burwell wrote:
> > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/lifecycle/SolidFirePrimaryDataStoreLifeCycle.java, line 196
> > <https://reviews.apache.org/r/11479/diff/1/?file=297885#file297885line196>
> >
> > Consider using Guava's Splitter to grab out the value.
Noted to investigate
> On June 12, 2013, 11:15 p.m., John Burwell wrote:
> > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/lifecycle/SolidFirePrimaryDataStoreLifeCycle.java, line 218
> > <https://reviews.apache.org/r/11479/diff/1/?file=297885#file297885line218>
> >
> > See previous comments about using java.net.URI.
Really just key/value pairs. The name url comes from the API where the plug-in-specific info that's passed to the driver is called this.
- Mike
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11479/#review21347
-----------------------------------------------------------
On May 31, 2013, 9:18 p.m., Mike Tutkowski wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11479/
> -----------------------------------------------------------
>
> (Updated May 31, 2013, 9:18 p.m.)
>
>
> Review request for cloudstack, edison su and John Burwell.
>
>
> Description
> -------
>
> This patch implements a storage plug-in for SolidFire. The plug-in is based on the new storage framework that went in with 4.2, as well.
>
> In addition, there are GUI (and related) changes to enable admins and end users to specify a Min, Max, and Burst number of IOPS for a Disk Offering. These fields (although optional) tend to follow the pattern previously established for the Disk Size field.
>
> Also, the storage framework itself has been enhanced. For example, it now supports creating and deleting storage repositories as is necessary for a dynamic type of zone-wide primary storage (such as the SolidFire plug-in is).
>
> The desired behavior of the software is as such:
>
> * Allow an admin to invoke the CloudStack API to add Primary Storage based on the SolidFire plug-in.
>
> * Allow an admin to create a Disk Offering that specifies a Min, Max, and Burst number of IOPS or allows the admin to pass this ability on to the end user.
>
> * Allow an end user to execute such a Disk Offering. As is the case for any Disk Offering, this leads to the creation of a row in the volumes table.
>
> * Allow an end user to attach the resultant volume (noted in the DB) to a VM. The storage framework invokes logic in the plug-in and the plug-in creates a volume on its SAN with the correct size and IOPS values. The agent software for XenServer detects that such an attach is being requested and creates a Storage Repository (and single VDI within the SR) based on the storage IP address of the SAN and the IQN of the volume. The VDI is then hooked up to the VM.
>
> * Allow an end user to detach the volume. This leads to the destruction of the SR, but the SAN volume remains intact and can be reattached later to any VM running in a XenServer resource pool in the zone.
>
> * Allow an end user to delete the volume. This leads to the volume being deleted on the SAN and being marked in the CloudStack DB as deleted.
>
>
> Diffs
> -----
>
> api/src/com/cloud/offering/DiskOffering.java dd77c70
> api/src/com/cloud/storage/Storage.java c130fe2
> api/src/com/cloud/storage/Volume.java 4903594
> api/src/org/apache/cloudstack/api/ApiConstants.java 1e9435f
> api/src/org/apache/cloudstack/api/command/admin/offering/CreateDiskOfferingCmd.java aa11599
> api/src/org/apache/cloudstack/api/command/user/volume/CreateVolumeCmd.java 86a494b
> api/src/org/apache/cloudstack/api/response/DiskOfferingResponse.java 377e66e
> api/src/org/apache/cloudstack/api/response/VolumeResponse.java 21d7d1a
> client/WEB-INF/classes/resources/messages.properties 1638be1
> client/pom.xml 0c38ecb
> client/tomcatconf/applicationContext.xml.in edf83a9
> core/src/com/cloud/agent/api/AttachVolumeCommand.java 302b8f8
> core/test/org/apache/cloudstack/api/agent/test/AttachVolumeAnswerTest.java 251a6cb
> core/test/org/apache/cloudstack/api/agent/test/AttachVolumeCommandTest.java 1ec416a
> engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/PrimaryDataStoreParameters.java b2b787c
> engine/schema/src/com/cloud/storage/DiskOfferingVO.java 909d7fe
> engine/schema/src/com/cloud/storage/VolumeVO.java 1699afd
> engine/storage/integration-test/test/org/apache/cloudstack/storage/allocator/StorageAllocatorTest.java 9444fa5
> engine/storage/src/org/apache/cloudstack/storage/allocator/ZoneWideStoragePoolAllocator.java e976980
> engine/storage/src/org/apache/cloudstack/storage/volume/datastore/PrimaryDataStoreHelper.java 5f8daf4
> engine/storage/volume/src/org/apache/cloudstack/storage/volume/VolumeObject.java ea31be3
> plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java 4680fde
> plugins/storage/volume/solidfire/pom.xml 9db0685
> plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/driver/SolidfirePrimaryDataStoreDriver.java f31126c
> plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/lifecycle/SolidFirePrimaryDataStoreLifeCycle.java PRE-CREATION
> plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/provider/SolidfirePrimaryDataStoreProvider.java 650cac8
> plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java PRE-CREATION
> server/src/com/cloud/api/query/dao/DiskOfferingJoinDaoImpl.java 283181f
> server/src/com/cloud/api/query/dao/VolumeJoinDaoImpl.java e27e2d9
> server/src/com/cloud/api/query/vo/DiskOfferingJoinVO.java 6d3cdcb
> server/src/com/cloud/api/query/vo/VolumeJoinVO.java 6ef8c91
> server/src/com/cloud/configuration/ConfigurationManager.java 8db037b
> server/src/com/cloud/configuration/ConfigurationManagerImpl.java 8baf1fb
> server/src/com/cloud/server/ConfigurationServerImpl.java bc52e9a
> server/src/com/cloud/storage/VolumeManager.java d198e5d
> server/src/com/cloud/storage/VolumeManagerImpl.java 4b654eb
> server/src/com/cloud/test/DatabaseConfig.java 70c8178
> server/test/com/cloud/vpc/MockConfigurationManagerImpl.java 21b3590
> setup/db/db/schema-410to420.sql cf4e98d
> tools/marvin/marvin/cloudstackConnection.py b092ef0
> ui/dictionary.jsp ded9ea0
> ui/scripts/configuration.js 211d7b7
> ui/scripts/docs.js 7c1aaf8
> ui/scripts/storage.js e816334
> ui/scripts/system.js 8b9a81f
>
> Diff: https://reviews.apache.org/r/11479/diff/
>
>
> Testing
> -------
>
> Manual testing has been performed:
>
> A plug-in can be added to CloudStack via an API call.
>
> Create a Disk Offering where the admin specifies both Disk Size and Disk IOPS. Disk IOPS are left blank.
> Create a Disk Offering where the admin allows the end user to customize both Disk Size and Disk IOPS. Disk IOPS are left blank.
> Create a Disk Offering where the admin specifies Disk Size, but allows the end user to customize Disk IOPS. Disk IOPS are left blank.
> Create a Disk Offering where the admin specifies Disk IOPS, but allows the end user to customize Disk Size. Disk IOPS are left blank.
>
> Create a Disk Offering where the admin specifies both Disk Size and Disk IOPS. Disk IOPS are all filled in.
> Create a Disk Offering where the admin allows the end user to customize both Disk Size and Disk IOPS. Disk IOPS are all filled in.
> Create a Disk Offering where the admin specifies Disk Size, but allows the end user to customize Disk IOPS. Disk IOPS are all filled in.
> Create a Disk Offering where the admin specifies Disk IOPS, but allows the end user to customize Disk Size. Disk IOPS are all filled in.
>
> A newly created volume is attached to a VM for the first time and an SR and VDI are created.
> This volume is detached and the VDI and SR are deleted.
> The volume is reattached and is properly introduced into the SR and VDI ("introduced" in the sense that the data that was previously on the volume is not destroyed upon reattach).
> This volume is detached and the VDI and SR are deleted.
>
>
> Thanks,
>
> Mike Tutkowski
>
>