From 593fc8ed085f58c945527b009c2f0685ec05e5b3 Mon Sep 17 00:00:00 2001 From: Abhijeeth Nuthan Date: Mon, 25 Feb 2019 10:58:09 -0800 Subject: [PATCH] MB-32880: Do not change length of vbucket chain in ... ... ns_janitor when cluster > madhatter. Pre mad-hatter, there is no change to the janitor behaviour. Post mad-hatter changes that follow as a result of the above mentioned change is, 1. Setting the vbucket map is allowed with different length chains. 2. Defer changing vbucket map to rebalance when changing num_replicas i.e., when the admin changes number of replicas for the bucket, do not update the vbucket map until rebalance is performed and not in the janitor prior to rebalance. 3. Explicitly disallow delta-recovery for a bucket if vbucket map of that bucket doesn't contain number of replicas according to Bucket Config. 4. This changeset can result in varied length chains if rebalance was interrupted for any reason. The janitor does not fix the length of the chains. Change-Id: Id7b0c50246796233de4ba688cfd5eaa81a53cad5 Reviewed-on: http://review.couchbase.org/105359 Reviewed-by: Aliaksey Artamonau Tested-by: Abhijeeth Nuthan --- src/ns_bucket.erl | 22 ++++++++++++++++++++- src/ns_janitor.erl | 41 +++++++++++++++++++++++++++++++++++----- src/ns_rebalancer.erl | 28 +++++++++++++++++++++------ src/ns_vbucket_mover.erl | 1 + 4 files changed, 80 insertions(+), 12 deletions(-) diff --git a/src/ns_bucket.erl b/src/ns_bucket.erl index 34492e347f..5667e3b956 100644 --- a/src/ns_bucket.erl +++ b/src/ns_bucket.erl @@ -17,6 +17,7 @@ -include("ns_common.hrl"). -include("ns_config.hrl"). +-include("cut.hrl"). -ifdef(TEST). -include_lib("eunit/include/eunit.hrl"). @@ -27,6 +28,7 @@ bucket_nodes/1, bucket_type/1, replication_type/1, + replica_change/1, create_bucket/3, credentials/1, delete_bucket/1, @@ -712,7 +714,14 @@ set_fast_forward_map(Bucket, Map) -> set_map(Bucket, Map) -> - true = mb_map:is_valid(Map), + case mb_map:is_valid(Map) of + true -> + ok; + different_length_chains -> + %% Never expect to set map with different_length_chains + %% pre-madhatter. + true = cluster_compat_mode:is_cluster_madhatter() + end, update_bucket_config( Bucket, fun (OldConfig) -> @@ -833,6 +842,16 @@ past_vbucket_maps(Config) -> false -> [] end. +replica_change(BucketConfig) -> + replica_change(num_replicas(BucketConfig), + proplists:get_value(map, BucketConfig)). + +replica_change(_NumReplicas, undefined) -> + false; +replica_change(NumReplicas, Map) -> + ExpectedChainLength = NumReplicas + 1, + lists:any(?cut(ExpectedChainLength =/= length(_)), Map). + needs_rebalance(BucketConfig, Nodes) -> Servers = proplists:get_value(servers, BucketConfig, []), case proplists:get_value(type, BucketConfig) of @@ -843,6 +862,7 @@ needs_rebalance(BucketConfig, Nodes) -> _ -> Map = proplists:get_value(map, BucketConfig), Map =:= undefined orelse + replica_change(BucketConfig) orelse lists:sort(Nodes) =/= lists:sort(Servers) orelse ns_rebalancer:map_options_changed(BucketConfig) orelse (ns_rebalancer:unbalanced(Map, BucketConfig) andalso diff --git a/src/ns_janitor.erl b/src/ns_janitor.erl index a32a6e9e3b..018b8d0b9c 100644 --- a/src/ns_janitor.erl +++ b/src/ns_janitor.erl @@ -363,7 +363,17 @@ compute_vbucket_map_fixup(Bucket, BucketConfig, States) -> ignore -> OldChain; _ -> NewChain end || {NewChain, OldChain} <- lists:zip(MapUpdates, Map)], - NewAdjustedMap = maybe_adjust_chain_size(NewMap, BucketConfig), + NewAdjustedMap = case cluster_compat_mode:is_cluster_madhatter() of + true -> + %% Defer adjusting chain length to rebalance, at + %% the time of writing this code the logic is in, + %% ns_rebalancer:do_rebalance_membase_bucket. + NewMap; + false -> + NumReplicas = ns_bucket:num_replicas(BucketConfig), + ns_janitor_map_recoverer:align_replicas(Map, + NumReplicas) + end, NewBucketConfig = case NewAdjustedMap =:= Map of true -> BucketConfig; @@ -374,10 +384,6 @@ compute_vbucket_map_fixup(Bucket, BucketConfig, States) -> end, {NewBucketConfig, IgnoredVBuckets}. -maybe_adjust_chain_size(Map, BucketConfig) -> - NumReplicas = ns_bucket:num_replicas(BucketConfig), - ns_janitor_map_recoverer:align_replicas(Map, NumReplicas). - construct_vbucket_states(VBucket, Chain, States) -> NodeStates = [{N, S} || {N, V, S} <- States, V == VBucket], ChainStates = [{N, proplists:get_value(N, @@ -565,4 +571,29 @@ enumerate_chains_test() -> EnumeratedChains2 = enumerate_chains(Map, undefined), [{0, [a, b, c], []}, {1, [b, c, a], []}] = EnumeratedChains2. +sanify_addition_of_replicas_test() -> + [a, b] = do_sanify_chain("B", [{a, 0, active}, + {b, 0, replica}], + [a, b], [a, b, c], 0), + [a, b] = do_sanify_chain("B", [{a, 0, active}, + {b, 0, replica}, + {c, 0, replica}], + [a, b], [a, b, c], 0), + + %% replica addition with possible move. + [a, b] = do_sanify_chain("B", [{a, 0, dead}, + {b, 0, replica}, + {c, 0, pending}], + [a, b], [c, a, b], 0), + [c, d, a] = do_sanify_chain("B", [{a, 0, dead}, + {b, 0, replica}, + {c, 0, active}, + {d, 0, replica}], + [a, b], [c, d, a], 0), + [c, d, a] = do_sanify_chain("B", [{a, 0, replica}, + {b, 0, replica}, + {c, 0, active}, + {d, 0, replica}], + [a, b], [c, d, a], 0). + -endif. diff --git a/src/ns_rebalancer.erl b/src/ns_rebalancer.erl index 7f17323f6c..402acabfc3 100644 --- a/src/ns_rebalancer.erl +++ b/src/ns_rebalancer.erl @@ -871,10 +871,21 @@ run_janitor_pre_rebalance(BucketName) -> do_rebalance_membase_bucket(Bucket, Config, KeepNodes, ProgressFun, DeltaRecoveryBuckets) -> Map = proplists:get_value(map, Config), + AdjustedMap = case cluster_compat_mode:is_cluster_madhatter() of + true -> + NumReplicas = ns_bucket:num_replicas(Config), + ns_janitor_map_recoverer:align_replicas(Map, + NumReplicas); + false -> + %% Expect equal length map pre mad-hatter, as the + %% janitor fixes it for us. + %% See fun ns_janitor:compute_vbucket_map_fixup. + Map + end, {FastForwardMap, MapOptions} = case lists:keyfind(Bucket, 1, DeltaRecoveryBuckets) of false -> - generate_vbucket_map(Map, KeepNodes, Config); + generate_vbucket_map(AdjustedMap, KeepNodes, Config); {_, _, V} -> V end, @@ -1180,7 +1191,8 @@ build_delta_recovery_buckets_loop(MappedConfigs, DeltaRecoveryBuckets, Acc) -> [{Bucket, BucketConfig, RecoverResult0} | RestMapped] = MappedConfigs, NeedBucket = lists:member(Bucket, DeltaRecoveryBuckets), - RecoverResult = case NeedBucket of + RecoverResult = case NeedBucket andalso + not ns_bucket:replica_change(BucketConfig) of true -> RecoverResult0; false -> @@ -1691,8 +1703,12 @@ membase_delta_recovery_buckets_test() -> ["b1", "b3"] = membase_delta_recovery_buckets(all, MembaseBuckets). build_delta_recovery_buckets_loop_test() -> - MappedConfigs = [{"b1", conf1, {map, opts}}, - {"b2", conf2, false}], + %% Fake num_replicas so that we don't crash in + %% build_delta_recovery_buckets_loop. + Conf1 = [{num_replicas, 1}, conf1], + Conf2 = [{num_replicas, 1}, conf2], + MappedConfigs = [{"b1", Conf1, {map, opts}}, + {"b2", Conf2, false}], All = membase_delta_recovery_buckets(all, [{"b1", conf}, {"b2", conf}]), {ok, []} = build_delta_recovery_buckets_loop([], All, []), @@ -1700,8 +1716,8 @@ build_delta_recovery_buckets_loop_test() -> {error, not_possible} = build_delta_recovery_buckets_loop(MappedConfigs, ["b2"], []), {error, not_possible} = build_delta_recovery_buckets_loop(MappedConfigs, ["b1", "b2"], []), {ok, []} = build_delta_recovery_buckets_loop(MappedConfigs, [], []), - ?assertEqual({ok, [{"b1", conf1, {map, opts}}]}, + ?assertEqual({ok, [{"b1", Conf1, {map, opts}}]}, build_delta_recovery_buckets_loop(MappedConfigs, ["b1"], [])), - ?assertEqual({ok, [{"b1", conf1, {map, opts}}]}, + ?assertEqual({ok, [{"b1", Conf1, {map, opts}}]}, build_delta_recovery_buckets_loop([hd(MappedConfigs)], All, [])). -endif. diff --git a/src/ns_vbucket_mover.erl b/src/ns_vbucket_mover.erl index 942e8e067f..6fdff43d9c 100644 --- a/src/ns_vbucket_mover.erl +++ b/src/ns_vbucket_mover.erl @@ -92,6 +92,7 @@ is_swap_rebalance(OldMap, NewMap) -> length(OldNodes) =/= length(NewNodes) andalso erlang:throw(not_swap), lists:foldl( fun ({_VB, OldChain, NewChain}, Dict0) -> + length(OldChain) =/= length(NewChain) andalso throw(not_swap), Changed = [Pair || {From, To} = Pair <- lists:zip(OldChain, NewChain), From =/= To, From =/= undefined,