[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] storage backend: Add RBD (RADOS Block Device) support



On 05/21/2012 05:00 AM, Daniel P. Berrange wrote:
> On Mon, May 14, 2012 at 11:06:42AM +0200, Wido den Hollander wrote:
>> This patch adds support for a new storage backend with RBD support.
>>
>> RBD is the RADOS Block Device and is part of the Ceph distributed storage system.
>>
>> It comes in two flavours: Qemu-RBD and Kernel RBD, this storage backend only supports
>> Qemu-RBD, thus limiting the use of this storage driver to Qemu only.
>>
>> To function this backend relies on librbd and librados being present on the local system.
>>
>> The backend also supports Cephx authentication for safe authentication with the Ceph cluster.
>>
>> For storing credentials it uses the build-in secret mechanism of libvirt.
>>
>> Signed-off-by: Wido den Hollander <wido widodh nl>

> 
> ACK to this patch

I did not closely review this patch, assuming that Daniel's review
covered any issues such as avoiding resource leaks.

Now pushed, with the following tweaks to keep 'make syntax-check' happy,
fix typos and grammar in the docs, and fix configuration/compilation
when librbd libraries (ceph-devel on Fedora) are not available, and fix
compilation with warnings enabled when ceph-devel is installed:


diff --git i/configure.ac w/configure.ac
index 669a1eb..06c6a4b 100644
--- i/configure.ac
+++ w/configure.ac
@@ -1981,21 +1981,21 @@ if test "$with_storage_mpath" = "check"; then
 fi
 AM_CONDITIONAL([WITH_STORAGE_MPATH], [test "$with_storage_mpath" = "yes"])

+LIBRBD_LIBS=
 if test "$with_storage_rbd" = "yes" || test "$with_storage_rbd" =
"check"; then
     AC_CHECK_HEADER([rbd/librbd.h], [LIBRBD_FOUND=yes; break;])

-    LIBRBD_LIBS="-lrbd -lrados -lcrypto"
-
     if test "$LIBRBD_FOUND" = "yes"; then
         with_storage_rbd=yes
-        LIBS="$LIBS $LIBRBD_LIBS"
+        LIBRBD_LIBS="-lrbd -lrados -lcrypto"
+        AC_DEFINE_UNQUOTED([WITH_STORAGE_RBD], [1],
+         [whether RBD backend for storage driver is enabled])
     else
         with_storage_rbd=no
     fi
-
-    AC_DEFINE_UNQUOTED([WITH_STORAGE_RBD], 1, [wether RBD backend for
storage driver is enabled])
 fi
 AM_CONDITIONAL([WITH_STORAGE_RBD], [test "$with_storage_rbd" = "yes"])
+AC_SUBST([LIBRBD_LIBS])

 LIBPARTED_CFLAGS=
 LIBPARTED_LIBS=
diff --git i/docs/schemas/storagepool.rng w/docs/schemas/storagepool.rng
index 7753493..75b6b51 100644
--- i/docs/schemas/storagepool.rng
+++ w/docs/schemas/storagepool.rng
@@ -485,7 +485,7 @@
       <empty/>
     </element>
   </define>
-
+
   <define name='sourcerbd'>
     <element name='source'>
       <ref name='sourceinfoname'/>
diff --git i/docs/storage.html.in w/docs/storage.html.in
index 277ea36..b3484e8 100644
--- i/docs/storage.html.in
+++ w/docs/storage.html.in
@@ -496,23 +496,20 @@

     <h2><a name="StorageBackendRBD">RBD pools</a></h2>
     <p>
