From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 053736EC55; Wed, 14 Jul 2021 21:35:31 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 053736EC55 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1626287731; bh=OvCJvC//GxnYVGhw4wxCF9tKYu0nikUY2gRxiEWS+Ws=; h=To:Cc:References:Date:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=LLp6+ebXLdvSzIUk53dYm/RrFPyObUPMSOGho48oehe51+YY9iK34S6AzN5d1M9cH QY23babVTrhLn7Uy1aBrtW+HNY6tPamPmJtClGq1RcGKOVVo4MxbGlSdRIqzKCLR2C URqsxGxL0/vsC/5ZojlVeH+ngeYazHFpn+coXJJo= Received: from smtp16.mail.ru (smtp16.mail.ru [94.100.176.153]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 3DDE56E224 for ; Wed, 14 Jul 2021 21:28:02 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 3DDE56E224 Received: by smtp16.mail.ru with esmtpa (envelope-from ) id 1m3jc1-0007Ya-Hn; Wed, 14 Jul 2021 21:28:01 +0300 To: Vladislav Shpilevoy , gorcunov@gmail.com Cc: tarantool-patches@dev.tarantool.org References: Message-ID: <3b4f7752-f68b-afce-c83a-355bcfbc28cc@tarantool.org> Date: Wed, 14 Jul 2021 21:28:00 +0300 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-GB X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD97BB0EF39AD2B33D52D9CC5C87942E9F1EA2CB6CC9AFB41C2182A05F538085040DC918DAB594A81FE2CB34C801A1F3701125D73581420508F59BA62C7B203FCB5 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7A2D2532E5D166A73EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637D19071B5A26B4BDC8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D860CAB500DDE898C25BCE9F03F4BC4148117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCF1175FABE1C0F9B6A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD18CB629EEF1311BF91D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B6D0C9BB9AE6BD5D69089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A53073B8B634958967AA1F10A6AE665D2C4DF6345C425D6FFFD59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75E1D4179F59451251410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34A5970F8A4685C8AB9520379B9ED777C701E31D7A8AA9DD19E9858196FA172D7A4FD1FF72666458601D7E09C32AA3244CD03F8328BBA29813DF2F6E342FAAAF6F4DBEAD0ED6C55A80FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojDdSFIg49M1QihG/LidF1IA== X-Mailru-Sender: 3B9A0136629DC9125D61937A2360A446144E298974287DF410A6C1811A763CAA720D3BCC8331086C424AE0EB1F3D1D21E2978F233C3FAE6EE63DB1732555E4A8EE80603BA4A5B0BC112434F685709FCF0DA7A0AF5A3A8387 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v3 07/12] box: introduce `box.ctl.demote` X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Serge Petrenko via Tarantool-patches Reply-To: Serge Petrenko Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 04.07.2021 15:27, Vladislav Shpilevoy пишет: > Thanks for working on this! Thanks for a thorough review! > See 11 comments below. > > 1. On this commit a lot of tests fail. Why? Must be because persisting limbo and raft state isn't yet implemented. I'll move the relevant patches before this one. > 2. On top of this commit and on top of the branch too I tried to > promote a candidate and got a strange error in the logs, although > the promotion was successful: > > -- > -- Instance 1 > -- > > -- Step 1 > box.cfg{ > listen = 3313, > election_mode = 'candidate', > replication_synchro_quorum = 2, > replication = {'localhost:3314', 'localhost:3313'} > } > box.schema.user.grant('guest', 'super') > > > -- > -- Instance 2 > -- > > -- Step 2 > box.cfg{ > listen = 3314, > election_mode = 'voter', > replication_synchro_quorum = 2, > replication = {'localhost:3314', 'localhost:3313'}, > read_only = true, > } > > -- Step 3 > box.cfg{read_only = false, election_mode = 'candidate'} > > -- Step 4 > box.ctl.promote() > > main/112/raft_worker box.cc:1538 E> ER_UNSUPPORTED: box.ctl.promote does not support simultaneous invocations > --- > ... That's because once a candidate becomes the leader, it tries to issue `box.ctl.promote()`, and fails, since we're already in `box.ctl.promote()` call. I'm not sure how to handle that properly. This doesn't break anything though. > 3. On top of the branch I tried demote on a leader with election mode > candidate 2 times. Demote didn't do anything on the second invocation. > The test: > > -- > -- Instance 1 > -- > > -- Step 1 > box.cfg{ > listen = 3313, > election_mode = 'candidate', > replication_synchro_quorum = 2, > replication = {'localhost:3314', 'localhost:3313'} > } > box.schema.user.grant('guest', 'super') > > -- Step 3 > -- I demote self but become leader again because I am a > -- candidate. This is fine. > box.ctl.demote() > > -- Step 4 > tarantool> box.info.election > --- > - state: leader > vote: 1 > leader: 1 > term: 3 > ... > > -- Step 5 > -- But now I try to demote again and nothing happens. > -- I expected the same behaviour - bump term and become > -- a leader again. > tarantool> box.ctl.demote() > --- > ... > > tarantool> box.info.election > --- > - state: leader > vote: 1 > leader: 1 > term: 3 <- the term is the same, so demote didn't do anything. > ... > > -- > -- Instance 2 > -- > > -- Step 2 > box.cfg{ > listen = 3314, > election_mode = 'voter', > replication_synchro_quorum = 2, > replication = {'localhost:3314', 'localhost:3313'}, > read_only = true, > } > This one is fixed now. > 4. I tried switching between candidate and manual, trying promote/demote > in different combinations and got promote not doing anything. The test > is the same as above but steps >= 3 are different: > > tarantool> box.ctl.demote() > 2021-07-04 14:22:49.550 [588] main/103/interactive I> RAFT: bump term to 3, follow > 2021-07-04 14:22:49.550 [588] main/113/raft_worker I> RAFT: persisted state {term: 3} > 2021-07-04 14:22:49.550 [588] main/113/raft_worker I> RAFT: vote for 1, follow > 2021-07-04 14:22:49.551 [588] main/113/raft_worker I> RAFT: persisted state {term: 3, vote: 1} > 2021-07-04 14:22:49.551 [588] main/113/raft_worker I> RAFT: enter candidate state with 1 self vote > --- > ... > > tarantool> 2021-07-04 14:22:49.552 [588] main/110/applier/localhost:3314 I> RAFT: message {term: 3, state: follower} from 2 > 2021-07-04 14:22:49.552 [588] main/110/applier/localhost:3314 I> RAFT: message {term: 3, vote: 1, state: follower} from 2 > 2021-07-04 14:22:49.552 [588] main/110/applier/localhost:3314 I> RAFT: enter leader state with quorum 2 > 2021-07-04 14:22:49.552 [588] main/110/applier/localhost:3314 I> RAFT: message {term: 3, vote: 1, state: follower} from 2 > 2021-07-04 14:22:49.552 [588] main/110/applier/localhost:3314 I> RAFT: vote request is skipped - the leader is already known - 1 > 2021-07-04 14:22:49.553 [588] relay/[::1]:54006/101/main I> recover from `./00000000000000000000.xlog' > > --- > ... > > tarantool> box.cfg{election_mode='manual'} > 2021-07-04 14:22:55.391 [588] main/103/interactive I> set 'election_mode' configuration option to "manual" > --- > ... > > tarantool> box.info.election > --- > - state: follower > vote: 1 > leader: 0 > term: 3 > ... > > tarantool> box.ctl.promote() > --- > ... > > tarantool> box.info.election > --- > - state: follower > vote: 1 > leader: 0 > term: 3 > ... > > As you can see, the last promote() didn't do anything. The node > stayed a follower, and no errors were raised. > > > From the comments 2-4 it seems to me the complexity of > box_clear_synchro_queue()/box_promote() is getting out of control with > the number of ifs and flags, sometimes having unexpected meaning (for > example, try_wait flag causes term bump somewhy). It might be good to > split it into separate functions and make promotion/demotion out of each > election mode linear and straight. I agree. I've reworked promote() to be more sane, please see the corresponding patches in v4. > >> @TarantoolBot document >> Title: box.ctl.demote >> >> `box.ctl.demote()` is a new function, which works exactly like >> `box.ctl.promote()`, with one exception that it results in the instance >> writing DEMOTE request to WAL instead of a PROMOTE request. >> >> A DEMOTE request (DEMOTE = 32) copies PROMOTE behaviour (it clears the >> limbo as well), but clears the synchronous transaction queue ownership instead >> of assigning it to a new instance. > 5. It is worth mentioning that demote can only be called on a master. Sure. >> diff --git a/src/box/box.cc b/src/box/box.cc >> index 1d894be97..86c5967b9 100644 >> --- a/src/box/box.cc >> +++ b/src/box/box.cc >> @@ -1674,15 +1680,22 @@ box_promote(void) >> rc = -1; >> } else { >> promote: >> - if (try_wait) { >> + if (try_wait || demote) { >> raft_new_term(box_raft()); >> if (box_raft_wait_persisted() < 0) >> return -1; > 6. What if now some other node won the elections of the term > you just bumped? Then you wouldn't have the right to perform > DEMOTE as the term is not yours. Yes, indeed. Here's how I reworked this bit: let's not write demote at all, when elections are enabled. Simply bumping the term should be enough. Another leader (or maybe this instance) will take over the limbo in the new term. When elections are off, assume nothing can happen other than someone else writes a promote/demote earlier than we do. I handle it as well in the new patch. >> } >> uint64_t term = box_raft()->term; >> - txn_limbo_write_promote(&txn_limbo, wait_lsn, term); >> + if (demote) { >> + txn_limbo_write_demote(&txn_limbo, wait_lsn, >> + term); >> + } else { >> + txn_limbo_write_promote(&txn_limbo, wait_lsn, >> + term); >> + } >> + uint16_t type = demote ? IPROTO_DEMOTE : IPROTO_PROMOTE; >> struct synchro_request req = { >> - .type = IPROTO_PROMOTE, >> + .type = type, >> .replica_id = former_leader_id, >> .origin_id = instance_id, >> .lsn = wait_lsn, >> diff --git a/test/replication/gh-5446-qsync-eval-quorum.test.lua b/test/replication/gh-5446-qsync-eval-quorum.test.lua >> index 6b9e324ed..b969df836 100644 >> --- a/test/replication/gh-5446-qsync-eval-quorum.test.lua >> +++ b/test/replication/gh-5446-qsync-eval-quorum.test.lua >> @@ -135,3 +136,5 @@ box.cfg{ >> replication_synchro_quorum = old_synchro_quorum, \ >> replication_synchro_timeout = old_synchro_timeout, \ >> } >> +box.ctl.demote() >> + > 7. Git diff highlights this new line with red as unnecessary. Sure, fixed. >> diff --git a/test/replication/gh-6034-limbo-ownership.result b/test/replication/gh-6034-limbo-ownership.result >> new file mode 100644 >> index 000000000..e412b8d53 >> --- /dev/null >> +++ b/test/replication/gh-6034-limbo-ownership.result > 8. Could you please write qsync tests with gh-####-qsync-... > pattern? It is useful when you want to run all qsync tests > and can do `python test-run.py qsync` then. Yep. >> @@ -0,0 +1,189 @@ >> +-- test-run result file version 2 >> +test_run = require('test_run').new() >> + | --- >> + | ... >> +fiber = require('fiber') > 9. Fiber is not used in the test. Removed, thanks! >> + | --- >> + | ... >> + > <...> > >> +assert(box.info.synchro.queue.owner == test_run:eval('default', 'return box.info.id')[1]) > 10. There is test_run:get_server_id(). Hm, didn't know that, thanks! >> + | --- >> + | - true >> + | ... >> +box.space.async:insert{2} -- failure. >> + | --- >> + | - error: Can't modify data because this instance is in read-only mode. >> + | ... >> + >> +-- Promotion on the other node. Default should become ro. >> +box.ctl.promote() >> + | --- >> + | ... >> +assert(not box.info.ro) >> + | --- >> + | - true >> + | ... >> +assert(box.info.synchro.queue.owner == box.info.id) >> + | --- >> + | - true >> + | ... >> +box.space.sync:insert{2} -- success. >> + | --- >> + | - [2] >> + | ... >> + >> +test_run:switch('default') >> + | --- >> + | - true >> + | ... >> +assert(box.info.ro) >> + | --- >> + | - true >> + | ... >> +assert(box.info.synchro.queue.owner == test_run:eval('replica', 'return box.info.id')[1]) > 11. Ditto. Fixed. -- Serge Petrenko