Disable brick daemon from incorrect brick directory

Summary:
Currently the bricks can open any mount directory from the given volume. This patch adds a provision to prevent
bricks from opening brick directories that aren't created for them. This will help with operating gluster on large
scale.

We add a new xattr GF_XATTR_BRICK_NAME to the brick directory. When we start a brick daemon, we make sure the path on
disk matches with the config provided. For backward compatibility, we ignore if there is no value for
GF_XATTR_BRICK_NAME and set the current brick daemon's path as value.

We ignore GF_XATTR_BRICK_NAME during healing and reset GF_XATTR_BRICK_NAME on brick replace.

Test Plan: Run fb-smoke

Reviewers: jdarcy, sshreyas

Reviewed By: sshreyas

Differential Revision: https://phabricator.intern.facebook.com/D5448921

Porting note: disabled some checks to deal with the snapshot case

Change-Id: I98e62033dfd07f30ad3b99ac003ce94c8d935e5f
Signed-off-by: Jeff Darcy <jdarcy@fb.com>
Reviewed-on: https://review.gluster.org/18275
Reviewed-by: Jeff Darcy <jeff@pl.atyp.us>
Tested-by: Jeff Darcy <jeff@pl.atyp.us>
CentOS-regression: Gluster Build System <jenkins@build.gluster.org>
Smoke: Gluster Build System <jenkins@build.gluster.org>

11 files changed, 247 insertions, 1 deletions

diff --git a/libglusterfs/src/common-utils.c b/libglusterfs/src/common-utils.c
index dbb33812287..d4ddff2090e 100644
--- a/libglusterfs/src/common-utils.c
+++ b/libglusterfs/src/common-utils.c
@@ -4578,3 +4578,59 @@ err:
 out:
         return ret;
 }
+
+
+int
+gf_compare_or_fix_xattr_brick_path (const char* path)
+{
+        xlator_t* this = NULL;
+        char brick_path_ondisk[PATH_MAX] = "";
+        int ret = -1;
+
+        this = THIS;
+
+        GF_ASSERT (path);
+
+        /* Check for brickinfo->path to GF_XATTR_BRICK_PATH xattr
+         * If present compare to brickinfo->path
+         * If not present set GF_XATTR_BRICK_PATH to brickinfo->path
+         */
+         ret = sys_lgetxattr (path, GF_XATTR_BRICK_PATH, brick_path_ondisk,
+                              min (strlen (path), PATH_MAX));
+
+        if (ret == -1) {
+                gf_log (this->name, GF_LOG_WARNING, "Failed to get "
+                        "extended attribute %s for brick dir %s. "
+                        "Reason : %s. Check ignored.", GF_XATTR_BRICK_PATH,
+                        path, strerror (errno));
+
+                if (errno == ENODATA) {
+                        ret = sys_lsetxattr (path, GF_XATTR_BRICK_PATH,
+                                             path, strlen (path), XATTR_CREATE);
+
+                        if (ret == -1) {
+                                gf_log (this->name, GF_LOG_ERROR, "Error "
+                                        "setting %s xattr. Reason: %s",
+                                        GF_XATTR_BRICK_PATH, strerror (errno));
+                        }
+
+                        ret = 0;
+                        goto out;
+                }
+
+                ret = -1;
+                goto out;
+        }
+
+        GF_ASSERT (ret >= 0);
+        ret = strcmp (brick_path_ondisk, path);
+
+        if (ret != 0) {
+                gf_log (this->name, GF_LOG_ERROR,
+                        "Brick path mismatch for brick " "%s. Found %s",
+                        path, brick_path_ondisk);
+        }
+
+out:
+        return ret;
+}
diff --git a/libglusterfs/src/common-utils.h b/libglusterfs/src/common-utils.h
index ae96c9bc1a1..d917671fedf 100644
--- a/libglusterfs/src/common-utils.h
+++ b/libglusterfs/src/common-utils.h
@@ -849,4 +849,7 @@ gf_bits_index (uint64_t n);
 int
 gf_peerinfo_to_hostname_and_port (const char *peerinfo, char **hostname);
 
+int
+gf_compare_or_fix_xattr_brick_path (const char* path);
+
 #endif /* _COMMON_UTILS_H */
diff --git a/libglusterfs/src/glusterfs.h b/libglusterfs/src/glusterfs.h
index dbe18f2c243..0b033d8bfcf 100644
--- a/libglusterfs/src/glusterfs.h
+++ b/libglusterfs/src/glusterfs.h
@@ -85,6 +85,7 @@
 #define GF_XATTR_NODE_UUID_KEY  "trusted.glusterfs.node-uuid"
 #define GF_REBAL_FIND_LOCAL_SUBVOL "glusterfs.find-local-subvol"
 #define GF_XATTR_VOL_ID_KEY   "trusted.glusterfs.volume-id"
