From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp33.i.mail.ru (smtp33.i.mail.ru [94.100.177.93]) (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 61900445320 for ; Tue, 7 Jul 2020 15:12:26 +0300 (MSK) Date: Tue, 7 Jul 2020 15:12:24 +0300 From: Sergey Bronnikov Message-ID: <20200707121224.GA58485@pony.bronevichok.ru> References: <012c8c196396cf963a0aa1f2d23814ff84b81cfb.1593723973.git.sergeyb@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH 2/4] replication: add advanced tests for sync replication List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org On 01:31 Tue 07 Jul , Vladislav Shpilevoy wrote: > Коммит уже сверху поменялся другими коммитами, так что далее я > копипасчу тест как он есть на самом верху ветки. > > См. 16 комментариев далее по тексту. > > > env = require('test_run') > > test_run = env.new() > > engine = test_run:get_cfg('engine') > > fiber = require('fiber') > > > > orig_synchro_quorum = box.cfg.replication_synchro_quorum > > orig_synchro_timeout = box.cfg.replication_synchro_timeout > > > > NUM_INSTANCES = 2 > > BROKEN_QUORUM = NUM_INSTANCES + 1 > > > > test_run:cmd("setopt delimiter ';'") > > disable_sync_mode = function() > > local s = box.space._space:get(box.space.sync.id) > > local new_s = s:update({{'=', 6, {is_sync=false}}}) > > box.space._space:replace(new_s) > > 1. Я не уверен, что есть смысл сейчас тестировать смену типа спейса с > асинхронного на синхронный и наоборот. Потому как непонятно до конца, > как это должно работать. Нужен ли кворум на эту DDL транзакцию? Если > да, то как его запрашивать? _space не синхронный. И надо ли его > запрашивать вообще, или первая же транзакция на этот спейс уже "пропушает" > его синхронность? Надо на эти вопросы ответить, а потом их уже > тестить кмк. У нас есть RFC, который мы долго обсуждали и если есть вопросы по дизайну, то нужно дорабатывать RFC, а не выпиливать тесты. Пока оставляю этот вопрос открытым. > > end; > > test_run:cmd("setopt delimiter ''"); > > > > box.schema.user.grant('guest', 'replication') > > > > -- Setup an async cluster with two instances. > > 2. Вообще говоря, у нас нет понятия асинхронный или синхронный кластер. Это > спейсы могут быть синхронными или нет. Да, все так, убрал указание типа кластера. --- Setup an async cluster with two instances. +-- Setup an cluster with two instances. > > test_run:cmd('create server replica with rpl_master=default,\ > > script="replication/replica.lua"') > > test_run:cmd('start server replica with wait=True, wait_load=True') > > > > -- Successful write. > > -- Testcase setup. > > 3. Этот тест явно не выглядит как advanced. И уже вполне покрыт в basic тестах. > Кроме того, таймаута 0.1 может не хватить. Тест может чуть подвиснуть под нагрузкой > и все полетит в ужасно огромный дифф с кучей ошибок. Если используешь > таймауты, то надо стараться делать так: если хочешь, чтоб таймаут не истек, > ставь его огромным, в тысячи; если хочешь, чтоб истек - ставь в ноль или в очень-очень > малое число (0.0001, например) + можно добавить fiber.sleep(<этот таймаут>), > чтоб все дедлайны в коде наверняка просрались. Когда я начинал писать тесты мне нужно было вначале сделать smoke тесты, чтобы перед запуском более сложных быть уверенным, что базовый сценарий работает. > Все тоже самое про тест ниже (Unsuccessful write). Убрал оба. > > test_run:switch('default') > > box.cfg{replication_synchro_quorum=NUM_INSTANCES, replication_synchro_timeout=0.1} > > _ = box.schema.space.create('sync', {is_sync=true, engine=engine}) > > _ = box.space.sync:create_index('pk') > > -- Testcase body. > > box.space.sync:insert{1} -- success > > test_run:cmd('switch replica') > > box.space.sync:select{} -- 1 > > -- Testcase cleanup. > > test_run:switch('default') > > box.space.sync:drop() > > > > -- Unsuccessfull write. > > 4. Unsuccessfull -> Unsuccessful. > Да, но неактуально, потому что удалил эти тесты. > > -- Testcase setup. > > test_run:switch('default') > > box.cfg{replication_synchro_quorum=BROKEN_QUORUM, replication_synchro_timeout=0.1} > > _ = box.schema.space.create('sync', {is_sync=true, engine=engine}) > > _ = box.space.sync:create_index('pk') > > -- Testcase body. > > box.space.sync:insert{1} > > test_run:switch('replica') > > box.space.sync:select{} -- none > > -- Testcase cleanup. > > test_run:switch('default') > > box.space.sync:drop() > > > > -- Updated replication_synchro_quorum doesn't affect existed tx. > > 5. Интересно, что когда я сделал, чтоб он начал аффектить, то этот тест > тоже прошел. В чем его полезность тогда? Кроме того, он теперь > покрыт в qsync_basic.test.lua. Полезность тестов не только в том, чтобы сейчас выявлять какие то баги, но и в том, чтобы зафиксировать желаемое поведение и при рефакторингах или редизайне фичи срабатывать при изменении поведения. Но тест я удалил, потому что я не знаю, как сделать надежное создание состояние для этого теста. > > -- Testcase setup. > > test_run:switch('default') > > _ = box.schema.space.create('sync', {is_sync=true, engine=engine}) > > _ = box.space.sync:create_index('pk') > > -- Testcase body. > > box.cfg{replication_synchro_quorum=BROKEN_QUORUM, replication_synchro_timeout=0.0001} > > box.error.injection.set('ERRINJ_SYNC_TIMEOUT', true) > > test_run:cmd("setopt delimiter ';'") > > _ = fiber.create(function() > > box.space.sync:insert{1} > > end); > > test_run:cmd("setopt delimiter ''"); > > box.cfg{replication_synchro_quorum=NUM_INSTANCES} > > box.error.injection.set('ERRINJ_SYNC_TIMEOUT', false) > > box.space.sync:select{} -- none > > test_run:switch('replica') > > box.space.sync:select{} -- none > > -- Testcase cleanup. > > test_run:switch('default') > > box.space.sync:drop() > > > > -- [RFC, quorum commit] attempt to write multiple transactions, expected the > > -- same order as on client in case of achieved quorum. > > -- Testcase setup. > > 6. Опять же, все эти вещи покрыты в basic. Зачем это дублирование? Хороший риторический вопрос. Перед тем как делать синхронную репликацию мы проработали RFC и я описал тесты по этому RFC, которые я планировал делать. Тесты отправлял на ревью и до того, как я начал их писать можно было ознакомиться с планом. По мере того, как я писал тесты, я не всегда успевал следить за тем, какие тесты добавляли в ветку. А те, кто пушил тесты в ветку похоже не видел мой план по тестам. Вот так получилось дублирование. > > test_run:switch('default') > > box.cfg{replication_synchro_quorum=NUM_INSTANCES, replication_synchro_timeout=0.1} > > _ = box.schema.space.create('sync', {is_sync=true, engine=engine}) > > _ = box.space.sync:create_index('pk') > > -- Testcase body. > > box.space.sync:insert{1} > > box.space.sync:insert{2} > > box.space.sync:insert{3} > > box.space.sync:select{} -- 1, 2, 3 > > test_run:switch('replica') > > box.space.sync:select{} -- 1, 2, 3 > > -- Testcase cleanup. > > test_run:switch('default') > > box.space.sync:drop() > > > > -- Synchro timeout is not bigger than replication_synchro_timeout value. > > -- Testcase setup. > > test_run:switch('default') > > box.cfg{replication_synchro_quorum=BROKEN_QUORUM, replication_synchro_timeout=orig_synchro_timeout} > > _ = box.schema.space.create('sync', {is_sync=true, engine=engine}) > > _ = box.space.sync:create_index('pk') > > -- Testcase body. > > start = os.time() > > box.space.sync:insert{1} > > (os.time() - start) == box.cfg.replication_synchro_timeout -- true > > 7. Очень плохая идея. Если процесс подвиснет тут ненадолго, то эта проверка > упадет. Не должно быть тестов, которые полагаются на то, что процесс будет > выполняться стабильно. Ты предлагаешь не проверять или есть более надежные способы проверки, что таймаут именно такой величины, каким его выставили? > > -- Testcase cleanup. > > test_run:switch('default') > > box.space.sync:drop() > > > > -- replication_synchro_quorum > > test_run:switch('default') > > INT_MIN = -2147483648 > > INT_MAX = 2147483648 > > box.cfg{replication_synchro_quorum=INT_MAX} -- error > > box.cfg.replication_synchro_quorum -- old value > > box.cfg{replication_synchro_quorum=INT_MIN} -- error > > box.cfg.replication_synchro_quorum -- old value > > 8. Это тоже явно не advanced тесты. Это самые базовые проверки. Я изначально делал тесты в отдельном файле, чтобы проще было изменять это в общей ветке, без мержей, ребейзов и прочих вещей. Тесты назвались advanced, потомы что должны были покрывать высокоуровневые требования из RFC. Я могу перенести эти тесты в qsync_basic, если возражений по сути тестов нет. > > -- replication_synchro_timeout > > test_run:switch('default') > > DOUBLE_MAX = 9007199254740992 > > box.cfg{replication_synchro_timeout=DOUBLE_MAX} > > box.cfg.replication_synchro_timeout -- DOUBLE_MAX > > box.cfg{replication_synchro_timeout=DOUBLE_MAX+1} > > box.cfg.replication_synchro_timeout -- DOUBLE_MAX > > box.cfg{replication_synchro_timeout=-1} -- error > > box.cfg.replication_synchro_timeout -- old value > > box.cfg{replication_synchro_timeout=0} -- error > > box.cfg.replication_synchro_timeout -- old value > > > > -- TX is in synchronous replication. > > 9. Не понял комментарий. И не понял, чем этот тест отличается от > "[RFC, quorum commit]" выше. По-моему я этот тест первым сделал и оставил для простой проверки того, что tx работают. Сейчас да, дублирование есть. Удалю его. > > -- Testcase setup. > > test_run:switch('default') > > box.cfg{replication_synchro_quorum=NUM_INSTANCES, replication_synchro_timeout=0.1} > > _ = box.schema.space.create('sync', {is_sync=true, engine=engine}) > > _ = box.space.sync:create_index('pk') > > -- Testcase body. > > box.begin() box.space.sync:insert({1}) box.commit() > > box.begin() box.space.sync:insert({2}) box.commit() > > -- Testcase cleanup. > > box.space.sync:drop() > > > > -- [RFC, summary] switch sync replicas into async ones, expected success and > > -- data consistency on a leader and replicas. > > 10. Это пожалуй пока единственный тест, который тут можно было бы оставить. > То есть 'advanced'. Но коммент неверен - нет никаких синхронных реплик. > Есть синхронные транзакции. Которые определяются синхронными спейсами. RFC: "ability to switch async replicas into sync ones and vice versa" ^^^^^^^^^^^^^^^^^^^ В тесте поправлю комментарий. Еще, как я понял, у тебя были возражения по поводу того, как делаем выключение синхронной репликации, чтобы она стала асинхронной. Или запись в системный спейс это ок? > > -- Testcase setup. > > test_run:switch('default') > > box.cfg{replication_synchro_quorum=NUM_INSTANCES, replication_synchro_timeout=0.1} > > _ = box.schema.space.create('sync', {is_sync=true, engine=engine}) > > _ = box.space.sync:create_index('pk') > > -- Testcase body. > > box.space.sync:insert{1} > > box.space.sync:select{} -- 1 > > test_run:switch('replica') > > box.space.sync:select{} -- 1 > > test_run:switch('default') > > -- Disable synchronous mode. > > disable_sync_mode() > > -- Space is in async mode now. > > box.cfg{replication_synchro_quorum=NUM_INSTANCES} > > box.space.sync:insert{2} -- success > > box.space.sync:insert{3} -- success > > box.cfg{replication_synchro_quorum=BROKEN_QUORUM} > > box.space.sync:insert{4} -- success > > box.cfg{replication_synchro_quorum=NUM_INSTANCES} > > box.space.sync:insert{5} -- success > > box.space.sync:select{} -- 1, 2, 3, 4, 5 > > test_run:cmd('switch replica') > > box.space.sync:select{} -- 1, 2, 3, 4, 5 > > -- Testcase cleanup. > > test_run:switch('default') > > box.space.sync:drop() > > > > -- [RFC, Synchronous replication enabling] "As soon as last operation of > > -- synchronous transaction appeared in leader's WAL, it will cause all > > -- following transactions - no matter if they are synchronous or not - wait for > > -- the quorum. In case quorum is not achieved the 'rollback' operation will > > -- cause rollback of all transactions after the synchronous one." > > 11. Это уже протестировано в qsync_errinj.test.lua. Окей, удалил. Тем более, что с ERRINJ_SYNC_TIMEOUT у меня не получилось сделать как я хотел. > > -- Testcase setup. > > test_run:switch('default') > > box.cfg{replication_synchro_quorum=NUM_INSTANCES, replication_synchro_timeout=0.1} > > _ = box.schema.space.create('sync', {is_sync=true, engine=engine}) > > _ = box.space.sync:create_index('pk') > > -- Testcase body. > > box.space.sync:insert{1} > > box.space.sync:select{} -- 1 > > test_run:switch('replica') > > box.space.sync:select{} -- 1 > > test_run:switch('default') > > box.cfg{replication_synchro_quorum=BROKEN_QUORUM, replication_synchro_timeout=0.1} > > box.error.injection.set('ERRINJ_SYNC_TIMEOUT', true) > > 12. Если тест использует errinj, он не должен запускаться в release сборке. > См release_disabled в suite.ini файлах. Да, я уже понял это и тест был поделен на две части, но в общую ветку это не добавлял. Все изменения по этому ревью будут уже в разделенных тестах. > > test_run:cmd("setopt delimiter ';'") > > _ = fiber.create(function() > > box.space.sync:insert{2} > > end); > > test_run:cmd("setopt delimiter ''"); > > -- Disable synchronous mode. > > disable_sync_mode() > > -- Space is in async mode now. > > box.space.sync:insert{3} -- async operation must wait sync one > > box.error.injection.set('ERRINJ_SYNC_TIMEOUT', false) > > box.space.sync:select{} -- 1 > > test_run:cmd('switch replica') > > box.space.sync:select{} -- 1 > > -- Testcase cleanup. > > test_run:switch('default') > > box.space.sync:drop() > > > > -- Warn user when setting `replication_synchro_quorum` to a value > > -- greater than number of instances in a cluster, see gh-5122. > > box.cfg{replication_synchro_quorum=BROKEN_QUORUM} -- warning > > 13. Этот тест походу вообще ничего не проверяет. Варнинг сейчас не пишется, > и тест проходит. Обычный процесс такой: если тест падает, то, пока есть открытая проблема, добавляют XFAIL и при изменении поведения XFAIL меняется на XPASS, чтобы убрать XFAIL. У нас нет такого механизма, поэтому добавил тест на будущее и когда варнинг добавят, то тест сломается и result файл обновят. Мне кажется, что это вполне себе ок. > > -- [RFC, summary] switch from leader to replica and vice versa, expected > > -- success and data consistency on a leader and replicas (gh-5124). > > 14. Вот это норм тест для advanced. Серега П. добавил API чтоб очищать лимб > при переключении. Надо его тоже протестировать, сейчас на это ни одного > теста нет. Обсудили, нужно сделать тест для патча https://github.com/tarantool/tarantool/commit/9b1ddeb69a4944c6e2701e5d6fbdddb586f94eb2#diff-25ed8a97a328d59f5830ec4f4e6e3ea3R94 Я сделаю. > > -- Testcase setup. > > test_run:switch('default') > > box.cfg{replication_synchro_quorum=NUM_INSTANCES, replication_synchro_timeout=0.1} > > _ = box.schema.space.create('sync', {is_sync=true, engine=engine}) > > _ = box.space.sync:create_index('pk') > > -- Testcase body. > > box.space.sync:insert{1} > > box.space.sync:select{} -- 1 > > test_run:switch('replica') > > box.space.sync:select{} -- 1 > > box.cfg{read_only=false} -- promote replica to master > > test_run:switch('default') > > box.cfg{read_only=true} -- demote master to replica > > test_run:switch('replica') > > box.space.sync:insert{2} > > box.space.sync:select{} -- 1, 2 > > test_run:switch('default') > > box.space.sync:select{} -- 1, 2 > > -- Revert cluster configuration. > > test_run:switch('default') > > box.cfg{read_only=false} > > test_run:switch('replica') > > box.cfg{read_only=true} > > -- Testcase cleanup. > > test_run:switch('default') > > box.space.sync:drop() > > > > -- Check behaviour with failed write to WAL on master (ERRINJ_WAL_IO). > > -- Testcase setup. > > test_run:switch('default') > > box.cfg{replication_synchro_quorum=NUM_INSTANCES, replication_synchro_timeout=0.1} > > _ = box.schema.space.create('sync', {is_sync=true, engine=engine}) > > _ = box.space.sync:create_index('pk') > > -- Testcase body. > > box.space.sync:insert{1} > > box.space.sync:select{} -- 1 > > box.error.injection.set('ERRINJ_WAL_IO', true) > > box.space.sync:insert{2} > > box.error.injection.set('ERRINJ_WAL_IO', false) > > box.space.sync:select{} -- 1 > > test_run:switch('replica') > > box.space.sync:select{} -- 1 > > -- Testcase cleanup. > > test_run:switch('default') > > box.space.sync:drop() > > > > -- [RFC, quorum commit] check behaviour with failure answer from a replica > > -- (ERRINJ_WAL_SYNC) during write, expected disconnect from the replication > > 15. Здесь не используется ERRINJ_WAL_SYNC. Неконсистентность комментария и теста, убрал название ERRINJ из комментария. > > -- (gh-5123, set replication_synchro_quorum to 1). > > -- Testcase setup. > > test_run:switch('default') > > box.cfg{replication_synchro_quorum=2, replication_synchro_timeout=0.1} > > _ = box.schema.space.create('sync', {is_sync=true, engine=engine}) > > _ = box.space.sync:create_index('pk') > > -- Testcase body. > > box.space.sync:insert{1} > > box.space.sync:select{} -- 1 > > test_run:switch('replica') > > box.error.injection.set('ERRINJ_WAL_IO', true) > > test_run:switch('default') > > box.space.sync:insert{2} > > test_run:switch('replica') > > box.error.injection.set('ERRINJ_WAL_IO', false) > > box.space.sync:select{} -- 1 > > -- Testcase cleanup. > > test_run:switch('default') > > box.space.sync:drop() > > > > -- Teardown. > > test_run:cmd('switch default') > > test_run:cmd('stop server replica') > > test_run:cmd('delete server replica') > > test_run:cleanup_cluster() > > box.schema.user.revoke('guest', 'replication') > > box.cfg{ \ > > replication_synchro_quorum = orig_synchro_quorum, \ > > replication_synchro_timeout = orig_synchro_timeout, \ > > } > > > > -- Setup an async cluster. > > box.schema.user.grant('guest', 'replication') > > test_run:cmd('create server replica with rpl_master=default,\ > > script="replication/replica.lua"') > > test_run:cmd('start server replica with wait=True, wait_load=True') > > 16. Зачем все пересоздавать? по-моему когда конверсию async -> sync хотел тестировать, то решил тестовое окружение заново сделать. сейчас в этом нет необходимости. > > -- [RFC, summary] switch async replica into sync one, expected > > -- success and data consistency on a leader and replica. > > -- Testcase setup. > > _ = box.schema.space.create('sync', {engine=engine}) > > _ = box.space.sync:create_index('pk') > > box.space.sync:insert{1} -- success > > test_run:cmd('switch replica') > > box.space.sync:select{} -- 1 > > test_run:switch('default') > > -- Enable synchronous mode. > > disable_sync_mode() > > -- Space is in sync mode now. > > box.cfg{replication_synchro_quorum=NUM_INSTANCES, replication_synchro_timeout=0.1} > > box.space.sync:insert{2} -- success > > box.cfg{replication_synchro_quorum=BROKEN_QUORUM, replication_synchro_timeout=0.1} > > box.space.sync:insert{3} -- success > > box.space.sync:select{} -- 1, 2, 3 > > test_run:cmd('switch replica') > > box.space.sync:select{} -- 1, 2, 3 > > -- Testcase cleanup. > > test_run:switch('default') > > box.space.sync:drop() > > > > -- Teardown. > > test_run:cmd('switch default') > > test_run:cmd('stop server replica') > > test_run:cmd('delete server replica') > > test_run:cleanup_cluster() > > box.schema.user.revoke('guest', 'replication') > > box.cfg{ \ > > replication_synchro_quorum = orig_synchro_quorum, \ > > replication_synchro_timeout = orig_synchro_timeout, \ > > } -- sergeyb@