From 71fc5b7949e00c4448f5ec1291e756b201a70082 Mon Sep 17 00:00:00 2001 From: Ravishankar N Date: Thu, 29 Apr 2021 18:34:57 +0530 Subject: [PATCH 543/543] glusterd: handle custom xlator failure cases Problem-1: custom xlator insertion was failing for those xlators in the brick graph whose dbg_key was NULL in the server_graph_table. Looking at the git log, the dbg_key was added in commit d1397dbd7d6cdbd2d81d5d36d608b6175d449db4 for inserting debug xlators. Fix: I think it is fine to define it for all brick xlators below server. Problem-2: In the commit-op phase, glusterd_op_set_volume() updates the volinfo dict with the key-value pairs and then proceeds to create the volfiles. If any of the steps fail, the volinfo dict retains those key-values, until glusterd is restarted or `gluster vol reset $VOLNAME` is issued. Fix: Make a copy of the volinfo dict and if there are any failures in proceeding with the set volume logic, restore the dict to its original state. Backport of: > Upstream-patch-link: https://github.com/gluster/glusterfs/pull/2371 > Change-Id: I9010dab33d0139b8e6d603308e331b6d220a4849 > Updates: #2370 > Signed-off-by: Ravishankar N Change-Id: I9010dab33d0139b8e6d603308e331b6d220a4849 BUG: 1953901 Signed-off-by: Ravishankar N Reviewed-on: https://code.engineering.redhat.com/gerrit/239889 Reviewed-by: Sunil Kumar Heggodu Gopala Acharya --- tests/basic/user-xlator.t | 16 ++++++++++++++-- xlators/mgmt/glusterd/src/glusterd-op-sm.c | 16 ++++++++++++++++ xlators/mgmt/glusterd/src/glusterd-volgen.c | 14 +++++++------- 3 files changed, 37 insertions(+), 9 deletions(-) diff --git a/tests/basic/user-xlator.t b/tests/basic/user-xlator.t index a711f9f..ed2d831 100755 --- a/tests/basic/user-xlator.t +++ b/tests/basic/user-xlator.t @@ -35,8 +35,18 @@ EXPECT_WITHIN $PROCESS_UP_TIMEOUT "1" brick_up_status $V0 $H0 $B0/${V0}4 EXPECT_WITHIN $PROCESS_UP_TIMEOUT "1" brick_up_status $V0 $H0 $B0/${V0}5 EXPECT_WITHIN $PROCESS_UP_TIMEOUT "1" brick_up_status $V0 $H0 $B0/${V0}6 -TEST $CLI volume set $V0 user.xlator.hoge trash -TEST grep -q 'user/hoge' ${SERVER_VOLFILE} +# Test that the insertion at all positions between server and posix is successful. +# It is not guaranteed that the brick process will start/work in all positions though. +TESTS_EXPECTED_IN_LOOP=34 +declare -a brick_side_xlators=("decompounder" "io-stats" "quota" "index" "barrier" + "marker" "selinux" "io-threads" "upcall" "leases" + "read-only" "worm" "locks" "access-control" + "bitrot-stub" "changelog" "trash") +for xlator in "${brick_side_xlators[@]}" + do + TEST_IN_LOOP $CLI volume set $V0 user.xlator.hoge $xlator + TEST_IN_LOOP grep -q 'user/hoge' ${SERVER_VOLFILE} + done TEST $CLI volume stop $V0 TEST $CLI volume start $V0 @@ -49,6 +59,8 @@ EXPECT_WITHIN $PROCESS_UP_TIMEOUT "1" brick_up_status $V0 $H0 $B0/${V0}6 TEST ! $CLI volume set $V0 user.xlator.hoge unknown TEST grep -q 'user/hoge' ${SERVER_VOLFILE} # When the CLI fails, the volfile is not modified. +# User xlator insert failures must not prevent setting other volume options. +TEST $CLI volume set $V0 storage.reserve 10% TEST $CLI volume stop $V0 TEST $CLI volume start $V0 diff --git a/xlators/mgmt/glusterd/src/glusterd-op-sm.c b/xlators/mgmt/glusterd/src/glusterd-op-sm.c index 1e84f5f..893af29 100644 --- a/xlators/mgmt/glusterd/src/glusterd-op-sm.c +++ b/xlators/mgmt/glusterd/src/glusterd-op-sm.c @@ -2911,6 +2911,7 @@ glusterd_op_set_volume(dict_t *dict, char **errstr) uint32_t new_op_version = 0; gf_boolean_t quorum_action = _gf_false; glusterd_svc_t *svc = NULL; + dict_t *volinfo_dict_orig = NULL; this = THIS; GF_ASSERT(this); @@ -2918,6 +2919,10 @@ glusterd_op_set_volume(dict_t *dict, char **errstr) priv = this->private; GF_ASSERT(priv); + volinfo_dict_orig = dict_new(); + if (!volinfo_dict_orig) + goto out; + ret = dict_get_int32n(dict, "count", SLEN("count"), &dict_count); if (ret) { gf_msg(this->name, GF_LOG_ERROR, 0, GD_MSG_DICT_GET_FAILED, @@ -2949,6 +2954,11 @@ glusterd_op_set_volume(dict_t *dict, char **errstr) goto out; } + if (dict_copy(volinfo->dict, volinfo_dict_orig) == NULL) { + ret = -ENOMEM; + goto out; + } + /* TODO: Remove this once v3.3 compatibility is not required */ check_op_version = dict_get_str_boolean(dict, "check-op-version", _gf_false); @@ -3171,6 +3181,12 @@ out: gf_msg_debug(this->name, 0, "returning %d", ret); if (quorum_action) glusterd_do_quorum_action(); + if (ret < 0 && count > 1) { + if (dict_reset(volinfo->dict) == 0) + dict_copy(volinfo_dict_orig, volinfo->dict); + } + if (volinfo_dict_orig) + dict_unref(volinfo_dict_orig); return ret; } diff --git a/xlators/mgmt/glusterd/src/glusterd-volgen.c b/xlators/mgmt/glusterd/src/glusterd-volgen.c index 71aed08..aa85bdb 100644 --- a/xlators/mgmt/glusterd/src/glusterd-volgen.c +++ b/xlators/mgmt/glusterd/src/glusterd-volgen.c @@ -2706,24 +2706,24 @@ out: static volgen_brick_xlator_t server_graph_table[] = { {brick_graph_add_server, NULL}, {brick_graph_add_decompounder, "decompounder"}, - {brick_graph_add_io_stats, "NULL"}, + {brick_graph_add_io_stats, "io-stats"}, {brick_graph_add_sdfs, "sdfs"}, {brick_graph_add_namespace, "namespace"}, - {brick_graph_add_cdc, NULL}, + {brick_graph_add_cdc, "cdc" }, {brick_graph_add_quota, "quota"}, {brick_graph_add_index, "index"}, - {brick_graph_add_barrier, NULL}, + {brick_graph_add_barrier, "barrier" }, {brick_graph_add_marker, "marker"}, {brick_graph_add_selinux, "selinux"}, {brick_graph_add_fdl, "fdl"}, {brick_graph_add_iot, "io-threads"}, {brick_graph_add_upcall, "upcall"}, {brick_graph_add_leases, "leases"}, - {brick_graph_add_pump, NULL}, - {brick_graph_add_ro, NULL}, - {brick_graph_add_worm, NULL}, + {brick_graph_add_pump, "pump" }, + {brick_graph_add_ro, "read-only" }, + {brick_graph_add_worm, "worm" }, {brick_graph_add_locks, "locks"}, - {brick_graph_add_acl, "acl"}, + {brick_graph_add_acl, "access-control"}, {brick_graph_add_bitrot_stub, "bitrot-stub"}, {brick_graph_add_changelog, "changelog"}, #if USE_GFDB /* changetimerecorder depends on gfdb */ -- 1.8.3.1