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

Draft: Namespaces #374

Closed
wants to merge 20 commits into from
Closed

Conversation

ederuiter
Copy link

@ederuiter ederuiter commented Oct 29, 2021

This is an proof of concept implementation for namespaces to provide multi-tenancy support (see issue #286 / https://community.keydb.dev/t/multi-tenancy-support/78 )

Background:

We are currently running separate redis instances for each website/application we host on our kubernetes clusters (>100 sites). Most of these sites are have a fairly low volume of traffic.
This is sub-optimal for several reasons:

  • extra number of pods
  • memory usage
  • currently no HA setup for all websites as this would increase the number of pods + memory significantly

I did a POC where I implemented the functionality in a proxy; this was based on key prefixes. but it is quite hard to make that transparent to the applications.

So I started looking into implementing this functionality in KeyDB itself; without relying on key prefixes. This would mean a different set of databases for each user. As far as I (google) could tell redis seems to scale pretty good up to 1000 databases .. as most applications only need 1 or 2 redis databases, this would mean supporting at least 100 users seems attainable.

note: Please be gentle; this is one of my first things I have done in C/C++ in a very long time.

Implementation:

The idea is simple; each user has an associated namespace. Users still see databases 0 .. <max db's>, however that are virtual databases; each namespace would have a different mapping to the global databases.
Allocations of those databases are done dynamically so we only need to maintain the databases that are used.

Besides databases also channels are namespaced. Instead of a global list of pubsub channels / patterns this is now maintained per namespace.

Most internal redis functionality does not need to know about namespaces. client->db still points to the current redis db. It is just that SELECT 0 will now select the virtual db 0, which can be mapped to the global database with id 10 for example.

TODO:

  • improve performance of allocation
  • support ::auto for as special namespace to automatically create a new unique namespace for a user
  • support switching of namespaces for users with admin privileges
  • move script cache to namespace
  • stats/info per namespace
  • add more tests
  • add documentation
  • remove some debugging asserts
  • automatically remove mapping when a database is empty an not in use by any clients anymore
  • fix race conditions in multi-master when allocating databases
  • add per namespace memory limit

@@ -3285,7 +3285,8 @@ void freeMasterInfo(redisMaster *mi)
zfree(mi->masteruser);
if (mi->repl_transfer_tmpfile)
zfree(mi->repl_transfer_tmpfile);
delete mi->staleKeyMap;
Copy link
Collaborator

Choose a reason for hiding this comment

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

It’s actually a bug in our delete override that makes this unsafe. You should be able to delete a null pointer. We have a fix coming.

Copy link
Author

Choose a reason for hiding this comment

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

Ah that explains it, I came across it when debugging some random memory corruption issues and valgrind pointed to here for an invalid delete.

@JohnSully
Copy link
Collaborator

Hi @ederuiter

This change looks awesome and we’ll work with you to get it merged.

@VivekSainiEQ
Copy link
Contributor

Hi @ederuiter,

I'm looking into merging your changes into the main branch.

@ederuiter
Copy link
Author

ederuiter commented Feb 26, 2022

@VivekSainiEQ Cool! Let me know if you need anything from me .. it has been a while since I have looked at this, but still very much interested in getting this merged

@VivekSainiEQ VivekSainiEQ mentioned this pull request Mar 8, 2022
@VivekSainiEQ
Copy link
Contributor

@ederuiter Sounds good! I've created another branch on the KeyDB repo and a subsequent PR in order to more effectively collaborate, found at #404. As a result, this ticket will be closed and further discussion can continue there.

In addition, we plan to merge your changes in without multimaster support as a first version, then add in a fix for the multimaster race condition at a later time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants