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

21 May 2012

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 @@
        </volume></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

    