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 <brick_name> for <volume_name>".

Change-Id: I151284ef78c25f52d1b39cdbd71ebfb9eb4b8471
BUG: 1121584
Signed-off-by: Gaurav Kumar Garg <ggarg@redhat.com>
Reviewed-on: http://review.gluster.org/9681
Tested-by: Gluster Build System <jenkins@build.gluster.com>
Reviewed-by: Krishnan Parthasarathi <kparthas@redhat.com>
Tested-by: Krishnan Parthasarathi <kparthas@redhat.com>

6 files changed, 105 insertions, 41 deletions

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)
 {