[leo_redundant_manager] racy accesses to ring/member tables

[mocchira]

racy accesses

There are some racy accesses to ring/member tables like the below

• https://github.com/leo-project/leo_redundant_manager/blob/1.9.31/src/leo_redundant_manager_api.erl#L481-L517
  • delete_all(missing any records from other readers(erlang processes) at this point) and insert each record back when an error happened(other readers may reference the ring table updated partially while inserting here)
• https://github.com/leo-project/leo_redundant_manager/blob/1.9.31/src/leo_redundant_manager_api.erl#L158-L175
  • insert each record(other readers may reference the member table updated partially while inserting here)
  • and more

that may cause https://github.com/leo-project/leofs/issues/572 or something similar ones.

Not only just fixing those cases but to prevent of recurrence of racy issues as possible I'd like to make the one data access layer that wraps all operations to ring/member tables and other modules have to access ring/member ones only through this layer that makes it easy for maintainers to understand how things goes on also grantee the correctness with minimum effort.

a few NOT atomic functions in leo_clustertbl(member|ring)

like

• https://github.com/leo-project/leo_redundant_manager/blob/1.9.31/src/leo_cluster_tbl_ring.erl#L352-L435 (delete_all and inserts independently that should be done in one transaction)
• https://github.com/leo-project/leo_redundant_manager/blob/1.9.31/src/leo_cluster_tbl_member.erl#L454-L490
• https://github.com/leo-project/leo_redundant_manager/blob/1.9.31/src/leo_cluster_tbl_member.erl#L510-L530

just makes it atomic.

transaction is not enough?

also now we are using transaction context when interacting with mnesia but sync_transaction is more reliable choice since transaction doesn't grantee some causality of distributed features like spawning a process on some remote node and so on. so doing always right jobs may be difficult for maintainers while taking care such premises. so also I'd like to change all transaction contexts into sync_transaction.

TODO

• [ ] fix leo_clustertbl(member|ring) to make all functions atomic
  • [ ] leo_cluster_tbl_ring:delete
  • [ ] leo_cluster_tbl_ring:overwrite
  • [ ] leo_cluster_tbl_member:insert
  • [ ] leo_cluster_tbl_member:delete
  • [ ] leo_cluster_tbl_member:delete_all
  • [ ] leo_cluster_tbl_member:overwrite
• [ ] make one data access layer
  • [x] make the full use of leo_clustertbl(member|ring)
  • [x] define atomic data access I/F for new one
  • [x] design review
  • [ ] impl/ut/bench
  • [ ] code review
• [ ] change other modules to use the new one
  • [ ] leo_redundant_manager
  • [ ] leo_redundant_manager_api
  • [ ] leo_redundant_manager_worker
  • [ ] leo_redundant_manager_chash
  • [ ] code review
• [ ] integration tests for new data access layer with others
• [ ] change transaction into sync_transaction
  • [ ] leo_mnesia in leo_commons
  • [ ] leo_clustertbl(member|ring)

[mocchira]

• racy list
  • api
  • https://github.com/leo-project/leo_redundant_manager/blob/1.9.31/src/leo_redundant_manager_api.erl#L83-L141
  • https://github.com/leo-project/leo_redundant_manager/blob/1.9.31/src/leo_redundant_manager_api.erl#L158-L175
  • https://github.com/leo-project/leo_redundant_manager/blob/1.9.31/src/leo_redundant_manager_api.erl#L389-L519
  • rebalance
    • https://github.com/leo-project/leo_redundant_manager/blob/1.9.31/src/leo_redundant_manager_api.erl#L694-L742
    • https://github.com/leo-project/leo_redundant_manager/blob/1.9.31/src/leo_redundant_manager_api.erl#L780-L923
  • https://github.com/leo-project/leo_redundant_manager/blob/1.9.31/src/leo_redundant_manager_api.erl#L753-L776
  • https://github.com/leo-project/leo_redundant_manager/blob/1.9.31/src/leo_redundant_manager_api.erl#L931-L983
  • leo_redundant_manager.erl is protected through its intrinsic gen_server's trait
  • a new data access layer should be built on top of this gen_server?
  • worker
  • https://github.com/leo-project/leo_redundant_manager/blob/1.9.31/src/leo_redundant_manager_worker.erl#L361-L451
  • https://github.com/leo-project/leo_redundant_manager/blob/1.9.31/src/leo_redundant_manager_worker.erl#L458-L627
  • https://github.com/leo-project/leo_redundant_manager/blob/1.9.31/src/leo_redundant_manager_worker.erl#L942-L982
  • https://github.com/leo-project/leo_redundant_manager/blob/1.9.31/src/leo_redundant_manager_worker.erl#L777-L788
  • sup
  • https://github.com/leo-project/leo_redundant_manager/blob/1.9.31/src/leo_redundant_manager_sup.erl#L259-L265 must fail when failing to create_table
  • chash
  • https://github.com/leo-project/leo_redundant_manager/blob/1.9.31/src/leo_redundant_manager_chash.erl#L116-L210
  • https://github.com/leo-project/leo_redundant_manager/blob/1.9.31/src/leo_redundant_manager_chash.erl#L266-L287

[mocchira]

Generic Concept

• Since every function get to have each own transaction context, the resources(tables and states stored in gen_servers) modified during an execution in a function get to be either completely updated or not modified at all.

  • Get all logic that touches data(rings|tables) into mnesia:activity with sync_transaction

• Provide consistent views when handling read requests

  • ex) when retrieve a pair of prev and current one(rings|members)
  • as is
    • get one by one
  • to be
    • get both at once(atomic)

Rollback Policy

