Re: [Tarantool-patches] [PATCH vshard 5/6] router: introduce automatic master discovery

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Thanks for the review!

>> This is getting especially tricky with built-in automatic master
>> elections which are not supported by vshard yet, but they will be,
>> they won't depend on any config. Master role won't be pinned to
>> one node then.
> 
> Is it possible to implement some simple test that shows if RAFT changes
> 
> a leader, vshard catches this changes? Of course, it will be test for new Tarantool versions.

Nope. Because the storages do not support it yet. 'Master' attribute
can only be set via the config now, unfortunately. This will change
in the future, but it is blocked by one of the bugs (about bucket refs
being useless on replicas).

>> diff --git a/vshard/error.lua b/vshard/error.lua
>> index bcbcd71..e2d1a31 100644
>> --- a/vshard/error.lua
>> +++ b/vshard/error.lua
>> @@ -153,6 +153,11 @@ local error_message_template = {
>>           name = 'BUCKET_RECV_DATA_ERROR',
>>           msg = 'Can not receive the bucket %s data in space "%s" at tuple %s: %s',
>>           args = {'bucket_id', 'space', 'tuple', 'reason'},
>> +    },
>> +    [31] = {
>> +        name = 'MULTIPLE_MASTERS_FOUND',
>> +        msg = 'Found more than one master in replicaset %s on nodes %s and %s',
>> +        args = {'replicaset_uuid', 'master1', 'master2'},
>>       }
>>   }
> 
> Is it possible to test this error?

It is tested, yes. See master_discovery.test.lua. Section
"-- Multiple masters logging". Although I couldn't find an
easy way to expose it into vshard.router.info(). So it is
simply logged now.

>>   diff --git a/vshard/replicaset.lua b/vshard/replicaset.lua
>> index fa048c9..7747258 100644
>> --- a/vshard/replicaset.lua
>> +++ b/vshard/replicaset.lua
>> @@ -345,18 +377,30 @@ local function replicaset_master_call(replicaset, func, args, opts)
>>       assert(opts == nil or type(opts) == 'table')
>>       assert(type(func) == 'string', 'function name')
>>       assert(args == nil or type(args) == 'table', 'function arguments')
>> -    local conn, err = replicaset_connect_master(replicaset)
>> -    if not conn then
>> -        return nil, err
>> -    end
>> -    if not opts then
>> -        opts = {timeout = replicaset.master.net_timeout}
>> -    elseif not opts.timeout then
>> -        opts = table.copy(opts)
>> -        opts.timeout = replicaset.master.net_timeout
>> +    local master = replicaset.master
>> +    if not master then
>> +        opts = opts and table.copy(opts) or {}
>> +        if opts.is_async then
>> +            return nil, lerror.vshard(lerror.code.MISSING_MASTER,
>> +                                      replicaset.uuid)
>> +        end
> 
> Could / should we here wakeup master discovery if "auto" is specified?

We can. But I am afraid that under considerable RW requests load we
might wakeup it too often. And if there are many routers, they might
create a storm of `vshard.storage._call('info')` requests putting a
notable load on the storages. So I decided it is not safe until the
subscriptions are implemented.

>> @@ -425,11 +469,6 @@ local function replicaset_template_multicallro(prefer_replica, balance)
>>                   return r
>>               end
>>           end
>> -        local conn, err = replicaset_connect_master(replicaset)
>> -        if not conn then
>> -            return nil, err
>> -        end
>> -        return master
>>       end
> 
> Why don't we need this part anymore?

Now the caller handles the case when couldn't find any node. See
the next diff hunk. I moved it outside, because I didn't want
pick_next_replica() to take a timeout argument. I wanted it to
be as simple as possible.

