[BP] MB-51458 do not truncate long dcp connection names if the version

...of the producer node is less than 6.6.5 or one of the following:
7.0.0, 7.0.1

Change-Id: I3f1c1ca63993c6e1f408a3ed9b76aa0d14efab85
Reviewed-on: https://review.couchbase.org/c/ns_server/+/172546
Well-Formed: Restriction Checker
Well-Formed: Build Bot <[email protected]>
Reviewed-by: Timofey Barmin <[email protected]>
Tested-by: Artem Stemkovski <[email protected]>
Reviewed-on: https://review.couchbase.org/c/ns_server/+/172741

Author: vzasade

src/dcp_replicator.erl

@@ -221,27 +221,49 @@ get_docs_estimate(Bucket, Partition, ConsumerNode) ->
     ns_memcached:get_dcp_docs_estimate(Bucket, Partition, Connection).
 
 get_connection_name(ConsumerNode, ProducerNode, Bucket) ->
-    ConsumerNodeList = atom_to_list(ConsumerNode),
-    ProducerNodeList = atom_to_list(ProducerNode),
-    CName = "replication:" ++ ProducerNodeList ++ "->" ++ ConsumerNodeList ++
-        ":" ++ Bucket,
+    CName = "replication:" ++ atom_to_list(ProducerNode) ++ "->" ++
+        atom_to_list(ConsumerNode) ++ ":" ++ Bucket,
 
     case length(CName) =< ?MAX_DCP_CONNECTION_NAME of
         true ->
             CName;
         false ->
-            %% Trim off the common prefix to shorten the names.
-            {CNode, PNode} = trim_common_prefix(ConsumerNode, ProducerNode),
+            case should_truncate_name(ConsumerNode, ProducerNode) of
+                true ->
+                    get_truncated_connection_name(
+                      CName, ConsumerNode, ProducerNode, Bucket);
+                false ->
+                    CName
+            end
+    end.
+
+get_truncated_connection_name(LongName, ConsumerNode, ProducerNode, Bucket) ->
+    %% Trim off the common prefix to shorten the names.
+    {CNode, PNode} = trim_common_prefix(ConsumerNode, ProducerNode),
 
-            Hash = binary_to_list(base64:encode(crypto:hash(sha, CName))),
-            Bkt = string:slice(Bucket, 0, 60),
+    Hash = binary_to_list(base64:encode(crypto:hash(sha, LongName))),
+    Bkt = string:slice(Bucket, 0, 60),
 
-            CName2 = "replication:" ++ PNode ++ "->" ++ CNode ++ ":" ++ Bkt ++
-                     ":" ++ Hash,
-            true = length(CName2) =< ?MAX_DCP_CONNECTION_NAME,
-            CName2
+    CName = "replication:" ++ PNode ++ "->" ++ CNode ++ ":" ++ Bkt ++
+        ":" ++ Hash,
+    true = length(CName) =< ?MAX_DCP_CONNECTION_NAME,
+    CName.
+
+node_supports_truncated_names(Node) ->
+    case Node == misc:this_node() of
+        true ->
+            true;
+        false ->
+            Quirks = rebalance_quirks:get_quirks([Node], long_names),
+            not rebalance_quirks:is_enabled(
+                  dont_truncate_long_names,
+                  rebalance_quirks:get_node_quirks(Node, Quirks))
     end.
 
+should_truncate_name(ConsumerNode, ProducerNode) ->
+    lists:all(fun node_supports_truncated_names/1,
+              [ConsumerNode, ProducerNode]).
+
 trim_common_prefix(Consumer, Producer) ->
     %% Find the longest common prefix for the two nodes and chop
     %% it off (but not below a minimal length).
@@ -324,59 +346,108 @@ maybe_shut_consumer(Reason, Consumer) ->
     end.
 
 -ifdef(TEST).
