[TNTP-2109] Switch to safe mode on vshard rebalance #467

ita-sammann · 2025-12-05T10:59:56Z

Implement "safe" mode to prevent writing data to wrong replicaset when vshard rebalance is in progress.
Auto switch to safe mode when rebalance process starts.
Manual return to fast mode.

I didn't forget about

Tests
Changelog
Documentation

crud/common/sharding/bucket_ref_unref.lua

crud/common/call.lua

crud/common/rebalance.lua

Satbek · 2025-12-08T07:52:44Z

test/integration/double_buckets_test.lua

+pgroup_duplicates.test_duplicates = function(g)
+    t.skip_if(
+        not (
+            utils.tarantool_version_at_least(3, 1) and (g.params.backend == "config")


todo: implement test for other backend types. 2.11 and cartridge

test/integration/double_buckets_test.lua

test/helper.lua

vakhov

Thanks for the PR! I’ve gone through the changes and left a few small questions.

crud/common/call.lua

crud/common/rebalance.lua

crud/common/sharding/bucket_ref_unref.lua

Serpentian · 2025-12-11T09:52:22Z

crud.lua

 local utils = require('crud.common.utils')
 local stats = require('crud.stats')
 local readview = require('crud.readview')
 local schema = require('crud.schema')


Guys, the commits are absolutely chaotic, this is not the way, we develop open-source modules. We should rebase that branch in master to make the commit history linear, and not just merge it. So, all commits like Minor changes according to review comments will be visible to our users. You can check out, how Georgy Moiseev did it: proper commits, proper commit messages, tests in every commit (e.g. 8d7cae0).

Now, I'm forced to review more than 1k lines in one step, which is very inconvenient and increases the chanse of missed bugs. And our users won't be able to check the individual commits, if they want to. Of course, it's up to you, since I'm not the maintainer of that module, but it's just not nice to develop and merge code like that.

IMHO, the features should be properly split between commits, every commit must include the associated tests, proper commit messages and mentioning of the #448 ticket. Of course, refactoring or code moving should be in the separate commits. Between commits all of the test must pass

Guys, the commits are absolutely chaotic

Of course these commits will not go to master, I will re-split them before merging the PR.

Now, I'm forced to review more than 1k lines in one step

My bad. Never thought of this PR as of multiple features that can be reviewed separately.

I've re-split PR into 3 commits.

Serpentian · 2025-12-11T09:56:38Z

crud.lua

+
+-- @refer rebalance.router_cache_length
+-- @function router_cache_length
+crud.rebalance.router_cache_length = rebalance.router.cache_length


Why do we export router_cache_length and router_cache_last_clear_ts to public API? I understand that for router_cache_clear, which is supposed to be called by the TCM/cartridge/ATE, but I don't understand, why getters are needed

I think they will be useful in TCM/Cartridge to indicate that cache has actually been cleared and when did it happen.

Serpentian · 2025-12-11T10:08:06Z

crud.lua

 function crud.init_router()
-   rawset(_G, 'crud', crud)
+    rawset(_G, 'crud', crud)
+    rebalance.metrics.enable_router_metrics()


Looks like a crutch. Will it be disabled, when user disables metrics? It seems the whole metrics part should be done in the metrics_registry

Metrics registry is a part of stats module, it provides statistics for crud requests, has a performance impact and can be enabled/disabled by user. I think these metrics should be independent from stats. I've simplified them and got rid of these enable_* functions.

Serpentian · 2025-12-11T10:09:30Z

crud/common/rebalance.lua

+    safe_mode = false,
+    safe_mode_enable_hooks = {},
+    safe_mode_disable_hooks = {},
+    _router_cache_last_clear_ts = fiber.time()


Why do we initialize the timestamp with current time on module load? It may lead to bugs, we didn't really cleared the router`s cache on module load

You are right, thanks! Changed initial value to zero.

Serpentian · 2025-12-11T10:14:34Z

crud/common/rebalance.lua

+    end)
+end
+
+M.init = rebalance_init


Nit: let's make the code consistent with all other modules, it seems we tend to make the variable with proper name and assign methods to it .

crud/crud/stats/local_registry.lua

Line 39 in c52a6f5

function registry.init(opts)

Changed M to a proper name.

Serpentian · 2025-12-11T14:19:39Z

crud/common/sharding/bucket_ref_unref.lua

+        if not ref_ok then
+
+            table.insert(bucket_ref_errs, bucket_refrw_err.bucket_ref_errs[1])
+            goto continue


Why do we continue on error and then just unref all of them? We should go to unref stright away

Thanks for noticing, it has to be brake, not continue. Fixed.

Serpentian · 2025-12-11T14:22:01Z

crud/common/sharding/bucket_ref_unref.lua

+        set_fast_mode()
+    end
+
+    if not hooks_registered then


You won't need it, if internal.trigger will be used

Serpentian · 2025-12-11T14:23:33Z

crud/common/sharding/bucket_ref_unref.lua

+    end
+end
+
+set_fast_mode()


Hmm, and what if rebalance module is currently in the safe mode, then the trigger won't work and the bucket_ref_unref will remain in the fast. Or am I missing smth?

You are right. Simplified this part, removed unnecessary init function, setting inital functions based on rebalance.safe_mode value.

Serpentian · 2025-12-11T14:26:11Z

crud/common/call.lua

+            end
+        end
+
+        if redirect_replicaset ~= nil then


Nit: why can't we just change the replicaset variable, seems redirect_replicaset is not needed here

Thanks, removed this extra var.

Serpentian · 2025-12-11T14:29:17Z

crud/insert.lua

-        noreturn = opts.noreturn,
-        fetch_latest_metadata = opts.fetch_latest_metadata,
-    })
+    local function make_insert()


