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