-get_connection_name_test() ->
-
-    %% Connection name fits into the maximum allowed
-
-    NodeA = 'nodeA.eng.couchbase.com',
-    NodeB = 'nodeB.eng.couchbase.com',
-    BucketAB = "bucket1",
-    ConnAB = get_connection_name(NodeA, NodeB, BucketAB),
-    ?assertEqual("replication:nodeB.eng.couchbase.com->"
-                 "nodeA.eng.couchbase.com:bucket1", ConnAB),
-    ?assertEqual(true, length(ConnAB) =< ?MAX_DCP_CONNECTION_NAME),
-
-    %% Test where the connection name, using the previous method, won't
-    %% fit into the maximum allowed.
-
-    Node1 = "[email protected]."
-            "couchbase-new-pxxxxxxx.svc",
-    Node2 = "[email protected]."
-            "couchbase-new-pxxxxxxx.svc",
-    Bucket12 = "com.yyyyyy.digital.ms.shoppingcart.shoppingcart.1234567890"
-               "12345678901234567890",
-    Conn12 = get_connection_name(list_to_atom(Node1), list_to_atom(Node2),
-                                 Bucket12),
-    ?assertEqual("replication:1.platform-couchbase-cluster.couchb->"
-                 "0.platform-couchbase-cluster.couchb:com.yyyyyy.digital.ms."
-                 "shoppingcart.shoppingcart.123456789012:"
-                 "TYFMH5ZD2gPLOaLgcuA2VijsZvc=", Conn12),
-
-    %% Test that the node names aren't shortened too much (note the only
-    %% difference is the last character).
-
-    Node3 = "ManyManyManyManyCommonCharacters_ns_1@platform-couchbase-cluster"
-            "-0000",
-    Node4 = "ManyManyManyManyCommonCharacters_ns_1@platform-couchbase-cluster"
-            "-0001",
+get_connection_name_test_() ->
     LongBucket = "travel-sample-with-a-very-very-very-very-long-bucket-name",