+#define GF_XATTR_BRICK_PATH   "trusted.glusterfs.brick-path"
 #define GF_XATTR_LOCKINFO_KEY   "trusted.glusterfs.lockinfo"
 #define GF_META_LOCK_KEY        "glusterfs.lock-migration-meta-lock"
 #define GF_META_UNLOCK_KEY      "glusterfs.lock-migration-meta-unlock"
diff --git a/tests/basic/brick-reopen.t b/tests/basic/brick-reopen.t
new file mode 100644
index 00000000000..1f25315e6e2
--- /dev/null
+++ b/tests/basic/brick-reopen.t
@@ -0,0 +1,128 @@
+#!/bin/bash
+
+. $(dirname $0)/../include.rc
+. $(dirname $0)/../volume.rc
+
+function write() {
+        dd if=/dev/urandom of=$M0/test bs=1k count=200
+}
+
+function swap_bricks {
+        # swap brick 0 and 1 directories
+        mv $B0/brick0 "$B0/brick0-tmp" &&
+        mv $B0/brick1 $B0/brick0 &&
+        mv "$B0/brick0-tmp" $B0/brick1
+}
+
+function kick_glusterd {
+        killall glusterd &&
+        ! pidof glusterd &&
+        glusterd &&
+        pidof glusterd
+
+        if [ $? -eq 0 ]; then
+                echo "Y"
+        else
+                echo "N"
+        fi
+}
+
+function mute {
+        cmd=$@
+        $cmd &> /dev/null
+        if [ $? -eq 0 ]; then
+                echo "Y"
+        else
+                echo "N"
+        fi
+}
+
+cleanup;
+
+# Setup
+TEST mute glusterd
+TEST mute pidof glusterd
+TEST mkdir -p $B0/brick{0,1}
+TEST $CLI volume create $V0 replica 2 $H0:$B0/brick{0,1}
+
+TEST $CLI volume start $V0
+TEST glusterfs --volfile-id=$V0 --volfile-server=$H0 $M0 --entry-timeout=0 --attribute-timeout=0;
+EXPECT_WITHIN $CHILD_UP_TIMEOUT "1" afr_child_up_status $V0 0
+
+# Stop volume and force start volume
+TEST write
+TEST $CLI volume stop $V0
+TEST ! write
+TEST $CLI volume start $V0 force
+EXPECT_WITHIN $CHILD_UP_TIMEOUT "1" afr_child_up_status $V0 0
+TEST write
+
+# Kill one brick and force restart volume
+TEST kill_brick $V0 $H0 $B0/brick0
+TEST $CLI volume start $V0 force
+EXPECT_WITHIN $CHILD_UP_TIMEOUT "1" afr_child_up_status $V0 0
+TEST write
+
+# Kill one brick and kill mgmt
+TEST kill_brick $V0 $H0 $B0/brick0
+TEST kick_glusterd
+EXPECT_WITHIN $CHILD_UP_TIMEOUT "1" afr_child_up_status $V0 0
+TEST write
+
+# Stop volume, swap brick directories and force start volume
+TEST $CLI volume stop $V0
+TEST swap_bricks
+TEST ! $CLI volume start $V0 force
+EXPECT "0" afr_child_up_status $V0 0
+TEST ! write
+
+# Re-swap the brick directories and force start volume
+TEST swap_bricks
+TEST $CLI volume start $V0 force
+EXPECT_WITHIN $CHILD_UP_TIMEOUT "1" afr_child_up_status $V0 0
+TEST write
+
+# retest scenario with kick glusterd
+TEST kill_brick $V0 $H0 $B0/brick0
+TEST kill_brick $V0 $H0 $B0/brick1
+TEST swap_bricks
+TEST kick_glusterd
+EXPECT_WITHIN $CHILD_UP_TIMEOUT "0" afr_child_up_status $V0 0
+TEST ! write
+
+TEST swap_bricks
+TEST kick_glusterd
+EXPECT_WITHIN $CHILD_UP_TIMEOUT "1" afr_child_up_status $V0 0
+TEST write
+
+# test backward compatibility
+TEST $CLI volume stop $V0
+TEST mute setfattr -x "trusted.glusterfs.brick-path" $B0/${V0}0
+TEST mute ! getfattr -n "trusted.glusterfs.brick-path" $B0/${V0}0
+TEST $CLI volume start $V0 force
+EXPECT_WITHIN $CHILD_UP_TIMEOUT "1" afr_child_up_status $V0 0
+TEST mute getfattr -n "trusted.glusterfs.brick-path" $B0/${V0}0
+TEST $CLI volume stop $V0
+TEST $CLI volume start $V0 force
+EXPECT_WITHIN $CHILD_UP_TIMEOUT "1" afr_child_up_status $V0 0
+
+# redo the test with kicking glusterd
+TEST kill_brick $V0 $H0 $B0/brick0
+TEST mute setfattr -x "trusted.glusterfs.brick-path" $B0/${V0}0
+TEST mute ! getfattr -n "trusted.glusterfs.brick-path" $B0/${V0}0
+TEST kick_glusterd
+EXPECT_WITHIN $CHILD_UP_TIMEOUT "1" afr_child_up_status $V0 0
+TEST mute getfattr -n "trusted.glusterfs.brick-path" $B0/${V0}0
+TEST kill_brick $V0 $H0 $B0/brick0
+TEST kick_glusterd
+EXPECT_WITHIN $CHILD_UP_TIMEOUT "1" afr_child_up_status $V0 0
+
+# replace brick and force start volume
+TEST kill_brick $V0 $H0 $B0/brick0
+TEST gluster volume replace-brick $V0 $H0:$B0/brick0 $H0:$B0/brick3 commit force
+TEST $CLI volume start $V0 force
+EXPECT_WITHIN $CHILD_UP_TIMEOUT "1" afr_child_up_status $V0 0
+TEST write
+
+cleanup
+
diff --git a/tests/basic/volume-snapshot-xml.t b/tests/basic/volume-snapshot-xml.t
index d58e898083a..a8ccfee8fe8 100755
--- a/tests/basic/volume-snapshot-xml.t
+++ b/tests/basic/volume-snapshot-xml.t
@@ -41,11 +41,14 @@ EXPECT "Started" get-xml "snapshot info snap1" "status"
 
 # Snapshot list xmls
 EXPECT "2" get-xml "snapshot list" "count"
