[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v19 5/5] block/gluster: add support for multiple
|
From: |
Prasanna Kalever |
|
Subject: |
Re: [Qemu-devel] [PATCH v19 5/5] block/gluster: add support for multiple gluster servers |
|
Date: |
Mon, 18 Jul 2016 17:21:40 +0530 |
On Mon, Jul 18, 2016 at 3:47 PM, Markus Armbruster <address@hidden> wrote:
> Prasanna Kumar Kalever <address@hidden> writes:
>
>> This patch adds a way to specify multiple volfile servers to the gluster
>> block backend of QEMU with tcp|rdma transport types and their port numbers.
>>
>> Problem:
>>
>> Currently VM Image on gluster volume is specified like this:
>>
>> file=gluster[+tcp]://host[:port]/testvol/a.img
>>
>> Say we have three hosts in a trusted pool with replica 3 volume in action.
>> When the host mentioned in the command above goes down for some reason,
>> the other two hosts are still available. But there's currently no way
>> to tell QEMU about them.
>>
>> Solution:
>>
>> New way of specifying VM Image on gluster volume with volfile servers:
>> (We still support old syntax to maintain backward compatibility)
>>
>> Basic command line syntax looks like:
>>
>> Pattern I:
>> -drive driver=gluster,
>> volume=testvol,path=/path/a.raw,[debug=N,]
>> server.0.type=tcp,
>> server.0.host=1.2.3.4,
>> [server.0.port=24007,]
>> server.1.type=unix,
>> server.1.socket=/path/socketfile
>>
>> Pattern II:
>> 'json:{"driver":"qcow2","file":{"driver":"gluster",
>> "volume":"testvol","path":"/path/a.qcow2",["debug":N,]
>> "server":[{hostinfo_1}, ...{hostinfo_N}]}}'
>>
>> driver => 'gluster' (protocol name)
>> volume => name of gluster volume where our VM image resides
>> path => absolute path of image in gluster volume
>> [debug] => libgfapi loglevel [(0 - 9) default 4 -> Error]
>>
>> {hostinfo} => {{type:"tcp",host:"1.2.3.4"[,port=24007]},
>> {type:"unix",socket:"/path/sockfile"}}
>>
>> type => transport type used to connect to gluster management
>> daemon,
>> it can be tcp|unix
>> host => host address (hostname/ipv4/ipv6 addresses/socket path)
>> [port] => port number on which glusterd is listening. (default 24007)
>> socket => path to socket file
>>
>> Examples:
>> 1.
>> -drive driver=qcow2,file.driver=gluster,
>> file.volume=testvol,file.path=/path/a.qcow2,file.debug=9,
>> file.server.0.type=tcp,
>> file.server.0.host=1.2.3.4,
>> file.server.0.port=24007,
>> file.server.1.type=tcp,
>> file.server.1.socket=/var/run/glusterd.socket
>> 2.
>> 'json:{"driver":"qcow2","file":{"driver":"gluster","volume":"testvol",
>> "path":"/path/a.qcow2","debug":9,"server":
>> [{type:"tcp",host:"1.2.3.4",port=24007},
>> {type:"unix",socket:"/var/run/glusterd.socket"}] } }'
>>
>> This patch gives a mechanism to provide all the server addresses, which are
>> in
>> replica set, so in case host1 is down VM can still boot from any of the
>> active hosts.
>>
>> This is equivalent to the backup-volfile-servers option supported by
>> mount.glusterfs (FUSE way of mounting gluster volume)
>>
>> credits: sincere thanks to all the supporters
>>
>> Signed-off-by: Prasanna Kumar Kalever <address@hidden>
>> ---
>> block/gluster.c | 347
>> +++++++++++++++++++++++++++++++++++++++++++++------
>> qapi/block-core.json | 2 +-
>> 2 files changed, 307 insertions(+), 42 deletions(-)
>>
>> diff --git a/block/gluster.c b/block/gluster.c
>> index ff1e783..fd2279d 100644
>> --- a/block/gluster.c
>> +++ b/block/gluster.c
>> @@ -12,8 +12,16 @@
>> #include "block/block_int.h"
>> #include "qapi/error.h"
>> #include "qemu/uri.h"
>> +#include "qemu/error-report.h"
>>
>> #define GLUSTER_OPT_FILENAME "filename"
>> +#define GLUSTER_OPT_VOLUME "volume"
>> +#define GLUSTER_OPT_PATH "path"
>> +#define GLUSTER_OPT_TYPE "type"
>> +#define GLUSTER_OPT_SERVER_PATTERN "server."
>> +#define GLUSTER_OPT_HOST "host"
>> +#define GLUSTER_OPT_PORT "port"
>> +#define GLUSTER_OPT_SOCKET "socket"
>> #define GLUSTER_OPT_DEBUG "debug"
>> #define GLUSTER_DEFAULT_PORT 24007
>> #define GLUSTER_DEBUG_DEFAULT 4
>> @@ -82,6 +90,77 @@ static QemuOptsList runtime_opts = {
>> },
>> };
>>
>> +static QemuOptsList runtime_json_opts = {
>> + .name = "gluster_json",
>> + .head = QTAILQ_HEAD_INITIALIZER(runtime_json_opts.head),
>> + .desc = {
>> + {
>> + .name = GLUSTER_OPT_VOLUME,
>> + .type = QEMU_OPT_STRING,
>> + .help = "name of gluster volume where VM image resides",
>> + },
>> + {
>> + .name = GLUSTER_OPT_PATH,
>> + .type = QEMU_OPT_STRING,
>> + .help = "absolute path to image file in gluster volume",
>> + },
>> + {
>> + .name = GLUSTER_OPT_DEBUG,
>> + .type = QEMU_OPT_NUMBER,
>> + .help = "Gluster log level, valid range is 0-9",
>> + },
>> + { /* end of list */ }
>> + },
>> +};
>> +
>> +static QemuOptsList runtime_type_opts = {
>> + .name = "gluster_type",
>> + .head = QTAILQ_HEAD_INITIALIZER(runtime_type_opts.head),
>> + .desc = {
>> + {
>> + .name = GLUSTER_OPT_TYPE,
>> + .type = QEMU_OPT_STRING,
>> + .help = "tcp|unix",
>> + },
>> + { /* end of list */ }
>> + },
>> +};
>> +
>> +static QemuOptsList runtime_unix_opts = {
>> + .name = "gluster_unix",
>> + .head = QTAILQ_HEAD_INITIALIZER(runtime_unix_opts.head),
>> + .desc = {
>> + {
>> + .name = GLUSTER_OPT_SOCKET,
>> + .type = QEMU_OPT_STRING,
>> + .help = "socket file path)",
>> + },
>> + { /* end of list */ }
>> + },
>> +};
>> +
>> +static QemuOptsList runtime_tcp_opts = {
>> + .name = "gluster_tcp",
>> + .head = QTAILQ_HEAD_INITIALIZER(runtime_tcp_opts.head),
>> + .desc = {
>> + {
>> + .name = GLUSTER_OPT_TYPE,
>> + .type = QEMU_OPT_STRING,
>> + .help = "tcp|unix",
>> + },
>> + {
>> + .name = GLUSTER_OPT_HOST,
>> + .type = QEMU_OPT_STRING,
>> + .help = "host address (hostname/ipv4/ipv6 addresses)",
>> + },
>> + {
>> + .name = GLUSTER_OPT_PORT,
>> + .type = QEMU_OPT_NUMBER,
>> + .help = "port number on which glusterd is listening (default
>> 24007)",
>> + },
>> + { /* end of list */ }
>> + },
>> +};
>>
>> static int parse_volume_options(BlockdevOptionsGluster *gconf, char *path)
>> {
>> @@ -157,7 +236,8 @@ static int qemu_gluster_parse_uri(BlockdevOptionsGluster
>> *gconf,
>> return -EINVAL;
>> }
>>
>> - gconf->server = gsconf = g_new0(GlusterServer, 1);
>> + gconf->server = g_new0(GlusterServerList, 1);
>> + gconf->server->value = gsconf = g_new0(GlusterServer, 1);
>>
>> /* transport */
>> if (!uri->scheme || !strcmp(uri->scheme, "gluster")) {
>> @@ -211,38 +291,34 @@ out:
>> return ret;
>> }
>>
>> -static struct glfs *qemu_gluster_init(BlockdevOptionsGluster *gconf,
>> - const char *filename, Error **errp)
>> +static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
>> + Error **errp)
>> {
>> - struct glfs *glfs = NULL;
>> + struct glfs *glfs;
>> int ret;
>> int old_errno;
>> -
>> - ret = qemu_gluster_parse_uri(gconf, filename);
>> - if (ret < 0) {
>> - error_setg(errp, "Usage: file=gluster[+transport]://[host[:port]]/"
>> - "volume/path[?socket=...]");
>> - errno = -ret;
>> - goto out;
>> - }
>> + GlusterServerList *server;
>>
>> glfs = glfs_new(gconf->volume);
>> if (!glfs) {
>> goto out;
>> }
>>
>> - if (gconf->server->type == GLUSTER_TRANSPORT_UNIX) {
>> - ret = glfs_set_volfile_server(glfs,
>> -
>> GlusterTransport_lookup[gconf->server->type],
>> - gconf->server->u.q_unix.socket, 0);
>> - } else {
>> - ret = glfs_set_volfile_server(glfs,
>> -
>> GlusterTransport_lookup[gconf->server->type],
>> - gconf->server->u.tcp.host,
>> - gconf->server->u.tcp.port);
>> - }
>> - if (ret < 0) {
>> - goto out;
>> + for (server = gconf->server; server; server = server->next) {
>> + if (server->value->type == GLUSTER_TRANSPORT_UNIX) {
>> + ret = glfs_set_volfile_server(glfs,
>> +
>> GlusterTransport_lookup[server->value->type],
>> + server->value->u.q_unix.socket,
>> 0);
>> + } else {
>> + ret = glfs_set_volfile_server(glfs,
>> +
>> GlusterTransport_lookup[server->value->type],
>> + server->value->u.tcp.host,
>> + server->value->u.tcp.port);
>
> server->value.u.tcp.port is optional. Using it without checking
> server->value.u.tcp.has_port relies on the default value being zero. We
> don't actually document that. Perhaps we should.
I have made sure to fill it in the code, also marked
gsconf->u.tcp.has_port = true;
>
>> + }
>> +
>> + if (ret < 0) {
>> + goto out;
>> + }
>> }
>>
>> ret = glfs_set_logging(glfs, "-", gconf->debug_level);
>> @@ -252,19 +328,19 @@ static struct glfs
>> *qemu_gluster_init(BlockdevOptionsGluster *gconf,
>>
>> ret = glfs_init(glfs);
>> if (ret) {
>> - if (gconf->server->type == GLUSTER_TRANSPORT_UNIX) {
>> - error_setg(errp,
>> - "Gluster connection for volume: %s, path: %s "
>> - "failed to connect on socket %s ",
>> - gconf->volume, gconf->path,
>> - gconf->server->u.q_unix.socket);
>> - } else {
>> - error_setg(errp,
>> - "Gluster connection for volume: %s, path: %s "
>> - "failed to connect host %s and port %d ",
>> - gconf->volume, gconf->path,
>> - gconf->server->u.tcp.host,
>> gconf->server->u.tcp.port);
>> + error_setg(errp, "Gluster connection for volume: %s, path: %s ",
>> + gconf->volume, gconf->path);
>> + for (server = gconf->server; server; server = server->next) {
>> + if (server->value->type == GLUSTER_TRANSPORT_UNIX) {
>> + error_append_hint(errp, "failed to connect on socket %s ",
>> + server->value->u.q_unix.socket);
>> + } else {
>> + error_append_hint(errp, "failed to connect host %s and port
>> %d ",
>> + server->value->u.tcp.host,
>> + server->value->u.tcp.port);
>> + }
>> }
>> + error_append_hint(errp, "Please refer to gluster logs for more info
>> ");
>
> Your code produces the error message "Gluster connection for volume:
> VOLUME, path: PATH ", which makes no sense.
>
> It also produces a hint that is a concatenation of one or more "failed
> to connect on FOO", followed by "Please refer to ..." without any
> punctuation, but with a trailing space.
>
> The error message must make sense on its own, without the hint.
>
> A fixed error message could look like this:
>
> Gluster connection for volume VOLUME, path PATH failed to connect on FOO,
> on BAR, and on BAZ
>
> or with a little less effort
>
> Gluster connection for volume VOLUME, path PATH failed to connect on FOO,
> BAR, BAZ
>
> or simply
>
> Can't connect to Gluster volume VOLUME, path PATH at FOO, BAR, BAZ
>
> You can't build up the error message with error_append_hint(). Using it
> to append a hint pointing to Gluster logs is fine.
okay.
>
>>
>> /* glfs_init sometimes doesn't set errno although docs suggest that
>> */
>> if (errno == 0) {
>> @@ -284,6 +360,195 @@ out:
>> return NULL;
>> }
>>
>> +static int qapi_enum_parse(const char *opt)
>> +{
>> + int i;
>> +
>> + if (!opt) {
>> + return GLUSTER_TRANSPORT__MAX;
>> + }
>> +
>> + for (i = 0; i < GLUSTER_TRANSPORT__MAX; i++) {
>> + if (!strcmp(opt, GlusterTransport_lookup[i])) {
>> + return i;
>> + }
>> + }
>> +
>> + return i;
>> +}
>> +
>> +/*
>> + * Convert the json formatted command line into qapi.
>> +*/
>> +static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
>> + QDict *options, Error **errp)
>> +{
>> + QemuOpts *opts;
>> + GlusterServer *gsconf;
>> + GlusterServerList *curr = NULL;
>> + QDict *backing_options = NULL;
>> + Error *local_err = NULL;
>> + char *str = NULL;
>> + const char *ptr;
>> + size_t num_servers;
>> + int i;
>> +
>> + /* create opts info from runtime_json_opts list */
>> + opts = qemu_opts_create(&runtime_json_opts, NULL, 0, &error_abort);
>> + qemu_opts_absorb_qdict(opts, options, &local_err);
>> + if (local_err) {
>> + goto out;
>> + }
>> +
>> + num_servers = qdict_array_entries(options, GLUSTER_OPT_SERVER_PATTERN);
>> + if (num_servers < 1) {
>> + error_setg(&local_err, "please provide 'server' option with valid "
>> + "fields in array of hostinfo ");
>
> This isn't an error message, it's instructions what to do. Such
> instructions can be useful when they're correct, but they can't replace
> an error message. The error message should state what's wrong. No
> less, no more.
Let me know, how to log the hint from a leaf function, where Error
object is not created yet.
I also feel its more like an error in the usage of the json command
>
> Moreover, avoid prefixes like "qemu_gluster:". Usually, the fact that
> this is about Gluster is obvious. When it isn't, providing context is
> the caller's job.
I think I have removed almost all 'qemu_gluster:' in v19 by respecting
you comments in v18
>
>> + goto out;
>> + }
>> +
>> + ptr = qemu_opt_get(opts, GLUSTER_OPT_VOLUME);
>> + if (!ptr) {
>> + error_setg(&local_err, "please provide 'volume' option ");
>
> Not an error message.
Also same here ...
>
>> + goto out;
>> + }
>> + gconf->volume = g_strdup(ptr);
>> +
>> + ptr = qemu_opt_get(opts, GLUSTER_OPT_PATH);
>> + if (!ptr) {
>> + error_setg(&local_err, "please provide 'path' option ");
>
> Not an error message.
here ...
>
>> + goto out;
>> + }
>> + gconf->path = g_strdup(ptr);
>> + qemu_opts_del(opts);
>> +
>> + for (i = 0; i < num_servers; i++) {
>> + str = g_strdup_printf(GLUSTER_OPT_SERVER_PATTERN"%d.", i);
>> + qdict_extract_subqdict(options, &backing_options, str);
>> +
>> + /* create opts info from runtime_type_opts list */
>> + opts = qemu_opts_create(&runtime_type_opts, NULL, 0, &error_abort);
>> + qemu_opts_absorb_qdict(opts, backing_options, &local_err);
>> + if (local_err) {
>> + goto out;
>> + }
>> +
>> + ptr = qemu_opt_get(opts, GLUSTER_OPT_TYPE);
>> + gsconf = g_new0(GlusterServer, 1);
>> + gsconf->type = qapi_enum_parse(ptr);
>> + if (!ptr) {
>> + error_setg(&local_err, "please provide 'type' in hostinfo.%d as
>> "
>> + "tcp|unix ", i);
>
> Not an error message.
and here ...
How do I say whats really wrong in the command, which could be long
(if provides N servers in the list)
>
>> + goto out;
>> +
>> + }
>> + if (gsconf->type == GLUSTER_TRANSPORT__MAX) {
>> + error_setg(&local_err, "'type' in hostinfo.%d can be tcp|unix
>> ", i);
>> + goto out;
>> + }
>> + qemu_opts_del(opts);
>> +
>> + if (gsconf->type == GLUSTER_TRANSPORT_TCP) {
>> + /* create opts info from runtime_tcp_opts list */
>> + opts = qemu_opts_create(&runtime_tcp_opts, NULL, 0,
>> &error_abort);
>> + qemu_opts_absorb_qdict(opts, backing_options, &local_err);
>> + if (local_err) {
>> + goto out;
>> + }
>> +
>> + ptr = qemu_opt_get(opts, GLUSTER_OPT_HOST);
>> + if (!ptr) {
>> + error_setg(&local_err, "please provide 'host' in
>> hostinfo.%d ",
>> + i);
>> + goto out;
>> + }
>> + gsconf->u.tcp.host = g_strdup(ptr);
>> + ptr = qemu_opt_get(opts, GLUSTER_OPT_PORT);
>> + gsconf->u.tcp.port = qemu_opt_get_number(opts, GLUSTER_OPT_PORT,
>> + GLUSTER_DEFAULT_PORT);
>> + gsconf->u.tcp.has_port = true;
>> + qemu_opts_del(opts);
>> + } else {
>> + /* create opts info from runtime_unix_opts list */
>> + opts = qemu_opts_create(&runtime_unix_opts, NULL, 0,
>> &error_abort);
>> + qemu_opts_absorb_qdict(opts, backing_options, &local_err);
>> + if (local_err) {
>> + goto out;
>> + }
>> +
>> + ptr = qemu_opt_get(opts, GLUSTER_OPT_SOCKET);
>> + if (!ptr) {
>> + error_setg(&local_err, "please provide 'socket' in
>> hostinfo.%d ",
>> + i);
>> + goto out;
>> + }
>> + gsconf->u.q_unix.socket = g_strdup(ptr);
>> + qemu_opts_del(opts);
>> + }
>> +
>> + if (gconf->server == NULL) {
>> + gconf->server = g_new0(GlusterServerList, 1);
>> + gconf->server->value = gsconf;
>> + curr = gconf->server;
>> + } else {
>> + curr->next = g_new0(GlusterServerList, 1);
>> + curr->next->value = gsconf;
>> + curr = curr->next;
>> + }
>> +
>> + qdict_del(backing_options, str);
>> + g_free(str);
>> + str = NULL;
>> + }
>> +
>> + return 0;
>> +
>> +out:
>> + if (local_err) {
>> + error_propagate(errp, local_err);
>> + }
>
> error_propagate() does the right thing when its second argument is
> null. Please drop the conditional.
Sure
>
>> + qemu_opts_del(opts);
>> + if (str) {
>> + qdict_del(backing_options, str);
>> + g_free(str);
>> + }
>> + errno = EINVAL;
>> + return -errno;
>> +}
>> +
>> +static struct glfs *qemu_gluster_init(BlockdevOptionsGluster *gconf,
>> + const char *filename,
>> + QDict *options, Error **errp)
>> +{
>> + int ret;
>> + if (filename) {
>> + ret = qemu_gluster_parse_uri(gconf, filename);
>> + if (ret < 0) {
>> + error_setg(errp, "Usage:
>> file=gluster[+transport]://[host[:port]]/"
>> + "volume/path[?socket=...]");
>
> Not an error message.
Can you suggest the change required here for displaying hints from a
leaf function, I am worried for missing your intention here, since you
have most of your v18 comments here.
IIRC, the leaf function which wants to display out a hint/msg/error
should have an Error object, which is created by error_setg() and
error_append_hind() appends to it.
Sorry for disappointing you Markus.
Thanks,
--
Prasanna
>
>> + errno = -ret;
>> + return NULL;
>> + }
>> + } else {
>> + ret = qemu_gluster_parse_json(gconf, options, errp);
>> + if (ret < 0) {
>> + error_append_hint(errp, "Usage: "
>> + "-drive driver=qcow2,file.driver=gluster,"
>> + "file.volume=testvol,file.path=/path/a.qcow2"
>> + "[,file.debug=9],file.server.0.type=tcp,"
>> + "file.server.0.host=1.2.3.4,"
>> + "[file.server.0.port=24007,]"
>> + "file.server.1.transport=unix,"
>> + "file.server.1.socket=/var/run/glusterd.socket
>> ...");
>> + errno = -ret;
>> + return NULL;
>> + }
>> +
>> + }
>> +
>> + return qemu_gluster_glfs_init(gconf, errp);
>> +}
>> +
>> static void qemu_gluster_complete_aio(void *opaque)
>> {
>> GlusterAIOCB *acb = (GlusterAIOCB *)opaque;
>> @@ -383,7 +648,7 @@ static int qemu_gluster_open(BlockDriverState *bs,
>> QDict *options,
>> gconf = g_new0(BlockdevOptionsGluster, 1);
>> gconf->debug_level = s->debug_level;
>> gconf->has_debug_level = true;
>> - s->glfs = qemu_gluster_init(gconf, filename, errp);
>> + s->glfs = qemu_gluster_init(gconf, filename, options, errp);
>> if (!s->glfs) {
>> ret = -errno;
>> goto out;
>> @@ -454,7 +719,7 @@ static int qemu_gluster_reopen_prepare(BDRVReopenState
>> *state,
>> gconf = g_new0(BlockdevOptionsGluster, 1);
>> gconf->debug_level = s->debug_level;
>> gconf->has_debug_level = true;
>> - reop_s->glfs = qemu_gluster_init(gconf, state->bs->filename, errp);
>> + reop_s->glfs = qemu_gluster_init(gconf, state->bs->filename, NULL,
>> errp);
>> if (reop_s->glfs == NULL) {
>> ret = -errno;
>> goto exit;
>> @@ -601,7 +866,7 @@ static int qemu_gluster_create(const char *filename,
>> }
>> gconf->has_debug_level = true;
>>
>> - glfs = qemu_gluster_init(gconf, filename, errp);
>> + glfs = qemu_gluster_init(gconf, filename, NULL, errp);
>> if (!glfs) {
>> ret = -errno;
>> goto out;
>> @@ -981,7 +1246,7 @@ static BlockDriver bdrv_gluster = {
>> .format_name = "gluster",
>> .protocol_name = "gluster",
>> .instance_size = sizeof(BDRVGlusterState),
>> - .bdrv_needs_filename = true,
>> + .bdrv_needs_filename = false,
>> .bdrv_file_open = qemu_gluster_open,
>> .bdrv_reopen_prepare = qemu_gluster_reopen_prepare,
>> .bdrv_reopen_commit = qemu_gluster_reopen_commit,
>> @@ -1009,7 +1274,7 @@ static BlockDriver bdrv_gluster_tcp = {
>> .format_name = "gluster",
>> .protocol_name = "gluster+tcp",
>> .instance_size = sizeof(BDRVGlusterState),
>> - .bdrv_needs_filename = true,
>> + .bdrv_needs_filename = false,
>> .bdrv_file_open = qemu_gluster_open,
>> .bdrv_reopen_prepare = qemu_gluster_reopen_prepare,
>> .bdrv_reopen_commit = qemu_gluster_reopen_commit,
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index d7b5c76..5557f1c 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -2137,7 +2137,7 @@
>> { 'struct': 'BlockdevOptionsGluster',
>> 'data': { 'volume': 'str',
>> 'path': 'str',
>> - 'server': 'GlusterServer',
>> + 'server': ['GlusterServer'],
>> '*debug_level': 'int' } }
>>
>> ##
- Re: [Qemu-devel] [PATCH v19 3/5] block/gluster: remove rdma transport, (continued)
[Qemu-devel] [PATCH v19 4/5] block/gluster: using new qapi schema, Prasanna Kumar Kalever, 2016/07/15
[Qemu-devel] [PATCH v19 5/5] block/gluster: add support for multiple gluster servers, Prasanna Kumar Kalever, 2016/07/15