-    Conn34 = get_connection_name(list_to_atom(Node3), list_to_atom(Node4),
-                                 LongBucket),
-    ?assertEqual("replication:s_1@platform-couchbase-cluster-0001->"
-                 "s_1@platform-couchbase-cluster-0000:travel-sample-with-a-"
-                 "very-very-very-very-long-bucket-name:"
-                 "D/D56MpAKsDt/0yqg6IXKBEaIcY=", Conn34),
-
-    %% Test with unique node names but one is much longer than the other.
-
-    Node5 = "AShortNodeName",
-    Node6 = "ManyManyManyManyCommonCharacters_ns_1@platform-couchbase-cluster"
-            "-AndEvenMoreCharactersToMakeThisNodeNameLongEnoughToRequireIt"
-            "ToBeShortened",
-    Conn56 = get_connection_name(list_to_atom(Node5), list_to_atom(Node6),
-                                 LongBucket),
-    ?assertEqual("replication:ManyManyManyManyCommonCharacters_ns->"
-                 "AShortNodeName:travel-sample-with-a-very-very-very-very-"
-                 "long-bucket-name:A3aPD1Sik+5ZIz43M6NNTGn9XFw=", Conn56).
-
+    ConsumerNode = list_to_atom(
+                     "ns_1@platform-couchbase-cluster-0000."
+                     "platform-couchbase-cluster.couchbase-new-pxxxxxxx.svc"),
+    ProducerNode = list_to_atom(
+                     "ns_1@platform-couchbase-cluster-0001."
+                     "platform-couchbase-cluster.couchbase-new-pxxxxxxx.svc"),
+    VeryLongBucket = "com.yyyyyy.digital.ms.shoppingcart."
+        "shoppingcart.123456789012345678901234567890",
+    TrimmedName =
+        "replication:1.platform-couchbase-cluster.couchb->0.platform-couchbase"
+        "-cluster.couchb:com.yyyyyy.digital.ms.shoppingcart.shoppingcart."
+        "123456789012:TYFMH5ZD2gPLOaLgcuA2VijsZvc=",
+    {foreach,
+     fun () ->
+             meck:new(rebalance_quirks, [passthrough]),
+             meck:new(misc, [passthrough])
+     end,
+     fun (_) ->
+             meck:unload(rebalance_quirks),
+             meck:unload(misc)
+     end,
+     [{"Connection name fits into the maximum allowed",
+       fun () ->
+               Conn = get_connection_name(
+                        'nodeA.eng.couchbase.com', 'nodeB.eng.couchbase.com',
+                        "bucket1"),
+               ?assertEqual("replication:nodeB.eng.couchbase.com->"
+                            "nodeA.eng.couchbase.com:bucket1", Conn),
+               ?assertEqual(true, length(Conn) =< ?MAX_DCP_CONNECTION_NAME)
+       end},
+      {"Connection name won't fit into the maximum allowed.",
+       fun () ->
+               meck:expect(rebalance_quirks, get_quirks,
+                           fun (_, long_names) -> [{ProducerNode, []}] end),
+               meck:expect(misc, this_node, fun () -> ConsumerNode end),
+               ?assertEqual(
+                  TrimmedName,
+                  get_connection_name(ConsumerNode, ProducerNode,
+                                      VeryLongBucket))
+       end},
+      {"Connection name won't fit into the maximum allowed. Pre 7.1 "
+       "Against the node that supports trimming",
+       fun () ->
+               meck:expect(rebalance_quirks, get_quirks,
+                           fun (_, long_names) -> [{ProducerNode, []}] end),
+               meck:expect(misc, this_node, fun () -> ConsumerNode end),
+               ?assertEqual(
+                  TrimmedName,
+                  get_connection_name(ConsumerNode, ProducerNode,
+                                      VeryLongBucket))
+       end},
+      {"Connection name won't fit into the maximum allowed, but name trimming "
+       "is not supported",
+       fun () ->
+               meck:expect(rebalance_quirks, get_quirks,
+                           fun (_, long_names) ->
+                                   [{ProducerNode, [dont_truncate_long_names]}]
+                           end),
+               meck:expect(misc, this_node, fun () -> ConsumerNode end),
+               Conn = get_connection_name(ConsumerNode, ProducerNode,
+                                          VeryLongBucket),
+               ?assertEqual(
+                  "replication:[email protected]"
+                  "-couchbase-cluster.couchbase-new-pxxxxxxx.svc->ns_1@platform"
+                  "-couchbase-cluster-0000.platform-couchbase-cluster.couchbase"
+                  "-new-pxxxxxxx.svc:com.yyyyyy.digital.ms.shoppingcart."
+                  "shoppingcart.123456789012345678901234567890", Conn)
+       end},
+      {"Test that the node names aren't shortened too much (note the only "
+       "difference is the last character).",
+       fun () ->
+               Node1 = "ManyManyManyManyCommonCharacters_ns_1@platform-"
+                   "couchbase-cluster-0000",
+               Node2 = "ManyManyManyManyCommonCharacters_ns_1@platform-"
+                   "couchbase-cluster-0001",
+               meck:expect(rebalance_quirks, get_quirks,
+                           fun (_, long_names) -> [{Node1, []}] end),
+               meck:expect(misc, this_node, fun () -> Node2 end),
+               Conn = get_connection_name(
+                        list_to_atom(Node1), list_to_atom(Node2), LongBucket),
+               ?assertEqual(
+                  "replication:s_1@platform-couchbase-cluster-0001->"
+                  "s_1@platform-couchbase-cluster-0000:travel-sample-with-a-"
+                  "very-very-very-very-long-bucket-name:"
+                  "D/D56MpAKsDt/0yqg6IXKBEaIcY=", Conn)
+       end},
+      {"Test with unique node names but one is much longer than the other.",
+       fun () ->
+               Node1 = "AShortNodeName",
+               Node2 = "ManyManyManyManyCommonCharacters_ns_1@platform-"
+                   "couchbase-cluster-AndEvenMoreCharactersToMakeThisNodeName"
+                   "LongEnoughToRequireItToBeShortened",
+               meck:expect(rebalance_quirks, get_quirks,
+                           fun (_, long_names) -> [{Node1, []}] end),
+               meck:expect(misc, this_node, fun () -> Node2 end),
+               Conn = get_connection_name(
+                        list_to_atom(Node1), list_to_atom(Node2), LongBucket),
+               ?assertEqual(
+                  "replication:ManyManyManyManyCommonCharacters_ns->"
+                  "AShortNodeName:travel-sample-with-a-very-very-very-very-"
+                  "long-bucket-name:A3aPD1Sik+5ZIz43M6NNTGn9XFw=", Conn)
+       end}]}.
 -endif.

src/misc.erl

@@ -334,6 +334,10 @@ position(E, [_|List], N) -> position(E, List, N+1).
 msecs_to_usecs(MilliSec) ->
     MilliSec * 1000.
 
