[libvirt] [PATCHv3 1/4] storage: initial support for linking with libgfapi

Tue Nov 12 18:45:47 UTC 2013

On 11/12/2013 09:48 AM, Daniel P. Berrange wrote:
> On Mon, Nov 11, 2013 at 09:19:28PM -0700, Eric Blake wrote:
>> diff --git a/libvirt.spec.in b/libvirt.spec.in
>> index 72815f4..a90ee2b 100644
>> --- a/libvirt.spec.in
>> +++ b/libvirt.spec.in
>> @@ -555,6 +561,10 @@ BuildRequires: device-mapper-devel
>>  BuildRequires: ceph-devel
>>      %endif
>>  %endif
>> +%if %{with_storage_gluster}
>> +BuildRequires: glusterfs-api-devel
>> +BuildRequires: glusterfs-devel
>> +%endif
> 
> This accepts any version of the packages
> 
>> diff --git a/m4/virt-gluster.m4 b/m4/virt-gluster.m4
>> new file mode 100644
>> index 0000000..4851e17
>> --- /dev/null
>> +++ b/m4/virt-gluster.m4
> 
>> +
>> +AC_DEFUN([LIBVIRT_CHECK_GLUSTER],[
>> +  LIBVIRT_CHECK_PKG([GLFS], [glusterfs-api], [3.0])
>> +])
> 
> But this suggests only version >= 3.0 is acceptable. Should we make
> the RPM dep explicit for this.

Okay.  For that matter, so far I have only tested on Fedora 19, with
3.4.1; and Peter reported an issue to me off-list where the 3.4.0 client
has a rather nasty bug that caused glfs_open/glfs_read to hang rather
than diagnose a permission denied error, so I'll go with the
conservative approach of requiring the minimum that I've tested with
(and leave it to a future patch, if any, if we decide to relax to
supporting earlier versions).

> 
> I think I'd prefer  s/GLFS/GLUSTERFS/ - its only used in a few
> places, so being more verbose doesn't hurt us much.

Sure.

> 
>> diff --git a/src/Makefile.am b/src/Makefile.am
>> index ad39b74..be9978a 100644
>> --- a/src/Makefile.am
>> +++ b/src/Makefile.am
>> @@ -1434,6 +1437,11 @@ if WITH_STORAGE_SHEEPDOG
>>  libvirt_driver_storage_impl_la_SOURCES += $(STORAGE_DRIVER_SHEEPDOG_SOURCES)
>>  endif WITH_STORAGE_SHEEPDOG
>>
>> +if WITH_STORAGE_GLUSTER
>> +libvirt_driver_storage_impl_la_SOURCES += $(STORAGE_DRIVER_GLUSTER_SOURCES)
>> +libvirt_driver_storage_impl_la_LIBADD += $(GLFS_LIBS)
> 
> Hmm, I don't see CFLAGS ever mentioning  GLFS_CFLAGS which could
> cause problems for anyone who has installed gluster outside of
> the /usr hiearchy.

Will fix.

> 
>> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
>> index 975e662..8299f2a 100644
>> --- a/src/conf/storage_conf.c
>> +++ b/src/conf/storage_conf.c
>> @@ -55,7 +55,7 @@ VIR_ENUM_IMPL(virStoragePool,
>>                VIR_STORAGE_POOL_LAST,
>>                "dir", "fs", "netfs",
>>                "logical", "disk", "iscsi",
>> -              "scsi", "mpath", "rbd", "sheepdog")
>> +              "scsi", "mpath", "rbd", "sheepdog", "gluster")
> 
> Hmm, well that sucks - in some places we use 'gluster' and others
> we use 'glusterfs' :-( We've released code using both so we can't
> standardize everywhere now.

I had to do some searching to see what you mean:

The 'netfs' pool supports three types: 'nfs', 'cifs', and 'glusterfs' -
but that applies to FUSE mounting.

The <disk type='network'><source protocol='...'> supports 'nbd', 'rbd',
'sheepdog', and 'gluster' (among others), the others have a storage pool
named with the same protocol name.

> 
> So guess we should stick with what you have here.

Yeah, having protocol='gluster' map to storage pool 'gluster' is more
consistent with what the other protocols do, while the 'netfs' pool is
more a selection of which mounting type is used (where the mount command
really uses -t glusterfs).  Also, going with the name 'gluster' for the
new pool (gfapi, no mount involved) distinguishes that it is more
efficient than 'netfs' use of glusterfs (which requires a mount point
and has FUSE overhead).

> 
>> +    {.poolType = VIR_STORAGE_POOL_GLUSTER,
>> +     .poolOptions = {
>> +         .flags = (VIR_STORAGE_POOL_SOURCE_HOST |
>> +                   VIR_STORAGE_POOL_SOURCE_NETWORK |
>> +                   VIR_STORAGE_POOL_SOURCE_NAME),
>> +     },
> 
> Are gluster volume names always wellformed paths ? If so is
> it worth using SOURCE_PATH instad of SOURCE_NAME ? I don't
> feel too strongly either way.

Qemu supports the following URIs for using gfapi:

gluster://host/volume/image
gluster://host/volume/dir/image

Internally, use of gfapi requires that 'volume' be used without a
leading slash, below there, you can either use 'dir/image' or
'/dir/image' to refer to an image within the gluster 'volume'.  But from
libvirt's perspective, I was exposing this as pool name 'volume' when
operating on the top-level directory (libvirt volume 'image' within the
pool named 'volume'), and pool name 'volume/dir' when operating on a
sub-directory (libvirt volume 'image' within the pool named
'volume/dir').  Since use of 'gluster(1)' command line tool does NOT use
a leading slash, I didn't want to require a leading slash in the libvirt
pool name.

Does this mean I'm overloading the <name> XML element with too much
information?  Should I split it into two elements, one for volume name
(no slash allowed), and an optional one for subdirectory name (leading
slash required)?

Another way of looking at this problem is what should the volume XML
look like?  With this round of patches, you get:

<volume>
  <name>img4</name>
  <key>/vol1/sub/img4</key>
  <target>
    <path>gluster://localhost/vol1/sub/img4</path>

I tried to make <key> encode both the volume name and subdirectory, so
that the image name is uniquely resolvable to a storage pool (that is, I
want unique keys for vol1/img, vol2/img, and vol2/sub/img, which are
necessarily accessed through three different pools, even though all
three volumes would have the same name of 'img').  This is similar to
how keys are unique for different directory pools.  Meanwhile, the
<target> path is the URI that qemu will eventually use (and which you
can copy-and-paste into command lines where you are using qemu-img).  I
did NOT want to encode the host name into <key> because gluster allows
you to access the volume from any host in its cluster (that is,
gluster://hosta/vol/file and gluster://hostb/vol/file can be the same
file, so reflecting 'hosta' or 'hostb' into <key> would get in the way
of using key lookup only providing differentiation between distinct
storage).  Of course, my current choice of <key> naming means that a
gluster volume name and an absolute path of a regular directory pool
will collide.  We still have time to tweak the scheme, if you can think
of any better plan for the choice of volume <key>s and pool <name>s.

> 
> ACK if you fix the nitpicks.

I'll wait for feedback on my reply before committing anything.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20131112/91a4fc01/attachment-0001.sig>