Ok, let me digest this a bit. I got the github responses but I'd also like to keep it on-list as well.
My initial thoughts are: 1) I don't think disk format and size are necessary parameters for connectPhysicalDisk, as the format can be determined by the adaptor, and the size is set during the createAsync call in the plugin. We really just need the disk path and the pool. 2) I thought this access group thing you mention are the grantAccess and revokeAccess calls in the storage plugin 2.0 design doc. Was that not implemented? I see you've added getters/setters for the attach cmd to pass the iscsi info you need. Would it perhaps be possible to send a details Map<String, String> instead? Then any plugin implementer could attach arbitrary data they need. So it might be connectPhysicalDisk(StoragePoolType type, String poolUuid, String volPath, Map<String, String> details)? I'll have to look and see where those cmd. attributes are set, ideally it would be all the way back in the plugin to avoid custom code for every adaptor that wants to set details. On Thu, Sep 26, 2013 at 7:35 PM, Mike Tutkowski <mike.tutkow...@solidfire.com> wrote: > Also, if we went the non-CHAP route, before attaching a volume to a VM, > we'd have to tell the plug-in to set up a volume access group. > > When a volume is detached from a VM, we'd have to tell the plug-in to > delete the volume access group. > > > On Thu, Sep 26, 2013 at 7:32 PM, Mike Tutkowski < > mike.tutkow...@solidfire.com> wrote: > >> I mention this is my comments on GitHub, as well, but CHAP info is >> associated with an account - not a storage pool. >> >> Ideally we could do without CHAP info entirely if we had a reliable way to >> tell the storage plug-in that a given host wants to access a given volume. >> In this case, my storage plug-in could create what we call a Volume Access >> Group on the SAN. It would essentially say, "The host with IQN <x> can >> access the volume with IQN <y> without using CHAP credentials." Of course >> we'd need a way to revoke this privilege in the event of a live migration >> of a VM. Right now, I do not believe such a facility is supported with the >> storage plug-ins. >> >> >> On Thu, Sep 26, 2013 at 4:56 PM, Marcus Sorensen <shadow...@gmail.com>wrote: >> >>> Looking at your code, is the chap info stored with the pool, so we >>> could pass the pool to the adaptor? That would be more agnostic, >>> anyone implementing a plugin could pull the specifics they need for >>> their stuff out of the pool on the adaptor side, rather than creating >>> custom signatures. >>> >>> Also, I think we may want to consider implementing connect/disconnect >>> as just dummy methods in LibvirtStorageAdaptor, so we don't have to be >>> picky about which adaptors/types in every single place we may want to >>> connect/disconnect (in 4.1 there were several, I'm not sure if >>> everything goes through this in 4.2). We can just call >>> adaptor.connectPhysicalDisk and the adaptor can decide if it needs to >>> do anything. >>> >>> Comments are attached to your commit, I just wanted to echo them here >>> on-list. >>> >>> On Thu, Sep 26, 2013 at 4:32 PM, Mike Tutkowski >>> <mike.tutkow...@solidfire.com> wrote: >>> > Oh, SnapshotTestWithFakeData is just modified because the code wasn't >>> > building until I corrected this. It has nothing really to do with my >>> real >>> > changes. >>> > >>> > >>> > On Thu, Sep 26, 2013 at 4:31 PM, Mike Tutkowski < >>> > mike.tutkow...@solidfire.com> wrote: >>> > >>> >> Hey Marcus, >>> >> >>> >> I implemented your recommendations regarding adding connect and >>> disconnect >>> >> methods. It is not yet checked in (as you know, having trouble with my >>> KVM >>> >> environment), but it is on GitHub here: >>> >> >>> >> >>> >> >>> https://github.com/mike-tutkowski/incubator-cloudstack/commit/f2897c65689012e6157c0a0c2ed7e5355900c59a >>> >> >>> >> Please let me know if you have any more comments. >>> >> >>> >> Thanks! >>> >> >>> >> >>> >> On Thu, Sep 26, 2013 at 4:05 PM, Marcus Sorensen <shadow...@gmail.com >>> >wrote: >>> >> >>> >>> Mike, everyone, >>> >>> As I've mentioned on the board, I'm working on getting our own >>> >>> internal KVM storage plugin working on 4.2. In the interest of making >>> >>> it forward compatible, I just wanted to confirm what you were doing >>> >>> with the solidfire plugin as far as attaching your iscsi LUNs. We had >>> >>> discussed a new connectPhysicalDisk method for the StorageAdaptor >>> >>> class, something perhaps like: >>> >>> >>> >>> public boolean connectPhysicalDisk(String volumeUuid, KVMStoragePool >>> >>> pool); >>> >>> >>> >>> then added to KVMStoragePoolManager: >>> >>> >>> >>> public boolean connectPhysicalDisk(StoragePoolType type, String >>> >>> poolUuid, String volPath) { >>> >>> StorageAdaptor adaptor = getStorageAdaptor(type); >>> >>> KVMStoragePool pool = adaptor.getStoragePool(poolUuid); >>> >>> return adaptor.connectPhysicalDisk(volPath, pool); >>> >>> } >>> >>> >>> >>> Something similar to this for disconnect as well. Then in the >>> >>> KVMStorageProcessor these can be called as needed for attach/detach. >>> >>> We can probably stub out one in LibvirtStorageAdaptor so there's no >>> >>> need to switch or if/else for pool types, for example in >>> >>> KVMStorageProcessor.attachVolume. >>> >>> >>> >>> I have debated on whether or not it should just be rolled into >>> >>> getPhysicalDisk, having it connect the disk if it's not already >>> >>> connected. getPhysicalDisk is called a lot, and I'm not sure it always >>> >>> needs to connect the disk when it does. In past iterations >>> >>> getPhysicalDisk has simply spoken to our SAN api and returned the disk >>> >>> details, nothing more. So it seemed more flexible and granular to do >>> >>> the connectPhysicalDisk (we have one now in our 4.1 version). >>> >>> >>> >> >>> >> >>> >> >>> >> -- >>> >> *Mike Tutkowski* >>> >> *Senior CloudStack Developer, SolidFire Inc.* >>> >> e: mike.tutkow...@solidfire.com >>> >> o: 303.746.7302 >>> >> Advancing the way the world uses the cloud< >>> http://solidfire.com/solution/overview/?video=play> >>> >> *™* >>> >> >>> > >>> > >>> > >>> > -- >>> > *Mike Tutkowski* >>> > *Senior CloudStack Developer, SolidFire Inc.* >>> > e: mike.tutkow...@solidfire.com >>> > o: 303.746.7302 >>> > Advancing the way the world uses the >>> > cloud<http://solidfire.com/solution/overview/?video=play> >>> > *™* >>> >> >> >> >> -- >> *Mike Tutkowski* >> *Senior CloudStack Developer, SolidFire Inc.* >> e: mike.tutkow...@solidfire.com >> o: 303.746.7302 >> Advancing the way the world uses the >> cloud<http://solidfire.com/solution/overview/?video=play> >> *™* >> > > > > -- > *Mike Tutkowski* > *Senior CloudStack Developer, SolidFire Inc.* > e: mike.tutkow...@solidfire.com > o: 303.746.7302 > Advancing the way the world uses the > cloud<http://solidfire.com/solution/overview/?video=play> > *™*
