Import-mode: Avoid expiration and eviction during data syncing #1185

lyq2333 · 2024-10-17T11:10:31Z

New config: import-mode (yes|no)

New command: CLIENT IMPORT-SOURCE (ON|OFF)

The config, when set to yes, disables eviction and deletion of expired keys, except for commands coming from a client which has marked itself as an import-source, the data source when importing data from another node, using the CLIENT IMPORT-SOURCE command.

When we sync data from the source Valkey to the destination Valkey using some sync tools like redis-shake, the destination Valkey can perform expiration and eviction, which may cause data corruption. This problem has been discussed in redis/redis#9760 (reply in thread) and Redis already have a solution. But in Valkey we haven't fixed it by now.

E.g. we call set key 1 ex 1 on the source server and transfer this command to the destination server. Then we call incr key on the source server before the key expired, we will have a key on the source server with a value of 2. But when the command arrived at the destination server, the key may be expired and has deleted. So we will have a key on the destination server with a value of 1, which is inconsistent with the source server.

In standalone mode, we can use writable replica to simplify the sync process. However, in cluster mode, we still need a sync tool to help us transfer the source data to the destination. The sync tool usually work as a normal client and the destination works as a primary which keep expiration and eviction.

In this PR, we add a new mode named 'import-mode'. In this mode, server stop expiration and eviction just like a replica. Notice that this mode exists only in sync state to avoid data inconsistency caused by expiration and eviction. Import mode only takes effect on the primary. Sync tools can mark their clients as an import source by CLIENT IMPORT-SOURCE, which work like a client from primary and can visit expired keys in lookupkey.

Notice: during the migration, other clients, apart from the import source, should not access the data imported by import source.

codecov · 2024-10-17T11:27:15Z

Codecov Report

Attention: Patch coverage is 89.47368% with 2 lines in your changes missing coverage. Please review.

Project coverage is 70.57%. Comparing base (a62d1f1) to head (7f04964).
Report is 62 commits behind head on unstable.

Files with missing lines	Patch %	Lines
src/networking.c	81.81%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #1185      +/-   ##
============================================
- Coverage     70.65%   70.57%   -0.09%     
============================================
  Files           114      115       +1     
  Lines         61799    63171    +1372     
============================================
+ Hits          43664    44582     +918     
- Misses        18135    18589     +454

Files with missing lines	Coverage Δ
src/commands.def	`100.00% <ø> (ø)`
src/config.c	`78.83% <ø> (+0.13%)`	⬆️
src/db.c	`89.27% <100.00%> (+0.75%)`	⬆️
src/evict.c	`97.75% <100.00%> (+0.02%)`	⬆️
src/expire.c	`96.57% <100.00%> (+0.14%)`	⬆️
src/server.c	`87.63% <100.00%> (-1.01%)`	⬇️
src/server.h	`100.00% <ø> (ø)`
src/networking.c	`88.50% <81.81%> (+<0.01%)`	⬆️

... and 87 files with indirect coverage changes

---- 🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests
JS Bundle Analysis - Avoid shipping oversized bundles

soloestoy · 2024-10-17T11:33:41Z

good work! @valkey-io/core-team please take a look

zuiderkwast

I like it. I think it's a little bit strange, but with some better documentation it's not too strange.

We need to have good support for migration tools, especially for users who want to migrate from proprietary software to open source software. 😄

Redis already have a solution.

I remember this discussion from the Redis times. Do you know what Redis solution is (the same REPLCONF PSEUDO-MASTER?) or is it secret?

Any better idea?

Have you considered the idea to let RediShake act as a primary and let the target database replicate from RediShake? It can act as a replication proxy?

+-----------+   PSYNC  +-----------+   PSYNC  +--------+
| Source DB |<---------| RediShake |<---------| Valkey |
+-----------+          +-----------+          +--------+

src/replication.c

valkey.conf

soloestoy · 2024-10-18T02:13:06Z

+-----------+   PSYNC  +-----------+   PSYNC  +--------+
| Source DB |<---------| RediShake |<---------| Valkey |
+-----------+          +-----------+          +--------+

interesting, and we have also considered such a method, but there are several issues.

