createAsync is just for creating the SAN (or whatever storage) volume. deleteAsync is the reverse.
Technically even the default plug-in should not call into the hypervisor layer. The storage layer should probably not be aware of the hypervisor layer. On Fri, Sep 27, 2013 at 5:14 PM, Mike Tutkowski < mike.tutkow...@solidfire.com> wrote: > Well, from what I saw with XenServer and VMware, that hypervisor logic's > attachVolume command also assumed a VDI/VMDK was created in advance. > > I had to put logic in those attachVolume methods to create the SR/VDI or > datastore/VMDK. > > However, thinking back on it, it might have made more sense for the > storage framework to detect if the storage in question was managed and - > before calling attach - call create. > > If that logic was in place, I could have left attach/detachVolume alone > and implemented create and delete in the hypervisor code to create my > SR/VDI or datastore/VMDK. > > That makes sense to me because it is CloudStack-managed storage (so > CloudStack is calling into the hypervisor to create and delete these types > of objects...it's managing them). > > > On Fri, Sep 27, 2013 at 5:00 PM, Marcus Sorensen <shadow...@gmail.com>wrote: > >> On Fri, Sep 27, 2013 at 4:22 PM, Mike Tutkowski >> <mike.tutkow...@solidfire.com> wrote: >> > Sure, sounds good - let me know when it's up on Review Board and I can >> take >> > a look. >> > >> > I made most of the changes you and I talked about: >> > >> > >> https://github.com/mike-tutkowski/incubator-cloudstack/commit/eb9b2edfc9062f9ca7961fecd5379b180ca3aed1 >> > >> > I have a new idea, though, that I think will simplify this: >> > >> > The main "weirdness" right now is when attachVolume is called that the >> > original logic assumed createVolume had been called already. >> > >> > In my case, this doesn't apply, so we had to place extra logic in >> > attachVolume to essentially "create" a volume. We decided to make a >> connect >> > method, which establishes the iSCSI connection and creates a >> > KVMPhysicalDisk that can be returned when attachVolume calls >> > getPhysicalDisk. >> > >> > The "normal" place where you'd create a KVMPhysicalDisk, however, would >> be >> > in the createVolume method. Since I don't currently "create" a volume, >> my >> > only chance to note the size of the volume is in the connect method. >> >> I don't think createVolume applies to plugins. My impression wash that >> createAsync is called on the mgmt server side. If createVolume IS >> being called, that's weird. The idea here is that mgmt server creates >> the LUN, and then on the KVM side attach is called (or StartCommand if >> it's a root volume and vm is being started), and it assumes that the >> LUN is already there, so we call connectPhysicalDisk to attach it to >> the KVM host. >> >> > >> > It ends up being kind of weird to pass a size into the connect method, >> as >> > you've noted. >> > >> > What if we essentially left the attachVolume and detachVolume methods >> alone >> > (as in how they were before my changes)? We could have >> > VolumeApiServiceImpl, before sending the AttachCommand, detect if the >> > storage in question is managed. If it is, VolumeApiServiceImpl could >> send a >> > CreateObjectCommand. I would then implement createPhysicalDisk to >> connect >> > my iSCSI target and create a KVMPhysicalDisk. >> > >> > On the reverse side, VolumeApiServiceImpl, after sending the >> DetachCommand, >> > could detect if the storage in question is managed. If it is, >> > VolumeApiServiceImpl could send a DeleteCommand. I would then implement >> the >> > deletePhysicalDisk method to disconnect my iSCSI session. >> > >> > What do you think? >> >> Maybe I'm just confused, but I thought the create and delete on the >> KVM side only apply to the default storage plugin, which has to pass >> everything on the agent. I thought the creation/deletion of LUNs >> occured via createAsync and deleteAsync in your plugin. >> >> > >> > >> > On Fri, Sep 27, 2013 at 3:21 PM, Marcus Sorensen <shadow...@gmail.com >> >wrote: >> > >> >> Ok, I've got our plugin working against 4.2. Tested start vm, stop vm, >> >> migrate vm, attach volume, detach volume. Other functions that we >> >> already had in our StorageAdaptor implementation, such as copying >> >> templates to primary storage, just worked without any modification >> >> from our 4.1 version. >> >> >> >> I'll post a patch to reviewboard with the applicable changes. I was >> >> correct that attachVolume and dettachVolume only apply to >> >> adding/removing disks from running VMs, so there were some more >> >> changes to LibvirtComputingResource. I don't intend for this patch to >> >> be applied (for one it's against 4.2), but I want you to take a look >> >> and see if it will work for you as well. If it does, then it's a good >> >> indicator that it should work for other plugins too, or if it needs to >> >> be tweaked we can work it out. >> >> >> >> The gist is that we needed a connectPhysicalDisk call that can accept >> >> the pool/volume info (which we've discussed), but also a version of >> >> connectPhysicalDisk that can take a vm specification >> >> (VirtualMachineTO) and figure out which pools/disks are needed and >> >> attach them. I largely copied the code we had custom inserted into our >> >> 4.1 and put it into KVMStoragePoolManager so that it will be adaptor >> >> agnostic. >> >> >> >> Same goes for disconnectPhysicalDisk. >> >> >> >> We also needed to pass the VirtualMachineTO in a few other places like >> >> MigrateCommand and StopCommand, it's otherwise hard to know which >> >> storage adaptors we need to deal with when all we have is a vm name or >> >> something like that. >> >> >> >> On Fri, Sep 27, 2013 at 12:56 AM, Mike Tutkowski >> >> <mike.tutkow...@solidfire.com> wrote: >> >> > Thanks for the clarification on how that works. >> >> > >> >> > Also, yeah, I think CHAP only grants you access to a volume. If >> multiple >> >> > hosts are using the CHAP credentials for a single volume, it's up to >> >> those >> >> > hosts to make sure they don't step on each other's toes (and this is >> - to >> >> > my understanding - how it works with XenServer and VMware). >> >> > >> >> > >> >> > On Fri, Sep 27, 2013 at 12:45 AM, Marcus Sorensen < >> shadow...@gmail.com >> >> >wrote: >> >> > >> >> >> On Fri, Sep 27, 2013 at 12:21 AM, Mike Tutkowski >> >> >> <mike.tutkow...@solidfire.com> wrote: >> >> >> > Maybe I should seek a little clarification as to how live >> migration >> >> works >> >> >> > in CS with KVM. >> >> >> > >> >> >> > Before we do a live migration of VM 1 from Host 1 to Host 2, do we >> >> detach >> >> >> > all disks from VM1? >> >> >> > >> >> >> > If so, then we're good to go there. >> >> >> > >> >> >> > I'm not as clear with HA. >> >> >> >> >> >> During live migration this is what we currently do in our modified >> >> >> 4.1, I'm not sure if the new framework is set up for this, but it >> >> >> should be made to do this if not. >> >> >> >> >> >> PrepareForMigrationCommand is called on destination host. In >> >> >> PrepareForMigrationCommand we added a few lines to call >> >> >> connectPhysicalDisk. This host connects the SAN disks to this new >> >> >> host, then creates a paused VM. >> >> >> >> >> >> MigrateCommand is called on the source host. This sends the proper >> >> >> command to transfer VM memory, then atomically cuts over to the >> >> >> destination host. During this time, the disks are attached on both >> >> >> sides, but the VM is still the only thing that is using them, and it >> >> >> atomically cuts over. There's no caching on the host (qemu is using >> >> >> directio), so this is safe. >> >> >> >> >> >> After MigrateCommand completes it's VM passoff, we detach the disks >> >> >> before returning. >> >> >> >> >> >> > >> >> >> > If VM 1 goes down because Host 1 crashes, is the attach-volume >> command >> >> >> > invoked as many times as need be (depending on how many volumes >> need >> >> to >> >> >> be >> >> >> > attached) when VM 1 is restarted on Host 2? >> >> >> >> >> >> From what I can tell, the attachVolume and dettachVolume seemed to >> >> >> only be for attaching disks to existing, running VMs (i.e. inserting >> >> >> new XML into an existing domain definition). Normally when >> starting a >> >> >> vm from scratch the vm definition, along with any currently attached >> >> >> disks, is passed in to StartCommand (which would also be called >> during >> >> >> HA restart of a VM). In our 4.1 branch we also have a call to >> >> >> connectPhysicalDisk here, where we loop through the disk definitions >> >> >> that were passed. >> >> >> >> >> >> Again, I should be able to flesh out the differences in 4.2 and how >> to >> >> >> go about making this suitable for everyone in the coming days, so >> long >> >> >> as you and anyone else writing plugins agree with the changes. >> >> >> >> >> >> These processes would make sure the disks are available on the hosts >> >> >> they need to be, but they don't really provide locking or ensure >> that >> >> >> only the necessary hosts can write to or see the disks at any given >> >> >> time. I don't think CHAP does that either. We currently generate >> ACLs >> >> >> via our SAN api during connectPhysicalDisk as a safety measure, but >> if >> >> >> CloudStack is working properly it will be in charge of controlling >> >> >> that the disks are only being used where they should be. The ACLs >> just >> >> >> ensure that if the VM somehow gets started in two different places >> >> >> (e.g. HA malfunction), only one of them will have access to the >> disks. >> >> >> >> >> >> > >> >> >> > >> >> >> > On Fri, Sep 27, 2013 at 12:17 AM, Mike Tutkowski < >> >> >> > mike.tutkow...@solidfire.com> wrote: >> >> >> > >> >> >> >> Let me clarify this line a bit: >> >> >> >> >> >> >> >> "We get away without this with XenServer and VMware because - as >> far >> >> as >> >> >> I >> >> >> >> know - CS delegates HA and live migration to those clusters and >> they >> >> >> handle >> >> >> >> it most likely with some kind of locking protocol on the >> >> SR/datastore." >> >> >> >> >> >> >> >> When I set up a XenServer or a VMware cluster, all nodes in the >> >> cluster >> >> >> >> have the proper CHAP credentials and can access a shared >> >> SR/datastore. >> >> >> >> >> >> >> >> HA and live migrations are OK here because the cluster controls >> >> access >> >> >> to >> >> >> >> the VDI on the SR (or VMDK on the datastore) with some kind of >> >> locking >> >> >> >> protocol, I expect. >> >> >> >> >> >> >> >> Since KVM isn't really in a cluster outside of the CloudStack >> world, >> >> >> >> CloudStack has to handle these intricacies. >> >> >> >> >> >> >> >> In my case, I'm just presenting a raw disk to a VM on a KVM host. >> >> >> >> >> >> >> >> In that case, HA and live migration depend on the storage plug-in >> >> being >> >> >> >> able to grant and revoke access to the volume for hosts as >> needed. >> >> >> >> >> >> >> >> I'd actually rather not even bother with CHAP when using KVM. >> >> >> >> >> >> >> >> >> >> >> >> On Fri, Sep 27, 2013 at 12:06 AM, Mike Tutkowski < >> >> >> >> mike.tutkow...@solidfire.com> wrote: >> >> >> >> >> >> >> >>> Hey Marcus, >> >> >> >>> >> >> >> >>> I agree that CHAP does not fulfill the same role as fencing. >> >> >> >>> >> >> >> >>> I think we're going to have trouble with HA and live migrations >> on >> >> KVM >> >> >> if >> >> >> >>> the storage plug-in doesn't have a way of knowing when a host >> wants >> >> to >> >> >> >>> access a volume and when we want to revoke access to that >> volume. >> >> >> >>> >> >> >> >>> We get away without this with XenServer and VMware because - as >> far >> >> as >> >> >> I >> >> >> >>> know - CS delegates HA and live migration to those clusters and >> they >> >> >> handle >> >> >> >>> it most likely with some kind of locking protocol on the >> >> SR/datastore. >> >> >> >>> >> >> >> >>> As far as the path field is concerned, I should be able to >> populate >> >> it >> >> >> >>> with the IQN of the volume in question. >> >> >> >>> >> >> >> >>> One problem I do see, however, is in the getPhysicalDisk method. >> >> >> >>> >> >> >> >>> How are you envisioning I keep track of KVMPhysicalDisks that I >> >> create >> >> >> in >> >> >> >>> my connect method? >> >> >> >>> >> >> >> >>> Initially I was thinking I'd just keep them in a map. Storage >> pool >> >> UUID >> >> >> >>> to KVMPhysicalDisks. >> >> >> >>> >> >> >> >>> The problem is, how do I reconstruct that map if the agent is >> >> restarted >> >> >> >>> (say the host crashes or is restarted). >> >> >> >>> >> >> >> >>> For storage pools, we get a message (ModifyStoragePoolCommand) >> from >> >> the >> >> >> >>> CS MS to tell us about all of the relevant storage pools. With >> this >> >> >> >>> message, I can reconstruct my cache of storage pools. No problem >> >> there. >> >> >> >>> >> >> >> >>> But how will I know which volumes belong to a given storage >> pool if >> >> I >> >> >> >>> have to rebuild that map? How will I even know which volumes >> are in >> >> >> use at >> >> >> >>> all? >> >> >> >>> >> >> >> >>> Thanks >> >> >> >>> >> >> >> >>> >> >> >> >>> On Thu, Sep 26, 2013 at 11:37 PM, Marcus Sorensen < >> >> shadow...@gmail.com >> >> >> >wrote: >> >> >> >>> >> >> >> >>>> On Thu, Sep 26, 2013 at 10:23 PM, Mike Tutkowski >> >> >> >>>> <mike.tutkow...@solidfire.com> wrote: >> >> >> >>>> > My comments are inline: >> >> >> >>>> > >> >> >> >>>> > >> >> >> >>>> > On Thu, Sep 26, 2013 at 9:10 PM, Marcus Sorensen < >> >> >> shadow...@gmail.com >> >> >> >>>> >wrote: >> >> >> >>>> > >> >> >> >>>> >> 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. >> >> >> >>>> >> >> >> >> >>>> > [Mike T.] I agree, format is not needed. The only reason I >> have >> >> size >> >> >> >>>> passed >> >> >> >>>> > in is because I need to create a KVMPhysicalDisk at the end >> of >> >> the >> >> >> >>>> connect >> >> >> >>>> > method. Since this KVMPhysicalDisk is (in the code on GitHub) >> >> being >> >> >> >>>> used to >> >> >> >>>> > create our XML to attach the disk, I figured we'd need that >> size. >> >> >> The >> >> >> >>>> > KVMPhysicalDisk I produce from my implementation of >> >> getPhysicalDisk >> >> >> is >> >> >> >>>> not >> >> >> >>>> > as good because I don't know the size of the disk at this >> point >> >> (I >> >> >> >>>> don't >> >> >> >>>> > keep that information around). The reason I don't keep that >> info >> >> >> >>>> around is >> >> >> >>>> > because I don't have a good way to reproduce that info if >> the KVM >> >> >> host >> >> >> >>>> is >> >> >> >>>> > rebooted. We get info about storage pools in the form of a >> >> >> >>>> > ModifyStoragePoolCommand, but nothing about the volumes >> inside of >> >> >> the >> >> >> >>>> > storage pool. >> >> >> >>>> >> >> >> >>>> getPhysicalDisk is called in a bunch of places. I'd rely on the >> >> >> >>>> connectPhysicalDisk to only make the block device appear on the >> >> host, >> >> >> >>>> and then getPhysicalDisk to find that block device and fill out >> >> things >> >> >> >>>> like disk size and path (the real path to the local block >> device) >> >> for >> >> >> >>>> passing and creating the disk XML. Trust me, unless things have >> >> >> >>>> changed significantly you need to be able to identify a given >> >> device >> >> >> >>>> as a specific local disk by whatever you are setting the 'path' >> >> >> >>>> attribute to be. getPhysicalDisk will be called on your >> storage >> >> pool >> >> >> >>>> with simply the path attribute, and via your adaptor with the >> pool >> >> and >> >> >> >>>> path. >> >> >> >>>> >> >> >> >>>> So you may set path as some combination of iqn and target/pool >> >> info, >> >> >> >>>> or if iqn is enough to identify a unique block device (in >> >> >> >>>> /dev/disk/by-id maybe?) on a host then just use that. Path just >> >> needs >> >> >> >>>> to be something, anything, to identify the disk on the host. In >> >> >> >>>> getPhysicalDisk, identify the local block device matching the >> info, >> >> >> >>>> create a new KVMPhysicalDisk with the local path, size, etc, >> and >> >> >> >>>> return it. >> >> >> >>>> >> >> >> >>>> > >> >> >> >>>> >> >> >> >> >>>> >> 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? >> >> >> >>>> >> >> >> >> >>>> > [Mike T.] Yeah, as I mentioned in an e-mail way back, those >> >> methods >> >> >> >>>> were >> >> >> >>>> > never implemented in 4.2. I think you said you were going to >> get >> >> >> around >> >> >> >>>> > them not being implemented by keeping certain logic that >> talks to >> >> >> the >> >> >> >>>> SAN >> >> >> >>>> > in the agent. I don't think we want any SolidFire-specific >> code >> >> in >> >> >> the >> >> >> >>>> > agent, however, so I can't go that route. If those methods >> do not >> >> >> get >> >> >> >>>> > implemented in 4.3, then I will need to use CHAP credentials >> for >> >> KVM >> >> >> >>>> (just >> >> >> >>>> > like I did with XenServer and VMware). >> >> >> >>>> >> >> >> >>>> I initially figured your StorageAdaptor implementation would >> be all >> >> >> >>>> solidfire specific, just like the mgmt server side plugin is. >> If it >> >> >> >>>> can be generic to all iscsi storage then that's great. I agree >> that >> >> >> >>>> ideally the agent wouldn't be making API calls to your SAN. I >> don't >> >> >> >>>> think it should be necessary given that you're not going to >> use the >> >> >> >>>> ACL route. I'm not sure CHAP fills the same purpose of fencing. >> >> >> >>>> >> >> >> >>>> > >> >> >> >>>> >> >> >> >> >>>> >> 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. >> >> >> >>>> >> >> >> >> >>>> > [Mike T.] If I'm not using the volumes.path field for >> anything, I >> >> >> can >> >> >> >>>> stick >> >> >> >>>> > the IQN in volumes.path (as well as leaving it in >> >> >> volumes.iscsi_name, >> >> >> >>>> which >> >> >> >>>> > is used elsewhere). That way we only have to ask for >> getPath(). >> >> >> >>>> >> >> >> >>>> Yeah, whatever it is that you'd need to find the right block >> device >> >> >> >>>> should go in the path. If you look through >> LibvirtComputingResource >> >> >> >>>> you'll see stuff like this sprinkled around: >> >> >> >>>> >> >> >> >>>> KVMPhysicalDisk volume = >> >> >> primaryPool.getPhysicalDisk(cmd >> >> >> >>>> .getVolumePath()); >> >> >> >>>> >> >> >> >>>> >> >> >> >>>> or: >> >> >> >>>> String volid = cmd.getPath(); >> >> >> >>>> KVMPhysicalDisk vol = pool.getPhysicalDisk(volid); >> >> >> >>>> >> >> >> >>>> or: >> >> >> >>>> >> >> >> >>>> KVMPhysicalDisk physicalDisk = >> >> >> >>>> _storagePoolMgr.getPhysicalDisk( store.getPoolType(), >> >> >> >>>> store.getUuid(), >> >> >> >>>> data.getPath()); >> >> >> >>>> >> >> >> >>>> Maybe some of it is short-circuited by the new >> KVMStorageProcessor, >> >> >> >>>> but I'd still implement a working one, and then attachVolume >> can >> >> call >> >> >> >>>> getPhysicalDisk after connectPhysicalDisk, even on your >> >> adaptor/pool. >> >> >> >>>> >> >> >> >>>> > >> >> >> >>>> >> >> >> >> >>>> >> 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> >> >> >> >>>> >> > *™* >> >> >> >>>> >> >> >> >> >>>> > >> >> >> >>>> > >> >> >> >>>> > >> >> >> >>>> > -- >> >> >> >>>> > *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> >> >> >> > *™* >> >> >> >> >> > >> >> > >> >> > >> >> > -- >> >> > *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> *™*
