Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 14 commits into from
Nov 19, 2024

Conversation

lyq2333
Copy link
Contributor

@lyq2333 lyq2333 commented Oct 17, 2024

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.

Copy link

codecov bot commented Oct 17, 2024

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:

@soloestoy
Copy link
Member

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

@lyq2333 lyq2333 requested a review from a team October 17, 2024 11:34
Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
valkey.conf Outdated Show resolved Hide resolved
@hwware hwware added the major-decision-pending Major decision pending by TSC team label Oct 17, 2024
@soloestoy
Copy link
Member

+-----------+   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.

Copy link
Member

@soloestoy soloestoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/evict.c Outdated Show resolved Hide resolved
src/replication.c Outdated Show resolved Hide resolved
@lyq2333
Copy link
Contributor Author

lyq2333 commented Oct 18, 2024

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
Copy link
Member

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
Copy link
Contributor Author

lyq2333 commented Oct 18, 2024

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 Outdated Show resolved Hide resolved
src/server.h Outdated Show resolved Hide resolved
valkey.conf Outdated Show resolved Hide resolved
valkey.conf Outdated Show resolved Hide resolved
@zuiderkwast
Copy link
Contributor

@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 <lvyanqi.lyq@alibaba-inc.com>
…mand

Signed-off-by: lvyanqi.lyq <lvyanqi.lyq@alibaba-inc.com>
Signed-off-by: lvyanqi.lyq <lvyanqi.lyq@alibaba-inc.com>
Signed-off-by: lvyanqi.lyq <lvyanqi.lyq@alibaba-inc.com>
Signed-off-by: lvyanqi.lyq <lvyanqi.lyq@alibaba-inc.com>
Signed-off-by: lvyanqi.lyq <lvyanqi.lyq@alibaba-inc.com>
Signed-off-by: lvyanqi.lyq <lvyanqi.lyq@alibaba-inc.com>
Signed-off-by: lvyanqi.lyq <lvyanqi.lyq@alibaba-inc.com>
@lyq2333
Copy link
Contributor Author

lyq2333 commented Oct 21, 2024

@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
Copy link
Member

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

@lyq2333
Copy link
Contributor Author

lyq2333 commented Oct 22, 2024

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.

  1. 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.
  2. 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.
  3. 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
Copy link
Member

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.

  1. 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.
  2. 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.
  3. 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.

lyq2333 and others added 2 commits November 12, 2024 09:10
Co-authored-by: Madelyn Olson <madelyneolson@gmail.com>
Signed-off-by: Yanqi Lv <lvyanqi.lyq@alibaba-inc.com>
Signed-off-by: lvyanqi.lyq <lvyanqi.lyq@alibaba-inc.com>
@lyq2333
Copy link
Contributor Author

lyq2333 commented Nov 12, 2024

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
Copy link
Contributor Author

lyq2333 commented Nov 12, 2024

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

Signed-off-by: lvyanqi.lyq <lvyanqi.lyq@alibaba-inc.com>
@lyq2333
Copy link
Contributor Author

lyq2333 commented Nov 13, 2024

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 Show resolved Hide resolved
src/expire.c Show resolved Hide resolved
Signed-off-by: lvyanqi.lyq <lvyanqi.lyq@alibaba-inc.com>
src/db.c Outdated Show resolved Hide resolved
Signed-off-by: lvyanqi.lyq <lvyanqi.lyq@alibaba-inc.com>
Copy link
Member

@ranshid ranshid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
@lyq2333
Copy link
Contributor Author

lyq2333 commented Nov 15, 2024

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 <lvyanqi.lyq@alibaba-inc.com>
@soloestoy 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
Copy link
Member

ranshid commented Nov 19, 2024

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
Copy link
Contributor

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.

Copy link
Member

@ranshid ranshid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zuiderkwast zuiderkwast changed the title Avoid expiration and eviction during data syncing Import-mode: Avoid expiration and eviction during data syncing Nov 19, 2024
@zuiderkwast zuiderkwast merged commit 4986310 into valkey-io:unstable Nov 19, 2024
49 checks passed
@zuiderkwast
Copy link
Contributor

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
Copy link
Contributor Author

lyq2333 commented Nov 20, 2024

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.

Copy link
Contributor

@hpatro hpatro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +4053 to +4055
if (!strcasecmp(c->argv[2]->ptr, "on")) {
c->flag.import_source = 1;
addReply(c, shared.ok);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, the client can't use CLIENT IMPORT-SOURCE OFF if import-mode has already been turned off. That's a minor annoyance I think. Import-source has no effect when import-mode is off.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should clean it up though. Also, shall we update CLIENT LIST output to reflect the import source mode?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just print the flag? or add a new type so client kill can also use the type?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should clean it up though. Also, shall we update CLIENT LIST output to reflect the import source mode?

shouldn't we fail the command unless the server import_mode is enabled? also great point on tagging this special client in CLIENT LIST.

Just print the flag? or add a new type so client kill can also use the type?

A new flag would suffice IMO.

@@ -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} {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/db.c Show resolved Hide resolved
src/server.c Show resolved Hide resolved
enjoy-binbin added a commit to enjoy-binbin/valkey that referenced this pull request Nov 20, 2024
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 <binloveplay1314@qq.com>
@zuiderkwast
Copy link
Contributor

@hpatro @enjoy-binbin Sorry for merging too fast. I didn't know you were still reviewing and testing this.

@enjoy-binbin
Copy link
Member

Sorry for merging too fast. I didn't know you were still reviewing and testing this.

no worry. i mute the notification at the very first beginning, and only review the code today (review and play with it while dealing the conflicts), the code LGTM overall.

enjoy-binbin added a commit that referenced this pull request Nov 27, 2024
After #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 <binloveplay1314@qq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major-decision-approved Major decision approved by TSC team needs-doc-pr This change needs to update a documentation page. Remove label once doc PR is open. release-notes This issue should get a line item in the release notes
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

9 participants