Can't we just make function wrappers for single bucket and multiple ones? Why do we always repeat the same code

Got rid of these local functions completely in all crud methods.

`crud.common.rebalance` module introduces on_safe_mode_toggle trigger that can be used to change the implementation of crud methods to work in safe mode during rebalance and in fast mode at other times. This mechanism automatically enables safe mode when vshard rebalance starts, but safe mode can only be disabled manually. Co-authored-by: Satbek Turganbayev <[email protected]>

For the most of crud methods, when safe mode is enabled, bucket_ref() for appropriate bucket is called before calling the method itself. This prevents from writing tuples to the wrong replicaset during or after vshard rebalance. Closes #448. Co-authored-by: Satbek Turganbayev <[email protected]>

ita-sammann mentioned this pull request Dec 5, 2025

TNTP-2109: call bucket_ref/bucket_unref on crud operations #463

Closed

3 tasks

ita-sammann requested a review from Satbek December 5, 2025 11:31

ita-sammann marked this pull request as ready for review December 5, 2025 13:38

ita-sammann requested review from DifferentialOrange, a1div0, p0rtale and vakhov December 5, 2025 13:38

Satbek requested changes Dec 8, 2025

View reviewed changes

vakhov reviewed Dec 8, 2025

View reviewed changes

crud/common/call.lua Outdated Show resolved Hide resolved

crud/common/rebalance.lua Outdated Show resolved Hide resolved

crud/common/sharding/bucket_ref_unref.lua Outdated Show resolved Hide resolved

crud/common/sharding/bucket_ref_unref.lua Show resolved Hide resolved

ita-sammann force-pushed the TNTP-2109-rebalance-safe-mode branch from 4a8e4d2 to bb11b86 Compare December 9, 2025 11:28

ita-sammann requested review from Satbek, Serpentian and vakhov December 10, 2025 08:19

vakhov approved these changes Dec 10, 2025

View reviewed changes

ita-sammann self-assigned this Dec 10, 2025

ita-sammann added the do not merge Not ready to be merged label Dec 10, 2025

Satbek approved these changes Dec 10, 2025

View reviewed changes

Serpentian suggested changes Dec 11, 2025

View reviewed changes

ita-sammann force-pushed the TNTP-2109-rebalance-safe-mode branch 4 times, most recently from d25a4ff to d25ad97 Compare December 22, 2025 18:33

ita-sammann requested a review from Serpentian December 22, 2025 19:08

ita-sammann and others added 3 commits December 23, 2025 20:47

test: add ?/init.lua to package.path in cartridge test entrypoints

57ebda0

ita-sammann force-pushed the TNTP-2109-rebalance-safe-mode branch from d25ad97 to 30c22b1 Compare December 23, 2025 17:48

[TNTP-2109] Switch to safe mode on vshard rebalance #467

Are you sure you want to change the base?

[TNTP-2109] Switch to safe mode on vshard rebalance #467

Uh oh!

Conversation

ita-sammann commented Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vakhov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ita-sammann Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ita-sammann Dec 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ita-sammann Dec 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ita-sammann commented Dec 5, 2025 •

edited

Loading

ita-sammann Dec 12, 2025 •

edited

Loading

ita-sammann Dec 22, 2025 •

edited

Loading

ita-sammann Dec 22, 2025 •

edited

Loading