osd: Roll back unstable writes of removed pools in the old store #106

Open
koolkhel wants to merge 1 commits from koolkhel/vitastor:removed-pools-v1 into master
Contributor

Description

Nothing stabilizes or rolls back unstable writes of a removed pool, its PGs are never peered again. The old store can't trim its journal past them, so the journal fills up and writes to all other pools on the same OSDs fail with -EAGAIN forever, restarts don't help.

Add a synchronous get_unstable_objects() which walks dirty_db of the old store, and roll back unstable writes of removed pools with it. The OSD only picks up a pool when it disappears from the pool configuration, or on start when the store has unstable writes of a pool that doesn't exist, and lists the store once the pool's PGs are removed and stopped. In normal operation it does nothing: PG changes only check that the set of removed pools is empty. The new store keeps such writes in its metadata without harm, so the OSD does nothing for it (blockstore_i::is_new()).

Tested:

  • test_blockstore_v1: new test_get_unstable_objects
  • osd_test: new test_rollback_removed_pool_on_start, test_rollback_removed_pool_online, test_rollback_removed_pool_idle (ordinary pool and PG changes don't list the store, the new store is never listed)
  • new tests/test_rm_pool_unstable.sh (old store): removes a pool with unstable writes online and while OSDs are down, then writes 4x the journal size to another pool. Without the patch it fails with "Pool removed online: writes to another pool got stuck"; with it every OSD logs "Rolled back 8 unstable object versions of removed pools" in both phases.

Not covered: an OSD whose journal is already completely full can't be healed, the rollback itself needs a journal sector.

Contributor Name

Yury Luneff (MIND Software LLC, https://mindsw.io)

CLA

### Description Nothing stabilizes or rolls back unstable writes of a removed pool, its PGs are never peered again. The old store can't trim its journal past them, so the journal fills up and writes to all other pools on the same OSDs fail with -EAGAIN forever, restarts don't help. Add a synchronous `get_unstable_objects()` which walks `dirty_db` of the old store, and roll back unstable writes of removed pools with it. The OSD only picks up a pool when it disappears from the pool configuration, or on start when the store has unstable writes of a pool that doesn't exist, and lists the store once the pool's PGs are removed and stopped. In normal operation it does nothing: PG changes only check that the set of removed pools is empty. The new store keeps such writes in its metadata without harm, so the OSD does nothing for it (`blockstore_i::is_new()`). Tested: - `test_blockstore_v1`: new `test_get_unstable_objects` - `osd_test`: new `test_rollback_removed_pool_on_start`, `test_rollback_removed_pool_online`, `test_rollback_removed_pool_idle` (ordinary pool and PG changes don't list the store, the new store is never listed) - new `tests/test_rm_pool_unstable.sh` (old store): removes a pool with unstable writes online and while OSDs are down, then writes 4x the journal size to another pool. Without the patch it fails with "Pool removed online: writes to another pool got stuck"; with it every OSD logs "Rolled back 8 unstable object versions of removed pools" in both phases. Not covered: an OSD whose journal is already completely full can't be healed, the rollback itself needs a journal sector. ### Contributor Name Yury Luneff (MIND Software LLC, https://mindsw.io) ### CLA - [x] I accept Vitastor CLA agreement: https://git.yourcmc.ru/vitalif/vitastor/src/branch/master/CLA-en.md
koolkhel added 1 commit 2026-10-26 20:54:42 +03:00
Nothing stabilizes or rolls back unstable writes of a removed pool, its
PGs are never peered again. The old store can't trim its journal past
them, so the journal fills up and writes to all other pools on the same
OSDs fail with -EAGAIN forever, restarts don't help.

Add a synchronous get_unstable_objects() which walks dirty_db of the old
store, and roll back unstable writes of objects outside of existing pools
with it: on start, and when a pool and its PG configuration are removed
and local PGs of the pool have stopped. The new store keeps such writes
in its metadata without harm, so it returns nothing.
vitalif requested changes 2026-10-27 00:28:12 +03:00
@@ -505,6 +505,10 @@ void osd_t::on_change_etcd_state_hook(std::map<std::string, etcd_kv_t> & changes
}
apply_pg_config();
}
if (pools || changes.find(st_cli->etcd_prefix+"/pg/config") != changes.end())
Owner

тут два раза чтоб не делать pgs =, можно выше pgs = 1 раз присвоить а тут только проверять

тут два раза чтоб не делать pgs =, можно выше pgs = 1 раз присвоить а тут только проверять
@@ -728,6 +732,7 @@ void osd_t::on_load_pgs_hook(bool success)
apply_pg_count();
apply_pg_config();
}
rollback_removed_pools();
Owner

