Re: [Tarantool-patches] [PATCH 2/4] replication: add advanced tests for sync replication

[Sergey Bronnikov <sergeyb@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@