First, as a cloud provider, we do not allow an instance to become a replica for external instances. This is a very risky operation, especially since the primary connection uses a super user, which has excessive permissions. Additionally, the replica needs to establish an outbound connection, this is also not allowed. These restrictions, I believe, are not unique to cloud providers, many users' security control policies also prohibit such actions.

Another point is that, in a cluster mode, the source and target instances for migration typically have different slot distributions, and redisShake can help with correctly routing the data.

soloestoy

Moreover, I think we should only allow write commands for pseudo-master client when server is in pseudo-replica mode.

src/evict.c

src/replication.c

lyq2333 · 2024-10-18T02:40:28Z

Redis already have a solution.

I remember this discussion from the Redis times. Do you know what Redis solution is (the same REPLCONF PSEUDO-MASTER?) or is it secret?

It's secret. I used to push this PR to Redis Community(#13077) and they say they will PR their implementation. But no news after that.

Any better idea?

Have you considered the idea to let RediShake act as a primary and let the target database replicate from RediShake? It can act as a replication proxy?
+-----------+   PSYNC  +-----------+   PSYNC  +--------+
| Source DB |<---------| RediShake |<---------| Valkey |
+-----------+          +-----------+          +--------+

What @soloestoy said is a major limitation for us. BTW, sometimes the destination already has some data, PSYNC will delete them.

enjoy-binbin · 2024-10-18T03:56:48Z

i did not read it carefully, internally we simply pause the expiration on both side i guess.
Redis has an issue that seems to be discussing this: redis/redis#13478

lyq2333 · 2024-10-18T08:15:58Z

i did not read it carefully, internally we simply pause the expiration on both side i guess. Redis has an issue that seems to be discussing this: redis/redis#13478

Interesting. I think this PR can solve that problem if the server has no other read. If the server has other read, one possible solution is to disable all expiration in lookupKey for all clients. But I don't want this PR to affect read/write from normal client.

src/networking.c

src/server.h

valkey.conf

zuiderkwast · 2024-10-21T09:47:42Z

@lyq2333 Can you commit the changes to commands.def?

When you run make locally and you have python3 installed, make updates commands.def if there are any new or changed commands.

Signed-off-by: lvyanqi.lyq <[email protected]>

…mand Signed-off-by: lvyanqi.lyq <[email protected]>

Signed-off-by: lvyanqi.lyq <[email protected]>

lyq2333 · 2024-10-21T11:02:03Z

@lyq2333 Can you commit the changes to commands.def?

When you run make locally and you have python3 installed, make updates commands.def if there are any new or changed commands.

@zuiderkwast Thanks. Sorry forgot it before. I also forgot to sign off and have to force push. No code is modified.

madolson · 2024-10-21T15:13:51Z

Weekly core meeting. No specific comments other than we should review this offline and make progress. Directionally seems like a good idea.

lyq2333 · 2024-10-22T09:38:01Z

The scenario we encountered is that some users want to migrate data from Server A to Server B. Both A and B work as primary and B may have some data before the migration. We find expiration and eviction may lead to data inconsistency. So we came up with a simple method, but it still has a few places to discuss.

We plan to introduce a new config(import-mode in this PR) to mark that this server is importing data. As for expiration, in import mode, active expiration in cron is prohibited and passive expiration in commands is limited depend on the client state. Clients marked as import source work like server.primary, which will not trigger passive expiration for read and write. But there are many ways to handle other normal clients. We think of some ways as follow.

Normal clients work as usual, which means they can perform passive expiration whether for read or write. The drawback is that normal clients accessing the migrating data will also trigger passive expiration and may affect the migration.
Normal clients are prohibited from write commands, meanwhile don't trigger passive expiration when read and don't read expired keys. The server works like a read-only replica. The drawback is that clients need to stop writing during the data migration and may affect users business.
Normal clients don't trigger passive expiration when read and don't read expired keys, but still trigger passive expiration when write. We can't prohibit passive expiration in write commands, because server will crash when we call incr/decr commands on expired keys. The reason is that these commands must delete expired keys first, otherwise it will hit the assert in dbAdd. The drawback is that normal clients calling write commands on the migrating data will trigger passive expiration. This is a trade-off between the above two ways and I think it's better because it won't affect the normal clients and have less impact on the migration process.