тут делается rollback_(), а выше только check_(). как будто не совсем сбалансированно, надо по идее одно и то же триггерить, ну либо в одном месте где-то триггерить

тут делается rollback_(), а выше только check_(). как будто не совсем сбалансированно, надо по идее одно и то же триггерить, ну либо в одном месте где-то триггерить
@@ -476,0 +482,4 @@
{
for (auto & pc: st_cli->pool_config)
{
if (pc.second.exists)
Owner

а я чот не понял, т.е. это не срабатывает, если есть вообще хотя бы 1 живой пул? так это бессмысленно, нужно-то чтобы срабатывало для удалённых

а я чот не понял, т.е. это не срабатывает, если есть вообще хотя бы 1 живой пул? так это бессмысленно, нужно-то чтобы срабатывало для удалённых
@@ -476,0 +486,4 @@
{
continue;
}
// Wait until PGs are removed too, so that their primaries stop sending writes
Owner

а тут можно сделать как-то без перебора всех PG, в конце концов этот check на каждом изменении конфигурации пулов или PG гоняется. просто как-нибудь находить одну, если есть то выходить

а тут можно сделать как-то без перебора всех PG, в конце концов этот check на каждом изменении конфигурации пулов или PG гоняется. просто как-нибудь находить одну, если есть то выходить
@@ -476,0 +506,4 @@
void osd_t::rollback_removed_pools()
{
if (!bs)
Owner

вот тут хочется, чтобы этот rollback вообще не вызывался даже, если стор новый. можно хоть даже флаг какой-то добавить, типа if !bs || !bs->is_new() -> не вызывать.

причём делать это не даже не тут на rollback(), а прям на check(), если он будет основной точкой входа.

вот тут хочется, чтобы этот rollback вообще не вызывался даже, если стор новый. можно хоть даже флаг какой-то добавить, типа if !bs || !bs->is_new() -> не вызывать. причём делать это не даже не тут на rollback(), а прям на check(), если он будет основной точкой входа.
@@ -476,0 +515,4 @@
{
return;
}
for (auto & pp: pgs)
Owner

аналогично, перебирать конфигурации всех активных PG - это прям стрём, нужно как-то лёгенько по какому-нибудь std::set ID пулов (имеющих unstable версии, вероятно) проходиться, находить если любая 1 pg есть в pgs, то не трогать его пока, как-то так.

короче надо так, чтобы: при обычной работе оно вообще ничего не делало и проверка была O(1), делать начинало только а) при старте б) при изменении конфигурации пулов, когда из неё пропадает пул, а в остальное время не смотрело на изменения PG. ну и да как выше уже написал - чтоб это вообще было только для старого стора. а то стрёмно мне чтобы это не полезло роллбэкать что-то когда не надо, хочется максимально ограничить кейсы.

причём теоретически в момент когда пропадает пул, если уж мы пытаемся красиво обработать кейс, когда злобный админ грохает пул не удалив данные и не остановив клиентов (а я так понимаю это как раз кейс который ты хочешь обработать в этом ПР...), unstable-ов может быть больше, а когда уже реально пропадут PG и пулы и мы реально пойдём всё уже удалять, их будет меньше, т.е. список-то объектов перезапрашивать конечно

аналогично, перебирать конфигурации всех активных PG - это прям стрём, нужно как-то лёгенько по какому-нибудь std::set ID пулов (имеющих unstable версии, вероятно) проходиться, находить если любая 1 pg есть в pgs, то не трогать его пока, как-то так. короче надо так, чтобы: при обычной работе оно вообще ничего не делало и проверка была O(1), делать начинало только а) при старте б) при изменении конфигурации пулов, когда из неё пропадает пул, а в остальное время не смотрело на изменения PG. ну и да как выше уже написал - чтоб это вообще было только для старого стора. а то стрёмно мне чтобы это не полезло роллбэкать что-то когда не надо, хочется максимально ограничить кейсы. причём теоретически в момент когда пропадает пул, если уж мы пытаемся красиво обработать кейс, когда злобный админ грохает пул не удалив данные и не остановив клиентов (а я так понимаю это как раз кейс который ты хочешь обработать в этом ПР...), unstable-ов может быть больше, а когда уже реально пропадут PG и пулы и мы реально пойдём всё уже удалять, их будет меньше, т.е. список-то объектов перезапрашивать конечно
koolkhel force-pushed removed-pools-v1 from 5ddaaaefc9 to c216e4929f 2026-10-28 12:25:50 +03:00 Compare
Author
Contributor