+%% just because we need to mock node() sometimes
+this_node() ->
+    node().
+
 % Returns just the node name string that's before the '@' char.
 % For example, returns "test" instead of "[email protected]".
 %

src/ns_vbucket_mover.erl

@@ -140,7 +140,7 @@ init({Bucket, Nodes, OldMap, NewMap, ProgressCallback}) ->
     ets:new(compaction_inhibitions, [named_table, private, set]),
     ets:new(workers, [named_table, private, set]),
 
-    Quirks = rebalance_quirks:get_quirks(Nodes),
+    Quirks = rebalance_quirks:get_quirks(Nodes, project_intact),
     SchedulerState = vbucket_move_scheduler:prepare(
                        OldMap, NewMap, Quirks,
                        menelaus_web_settings:get_rebalance_moves_per_node(),

src/rebalance_quirks.erl

@@ -12,18 +12,19 @@
 -define(WAIT_STATUSES_TIMEOUT,
         ns_config:get_timeout({?MODULE, wait_statuses}, 15000)).
 
--export([get_quirks/1, is_enabled/2, get_node_quirks/2,
+-export([get_quirks/2, is_enabled/2, get_node_quirks/2,
          default_config/0, upgrade_config_project_intact_patched/0]).
 -export_type([quirk/0]).
 
 -type quirk() :: takeover_via_orchestrator |
                  disable_old_master |
                  reset_replicas |
-                 trivial_moves.
+                 trivial_moves |
+                 dont_truncate_long_names.
 
 %% APIs
-get_quirks(Nodes) ->
-    Config = ns_config:get(),
+get_quirks(Nodes, Type) ->
+    Config = ns_config:latest(),
     OverrideQuirks =
         lists:filtermap(
           fun (Node) ->
@@ -36,7 +37,7 @@ get_quirks(Nodes) ->
           end, Nodes),
 
     OtherNodes     = Nodes -- proplists:get_keys(OverrideQuirks),
-    ComputedQuirks = compute_quirks(OtherNodes, Config),
+    ComputedQuirks = compute_quirks(OtherNodes, Config, Type),
 
     OverrideQuirks ++
         lists:map(
@@ -72,23 +73,28 @@ get_disabled_quirks(Node, Config) ->
     ns_config:search_node_with_default(Node, Config,
                                        disable_rebalance_quirks, []).
 
-compute_quirks(Nodes, Config) ->
+compute_quirks(Nodes, Config, project_intact) ->
     Unpatched = [N || N <- Nodes,
                       not has_project_intact_patches(N, Config)],
 
     case Unpatched of
         [] ->
             [{N, []} || N <- Nodes];
         _ ->
-            lists:map(fun ({Node, Status}) ->
-                              case get_version(Status) of
-                                  {ok, Version} ->
-                                      {Node, quirks_for_version(Version)};
-                                  no_version ->
-                                      exit({no_version_for_node, Node})
-                              end
-                      end, get_statuses(Nodes))
-    end.
+            compute_quirks(Nodes)
+    end;
+compute_quirks(Nodes, _Config, long_names) ->
+    compute_quirks(Nodes).
+
+compute_quirks(Nodes) ->
+    lists:map(fun ({Node, Status}) ->
+                      case get_version(Status) of
+                          {ok, Version} ->
+                              {Node, quirks_for_version(Version)};
+                          no_version ->
+                              exit({no_version_for_node, Node})
+                      end
+              end, get_statuses(Nodes)).
 
 get_statuses(Nodes) ->
     case ns_doctor:wait_statuses(Nodes, ?WAIT_STATUSES_TIMEOUT) of
@@ -120,12 +126,17 @@ all_quirks() ->
     [takeover_via_orchestrator,
      disable_old_master,
      reset_replicas,
-     trivial_moves].
+     trivial_moves,
+     dont_truncate_long_names].
 
 quirks_for_version(Version) ->
     [Quirk || Quirk <- all_quirks(),
               is_quirk_required(Quirk, Version)].
 
+is_quirk_required(dont_truncate_long_names, [7, 0, X]) when X < 2 ->
+    true;
+is_quirk_required(dont_truncate_long_names, Version) ->
+    Version < [6, 6, 5];
 is_quirk_required(takeover_via_orchestrator, Version) ->
     Version < [5, 1, 0];
 is_quirk_required(_, _) ->