On Tue, Jun 12, 2012 at 10:31:54AM +0200, Sebastian Wiedenroth wrote:
>
> Signed-off-by: Sebastian Wiedenroth <wiedi@xxxxxxxxxx>
Can you fill out the commit message with some descriptive text.
At least illustrate the pool XML format and the volume XML
format, describe what operations are supported, and illustrate
how the data from the volume XML doc is to be used to create a
corresponding <disk> element for a virtual machine.
> ---
>
> This patch adds support for sheepdog pools and volumes.
> Thanks go to Frank Spijkerman who started the initial work on this
> and Wido den Hollander for his RBD support on which this is partly based.
Nice job, particularly like that you added test cases !
> diff --git a/configure.ac b/configure.ac
> index dda764d..774a9dd 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -1807,6 +1807,8 @@ AC_ARG_WITH([storage-disk],
> AC_HELP_STRING([--with-storage-disk], [with GPartd Disk backend for the storage driver @<:@default=check@:>@]),[],[with_storage_disk=check])
> AC_ARG_WITH([storage-rbd],
> AC_HELP_STRING([--with-storage-rbd], [with RADOS Block Device backend for the storage driver @<:@default=check@:>@]),[],[with_storage_rbd=check])
> +AC_ARG_WITH([storage-sheepdog],
> + AC_HELP_STRING([--with-storage-sheepdog], [with Sheepdog backend for the storage driver @<:@default=check@:>@]),[],[with_storage_sheepdog=check])
>
> if test "$with_libvirtd" = "no"; then
> with_storage_dir=no
> @@ -1817,6 +1819,7 @@ if test "$with_libvirtd" = "no"; then
> with_storage_mpath=no
> with_storage_disk=no
> with_storage_rbd=no
> + with_storage_sheepdog=no
> fi
> if test "$with_storage_dir" = "yes" ; then
> AC_DEFINE_UNQUOTED([WITH_STORAGE_DIR], 1, [whether directory backend for storage driver is enabled])
> @@ -1991,6 +1994,25 @@ fi
> AM_CONDITIONAL([WITH_STORAGE_RBD], [test "$with_storage_rbd" = "yes"])
> AC_SUBST([LIBRBD_LIBS])
>
> +if test "$with_storage_sheepdog" = "yes" || test "$with_storage_sheepdog" = "check"; then
> + AC_PATH_PROG([COLLIE], [collie], [], [$PATH:/sbin:/usr/sbin])
> +
> + if test "$with_storage_sheepdog" = "yes" ; then
> + if test -z "$COLLIE" ; then AC_MSG_ERROR([We need collie for Sheepdog storage driver]) ; fi
> + else
> + if test -z "$COLLIE" ; then with_storage_sheepdog=no ; fi
> +
> + if test "$with_storage_sheepdog" = "check" ; then with_storage_sheepdog=yes ; fi
> + fi
> +
> + if test "$with_storage_sheepdog" = "yes" ; then
> + AC_DEFINE_UNQUOTED([WITH_STORAGE_SHEEPDOG], 1, [whether Sheepdog backend for storage driver is enabled])
> + AC_DEFINE_UNQUOTED([COLLIE],["$COLLIE"],[Location of collie program])
> + fi
> +fi
> +AM_CONDITIONAL([WITH_STORAGE_SHEEPDOG], [test "$with_storage_sheepdog" = "yes"])
You're adding a requirement for the 'collie' command here, and
a new '--with-storage-sheepdog' arg here. To go along with this
you should modify the libvirt.spec.in, to have a Requires line
on the RPM which provides 'collie', and also conditionally set
the --without-storage-sheepdog arg so that it is only enabled
for Fedora >= 16 (or whatever the min version was).
If you arent' familiar with RPM specfiles, just look in the
libvirt.spec.in file for any place mentioning 'with_storage_rbd'
and copy that style. The sheepdog stuff will be pretty much
exactly the same.
> diff --git a/docs/storage.html.in b/docs/storage.html.in
> index b3484e8..8730164 100644
> --- a/docs/storage.html.in
> +++ b/docs/storage.html.in
> @@ -110,6 +110,9 @@
> <li>
> <a href="#StorageBackendRBD">RBD (RADOS Block Device) backend</a>
> </li>
> + <li>
> + <a href="#StorageBackendSheepdog">Sheepdog backend</a>
> + </li>
> </ul>
>
> <h2><a name="StorageBackendDir">Directory pool</a></h2>
> @@ -565,5 +568,64 @@
> The RBD pool does not use the volume format type element.
> </p>
>
> + <h2><a name="StorageBackendSheepdog">Sheepdog pools</a></h2>
> + <p>
> + This provides a pool based on a Sheepdog Cluster.
> + Sheepdog is a distributed storage system for QEMU/KVM.
> + It provides highly available block level storage volumes that
> + can be attached to QEMU/KVM virtual machines.
> +
> + The cluster must already be formated.
> +
> + <span class="since">Since 0.9.13</span>
> + </p>
> +
> + <h3>Example pool input</h3>
> + <pre>
> + <pool type="sheepdog">
> + <name>mysheeppool</name>
> + <source>
> + <name>mysheeppool</name>
> + <host name='localhost' port='7000'/>
I think that host line is indented 2 spaces too many
> + </source>
> + </pool></pre>
> +
> + <h3>Example volume output</h3>
> + <pre>
> + <volume>
> + <name>myvol</name>
> + <key>sheep/myvol</key>
> + <source>
> + </source>
> + <capacity unit='bytes'>53687091200</capacity>
> + <allocation unit='bytes'>53687091200</allocation>
> + <target>
> + <path>sheepdog:myvol</path>
> + <format type='unknown'/>
> + <permissions>
> + <mode>00</mode>
> + <owner>0</owner>
> + <group>0</group>
> + </permissions>
> + </target>
> + </volume></pre>
> +
> + <h3>Example disk attachement</h3>
> + <p>Sheepdog images can be attached to Qemu guests.
> + Information about attaching a Sheepdog image to a
> + guest can be found
> + at the <a href="formatdomain.html#elementsDisks">format domain</a>
> + page.</p>
> +
> + <h3>Valid pool format types</h3>
> + <p>
> + The Sheepdog pool does not use the pool format type element.
> + </p>
> +
> + <h3>Valid volume format types</h3>
> + <p>
> + The Sheepdog pool does not use the volume format type element.
> + </p>
> +
> </body>
> </html>
> diff --git a/po/POTFILES.in b/po/POTFILES.in
> diff --git a/src/storage/storage_backend_sheepdog.c b/src/storage/storage_backend_sheepdog.c
> new file mode 100644
> index 0000000..b003856
> --- /dev/null
> +++ b/src/storage/storage_backend_sheepdog.c
> +static int
> +virStorageBackendSheepdogDeleteVol(virConnectPtr conn ATTRIBUTE_UNUSED,
> + virStoragePoolObjPtr pool
> + ATTRIBUTE_UNUSED,
Can you make sure the ATTRIBUTE_UNUSED annotation is on the same line as
the parameter it refers to - even if this makes the line go over 80 chars.
> + virStorageVolDefPtr vol,
> + unsigned int flags)
> +{
> +
> + if (flags & VIR_STORAGE_VOL_DELETE_ZEROED) {
> + VIR_WARN("%s", _("This storage backend does not supported zeroed removal of volumes"));
> + }
It is preferrable to use the virCheckFlags() macro instead of
this, so the caller gets a clear error.
> +
> + virCommandPtr cmd = virCommandNew(COLLIE);
> +
> + virCommandAddArgList(cmd, "vdi", "delete", vol->name, NULL);
> + virStorageBackendSheepdogAddHostArg(cmd, pool);
> + int ret = virCommandRun(cmd, NULL);
> +
> + virCommandFree(cmd);
> + return ret;
> +}
> +
> +
> +static int
> +virStorageBackendSheepdogCreateVol(virConnectPtr conn ATTRIBUTE_UNUSED,
> + virStoragePoolObjPtr pool
> + ATTRIBUTE_UNUSED,
Same here
> + virStorageVolDefPtr vol)
> +{
> +
> + int ret;
> +
> + if (vol->target.encryption != NULL) {
> + virStorageReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("storage pool does not support encrypted volumes"));
> + return -1;
I'd suggest s/storage pool/Sheepdog/
> + }
> +
> + virCommandPtr cmd = virCommandNew(COLLIE);
> +
> + virCommandAddArgList(cmd, "vdi", "create", vol->name, NULL);
> + virCommandAddArgFormat(cmd, "%llu", vol->capacity);
> + virStorageBackendSheepdogAddHostArg(cmd, pool);
> + ret = virCommandRun(cmd, NULL);
> +
> + virStorageBackendSheepdogRefreshVol(conn, pool, vol);
> +
> + virCommandFree(cmd);
> + return ret;
> +}
> +
> +static int
> +virStorageBackendSheepdogRefreshVol(virConnectPtr conn ATTRIBUTE_UNUSED,
> + virStoragePoolObjPtr pool
> + ATTRIBUTE_UNUSED,
And here
> + virStorageVolDefPtr vol)
> +{
> + int ret;
> +
> + char *output = NULL;
> +
> + virCommandPtr cmd;
Can remove those 2 blank lines between variable decls.
> +
> + cmd = virCommandNew(COLLIE);
> + virCommandAddArgList(cmd, "vdi", "list", vol->name, "-r", NULL);
> + virStorageBackendSheepdogAddHostArg(cmd, pool);
> + virCommandSetOutputBuffer(cmd, &output);
> + ret = virCommandRun(cmd, NULL);
> +
> + if (ret < 0)
> + goto cleanup;
> +
> + if ((ret = virStorageBackendSheepdogParseVdiList(vol, output)) < 0)
> + goto cleanup;
> +
> + vol->type = VIR_STORAGE_VOL_NETWORK;
> +
> + VIR_FREE(vol->key);
> + if (virAsprintf(&vol->key, "%s/%s",
> + pool->def->source.name, vol->name) == -1) {
> + virReportOOMError();
> + goto cleanup;
> + }
Does sheepdog associate any kind of globally unique UUID with
storage volumes ? If so, it'd be desirable to use that as the
key, instead of being name based, so that we have stronger
uniqueness & stability guarentees.
> +
> + VIR_FREE(vol->target.path);
> + if (virAsprintf(&vol->target.path, "sheepdog:%s", vol->name) == -1) {
> + virReportOOMError();
> + goto cleanup;
> + }
> +
> +
> +
> +cleanup:
> + virCommandFree(cmd);
> + return ret;
> +}
> +
> +
> +static int
> +virStorageBackendSheepdogResizeVol(virConnectPtr conn ATTRIBUTE_UNUSED,
> + virStoragePoolObjPtr pool
> + ATTRIBUTE_UNUSED,
And here
> + virStorageVolDefPtr vol,
> + unsigned long long capacity,
> + unsigned int flags)
> +{
> +
> + virCheckFlags(0, -1);
> +
> + virCommandPtr cmd = virCommandNew(COLLIE);
> +
> + virCommandAddArgList(cmd, "vdi", "resize", vol->name, NULL);
> + virCommandAddArgFormat(cmd, "%llu", capacity);
> + virStorageBackendSheepdogAddHostArg(cmd, pool);
> + int ret = virCommandRun(cmd, NULL);
> +
> + virCommandFree(cmd);
> + return ret;
> +
> +}
> +
> +
All in all, i think this looks good
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list