Переделал, запушил в ту же ветку.

  • Одна точка входа, только для старого стора. check_removed_pools() вызывается при загрузке PG (старт или перезагрузка из etcd) и при изменении /config/pools. Первой строкой там if (!bs || bs->is_new()) return;, is_new() добавил в blockstore_i (true в новом сторе). Для нового стора не вызывается вообще ничего, в том числе get_unstable_objects().
  • В обычной работе ничего не делает. Есть std::set<pool_id_t> rm_pools: удалённые пулы, по которым ещё надо откатить. Обычно он пустой, и изменения pg/config и остановка PG проверяют только его: rm_pools.size() в хуке и rm_pools.find(pg.pool_id) в finish_stop_pg. Перебора всех PG больше нет.
  • Когда пул пропадает. При изменении /config/pools прохожу pool_config (пулы, не PG) и сравниваю с набором пулов, которые были в конфиге раньше. Тот, который был и пропал, попадает в rm_pools. На старте в rm_pools попадают несуществующие пулы из списка unstable-объектов стора: пул могли удалить, пока OSD лежал.
  • Ждём, пока реально пропадут PG. Для пула из rm_pools смотрю только его PG: pgs.lower_bound({ pool_id, 0 }) до первой не-OFFLINE и его pg_config до первой с config_exists. Если что-то есть, пул остаётся в rm_pools до следующего изменения pg/config или остановки его PG.
  • Список перезапрашивается. Стор листится только в момент отката, когда PG пула уже удалены из конфига и остановлены. При самом удалении пула не листится ничего, так что unstable-версии, которые к тому времени успели стабилизироваться, в откат не попадают.
  • pgs в хуке теперь вычисляется один раз.
  • Если пул с тем же ID создали заново до отката, он убирается из rm_pools без отката.

Про osd_flush.cpp:485: там был continue для существующих пулов, а не выход, то есть срабатывало для любого удалённого пула, у которого уже не было PG. Но этого кода теперь нет.

Тесты: osd_test (test_rollback_removed_pool_on_start, _online, и новый _idle: при обычных изменениях пулов и PG стор не листится, а для нового стора не листится вообще), test_blockstore_v1, OLD=1 tests/test_rm_pool_unstable.sh (все OSD пишут "Rolled back 8 …" в обеих фазах). Ветка перебазирована на текущий master.