+#$CLI snapshot list $V0 > /dev/tty
 EXPECT "snap2" get-xml "snapshot list $V0" "snapshot"
 
 # Snapshot status xmls
+#$CLI snapshot status > /dev/tty
 EXPECT "snap2" get-xml "snapshot status" "name"
 EXPECT "snap2" get-xml "snapshot deactivate snap2" "name"
+#$CLI snapshot status > /dev/tty
 EXPECT "N/A" get-xml "snapshot status" "pid"
 EXPECT "snap1" get-xml "snapshot status snap1" "name"
 EXPECT "Yes" get-xml "snapshot status snap1" "brick_running"
diff --git a/xlators/cluster/afr/src/afr-common.c b/xlators/cluster/afr/src/afr-common.c
index f5210bd7d5d..b3f1fd9848e 100644
--- a/xlators/cluster/afr/src/afr-common.c
+++ b/xlators/cluster/afr/src/afr-common.c
@@ -1892,6 +1892,7 @@ afr_frame_return (call_frame_t *frame)
 static char *afr_ignore_xattrs[] = {
         GF_SELINUX_XATTR_KEY,
         QUOTA_SIZE_KEY,
+        GF_XATTR_BRICK_PATH,
         NULL
 };
 
diff --git a/xlators/cluster/ec/src/ec-heal.c b/xlators/cluster/ec/src/ec-heal.c
index 2373e6f93d5..29afda6625d 100644
--- a/xlators/cluster/ec/src/ec-heal.c
+++ b/xlators/cluster/ec/src/ec-heal.c
@@ -54,6 +54,7 @@ struct ec_name_data {
 static char *ec_ignore_xattrs[] = {
         GF_SELINUX_XATTR_KEY,
         QUOTA_SIZE_KEY,
+        GF_XATTR_BRICK_PATH,
         NULL
 };
 
diff --git a/xlators/mgmt/glusterd/src/glusterd-brick-ops.c b/xlators/mgmt/glusterd/src/glusterd-brick-ops.c
index 706dadef71e..2fcd769e136 100644
--- a/xlators/mgmt/glusterd/src/glusterd-brick-ops.c
+++ b/xlators/mgmt/glusterd/src/glusterd-brick-ops.c
@@ -22,6 +22,8 @@
 #include "glusterd-server-quorum.h"
 #include "run.h"
 #include "glusterd-volgen.h"
+#include "syscall.h"
+#include "compat-errno.h"
 #include <sys/signal.h>
 
 /* misc */
@@ -1609,6 +1611,14 @@ glusterd_op_perform_remove_brick (glusterd_volinfo_t  *volinfo, char *brick,
                 goto out;
         }
 
+        ret = sys_lremovexattr (brickinfo->path, GF_XATTR_BRICK_PATH);
+        if (ret == -1 && errno != ENODATA) {
+                gf_log (THIS->name, GF_LOG_ERROR,
+                        "Error removing xattr %s. Reaason: %s",
+                        GF_XATTR_BRICK_PATH, strerror (errno));
+                goto out;
+        }
+
         brickinfo->decommissioned = 1;
         ret = 0;
 out:
diff --git a/xlators/mgmt/glusterd/src/glusterd-utils.c b/xlators/mgmt/glusterd/src/glusterd-utils.c
index 27bf68d50e1..948670632bb 100644
--- a/xlators/mgmt/glusterd/src/glusterd-utils.c
+++ b/xlators/mgmt/glusterd/src/glusterd-utils.c
@@ -6262,8 +6262,10 @@ glusterd_check_and_set_brick_xattr (char *host, char *path, uuid_t uuid,
         }
 
 
-        if (!is_force)
+        if (!is_force) {
                 flags = XATTR_CREATE;
+                //sys_lremovexattr (path, GF_XATTR_BRICK_PATH);
+        }
 
         ret = sys_lsetxattr (path, GF_XATTR_VOL_ID_KEY, uuid, 16,
                              flags);
@@ -6274,6 +6276,16 @@ glusterd_check_and_set_brick_xattr (char *host, char *path, uuid_t uuid,
                 goto out;
         }
 
+        ret = sys_lsetxattr (path, GF_XATTR_BRICK_PATH, path, strlen (path),
+                             flags);
+
+        if (ret == -1) {
+                snprintf (msg, sizeof (msg), "Failed to set extended "
+                          "attributes %s, reason: %s",
+                          GF_XATTR_BRICK_PATH, strerror (errno));
+                goto out;
+        }
+
         ret = 0;
 out:
         if (strlen (msg))
diff --git a/xlators/mgmt/glusterd/src/glusterd-volume-ops.c b/xlators/mgmt/glusterd/src/glusterd-volume-ops.c
index 97fb5605715..9fb60cc0802 100644
--- a/xlators/mgmt/glusterd/src/glusterd-volume-ops.c
+++ b/xlators/mgmt/glusterd/src/glusterd-volume-ops.c
@@ -1610,6 +1610,24 @@ glusterd_op_stage_start_volume (dict_t *dict, char **op_errstr,
                         goto out;
                 }
 
+                /* check GF_XATTR_BRICK_PATH is set to brick path */
+                ret = gf_compare_or_fix_xattr_brick_path (brickinfo->path);
+
+                if (ret != 0) {
+                        gf_log (this->name, GF_LOG_ERROR, "Error comparing %s "
+                                "on %s", GF_XATTR_BRICK_PATH, brickinfo->path);
+                        if (strstr (brickinfo->path, "/snaps/")) {
+                                /*
+                                 * This is "naturally" going to be wrong on
+                                 * snap bricks, but that's OK.
+                                 */
+                                ret = 0;
+                        } else {
+                                ret = -1;
+                                goto out;
+                        }
+                }
+
                 /* A bricks mount dir is required only by snapshots which were
                  * introduced in gluster-3.6.0
                  */
diff --git a/xlators/storage/posix/src/posix.c b/xlators/storage/posix/src/posix.c
index b461d4ea4a1..f8c180e7d4b 100644
--- a/xlators/storage/posix/src/posix.c
+++ b/xlators/storage/posix/src/posix.c
@@ -7314,6 +7314,19 @@ init (xlator_t *this)
                 }
         }
 
+#if 0
+        /* check brick path is set on GF_XATTR_BRICK_PATH */
+        ret = gf_compare_or_fix_xattr_brick_path (dir_data->data);
+
+        if (ret != 0) {
+                gf_log (this->name, GF_LOG_ERROR, "Error comparing %s on %s",
+                        GF_XATTR_BRICK_PATH, dir_data->data);
+                ret = -1;
+                goto out;
+        }
+#endif
+
+        /* check volume-id */
         tmp_data = dict_get (this->options, "volume-id");
         if (tmp_data) {
                 op_ret = gf_uuid_parse (tmp_data->data, dict_uuid);