>>       return function(replicaset, func, args, opts)
>> @@ -444,9 +483,13 @@ local function replicaset_template_multicallro(prefer_replica, balance)
>>           end
>>           local end_time = fiber_clock() + timeout
>>           while not net_status and timeout > 0 do
>> -            replica, err = pick_next_replica(replicaset)
>> +            replica = pick_next_replica(replicaset)
>>               if not replica then
>> -                return nil, err
>> +                replica, timeout = replicaset_wait_master(replicaset, timeout)
>> +                if not replica then
>> +                    return nil, timeout
>> +                end
>> +                replicaset_connect_to_replica(replicaset, replica)
>>               end
>>               opts.timeout = timeout
>>               net_status, storage_status, retval, err =
>> @@ -508,7 +551,128 @@ local function rebind_replicasets(replicasets, old_replicasets)

<...>

>> +    local master_uuid
>> +    for replica_uuid, f in pairs(futures) do
>> +        if timeout < 0 then
>> +            -- Netbox uses cond var inside, whose wait throws an error when gets
>> +            -- a negative timeout. Avoid that.
>> +            timeout = 0
>> +        end
>> +        res, err = f:wait_result(timeout)
>> +        timeout = deadline - fiber_clock()
>> +        if not res then
>> +            f:discard()
>> +            last_err = err
>> +            goto next_wait
> 
> If timeout will be negative we anyway go to next_wait and turn it to 0 at the next iteration.

Yup. This is intentional. With zero timeout the worst what can happen is a
yield. But on the other hand, while we were waiting for one request, the
others could complete and one of them could be the master. This is why I
continue the iteration - don't want to miss any arrived response.

<...>

>> +
>> +local function locate_masters(replicasets)
>> +    local is_all_done = true
>> +    local is_all_nop = true
>> +    local last_err
>> +    for _, replicaset in pairs(replicasets) do
>> +        local is_done, is_nop, err = replicaset_locate_master(replicaset)
> 
> I think we should log result of master discovery. Especially if error occurred.

It is logged by the caller. See master_search_f(), init.lua:1067.

>> diff --git a/vshard/router/init.lua b/vshard/router/init.lua
>> index 5e2a96b..9407ccd 100644
>> --- a/vshard/router/init.lua
>> +++ b/vshard/router/init.lua
>> @@ -1030,6 +1032,93 @@ local function failover_f(router)

<...>

>> +--
>> +-- Master discovery background function. It is supposed to notice master changes
>> +-- and find new masters in the replicasets, which are configured for that.
>> +--
>> +-- XXX: due to polling the search might notice master change not right when it
> Is this TODO related to https://github.com/tarantool/vshard/issues/284 ?

Yes, that is one of the "solutions", although I would rather call it a crutch.
I prefer to fix it by doing fair subscriptions. And for that I at least need a
way to get netbox call result asynchronously, without having to call wait_result()
anywhere.

<...>

>> +local function master_search_set(router)
>> +    local enable = false
>> +    for _, rs in pairs(router.replicasets) do
>> +        if rs.is_auto_master then
>> +            enable = true
>> +            break
>> +        end
>> +    end
>> +    local search_fiber = router.master_search_fiber
>> +    if enable and search_fiber == nil then
>> +        log.info('Master auto search is enabled')
>> +        router.master_search_fiber = util.reloadable_fiber_create(
>> +            'vshard.master_search.' .. router.name, M, 'master_search_f',
>> +            router)
> 
> On 1.10 fiber name is limited with 32 symbols.
> 
> tarantool> #'vshard.master_search.'
> ---
> - 21
> ...
> 
> That mean that router name is limited with 11 chars.
> 
> It could be a problem. As I see you don't use {truncate = true} when assign a name
> 
> https://github.com/tarantool/vshard/blob/a394e3f4008a7436f011c81028dcaadc270eebf6/vshard/util.lua#L93

Holy shit, that is a good find. I fixed it in a new commit on top of the
branch. For all the background fibers at once.

> Also it's possible to wakeup discovery and rebalanser fibers. Probably we should give a way to
> 
> wakeup master_search_fiber.

I didn't want people to rely on that. Because when the subscriptions
are implemented, there won't be a fiber. But I could make it NOP later
probably.

