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 {


romange · 2024-07-07T15:26:46Z

src/server/container_utils.cc

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


seems you do not use ns ?

See line 343 below: ns->GetDbSlice()...

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

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

chakaz added 3 commits July 7, 2024 23:17

fix build

e517fe7

fixes

078d8bd

hld md

28bf75d

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

romange Jul 7, 2024

chakaz Jul 7, 2024

chakaz commented Jul 7, 2024

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

Are you sure you want to change the base?

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chakaz commented Jul 7, 2024

chakaz commented Jul 3, 2024 •

edited

Loading

romange Jul 4, 2024 •

edited

Loading