[leo_storage] read repairer does not fix all objects

[windkit]

Description

I launched a cluster with 5 storage nodes, put 4 mil objects into the cluster with 3 replicas

What I did

1. Stop storage node (bin/leo_storage stop) on S1@192.168.100.37
2. Remove AVS directory /ssd/avs
3. Start storage node (bin/leo_storage start) on S1@192.168.100.37
4. Read through all the 4 mil object once

Observation

1. Read performance is similar to fully operational cluster
2. Warning from leo_storage_read_repairer

[W]     S1@192.168.100.37       2017-10-16 10:03:35.638060 +0900        1508115815      leo_storage_read_repairer:compare/4     167     [{node,'S1@192.168.100.37'},{addr_id,262089314529519640105457477379090544578},{key,<<"test/1562482">>},{clock,1508113160551767},{cause,not_found}]
[W]     S1@192.168.100.37       2017-10-16 10:03:35.654676 +0900        1508115815      leo_storage_read_repairer:compare/4     167     [{node,'S1@192.168.100.37'},{addr_id,97979787013326817027861459251467535726},{key,<<"test/1562489">>},{clock,1508113160570634},{cause,not_found}]
[W]     S1@192.168.100.37       2017-10-16 10:03:35.666188 +0900        1508115815      leo_storage_read_repairer:compare/4     167     [{node,'S1@192.168.100.37'},{addr_id,145216544548550617579951455369514880573},{key,<<"test/1562494">>},{clock,1508113160584383},{cause,not_found}]
[W]     S1@192.168.100.37       2017-10-16 10:03:35.676827 +0900        1508115815      leo_storage_read_repairer:compare/4     167     [{node,'S1@192.168.100.37'},{addr_id,128647732487682273230731598297851900247},{key,<<"test/1562498">>},{clock,1508113160595215},{cause,not_found}]

[W]     S2@192.168.100.38       2017-10-16 10:03:36.325356 +0900        1508115816      leo_storage_read_repairer:compare/4     167     [{node,'S1@192.168.100.37'},{addr_id,72846239249748048471647548013304151801},{key,<<"test/1562438">>},{clock,1508113161235638},{cause,not_found}]
[W]     S2@192.168.100.38       2017-10-16 10:03:36.358660 +0900        1508115816      leo_storage_read_repairer:compare/4     167     [{node,'S1@192.168.100.37'},{addr_id,213401350361107074116716132585815701224},{key,<<"test/1562449">>},{clock,1508113161265654},{cause,not_found}]
[W]     S2@192.168.100.38       2017-10-16 10:03:36.369805 +0900        1508115816      leo_storage_read_repairer:compare/4     167     [{node,'S1@192.168.100.37'},{addr_id,146984899329602051518955140785947694688},{key,<<"test/1562453">>},{clock,1508113161277052},{cause,not_found}]

3. Some background traffic to S1@192.168.100.37 to fix the objects

Issue

S1@192.168.100.37 is not fully repaired, the missing objects are those S1@192.168.100.37 as the primary

leofs-adm whereis test/1562498
-------+------------------------+--------------------------------------+------------+--------------+----------------+----------------+----------------+----------------------------
 del?  |          node          |             ring address             |    size    |   checksum   |  has children  |  total chunks  |     clock      |             when
-------+------------------------+--------------------------------------+------------+--------------+----------------+----------------+----------------+----------------------------
       | S1@192.168.100.37      |                                      |            |              |                |                |                |
       | S2@192.168.100.38      | 60c8a6eb471f0fc585846b32a4363157     |        20K |   41dd6d2dc1 | false          |              0 | 55b9ef537e70f  | 2017-10-16 09:19:21 +0900
       | S5@192.168.100.41      | 60c8a6eb471f0fc585846b32a4363157     |        20K |   41dd6d2dc1 | false          |              0 | 55b9ef537e70f  | 2017-10-16 09:19:21 +0900

[windkit]

Tracing down the execution path, the repair message is enqueued and consumed
https://github.com/leo-project/leofs/blame/master/apps/leo_storage/src/leo_storage_mq.erl#L346
But the check here would fail, as leo_storage_handler_object:head/3 would return not_found when the object is missing