====================
diff --git a/test/router/master_discovery.result b/test/router/master_discovery.result
index 2fca1e4..6dcdf45 100644
--- a/test/router/master_discovery.result
+++ b/test/router/master_discovery.result
@@ -87,7 +87,7 @@ end
 function check_all_masters_found()                                              \
     for _, rs in pairs(vshard.router.static.replicasets) do                     \
         if not rs.master then                                                   \
-            vshard.router.static.master_search_fiber:wakeup()                   \
+            vshard.router.master_search_wakeup()                                \
             return false                                                        \
         end                                                                     \
     end                                                                         \
@@ -101,7 +101,7 @@ function check_master_for_replicaset(rs_id, master_name)
     local master_uuid = util.name_to_uuid[master_name]                          \
     local master = vshard.router.static.replicasets[rs_uuid].master             \
     if not master or master.uuid ~= master_uuid then                            \
-        vshard.router.static.master_search_fiber:wakeup()                       \
+        vshard.router.master_search_wakeup()                                    \
         return false                                                            \
     end                                                                         \
     return true                                                                 \
@@ -124,7 +124,7 @@ master_search_helper_f = nil
  | ...
 function aggressive_master_search_f()                                           \
     while true do                                                               \
-        vshard.router.static.master_search_fiber:wakeup()                       \
+        vshard.router.master_search_wakeup()                                    \
         fiber.sleep(0.001)                                                      \
     end                                                                         \
 end
diff --git a/test/router/master_discovery.test.lua b/test/router/master_discovery.test.lua
index e087f58..653725a 100644
--- a/test/router/master_discovery.test.lua
+++ b/test/router/master_discovery.test.lua
@@ -49,7 +49,7 @@ end
 function check_all_masters_found()                                              \
     for _, rs in pairs(vshard.router.static.replicasets) do                     \
         if not rs.master then                                                   \
-            vshard.router.static.master_search_fiber:wakeup()                   \
+            vshard.router.master_search_wakeup()                                \
             return false                                                        \
         end                                                                     \
     end                                                                         \
@@ -61,7 +61,7 @@ function check_master_for_replicaset(rs_id, master_name)
     local master_uuid = util.name_to_uuid[master_name]                          \
     local master = vshard.router.static.replicasets[rs_uuid].master             \
     if not master or master.uuid ~= master_uuid then                            \
-        vshard.router.static.master_search_fiber:wakeup()                       \
+        vshard.router.master_search_wakeup()                                    \
         return false                                                            \
     end                                                                         \
     return true                                                                 \
@@ -78,7 +78,7 @@ end
 master_search_helper_f = nil
 function aggressive_master_search_f()                                           \
     while true do                                                               \
-        vshard.router.static.master_search_fiber:wakeup()                       \
+        vshard.router.master_search_wakeup()                                    \
         fiber.sleep(0.001)                                                      \
     end                                                                         \
 end
diff --git a/vshard/router/init.lua b/vshard/router/init.lua
index 9407ccd..baaeee4 100644
--- a/vshard/router/init.lua
+++ b/vshard/router/init.lua
@@ -1119,6 +1119,13 @@ local function master_search_set(router)
     end
 end
 
+local function master_search_wakeup(router)
+    local f = router.master_search_fiber
+    if f then
+        f:wakeup()
+    end
+end
+
 --------------------------------------------------------------------------------
 -- Configuration
 --------------------------------------------------------------------------------
@@ -1545,6 +1552,7 @@ local router_mt = {
         bootstrap = cluster_bootstrap;
         bucket_discovery = bucket_discovery;
         discovery_wakeup = discovery_wakeup;
+        master_search_wakeup = master_search_wakeup,
         discovery_set = discovery_set,
         _route_map_clear = route_map_clear,
         _bucket_reset = bucket_reset,
====================

To the commit message I appended the following text:

    Master discovery works in its own fiber on the router, which is
    activated only if at least one replicaset is configured to look
    for the master. It wakes up with a certain period. But it is
    possible to wake it up on demand using
    `vshard.router.master_search_wakeup()` function. It does not do
    anything if master search is not configured for any replicaset.