feat(namespaces): Initial support for multi-tenant #3260

chakaz · 2024-07-03T10:54:46Z

This PR introduces a way to create multiple, separate and isolated namespaces in Dragonfly. Each user can be associated with a single namespace, and will not be able to interact with other namespaces.

This is still experimental, and lacks some important features, such as:

Replication and RDB saving completely ignores non-default namespaces
Defrag and statistics either use the default namespace or all namespaces without separation

To associate a user with a namespace, use the ACL command with the NAMESPACE:<namespace> flag:

ACL SETUSER user NAMESPACE:namespace1 ON >user_pass +@all ~*

For more examples and up to date info check
tests/dragonfly/acl_family_test.py - specifically the test_namespaces function.

This PR introduces a way to create multiple, separate and isolated namespaces in Dragonfly. Each user can be associated with a single namespace, and will not be able to interact with other namespaces. This is still experimental, and lacks some important features, such as: * Replication and RDB saving completely ignores non-default namespaces * Defrag and statistics either use the default namespace or all namespaces without separation To associate a user with a namespace, use the `ACL` command with the `TENANT:<namespace>` flag: ``` ACL SETUSER user TENANT:namespace1 ON >user_pass +@ALL ~* ``` For more examples and up to date info check `tests/dragonfly/acl_family_test.py` - specifically the `test_namespaces` function.

src/server/engine_shard_set.cc

romange · 2024-07-04T13:03:20Z

Is it the whole PR or the first one in the series?
I am asking because, we talked about having an HLD .md document in docs/, and also having non-functional changes first to ease the review.

src/server/acl/helpers.cc

romange · 2024-07-04T13:06:34Z

src/server/acl/user.h

    // TODO allow reset all
    // bool reset_all{false};
+
+    std::optional<std::string> ns{};


subjective nit (related to the previous comment) - can be just a string with empty indicating it's not set.