As for eviction, should server disable eviction automatically to ensure data consistency in import mode？Or let users choose the maxmeory-policy, I guess no one will choose an option other than noeviction.

@valkey-io/core-team WDYT? Any suggestions would be greatly appreciated.

soloestoy · 2024-10-23T03:36:00Z

The scenario we encountered is that some users want to migrate data from Server A to Server B. Both A and B work as primary and B may have some data before the migration. We find expiration and eviction may lead to data inconsistency. So we came up with a simple method, but it still has a few places to discuss.

We plan to introduce a new config(import-mode in this PR) to mark that this server is importing data. As for expiration, in import mode, active expiration in cron is prohibited and passive expiration in commands is limited depend on the client state. Clients marked as import source work like server.primary, which will not trigger passive expiration for read and write. But there are many ways to handle other normal clients. We think of some ways as follow.

Normal clients work as usual, which means they can perform passive expiration whether for read or write. The drawback is that normal clients accessing the migrating data will also trigger passive expiration and may affect the migration.

Normal clients are prohibited from write commands, meanwhile don't trigger passive expiration when read and don't read expired keys. The server works like a read-only replica. The drawback is that clients need to stop writing during the data migration and may affect users business.

Normal clients don't trigger passive expiration when read and don't read expired keys, but still trigger passive expiration when write. We can't prohibit passive expiration in write commands, because server will crash when we call incr/decr commands on expired keys. The reason is that these commands must delete expired keys first, otherwise it will hit the assert in dbAdd. The drawback is that normal clients calling write commands on the migrating data will trigger passive expiration. This is a trade-off between the above two ways and I think it's better because it won't affect the normal clients and have less impact on the migration process.

As for eviction, should server disable eviction automatically to ensure data consistency in import mode？Or let users choose the maxmeory-policy, I guess no one will choose an option other than noeviction.

@valkey-io/core-team WDYT? Any suggestions would be greatly appreciated.

The core issue is that for the target node of data migration that is active, we need to provide normal read and write services. However, we don't want to affect the data being migrated and can't identify which data is being migrated.

Method 3 seems to be a relatively suitable option, but of course, it requires users to ensure they do not perform write operations on the data being migrated.

zuiderkwast

Looks good to me.

It would be nice to have a documentation page for data migration. We have https://valkey.io/topics/migration/ but I think we should explain how to move data using this feature as well.

Question: What happens if a cluster node is a primary in import mode and there is a failover so another node becomes the primary? The node becomes a replica. Do we automatically exit import-mode?

Co-authored-by: Madelyn Olson <[email protected]> Signed-off-by: Yanqi Lv <[email protected]>

Signed-off-by: lvyanqi.lyq <[email protected]>

lyq2333 · 2024-11-12T02:14:24Z

Question: What happens if a cluster node is a primary in import mode and there is a failover so another node becomes the primary? The node becomes a replica. Do we automatically exit import-mode?

@zuiderkwast Thanks. Nice question. You remind me. How about using (!server.primary_host && server.import_mode) as the condition? i.e. if (server.maxmemory_policy == MAXMEMORY_NO_EVICTION || server.import_mode) change to if (server.maxmemory_policy == MAXMEMORY_NO_EVICTION || (!server.primary_host && server.import_mode)).

lyq2333 · 2024-11-12T02:16:30Z

Force push because I forgot to sign off again. Just update the commands.def.

Signed-off-by: lvyanqi.lyq <[email protected]>

lyq2333 · 2024-11-13T06:21:39Z

Question: What happens if a cluster node is a primary in import mode and there is a failover so another node becomes the primary? The node becomes a replica. Do we automatically exit import-mode?

@zuiderkwast Thanks. Nice question. You remind me. How about using (!server.primary_host && server.import_mode) as the condition? i.e. if (server.maxmemory_policy == MAXMEMORY_NO_EVICTION || server.import_mode) change to if (server.maxmemory_policy == MAXMEMORY_NO_EVICTION || (!server.primary_host && server.import_mode)).

DONE.

src/db.c