-      This storage driver provides a pool which contains all RBD images
in a RADOS pool.
-      <br />
-      RBD (RADOS Block Device) is part of the Ceph distributed storage
project.
-      <br/>
-      This backend <i>only</i> supports Qemu with RBD support. Kernel
RBD which exposes
-      RBD devices as block devices in /dev is <i>not</i> supported.
-      <br/>
-      RBD images created with this storage backend can be accessed
through kernel RBD if
-      configured manually, but this backend does not provide mapping
these images.
-      <br/>
-      Images created with this backend can be attached to Qemu guests
when Qemu is build
-      with RBD support (Since Qemu 0.14.0)</a>.
-      <br/>
-      The backend supports cephx authentication for communication with
the Ceph cluster.
-      <br/>
-      Storing the cephx authentication key is done with the libvirt
secret mechanism.
-      The UUID in the example pool input refers to the UUID of the
stored secret.
+      This storage driver provides a pool which contains all RBD
+      images in a RADOS pool.  RBD (RADOS Block Device) is part
+      of the Ceph distributed storage project.<br/>
+      This backend <i>only</i> supports Qemu with RBD support. Kernel RBD
+      which exposes RBD devices as block devices in /dev is <i>not</i>
+      supported. RBD images created with this storage backend
+      can be accessed through kernel RBD if configured manually, but
+      this backend does not provide mapping for these images.<br/>
+      Images created with this backend can be attached to Qemu guests
+      when Qemu is build with RBD support (Since Qemu 0.14.0). The
+      backend supports cephx authentication for communication with the
+      Ceph cluster. Storing the cephx authentication key is done with
+      the libvirt secret mechanism. The UUID in the example pool input
+      refers to the UUID of the stored secret.
       <span class="since">Since 0.9.13</span>
     </p>

@@ -552,7 +549,11 @@
        &lt;/volume&gt;</pre>

     <h3>Example disk attachement</h3>
-    <p>RBD images can be attached to Qemu guests when Qemu is build
with RBD support. Information about attaching a RBD image to a guest can
be found at <a href="formatdomain.html#elementsDisks">format domain</a>
page.</p>
+    <p>RBD images can be attached to Qemu guests when Qemu is built
+    with RBD support. Information about attaching a RBD image to a
+    guest can be found
+    at <a href="formatdomain.html#elementsDisks">format domain</a>
+    page.</p>

     <h3>Valid pool format types</h3>
     <p>
diff --git i/po/POTFILES.in w/po/POTFILES.in
index cfa9d44..91216cb 100644
--- i/po/POTFILES.in
+++ w/po/POTFILES.in
@@ -104,6 +104,7 @@ src/storage/storage_backend_fs.c
 src/storage/storage_backend_iscsi.c
 src/storage/storage_backend_logical.c
 src/storage/storage_backend_mpath.c
+src/storage/storage_backend_rbd.c
 src/storage/storage_backend_scsi.c
 src/storage/storage_driver.c
 src/test/test_driver.c
diff --git i/src/Makefile.am w/src/Makefile.am
index 413464b..e9621c1 100644
--- i/src/Makefile.am
+++ w/src/Makefile.am
@@ -1052,7 +1052,7 @@ endif

 if WITH_STORAGE_RBD
 libvirt_driver_storage_la_SOURCES += $(STORAGE_DRIVER_RBD_SOURCES)
-libvirt_la_LIBADD += $(LIBRBD_LIBS)
+libvirt_driver_storage_la_LIBADD += $(LIBRBD_LIBS)
 endif

 if WITH_NODE_DEVICES
diff --git i/src/storage/storage_backend_rbd.c
w/src/storage/storage_backend_rbd.c
index 5319749..46bd892 100644
--- i/src/storage/storage_backend_rbd.c
+++ w/src/storage/storage_backend_rbd.c
@@ -55,6 +55,8 @@ static int
virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr *ptr,
     virBuffer mon_host = VIR_BUFFER_INITIALIZER;
     virSecretPtr secret = NULL;
     char secretUuid[VIR_UUID_STRING_BUFLEN];
+    int i;
+    char *mon_buff = NULL;

     VIR_DEBUG("Found Cephx username: %s",
               pool->def->source.auth.cephx.username);
@@ -130,9 +132,8 @@ static int
virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr *ptr,
     }

     VIR_DEBUG("Found %Zu RADOS cluster monitors in the pool configuration",
-              pool->def->source.nhost);
+              pool->def->source.nhost);