Removing the test on compare_between_metadatas/1 fixed the recovery issue, but I am not so sure if there are side effects.

@yosukehara @mocchira

[mocchira]

@windkit Thanks for digging. I also just found the root cause (yes not_found causes this problem).

Removing the test on compare_between_metadatas/1 fixed the recovery issue, but I am not so sure if there are side effects.

I think we should vet more closely to confirm there is no side effects.
As the current impl of leo_storage_mq assumes there must be the object to be recovered on the local node, now read_repair also recover-node can fail to work in case the primary physically doesn't exist (in case the primary logically deleted are expected to work).

[mocchira]

@yosukehara @windkit IMO, the below patch seems to be correct. I'll send PR after getting enough tests done.

diff --git a/apps/leo_storage/src/leo_storage_mq.erl b/apps/leo_storage/src/leo_storage_mq.erl
index 870e37f..3ee760b 100644
--- a/apps/leo_storage/src/leo_storage_mq.erl
+++ b/apps/leo_storage/src/leo_storage_mq.erl
@@ -222,7 +222,7 @@ publish({?QUEUE_ID_DEL_DIR, WorkerId}, AddrId, Key, EnqueuedAt) ->
                                 addr_id = AddrId,
                                 key = Key,
                                 %% To compare it with its latest metadata
-                                %% See `compare_between_metadatas/1`
+                                %% See `compare_between_metadatas/2`
                                 meta = #?METADATA{addr_id = AddrId,
                                                   key = Key,
                                                   clock = EnqueuedAt},
@@ -343,7 +343,7 @@ handle_call({consume, ?QUEUE_ID_PER_OBJECT, MessageBin}) ->
                         true ->
                             send_object_to_remote_node(SyncNode, AddrId, Key);
                         false ->
-                            case compare_between_metadatas(Metadata) of
+                            case compare_between_metadatas(Metadata, false) of
                                 true ->
                                     case correct_redundancies(Key) of
                                         ok ->
@@ -1059,7 +1059,7 @@ remove_objects_under_dir(MessageBin) ->
                 {ok, #?MSG_ASYNC_DELETION{addr_id = AddrId,
                                           key = Key,
                                           meta = Metadata}} ->
-                    case compare_between_metadatas(Metadata) of
+                    case compare_between_metadatas(Metadata, true) of
                         true ->
                             case catch leo_storage_handler_object:delete(
                                          #?OBJECT{addr_id = AddrId,
@@ -1089,24 +1089,31 @@ remove_objects_under_dir(MessageBin) ->


 %% @doc Compare MQ's metadata with its latest metadata
--spec(compare_between_metadatas(Metadata) ->
+-spec(compare_between_metadatas(Metadata, IsDelete) ->
              Result | {error, Cause} when Metadata::#?METADATA{},
+                                          IsDelete::boolean(),
                                           Result::boolean(),
                                           Cause::any()).
-compare_between_metadatas(undefined) ->
+compare_between_metadatas(undefined, _) ->
     true;
 compare_between_metadatas(#?METADATA{addr_id = AddrId,
                                      key = Key,
-                                     clock = MsgClock}) ->
+                                     clock = MsgClock}, IsDelete) ->
     case leo_storage_handler_object:head(AddrId, Key, false) of
         {ok, #?METADATA{clock = Clock}} when MsgClock >= Clock ->
             true;
         {ok, _} ->
             false;
-        not_found ->
+        not_found when IsDelete =:= true ->
+            %% for remove_objects_under_dir
+            %% return false as the object already deleted on this node
             false;
+        not_found ->
+            %% for consume with QUEUE_ID_PER_OBJECT
+            %% return true as the object to be recovered on this node
+            true;
         {error, Cause} ->
-            ?error("compare_between_metadatas/1",
+            ?error("compare_between_metadatas/2",
                    [{addr_id, AddrId}, {key, Key},
                     {cause, Cause}]),
             {error, ?ERROR_COULD_NOT_GET_META}

[mocchira]

Sent PR: #886

Confirmed both cases

• the primary physically deleted get recovered through read-repair
• the primary physically deleted get recovered through recover-(file|node)

[yosukehara]

@mocchira @windkit I've reviewed @mocchira's PR, #886 which is LGTM.