> 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 > >
