From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 944EA26140 for ; Thu, 21 Jun 2018 08:54:27 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Qa5eDfR4oDka for ; Thu, 21 Jun 2018 08:54:27 -0400 (EDT) Received: from smtp63.i.mail.ru (smtp63.i.mail.ru [217.69.128.43]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 50F0D26290 for ; Thu, 21 Jun 2018 08:54:27 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 2/2] Fix discovery/reconfigure race References: From: Vladislav Shpilevoy Message-ID: Date: Thu, 21 Jun 2018 15:54:25 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org, AKhatskevich Thanks for the patch! See 5 comments below. On 15/06/2018 15:47, AKhatskevich wrote: > This commit prevents discovery fiber from discovering old replicasets > and spoiling `route_map`. > --- > test/router/router.result | 62 +++++++++++++++++++++++++++++++++++++++++++++ > test/router/router.test.lua | 42 ++++++++++++++++++++++++++++++ > vshard/router/init.lua | 15 ++++++++++- > 3 files changed, 118 insertions(+), 1 deletion(-) > > diff --git a/test/router/router.result b/test/router/router.result > index 5643f3e..e61505e 100644 > --- a/test/router/router.result > +++ b/test/router/router.result > @@ -1095,6 +1095,68 @@ for bucket, old_rs in pairs(bucket_to_old_rs) do > end; > --- > ... > +-- > +-- Check route_map is not filled with old replica objects after > +-- recpnfigure. 1. Typo. > +-- > +-- Perform #replicasets phases of discovery, to update replicasets > +-- object in for loop of discovery fiber since previous cfg. > +for _, __ in pairs(vshard.router.internal.replicasets) do > + vshard.router.discovery_wakeup() > + fiber.sleep(0.02) > +end; > +--- > +... > +-- Simulate long `callro`. > +-- Stuck on first rs in replicasets. > +vshard.router.internal.errinj.LONG_DISCOVERY = true; 2. I do not see this error injection in the M.errinj declaration. > +--- > +... > +for _, __ in pairs(vshard.router.internal.replicasets) do > + vshard.router.discovery_wakeup() > + fiber.sleep(0.02) > +end; 3. This cycle makes no sense. With set LONG_DISCOVERY it is equivalent to calling router.discovery_wakeup() once. > +--- > +... > +vshard.router.cfg(cfg); > +--- > +... > +vshard.router.internal.errinj.LONG_DISCOVERY = nil; > +--- > +... > +-- Do discovery iteration. > +vshard.router.discovery_wakeup() > +fiber.sleep(0.02) 4. Concrete timeouts are the way to create an unstable test. Please, get rid of them and replace with 'while not cond do wait end' where necessary. > + > +rs_cnt = 0; > +--- > +... > +new_replicasets = {} > +for _, rs in pairs(vshard.router.internal.replicasets) do > + new_replicasets[rs] = true > + rs_cnt = rs_cnt + 1 > +end; > +--- > +... > +rs_cnt; > +--- > +- 2 > +... > +bucket_cnt = 0; > +--- > +... > +for bucket_id, rs in pairs(vshard.router.internal.route_map) do > + if not new_replicasets[rs] then> + error('Old object added to route_map.') > + end > + bucket_cnt = bucket_cnt + 1 > +end; > +--- > +... > +bucket_cnt; > +--- > +- 3000 > +... > test_run:cmd("setopt delimiter ''"); > --- > - true > diff --git a/vshard/router/init.lua b/vshard/router/init.lua > index 7e765fa..df5b343 100644 > --- a/vshard/router/init.lua > +++ b/vshard/router/init.lua > @@ -127,10 +127,23 @@ local function discovery_f(module_version) > local iterations_until_lua_gc = > consts.COLLECT_LUA_GARBAGE_INTERVAL / consts.DISCOVERY_INTERVAL > while module_version == M.module_version do > - for _, replicaset in pairs(M.replicasets) do > + local old_replicasets = M.replicasets > + for rs_uuid, replicaset in pairs(M.replicasets) do > local active_buckets, err = > replicaset:callro('vshard.storage.buckets_discovery', {}, > {timeout = 2}) > + while M.errinj.LONG_DISCOVERY do > + -- Stuck on the first replicaset. > + if rs_uuid ~= select(1, next(M.replicasets)) then > + break > + end > + lfiber.sleep(0.01) > + end > + -- Renew replicasets object in case of reconfigure > + -- and reload events. 5. You do not renew here anything. > + if M.replicasets ~= old_replicasets then > + break > + end > if not active_buckets then > log.error('Error during discovery %s: %s', replicaset, err) > else >