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 <[email protected]>
Tested-by: Abhijeeth Nuthan <[email protected]>

Author: anuthan

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

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.

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,17 +1703,21 @@ 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, []),
     {error, not_possible} = build_delta_recovery_buckets_loop(MappedConfigs, All, []),
     {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.

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,