src/expire.c

Signed-off-by: lvyanqi.lyq <[email protected]>

src/db.c

Signed-off-by: lvyanqi.lyq <[email protected]>

ranshid

Still did not take a very close look. Overall it seems aligned with the discussions.
I would like to ask: what is the migration strategy regarding ACLs? I mean replica is basically permitted to all actions, but in case of a migration the Source would have to adjust it's user permissions?

src/server.c

lyq2333 · 2024-11-15T06:24:13Z

Still did not take a very close look. Overall it seems aligned with the discussions. I would like to ask: what is the migration strategy regarding ACLs? I mean replica is basically permitted to all actions, but in case of a migration the Source would have to adjust it's user permissions?

Yes, I suggest creating a new user for data migration.

Signed-off-by: lvyanqi.lyq <[email protected]>

ranshid · 2024-11-19T16:15:37Z

Still did not take a very close look. Overall it seems aligned with the discussions. I would like to ask: what is the migration strategy regarding ACLs? I mean replica is basically permitted to all actions, but in case of a migration the Source would have to adjust it's user permissions?

Yes, I suggest creating a new user for data migration.

So just to circle back and clarify this: the client which migrates the data will NOT automatically granted with superuser permissions but wil have to count on it's authenticated use to support all needed permissions?

zuiderkwast · 2024-11-19T17:33:23Z

So just to circle back and clarify this: the client which migrates the data will NOT automatically granted with superuser permissions but wil have to count on it's authenticated use to support all needed permissions?

That's right. It would be good to mention this in the docs.

It wouldn't be very secure if a client could get superuser permissions just by sending the command CLIENT IMPORT-SOURCE ON.

ranshid

LGTM

zuiderkwast · 2024-11-19T20:56:59Z

Looking forward to a doc PR for

commands/client-import-source.md
topics/migration.md

In the migration guide, I hope we can have some guide about how to migrate data with import-mode, for example using redishake.

lyq2333 · 2024-11-20T01:27:52Z

Looking forward to a doc PR for

commands/client-import-source.md

topics/migration.md

In the migration guide, I hope we can have some guide about how to migrate data with import-mode, for example using redishake.

@suxb201 hi, I think this pr is helpful for RedisShake. PTAL.

hpatro

my 2 cents: easy to introduce bug in the future where a dev can forget to check the import mode flag. It would be nice to introduce a macro for serverInImportMode / serverInDataServingMode and use it everywhere instead of checking if it's primary and in/not-in import mode independently.

hpatro · 2024-11-20T04:26:48Z

src/networking.c