SG. + @kostasrim to make sure this UpdateRequest class is always expected to contain all information (i.e. that there can't be only deltas in it)

romange · 2024-07-04T13:11:28Z

src/server/namespaces.h

+
+namespace dfly {
+
+class Namespace {


maybe add class description, including a threading/ownership model?
can also be a reference to .md file if you write it there.

romange · 2024-07-04T13:11:41Z

src/server/namespaces.h

+
+class Namespace {
+ public:
+  explicit Namespace();


explicit not needed?

romange · 2024-07-04T13:22:21Z

src/server/bitops_family.cc

@@ -310,15 +310,15 @@ class ElementAccess {
 };

 std::optional<bool> ElementAccess::Exists(EngineShard* shard) {
-  auto res = shard->db_slice().FindReadOnly(context_, key_, OBJ_STRING);
+  auto res = context_.ns->GetCurrentDbSlice().FindReadOnly(context_, key_, OBJ_STRING);


Our call chain is already designed to pass shard by argument, therefore it's already in a register. I suggest that you actually use here GetDbSlice(ShardId) and avoid doing it implicitly by calling GetCurrentDbSlice.
Now, it complicates the whole expression in so many places, so I also suggest to add a convenience wrapper ConnectionContext::FindReadOnly.
why? because we use context_ twice here, so instead we could shorten this
in some many places to just context_.FindReadOnly(key_, OBJ_STRING)
Of course it also applies to other frequent operations like FindMutable

There is a small challenge with this proposal: transaction that's part of the connection context can sometimes be null. If it's null, we can't tell which tx time to pass to FindReadOnly() (which requires a DbContext)

Wait, sorry, my bad. I thought that your proposal was to make this change in ConnectionContext. I now understand you meant in DbContext..

It looks like the only place which uses DbContext, but not using OpArgs, is bitops family, so I didn't do that.
But I used shard->shard_id() as you proposed.

Yeah, I confused both types.

romange · 2024-07-04T13:25:04Z

src/server/bitops_family.cc

@@ -355,7 +355,7 @@ OpResult<bool> BitNewValue(const OpArgs& args, std::string_view key, uint32_t of
                           bool bit_value) {
  EngineShard* shard = args.shard;
  ElementAccess element_access{key, args};
-  auto& db_slice = shard->db_slice();
+  auto& db_slice = args.db_cntx.ns->GetCurrentDbSlice();


similarly, please add a convenience accessor GetDbSlice in OpArgs that uses the shard variable inside OpArgs

Adding it in OpArgs is a great idea, it saves a lot of boilerplate, thanks!

chakaz · 2024-07-07T13:21:44Z

Is it the whole PR or the first one in the series?

This is the whole PR.
Indeed, as you mentioned, we talked about adding a stub where there's an empty API that always returns the same pointer.
However, after getting back to work on it, I decided that it's kinda ridiculous to do that, because the "implementation" is just a map... (check out namespaces.h and .cc). The challenges are:

Getting everything to compile
Getting the initialization and turn down to work properly
Both are not affected by whether the API works or not.
So I decided to have it all in the same PR

I am asking because, we talked about having an HLD .md document in docs/, and also having non-functional changes first to ease the review.

Indeed, I forgot about the .md hld. I'll add that very soon.

romange · 2024-07-07T15:24:04Z

src/server/acl/user.cc

+  namespace_ = ns;
+}
+
+std::string User::Namespace() const {


src/server/container_utils.cc

chakaz · 2024-07-07T20:57:13Z

I added the high level design .md doc. Please let me know what else you would like me to include in it. I captured everything I could think of, but I probably have missed some details...

romange

I suggest the following:
Restrict the first PR to changes related to passing DbContext where it's needed, the calls to op_args.GetDbSlice(), Transaction::GetCurrentDbSlice, and the design document. Basically everything without namespaces.* files. This will constitute a large portion of the diff without introducing any functional changes, making it relatively easy to review. We can even submit this part before the 1.20 release.

This PR is huge, so I think such split will help us to review it faster 🙏🏼

romange · 2024-07-08T06:16:06Z

docs/namespaces.md

+We call this feature _namespaces_, and it allows using a single Dragonfly server with multiple
+tenants, each using their own data, without being able to mix them together.
+
+This feature can also be achieved by having each user `SELECT` a different (numeric) database, or by


Note that this feature can alternatively be ...

romange · 2024-07-08T06:19:02Z

docs/namespaces.md

+To access all `Namespace`s, we also added a registry with the original name `Namespaces`. It is a
+global, thread safe class that allows accessing all registered namespaces, and registering new ones
+on the fly. Note that, while it is thread safe, it shouldn't be a bottle neck because it is supposed
+to only be used during the authentication of a connection (or when adding new namespaces).


please note that even with connection initialization we have workloads reaching 10K connects per second. I suggest using SharedMutex for this access to reduce connection between different connects.

romange · 2024-07-08T06:21:34Z

src/server/blocking_controller.h


 class BlockingController {
 public:
-  explicit BlockingController(EngineShard* owner);
+  explicit BlockingController(EngineShard* owner, Namespace* ns);


I assume that BlockingController is a global creature, so this binding is i guess a workaround to make it work with default Namespace? If I am correct, please add a TODO.

BlockingController is actually per namespace, not global

romange · 2024-07-08T06:23:47Z

src/server/container_utils.cc

-  const auto key_checker = [req_obj_type](EngineShard* owner, const DbContext& context,
-                                          Transaction*, std::string_view key) -> bool {
-    return owner->db_slice().FindReadOnly(context, key, req_obj_type).ok();
+  auto ns = &trans->GetNamespace();


nit: auto* to help a boomer to read the code.

romange · 2024-07-08T06:25:54Z

src/server/db_slice.cc

  auto obj_type = it->second.ObjType();

  if (doc_del_cb_ && (obj_type == OBJ_JSON || obj_type == OBJ_HASH)) {
    string tmp;
    string_view key = it->first.GetSlice(&tmp);
-    doc_del_cb_(key, DbContext{db_ind, GetCurrentTimeMs()}, it->second);
+    doc_del_cb_(key, DbContext{cntx.ns, cntx.db_index, GetCurrentTimeMs()}, it->second);


cntx is not of type DbContext?
why not just pass it?

romange · 2024-07-08T06:29:28Z

src/server/engine_shard_set.cc

@@ -324,7 +326,7 @@ bool EngineShard::DoDefrag() {
      }
    });
    traverses_count++;
-  } while (traverses_count < kMaxTraverses && cur);
+  } while (traverses_count < kMaxTraverses && cur && namespaces.IsInitialized());


I do not understand this change

Defrag is a background task that can trigger (mostly in tests) before the namespaces is initialized or after it is destroyed

romange · 2024-07-08T06:29:47Z

src/server/engine_shard_set.cc

@@ -355,11 +357,14 @@ bool EngineShard::DoDefrag() {
 //     priority.
 //     otherwise lower the task priority so that it would not use the CPU when not required
 uint32_t EngineShard::DefragTask() {
+  if (!namespaces.IsInitialized()) {


romange · 2024-07-08T06:32:34Z

src/server/engine_shard_set.cc

 void EngineShard::DestroyThreadLocal() {
  if (!shard_)
    return;

-  uint32_t index = shard_->db_slice_.shard_id();
+  uint32_t index = shard_->shard_id();


romange · 2024-07-08T06:34:58Z

src/server/zset_family.cc

@@ -213,10 +213,11 @@ OpResult<DbSlice::ItAndUpdater> FindZEntry(const ZParams& zparams, const OpArgs&
      return OpStatus::WRONG_TYPE;
  }

-  if (add_res.is_new && op_args.shard->blocking_controller()) {
+  auto blocking_controller = op_args.db_cntx.ns->GetBlockingController(op_args.shard->shard_id());


romange · 2024-07-08T06:46:54Z

src/server/CMakeLists.txt

@@ -47,7 +47,7 @@ add_library(dragonfly_lib bloom_family.cc engine_shard_set.cc
            config_registry.cc conn_context.cc debugcmd.cc dflycmd.cc
            generic_family.cc hset_family.cc http_api.cc json_family.cc
            list_family.cc main_service.cc memory_cmd.cc rdb_load.cc rdb_save.cc replica.cc
-            protocol_client.cc
+            protocol_client.cc namespaces.cc


please note that you can not access namespaces from the transaction code due to backward dependency.

so shall I move namespaces.cc to dfly_transaction lib?

chakaz · 2024-07-11T19:29:47Z

I suggest the following: Restrict the first PR to changes related to passing DbContext where it's needed, the calls to op_args.GetDbSlice(), Transaction::GetCurrentDbSlice, and the design document. Basically everything without namespaces.* files. This will constitute a large portion of the diff without introducing any functional changes, making it relatively easy to review. We can even submit this part before the 1.20 release.

This PR is huge, so I think such split will help us to review it faster 🙏🏼

Done: #3311

romange

LGTM and god help us all

chakaz added 2 commits July 3, 2024 13:53

global

e04f524

chakaz changed the title ~~feat(namespaces): Initial support for multi-tenant #3050~~ feat(namespaces): Initial support for multi-tenant Jul 3, 2024

chakaz requested a review from adiholden July 3, 2024 20:05

fix

e0e67de

romange reviewed Jul 4, 2024

View reviewed changes

src/server/engine_shard_set.cc Outdated Show resolved Hide resolved

oops

3e31740

romange reviewed Jul 4, 2024

View reviewed changes

src/server/acl/helpers.cc Outdated Show resolved Hide resolved

romange reviewed Jul 4, 2024

View reviewed changes

chakaz added 9 commits July 7, 2024 08:35

NAMESPACE: instead of TENANT:

6e45b2b

desc, term

8671d64

explicit not needed

cc30cbc

NAMESPACES

dca3c39

OpArgs::GetDbSlice()

9a842ae

shard_id

45627b7

Merge branch 'main' into tenants

6e35279

comment

cc7af2f

shard_id v2

7546ff6

Merge branch 'main' into tenants

92053db

romange reviewed Jul 7, 2024

View reviewed changes

src/server/container_utils.cc Show resolved Hide resolved

chakaz added 3 commits July 7, 2024 23:17

fix build

e517fe7

fixes

078d8bd

hld md

28bf75d

chakaz requested a review from romange July 8, 2024 06:13

romange reviewed Jul 8, 2024

View reviewed changes

chakaz added 3 commits July 11, 2024 09:00

Merge branch 'main' into tenants

04c2a52

fixes

b7e7192

DbContext::GetDbSlice

06e58b9

chakaz added 2 commits July 12, 2024 09:18

Merge branch 'main' into tenants

351b4a0

shard->shard_id()

3729241

chakaz requested a review from romange July 12, 2024 08:45

chakaz added 2 commits July 16, 2024 14:58

Merge branch 'main' into tenants

e6194b9

Re-add parsing

b5024c0

romange approved these changes Jul 16, 2024

View reviewed changes

chakaz merged commit 18ca61d into main Jul 16, 2024
10 checks passed

chakaz deleted the tenants branch July 16, 2024 16:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(namespaces): Initial support for multi-tenant #3260

feat(namespaces): Initial support for multi-tenant #3260

chakaz commented Jul 3, 2024 •

edited

Loading

romange commented Jul 4, 2024

romange Jul 4, 2024

chakaz Jul 7, 2024

romange Jul 4, 2024

romange Jul 4, 2024

romange Jul 4, 2024 •

edited

Loading

chakaz Jul 7, 2024

chakaz Jul 7, 2024

chakaz Jul 7, 2024

romange Jul 7, 2024

romange Jul 4, 2024

chakaz Jul 7, 2024

chakaz commented Jul 7, 2024

romange Jul 7, 2024

chakaz commented Jul 7, 2024

romange left a comment •

edited

Loading

romange Jul 8, 2024

romange Jul 8, 2024

romange Jul 8, 2024

chakaz Jul 11, 2024

romange Jul 8, 2024

romange Jul 8, 2024

romange Jul 8, 2024

chakaz Jul 11, 2024

romange Jul 8, 2024

chakaz Jul 11, 2024

romange Jul 8, 2024

romange Jul 8, 2024

romange Jul 8, 2024

chakaz Jul 11, 2024

chakaz commented Jul 11, 2024

romange left a comment

feat(namespaces): Initial support for multi-tenant #3260

feat(namespaces): Initial support for multi-tenant #3260

Conversation

chakaz commented Jul 3, 2024 • edited Loading

romange commented Jul 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romange Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chakaz commented Jul 7, 2024

Choose a reason for hiding this comment

chakaz commented Jul 7, 2024

romange left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chakaz commented Jul 11, 2024

romange left a comment

Choose a reason for hiding this comment

chakaz commented Jul 3, 2024 •

edited

Loading

romange Jul 4, 2024 •

edited

Loading

romange left a comment •

edited

Loading