Переделал, запушил в ту же ветку. - **Одна точка входа, только для старого стора.** `check_removed_pools()` вызывается при загрузке PG (старт или перезагрузка из etcd) и при изменении `/config/pools`. Первой строкой там `if (!bs || bs->is_new()) return;`, `is_new()` добавил в `blockstore_i` (true в новом сторе). Для нового стора не вызывается вообще ничего, в том числе `get_unstable_objects()`. - **В обычной работе ничего не делает.** Есть `std::set<pool_id_t> rm_pools`: удалённые пулы, по которым ещё надо откатить. Обычно он пустой, и изменения `pg/config` и остановка PG проверяют только его: `rm_pools.size()` в хуке и `rm_pools.find(pg.pool_id)` в `finish_stop_pg`. Перебора всех PG больше нет. - **Когда пул пропадает.** При изменении `/config/pools` прохожу `pool_config` (пулы, не PG) и сравниваю с набором пулов, которые были в конфиге раньше. Тот, который был и пропал, попадает в `rm_pools`. На старте в `rm_pools` попадают несуществующие пулы из списка unstable-объектов стора: пул могли удалить, пока OSD лежал. - **Ждём, пока реально пропадут PG.** Для пула из `rm_pools` смотрю только его PG: `pgs.lower_bound({ pool_id, 0 })` до первой не-OFFLINE и его `pg_config` до первой с `config_exists`. Если что-то есть, пул остаётся в `rm_pools` до следующего изменения `pg/config` или остановки его PG. - **Список перезапрашивается.** Стор листится только в момент отката, когда PG пула уже удалены из конфига и остановлены. При самом удалении пула не листится ничего, так что unstable-версии, которые к тому времени успели стабилизироваться, в откат не попадают. - `pgs` в хуке теперь вычисляется один раз. - Если пул с тем же ID создали заново до отката, он убирается из `rm_pools` без отката. Про `osd_flush.cpp:485`: там был `continue` для существующих пулов, а не выход, то есть срабатывало для любого удалённого пула, у которого уже не было PG. Но этого кода теперь нет. Тесты: `osd_test` (`test_rollback_removed_pool_on_start`, `_online`, и новый `_idle`: при обычных изменениях пулов и PG стор не листится, а для нового стора не листится вообще), `test_blockstore_v1`, `OLD=1 tests/test_rm_pool_unstable.sh` (все OSD пишут "Rolled back 8 …" в обеих фазах). Ветка перебазирована на текущий master.
Owner

Короче всё равно что-то меня в этом подходе смущает...

А именно, смущает то, что этой логикой мы нарушаем правило, что все модификации вносит primary. Тут свой стор начинает мутировать сам OSD. Если мы почему-то прочитаем некорректную конфигурацию пулов - мы начнём удалять (и возможно терять) какие-то данные, т.к. удалять unstable просто так нельзя: если хотя бы на одном OSD из набора версия stable, на других она точно должна быть в наличии, но может ещё быть unstable, и если этот unstable удалить, она станет incomplete. Если во время уже начатой чистки unstable вернётся primary, удаление unstable в теории может произойти даже после пиринга, и привести к спецэффектам этого самого пиринга. Ещё есть pg_locks кстати, которые рассчитаны на то, что только залочивший первичный OSD модифицирует стор вторичного. И это по идее штука, которая в будущем должна когда-то стать основным режимом работы. С этим бы тоже как-то увязать чистку...

При этом сама логика-то в общем действительно не совсем бесполезная, хоть и лечит довольно странные кейсы:

  1. Админ - дурак, взял и удалил EC пул, не удалив из него данные и даже не остановив клиентов. Но админ-то может и дурак, а такое удаление действительно сейчас легко может привести к залипанию работы вполне себе живых OSD с другими пулами, если у них старый стор.

  2. OSD умер, пул продолжил ехать на старых OSD, потом пул удалили, и потом старый OSD вернули в строй. В этом случае старый OSD тоже может залипнуть, если на нём болтаются unstable. Но это только кейс первичной чистки при запуске OSD.

  3. Умерло больше, чем разрешено, OSD определённого EC пула и пул пришёл в негодность, весь ушёл в INCOMPLETE, грязные записи не почистились и все OSD залипли. Мы хотим забить на этот пул, удалить его, но чтобы дальше всё работало. Однако тут надо понимать, что наверняка мы в этом случае ещё захотим почистить данные пула, то есть тогда ещё в идеале нужно учиться чистить их. Ну только опять же не как в том патче с GC, не со стороны самого OSD, ещё и удалять данные из своего стора - это вообще стрём... :-)

  4. Как и в прошлом варианте, пул пришёл в INCOMPLETE, но мы на него не забили и ещё надеемся, что его OSD вернутся. В этом случае также всё будет плохо даже с доработкой, потому что пул-то не удалён, а грязные записи точно так же на месте и не почистятся.

  5. Ну и ещё есть новый стор, где всё это как раз и не является проблемой вообще.

Всё это наводит на некие мысли, что лечим "не там" и всё это немного костыли.

В общем скажи, тебе конкретно ради каких кейсов эта доработка нужна? Можно ещё замержить под флагом (настройкой), как вариант. А по дефолту я её включать не буду. :-)

