cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mike Tutkowski <mike.tutkow...@solidfire.com>
Subject Re: add connect method on KVM storage code
Date Fri, 27 Sep 2013 04:23:03 GMT
My comments are inline:


On Thu, Sep 26, 2013 at 9:10 PM, Marcus Sorensen <shadowsor@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.

>
> 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 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().

>
> On Thu, Sep 26, 2013 at 7:35 PM, Mike Tutkowski
> <mike.tutkowski@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.tutkowski@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 <shadowsor@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.tutkowski@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.tutkowski@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 <
> shadowsor@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.tutkowski@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.tutkowski@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.tutkowski@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.tutkowski@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.tutkowski@solidfire.com
o: 303.746.7302
Advancing the way the world uses the
cloud<http://solidfire.com/solution/overview/?video=play>
*™*

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message