From 2788ddd3a0afa98f78128247cca89427a323b090 Mon Sep 17 00:00:00 2001 From: Gaurav Kumar Garg Date: Sun, 15 Feb 2015 19:22:13 +0530 Subject: glusterd: remove-brick status/stop should not show output for non-existing brick Previously when user start remove-brick operation on a volume then by giving non-existing brick for remove-brick status/stop command it was showing remove-brick status/stoping remove-brick operation on a volume. With this fix it will validate bricks which user have given for remove-brick status/stop command and if bricks are part of volume then it will show statistics of remove-brick operation otherwise it will show error "Incorrect brick for ". Change-Id: I151284ef78c25f52d1b39cdbd71ebfb9eb4b8471 BUG: 1121584 Signed-off-by: Gaurav Kumar Garg Reviewed-on: http://review.gluster.org/9681 Tested-by: Gluster Build System Reviewed-by: Krishnan Parthasarathi Tested-by: Krishnan Parthasarathi --- cli/src/cli-cmd-parser.c | 8 ++- cli/src/cli-rpc-ops.c | 22 +------ tests/bugs/distribute/bug-1122443.t | 3 +- tests/bugs/glusterd/bug-1120647.t | 3 +- ...sting-validation-for-remove-brick-status-stop.t | 34 ++++++++++ xlators/mgmt/glusterd/src/glusterd-rebalance.c | 76 +++++++++++++++++----- 6 files changed, 105 insertions(+), 41 deletions(-) create mode 100644 tests/bugs/glusterd/bug-1121584-brick-existing-validation-for-remove-brick-status-stop.t diff --git a/cli/src/cli-cmd-parser.c b/cli/src/cli-cmd-parser.c index a334fd931bf..140b021e363 100644 --- a/cli/src/cli-cmd-parser.c +++ b/cli/src/cli-cmd-parser.c @@ -1721,9 +1721,11 @@ cli_cmd_volume_remove_brick_parse (const char **words, int wordcount, goto out; } - ret = dict_set_int32 (dict, "count", brick_count); - if (ret) - goto out; + if (command != GF_OP_CMD_STATUS && command != GF_OP_CMD_STOP) { + ret = dict_set_int32 (dict, "count", brick_count); + if (ret) + goto out; + } *options = dict; diff --git a/cli/src/cli-rpc-ops.c b/cli/src/cli-rpc-ops.c index 6bfe78e4354..5a95cff88e3 100644 --- a/cli/src/cli-rpc-ops.c +++ b/cli/src/cli-rpc-ops.c @@ -3925,7 +3925,7 @@ out: int32_t gf_cli_remove_brick (call_frame_t *frame, xlator_t *this, - void *data) + void *data) { gf_cli_req req = {{0,}};; gf_cli_req status_req = {{0,}};; @@ -3933,7 +3933,6 @@ gf_cli_remove_brick (call_frame_t *frame, xlator_t *this, dict_t *dict = NULL; int32_t command = 0; char *volname = NULL; - dict_t *req_dict = NULL; int32_t cmd = 0; if (!frame || !this || !data) { @@ -3961,25 +3960,12 @@ gf_cli_remove_brick (call_frame_t *frame, xlator_t *this, cli_rpc_prog, NULL); } else { /* Need rebalance status to be sent :-) */ - req_dict = dict_new (); - if (!req_dict) { - ret = -1; - goto out; - } - - ret = dict_set_str (req_dict, "volname", volname); - if (ret) { - gf_log (this->name, GF_LOG_ERROR, - "Failed to set dict"); - goto out; - } - if (command == GF_OP_CMD_STATUS) cmd |= GF_DEFRAG_CMD_STATUS; else cmd |= GF_DEFRAG_CMD_STOP; - ret = dict_set_int32 (req_dict, "rebalance-command", (int32_t) cmd); + ret = dict_set_int32 (dict, "rebalance-command", (int32_t) cmd); if (ret) { gf_log (this->name, GF_LOG_ERROR, "Failed to set dict"); @@ -3988,15 +3974,13 @@ gf_cli_remove_brick (call_frame_t *frame, xlator_t *this, ret = cli_to_glusterd (&status_req, frame, gf_cli3_remove_brick_status_cbk, - (xdrproc_t) xdr_gf_cli_req, req_dict, + (xdrproc_t) xdr_gf_cli_req, dict, GLUSTER_CLI_DEFRAG_VOLUME, this, cli_rpc_prog, NULL); } out: - if (req_dict) - dict_unref (req_dict); gf_log ("cli", GF_LOG_DEBUG, "Returning %d", ret); GF_FREE (req.dict.dict_val); diff --git a/tests/bugs/distribute/bug-1122443.t b/tests/bugs/distribute/bug-1122443.t index 3e2455e6382..d944a066eba 100644 --- a/tests/bugs/distribute/bug-1122443.t +++ b/tests/bugs/distribute/bug-1122443.t @@ -1,6 +1,7 @@ #!/bin/bash . $(dirname $0)/../../include.rc +. $(dirname $0)/../../volume.rc . $(dirname $0)/../../dht.rc make_files() { @@ -46,7 +47,7 @@ BEFORE="$(stat -c %n:%Y $M0/subdir/* | tr '\n' ',')" # Migrate brick TEST $CLI volume add-brick $V0 $H0:$B0/${V0}1 TEST $CLI volume remove-brick $V0 $H0:$B0/${V0}0 start -EXPECT_WITHIN $REBALANCE_TIMEOUT "0" remove_brick_completed +EXPECT_WITHIN 10 "completed" remove_brick_status_completed_field "$V0 $H0:$B0/${V0}0" TEST $CLI volume remove-brick $V0 $H0:$B0/${V0}0 commit # Get mtime after migration diff --git a/tests/bugs/glusterd/bug-1120647.t b/tests/bugs/glusterd/bug-1120647.t index 0223f460398..90d069ca502 100644 --- a/tests/bugs/glusterd/bug-1120647.t +++ b/tests/bugs/glusterd/bug-1120647.t @@ -10,7 +10,8 @@ TEST pidof glusterd TEST $CLI volume create $V0 replica 2 $H0:$B0/brick{1..4} TEST $CLI volume start $V0 TEST $CLI volume remove-brick $V0 $H0:$B0/brick{3..4} start -EXPECT_WITHIN 10 "completed" remove_brick_status_completed_field "$V0 $H0:$B0/brick{3..4}" +EXPECT_WITHIN 10 "completed" remove_brick_status_completed_field "$V0 $H0:$B0/brick3" +EXPECT_WITHIN 10 "completed" remove_brick_status_completed_field "$V0 $H0:$B0/brick4" TEST $CLI volume remove-brick $V0 $H0:$B0/brick{3..4} commit TEST $CLI volume remove-brick $V0 replica 1 $H0:$B0/brick2 force diff --git a/tests/bugs/glusterd/bug-1121584-brick-existing-validation-for-remove-brick-status-stop.t b/tests/bugs/glusterd/bug-1121584-brick-existing-validation-for-remove-brick-status-stop.t new file mode 100644 index 00000000000..de80afcc2eb --- /dev/null +++ b/tests/bugs/glusterd/bug-1121584-brick-existing-validation-for-remove-brick-status-stop.t @@ -0,0 +1,34 @@ +#!/bin/bash + +## Test case for BZ-1121584. Execution of remove-brick status/stop command +## should give error for brick which is not part of volume. + +. $(dirname $0)/../../include.rc +. $(dirname $0)/../../volume.rc +. $(dirname $0)/../../dht.rc + +cleanup; + +## Start glusterd +TEST glusterd +TEST pidof glusterd + +## Lets Create and start volume +TEST $CLI volume create $V0 $H0:$B0/${V0}1 $H0:$B0/${V0}2 +TEST $CLI volume start $V0 + +## Start remove-brick operation on the volume +TEST $CLI volume remove-brick $V0 $H0:$B0/${V0}1 start + +## By giving non existing brick for remove-brick status/stop command should +## give error. +TEST ! $CLI volume remove-brick $V0 $H0:$B0/ABCD status +TEST ! $CLI volume remove-brick $V0 $H0:$B0/ABCD stop + +## By giving brick which is part of volume for remove-brick status/stop command +## should print statistics of remove-brick operation or stop remove-brick +## operation. +TEST $CLI volume remove-brick $V0 $H0:$B0/${V0}1 status +TEST $CLI volume remove-brick $V0 $H0:$B0/${V0}1 stop + +cleanup; diff --git a/xlators/mgmt/glusterd/src/glusterd-rebalance.c b/xlators/mgmt/glusterd/src/glusterd-rebalance.c index ad7038b0e51..f5bb319cb7d 100644 --- a/xlators/mgmt/glusterd/src/glusterd-rebalance.c +++ b/xlators/mgmt/glusterd/src/glusterd-rebalance.c @@ -526,19 +526,48 @@ glusterd_handle_defrag_volume (rpcsvc_request_t *req) return glusterd_big_locked_handler (req, __glusterd_handle_defrag_volume); } +static int +glusterd_brick_validation (dict_t *dict, char *key, data_t *value, + void *data) +{ + int32_t ret = -1; + xlator_t *this = NULL; + glusterd_volinfo_t *volinfo = data; + glusterd_brickinfo_t *brickinfo = NULL; + + this = THIS; + GF_ASSERT (this); + + ret = glusterd_volume_brickinfo_get_by_brick (value->data, volinfo, + &brickinfo); + if (ret) { + gf_log (this->name, GF_LOG_ERROR, "Incorrect brick %s for " + "volume %s", value->data, volinfo->volname); + return ret; + } + + if (!brickinfo->decommissioned) { + gf_log (this->name, GF_LOG_ERROR, "Incorrect brick %s for " + "volume %s", value->data, volinfo->volname); + ret = -1; + return ret; + } + + return ret; +} int glusterd_op_stage_rebalance (dict_t *dict, char **op_errstr) { - char *volname = NULL; - char *cmd_str = NULL; - int ret = 0; - int32_t cmd = 0; - char msg[2048] = {0}; - glusterd_volinfo_t *volinfo = NULL; + char *volname = NULL; + char *cmd_str = NULL; + int ret = 0; + int32_t cmd = 0; + char msg[2048] = {0}; + glusterd_volinfo_t *volinfo = NULL; char *task_id_str = NULL; - dict_t *op_ctx = NULL; - xlator_t *this = 0; + dict_t *op_ctx = NULL; + xlator_t *this = 0; this = THIS; GF_ASSERT (this); @@ -630,17 +659,31 @@ glusterd_op_stage_rebalance (dict_t *dict, char **op_errstr) ret = -1; goto out; } - if ((strstr(cmd_str,"rebalance") != NULL) && - (volinfo->rebal.op != GD_OP_REBALANCE)){ - snprintf (msg,sizeof(msg),"Rebalance not started."); + if ((strstr(cmd_str, "rebalance") != NULL) && + (volinfo->rebal.op != GD_OP_REBALANCE)) { + snprintf (msg, sizeof(msg), "Rebalance not started."); ret = -1; goto out; } - if ((strstr(cmd_str,"remove-brick")!= NULL) && - (volinfo->rebal.op != GD_OP_REMOVE_BRICK)){ - snprintf (msg,sizeof(msg),"remove-brick not started."); - ret = -1; - goto out; + if (strstr(cmd_str, "remove-brick") != NULL) { + if (volinfo->rebal.op != GD_OP_REMOVE_BRICK) { + snprintf (msg, sizeof(msg), "remove-brick not " + "started."); + ret = -1; + goto out; + } + + /* For remove-brick status/stop command check whether + * given input brick is part of volume or not.*/ + + ret = dict_foreach_fnmatch (dict, "brick*", + glusterd_brick_validation, + volinfo); + if (ret == -1) { + snprintf (msg, sizeof (msg), "Incorrect brick" + " for volume %s", volinfo->volname); + goto out; + } } break; default: @@ -655,7 +698,6 @@ out: return ret; } - int glusterd_op_rebalance (dict_t *dict, char **op_errstr, dict_t *rsp_dict) { -- cgit