Короче всё равно что-то меня в этом подходе смущает... А именно, смущает то, что этой логикой мы нарушаем правило, что все модификации вносит primary. Тут свой стор начинает мутировать сам OSD. Если мы почему-то прочитаем некорректную конфигурацию пулов - мы начнём удалять (и возможно терять) какие-то данные, т.к. удалять unstable просто так нельзя: если хотя бы на одном OSD из набора версия stable, на других она точно должна быть в наличии, но может ещё быть unstable, и если этот unstable удалить, она станет incomplete. Если во время уже начатой чистки unstable вернётся primary, удаление unstable в теории может произойти даже после пиринга, и привести к спецэффектам этого самого пиринга. Ещё есть pg_locks кстати, которые рассчитаны на то, что только залочивший первичный OSD модифицирует стор вторичного. И это по идее штука, которая в будущем должна когда-то стать основным режимом работы. С этим бы тоже как-то увязать чистку... При этом сама логика-то в общем действительно не совсем бесполезная, хоть и лечит довольно странные кейсы: 1) Админ - дурак, взял и удалил EC пул, не удалив из него данные и даже не остановив клиентов. Но админ-то может и дурак, а такое удаление действительно сейчас легко может привести к залипанию работы вполне себе живых OSD с _другими_ пулами, если у них старый стор. 2) OSD умер, пул продолжил ехать на старых OSD, потом пул удалили, и потом старый OSD вернули в строй. В этом случае старый OSD тоже может залипнуть, если на нём болтаются unstable. Но это только кейс первичной чистки при запуске OSD. 3) Умерло больше, чем разрешено, OSD определённого EC пула и пул пришёл в негодность, весь ушёл в INCOMPLETE, грязные записи не почистились и все OSD залипли. Мы хотим забить на этот пул, удалить его, но чтобы дальше всё работало. Однако тут надо понимать, что наверняка мы в этом случае ещё захотим почистить данные пула, то есть тогда ещё в идеале нужно учиться чистить их. Ну только опять же не как в том патче с GC, не со стороны самого OSD, ещё и удалять данные из своего стора - это вообще стрём... :-) 4) Как и в прошлом варианте, пул пришёл в INCOMPLETE, но мы на него не забили и ещё надеемся, что его OSD вернутся. В этом случае также всё будет плохо даже с доработкой, потому что пул-то не удалён, а грязные записи точно так же на месте и не почистятся. 5) Ну и ещё есть новый стор, где всё это как раз и не является проблемой вообще. Всё это наводит на некие мысли, что лечим "не там" и всё это немного костыли. В общем скажи, тебе конкретно ради каких кейсов эта доработка нужна? Можно ещё замержить под флагом (настройкой), как вариант. А по дефолту я её включать не буду. :-)
Author
Contributor

Закрываю в пользу #115: там чинится не последствие, а то, как мы в это попадаем.

Причина: при паузе PG перед удалением пула stop_pg() → drop_dirty_pg_connections() просто забывает unstable-записи PG, рассчитывая на пиринг следующего primary, а удалённый пул никто больше не пирит. В #115 primary такой PG (пауза + пула уже нет в /config/pools) перед остановкой, пока PG ещё active и залочена, отказывает новым записям, дожидается летящих, делает обычный sync со stabilize и только потом останавливает PG. Свой стор OSD сам не трогает, паузы при смене pg_count работают как раньше.

Закрываю в пользу https://git.yourcmc.ru/vitalif/vitastor/pulls/115: там чинится не последствие, а то, как мы в это попадаем. Причина: при паузе PG перед удалением пула `stop_pg()` → `drop_dirty_pg_connections()` просто забывает unstable-записи PG, рассчитывая на пиринг следующего primary, а удалённый пул никто больше не пирит. В #115 primary такой PG (пауза + пула уже нет в `/config/pools`) перед остановкой, пока PG ещё active и залочена, отказывает новым записям, дожидается летящих, делает обычный sync со stabilize и только потом останавливает PG. Свой стор OSD сам не трогает, паузы при смене pg_count работают как раньше.
koolkhel closed this pull request 2026-11-02 17:13:58 +03:00
vitalif reopened this pull request 2026-11-05 18:46:31 +03:00
This pull request can be merged automatically.
This branch is out-of-date with the base branch
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u removed-pools-v1:koolkhel-removed-pools-v1
git checkout koolkhel-removed-pools-v1
Sign in to join this conversation.
No Reviewers
No Label
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: vitalif/vitastor#106
No description provided.