+        if (!strcasecmp(c->argv[2]->ptr, "on")) {
+            c->flag.import_source = 1;
+            addReply(c, shared.ok);


What happens to client import_source flag which were turned on explicitly when server import_mode is turned off?

hpatro · 2024-11-20T04:28:51Z

tests/unit/expire.tcl

@@ -832,6 +832,80 @@ start_server {tags {"expire"}} {
        close_replication_stream $repl
        assert_equal [r debug set-active-expire 1] {OK}
    } {} {needs:debug}
+
+    test {Import mode should forbid active expiration} {


It might be worthwhile to run these tests in cluster-enabled mode as well.

hpatro · 2024-11-20T04:31:11Z

src/db.c

+         * Notice: other clients, apart from the import source, should not access
+         * the data imported by import source.


Shouldn't this be enforced by server instead of providing a warning in the code comment/config file? The client's not marked with import_source should receive LOADING error if I'm understanding the use case correctly.

hpatro · 2024-11-20T04:38:15Z

src/server.c

@@ -1651,7 +1651,7 @@ void beforeSleep(struct aeEventLoop *eventLoop) {

    /* Run a fast expire cycle (the called function will return
     * ASAP if a fast cycle is not needed). */
-    if (server.active_expire_enabled && iAmPrimary()) activeExpireCycle(ACTIVE_EXPIRE_CYCLE_FAST);
+    if (server.active_expire_enabled && !server.import_mode && iAmPrimary()) activeExpireCycle(ACTIVE_EXPIRE_CYCLE_FAST);


nit: we could have had similar conditional statement above at line 1058

!server.import_mode && iAmPrimary()

After valkey-io#1185, a client in import-source state can visit expired key both in read commands and write commands, this commit handle keyIsExpired function to handle import-source state as well, so KEYS can visit the expired key. This is not particularly important, but it ensures the definition, also doing some cleanup around the test, verified that the client can indeed visit the expired key. Signed-off-by: Binbin <[email protected]>

lyq2333 force-pushed the pseudo-master branch from 52be593 to b23b09b Compare October 17, 2024 11:13

lyq2333 requested a review from soloestoy October 17, 2024 11:14

lyq2333 requested a review from a team October 17, 2024 11:34

zuiderkwast reviewed Oct 17, 2024

View reviewed changes

src/replication.c Outdated Show resolved Hide resolved

valkey.conf Outdated Show resolved Hide resolved

hwware added the major-decision-pending Major decision pending by TSC team label Oct 17, 2024

soloestoy reviewed Oct 18, 2024

View reviewed changes

src/evict.c Outdated Show resolved Hide resolved

src/replication.c Outdated Show resolved Hide resolved

zuiderkwast reviewed Oct 18, 2024

View reviewed changes

src/networking.c Outdated Show resolved Hide resolved

zuiderkwast reviewed Oct 18, 2024

View reviewed changes

src/server.h Outdated Show resolved Hide resolved

zuiderkwast reviewed Oct 18, 2024

View reviewed changes

valkey.conf Outdated Show resolved Hide resolved

zuiderkwast reviewed Oct 18, 2024

View reviewed changes

valkey.conf Outdated Show resolved Hide resolved

lyq2333 added 8 commits October 21, 2024 18:36

add pseudo-replica mode

f59c5e8

Signed-off-by: lvyanqi.lyq <[email protected]>

rename pseudo-replica to import-mode and add client import-source com…

7226e55

…mand Signed-off-by: lvyanqi.lyq <[email protected]>

revert the file permission change

c2e86bf

Signed-off-by: lvyanqi.lyq <[email protected]>

minor comments fix

937924c

Signed-off-by: lvyanqi.lyq <[email protected]>

minor comments fix

b3b5c46

Signed-off-by: lvyanqi.lyq <[email protected]>

fix typo

a79a51a

Signed-off-by: lvyanqi.lyq <[email protected]>

auto-generate commands.def

3e36548

Signed-off-by: lvyanqi.lyq <[email protected]>

auto-generate commands.def

7883e9f

Signed-off-by: lvyanqi.lyq <[email protected]>

lyq2333 force-pushed the pseudo-master branch from 0655bbc to 7883e9f Compare October 21, 2024 10:37

madolson added major-decision-approved Major decision approved by TSC team and removed major-decision-pending Major decision pending by TSC team labels Nov 11, 2024

madolson requested a review from ranshid November 11, 2024 15:11

zuiderkwast approved these changes Nov 11, 2024

View reviewed changes

lyq2333 and others added 2 commits November 12, 2024 09:10

Update src/commands/client-import-source.json

523b462

Co-authored-by: Madelyn Olson <[email protected]> Signed-off-by: Yanqi Lv <[email protected]>

update commands.def

be13edc

Signed-off-by: lvyanqi.lyq <[email protected]>

lyq2333 force-pushed the pseudo-master branch from e007d8d to be13edc Compare November 12, 2024 01:48

import-mode only takes effect on primary

7b1127e

Signed-off-by: lvyanqi.lyq <[email protected]>

soloestoy approved these changes Nov 13, 2024

View reviewed changes

src/db.c Show resolved Hide resolved

src/expire.c Show resolved Hide resolved

update comment

11f75bc

Signed-off-by: lvyanqi.lyq <[email protected]>

zuiderkwast reviewed Nov 13, 2024

View reviewed changes

src/db.c Outdated Show resolved Hide resolved

clang-format

20b005c

Signed-off-by: lvyanqi.lyq <[email protected]>

ranshid reviewed Nov 14, 2024

View reviewed changes

src/server.c Outdated Show resolved Hide resolved

minor refactor

7f04964

Signed-off-by: lvyanqi.lyq <[email protected]>

soloestoy added release-notes This issue should get a line item in the release notes needs-doc-pr This change needs to update a documentation page. Remove label once doc PR is open. labels Nov 18, 2024

ranshid approved these changes Nov 19, 2024

View reviewed changes

zuiderkwast changed the title ~~Avoid expiration and eviction during data syncing~~ Import-mode: Avoid expiration and eviction during data syncing Nov 19, 2024

zuiderkwast merged commit 4986310 into valkey-io:unstable Nov 19, 2024
49 checks passed

hpatro reviewed Nov 20, 2024

View reviewed changes

enjoy-binbin mentioned this pull request Nov 20, 2024

Make KEYS can visit expired key in import-source state #1326

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Import-mode: Avoid expiration and eviction during data syncing #1185

Import-mode: Avoid expiration and eviction during data syncing #1185

lyq2333 commented Oct 17, 2024 •

edited by zuiderkwast

Loading

codecov bot commented Oct 17, 2024 •

edited

Loading

soloestoy commented Oct 17, 2024

zuiderkwast left a comment

soloestoy commented Oct 18, 2024

soloestoy left a comment

lyq2333 commented Oct 18, 2024

enjoy-binbin commented Oct 18, 2024

lyq2333 commented Oct 18, 2024

zuiderkwast commented Oct 21, 2024

lyq2333 commented Oct 21, 2024

madolson commented Oct 21, 2024

lyq2333 commented Oct 22, 2024

soloestoy commented Oct 23, 2024

zuiderkwast left a comment

lyq2333 commented Nov 12, 2024

lyq2333 commented Nov 12, 2024

lyq2333 commented Nov 13, 2024

ranshid left a comment •

edited

Loading

lyq2333 commented Nov 15, 2024

ranshid commented Nov 19, 2024

zuiderkwast commented Nov 19, 2024

ranshid left a comment

zuiderkwast commented Nov 19, 2024

lyq2333 commented Nov 20, 2024

hpatro left a comment

hpatro Nov 20, 2024

hpatro Nov 20, 2024

hpatro Nov 20, 2024

hpatro Nov 20, 2024

		* Notice: other clients, apart from the import source, should not access
		* the data imported by import source.

Import-mode: Avoid expiration and eviction during data syncing #1185

Import-mode: Avoid expiration and eviction during data syncing #1185

Conversation

lyq2333 commented Oct 17, 2024 • edited by zuiderkwast Loading

codecov bot commented Oct 17, 2024 • edited Loading

Codecov Report

soloestoy commented Oct 17, 2024

zuiderkwast left a comment

Choose a reason for hiding this comment

soloestoy commented Oct 18, 2024

soloestoy left a comment

Choose a reason for hiding this comment

lyq2333 commented Oct 18, 2024

enjoy-binbin commented Oct 18, 2024

lyq2333 commented Oct 18, 2024

zuiderkwast commented Oct 21, 2024

lyq2333 commented Oct 21, 2024

madolson commented Oct 21, 2024

lyq2333 commented Oct 22, 2024

soloestoy commented Oct 23, 2024

zuiderkwast left a comment

Choose a reason for hiding this comment

lyq2333 commented Nov 12, 2024

lyq2333 commented Nov 12, 2024

lyq2333 commented Nov 13, 2024

ranshid left a comment • edited Loading

Choose a reason for hiding this comment

lyq2333 commented Nov 15, 2024

ranshid commented Nov 19, 2024

zuiderkwast commented Nov 19, 2024

ranshid left a comment

Choose a reason for hiding this comment

zuiderkwast commented Nov 19, 2024

lyq2333 commented Nov 20, 2024

hpatro left a comment

Choose a reason for hiding this comment

hpatro Nov 20, 2024

Choose a reason for hiding this comment

hpatro Nov 20, 2024

Choose a reason for hiding this comment

hpatro Nov 20, 2024

Choose a reason for hiding this comment

hpatro Nov 20, 2024

Choose a reason for hiding this comment

lyq2333 commented Oct 17, 2024 •

edited by zuiderkwast

Loading

codecov bot commented Oct 17, 2024 •

edited

Loading

ranshid left a comment •

edited

Loading