• Tables on mnesia can be rollbacked automatically when any error happened during a transaction context
• Others(tables on ets and states in gen_servers) need to be rollbacked manually when any error happened during a transaction
  • just following let it crash convention
  • Now ets belong to leo_redundant_manager_app so that need to move somewhere else that allow us to implement let it crash and rebuild member|ring ets tables
  • leo_redundant_manager or leo_redundant_manager_worker looks the most promising candidate to me
• The above two statements mean we need to handle each error in different way
  • rollback mnesia on leo_manager
  • let it crash (and rebuild member|ring ets tables) on leo_(gateway|storage)

TODO

• Tables on mnesia in manager nodes may no be synced with leo_redundant_manager's one if any error happened during update on managers. This should be fixed as another issue

The list of new I/F

To be (added|modified) on the existing leo_redundant_manager.

%% fix the current impl on chash to be atomic
-spec(range_of_vnodes(TableInfo, VNodeId) ->
             {ok, [tuple()]} | {error, any()} when TableInfo::table_info(),
                                  VNodeId::integer()).

%% fix the current impl around leo_redundant_manager(_api) to be atomic
-spec(rebalance() ->
             {ok, [tuple()]} | {error, any()}).

%% used by worker to sync member,ring information on gen_server with the source(mnesia|ets)
-spec(sync_worker(State) ->
             {ok, #state{}} | {error, any()} when State::#state{}).

%% used by api to sync source(mnesia|ets) with SyncData including the current, prev members
-spec(sync_source(SyncTarget, SyncData, Options) ->
             {ok, [{atom(), any()}]} |
             {error, any()} when SyncTarget::sync_target(),
                                 SyncData::[{atom(), any()}],
                                 Options::[{atom(), any()}]).

%% update the both(cur, prev) of ring tables based on the member table
-spec(create() -> ok).

%% https://github.com/leo-project/leo_redundant_manager/blob/1.9.31/src/leo_redundant_manager.erl#L648-L665
%% fix to be atomic
-spec(attach(Node, AwarenessL2, Clock, NumOfVNodes) ->
             ok | {error, any()} when Node::atom(),
                                      AwarenessL2::string(),
                                      Clock::integer(),
                                      NumOfVNodes::integer()).
-spec(attach(Node, AwarenessL2, Clock, NumOfVNodes, RPCPort) ->
             ok | {error, any()} when Node::atom(),
                                      AwarenessL2::string(),
                                      Clock::integer(),
                                      NumOfVNodes::integer(),
                                      RPCPort::integer()).
-spec(attach(TableInfo, Node, AwarenessL2, Clock, NumOfVNodes, RPCPort) ->
             ok | {error, any()} when TableInfo::ring_table_info(),
                                      Node::atom(),
                                      AwarenessL2::string(),
                                      Clock::integer(),
                                      NumOfVNodes::integer(),
                                      RPCPort::integer()).

%% https://github.com/leo-project/leo_redundant_manager/blob/1.9.31/src/leo_redundant_manager.erl#L670-L682
%% fix to be atomic
-spec(detach(Node, Clock) ->
             ok | {error, any()} when Node::atom(),
                                      Clock::integer()).
-spec(detach(TableInfo, Node, Clock) ->
             ok | {error, any()} when TableInfo::ring_table_info(),
                                      Node::atom(),
                                      Clock::integer()).

%% fix the current impl around leo_redundant_manager_(api|chash) to be atomic
%% when retrieving rings to retrieve both at once(to be) from retrieve each one by one(as is)
-spec(checksum(Type) ->
             {ok, integer()} |
             {ok, {integer(), integer()}} |
             {error, any()} when Type::checksum_type()).

%% fix the current impl around leo_redundant_manager(_api) to be atomic
%% when retrieving members to retrieve both at once(to be) from retrieve each one by one(as is)
-spec(get_all_ver_members() ->
             {ok, {CurMembers, PrevMembers}} |
             {error, Cause} when CurMembers::[#member{}],
                                 PrevMembers::[#member{}],
                                 Cause::any()).

%% fix the current impl around leo_redundant_manager_(api|worker) to be atomic
%% sync both at once(to be) from sync each one by one(as is)
-spec(force_sync_workers() ->
             ok).

%% fix the current impl around leo_redundant_manager(_api) to be atomic
-spec(update_members(Members) ->
             ok | {error, any()} when Members::[#member{}]).

%% fix the current impl around leo_redundant_manager(_api) to be atomic
-spec(update_member_by_node() -> ok).

%% fix the current impl around leo_redundant_manager_api to be atomic
-spec(get_alias(Node, GrpL2) ->
             {ok, tuple()} when Node::atom(),
                                GrpL2::string()).
-spec(get_alias(Table, Node, GrpL2) ->
             {ok, tuple()} when Table::member_table(),
                                Node::atom(),
                                GrpL2::string()).

%% no problem
%% -spec(reserve() -> ok).
%% -spec(suspend() -> ok).
%% -spec(dump() -> ok).
%% -spec(has_member() -> ok).
%% -spec(get_members() -> ok).
%% -spec(get_member_by_node() -> ok).
%% -spec(get_member_by_status() -> ok).
%% -spec(update_member() -> ok).
%% -spec(delete_member_by_node() -> ok).

[mocchira]

@yosukehara I've finished the first version at https://github.com/leo-project/leofs/issues/582#issuecomment-274725931 . Please review.

[yosukehara]

@mocchira I've reviewed https://github.com/leo-project/leofs/issues/582#issuecomment-274725931 . There is no issue, and I'm with you. Do you think you implement that?

[mocchira]

@yosukehara thanks for reviewing. I'll go forward based on this design.

[mocchira]

@yosukehara As I've updated the Rollback Policy section at https://github.com/leo-project/leofs/issues/582#issuecomment-274725931 in order to make it easy to implement rollback on memory information(ets, state on gen_server), PTAL.