-    int i;
     for (i = 0; i < pool->def->source.nhost; i++) {
         if (pool->def->source.hosts[i].name != NULL &&
             !pool->def->source.hosts[i].port) {
@@ -154,7 +155,7 @@ static int
virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr *ptr,
        goto cleanup;
     }

-    char *mon_buff = virBufferContentAndReset(&mon_host);
+    mon_buff = virBufferContentAndReset(&mon_host);
     VIR_DEBUG("RADOS mon_host has been set to: %s", mon_buff);
     if (rados_conf_set(ptr->cluster, "mon_host", mon_buff) < 0) {
        virStorageReportError(VIR_ERR_INTERNAL_ERROR,
@@ -178,6 +179,7 @@ cleanup:
     VIR_FREE(rados_key);
     virSecretFree(secret);
     virBufferFreeAndReset(&mon_host);
+    VIR_FREE(mon_buff);
     return ret;
 }




Hmm, I spoke too soon - I committed the above things, then found a few
more on a final review.  I'm also squashing in the following.  %Zu is an
obsolete glibc extension; we prefer the C99 %zu instead.  And you failed
to fit 80 columns in a number of places.

diff --git i/src/conf/storage_conf.c w/src/conf/storage_conf.c
index 06334c3..bf4567f 100644
--- i/src/conf/storage_conf.c
+++ w/src/conf/storage_conf.c
@@ -429,8 +429,8 @@ virStoragePoolDefParseAuthCephx(xmlXPathContextPtr ctxt,
     uuid = virXPathString("string(./auth/secret/@uuid)", ctxt);
     auth->secret.usage = virXPathString("string(./auth/secret/@usage)",
ctxt);
     if (uuid == NULL && auth->secret.usage == NULL) {
-        virStorageReportError(VIR_ERR_XML_ERROR,
-                              "%s", _("missing auth secret uuid or
usage attribute"));
+        virStorageReportError(VIR_ERR_XML_ERROR, "%s",
+                              _("missing auth secret uuid or usage
attribute"));
         return -1;
     }

@@ -465,10 +465,9 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt,

     source->name = virXPathString("string(./name)", ctxt);
     if (pool_type == VIR_STORAGE_POOL_RBD && source->name == NULL) {
-        virStorageReportError(VIR_ERR_XML_ERROR,
-                                  "%s", _("missing mandatory 'name'
field for RBD pool name"));
-            VIR_FREE(source->name);
-            goto cleanup;
+        virStorageReportError(VIR_ERR_XML_ERROR, "%s",
+                              _("missing mandatory 'name' field for RBD
pool name"));
+        goto cleanup;
     }

     if (options->formatFromString) {
@@ -799,7 +798,8 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) {
         }
     }

-    /* When we are working with a virtual disk we can skip the target
path and permissions */
+    /* When we are working with a virtual disk we can skip the target
+     * path and permissions */
     if (!(options->flags & VIR_STORAGE_POOL_SOURCE_NETWORK)) {
         if ((tmppath = virXPathString("string(./target/path)", ctxt))
== NULL) {
             virStorageReportError(VIR_ERR_XML_ERROR,
@@ -1011,7 +1011,8 @@ virStoragePoolDefFormat(virStoragePoolDefPtr def) {
     if (virStoragePoolSourceFormat(&buf, options, &def->source) < 0)
         goto cleanup;

-    /* RBD devices are no local block devs nor files, so it doesn't
have a target */
+    /* RBD devices are not local block devs nor files, so it doesn't
+     * have a target */
     if (def->type != VIR_STORAGE_POOL_RBD) {
         virBufferAddLit(&buf,"  <target>\n");

diff --git i/src/storage/storage_backend_rbd.c
w/src/storage/storage_backend_rbd.c
index 46bd892..3e7464c 100644
--- i/src/storage/storage_backend_rbd.c
+++ w/src/storage/storage_backend_rbd.c
@@ -131,7 +131,7 @@ static int
virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr *ptr,
         }
     }

-    VIR_DEBUG("Found %Zu RADOS cluster monitors in the pool configuration",
+    VIR_DEBUG("Found %zu RADOS cluster monitors in the pool configuration",
               pool->def->source.nhost);

     for (i = 0; i < pool->def->source.nhost; i++) {


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

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]