feat(server): support config set for some flags #1624

adiholden · 2023-08-02T12:10:08Z

support config set for the following flags: rename_command,dir,requirepass,masterauth
add tcp_keepalive flag and add supprot with config set
more print details when failing on dbfilename flag validation

chakaz · 2023-08-02T12:16:43Z

src/server/main_service.cc

@@ -617,6 +617,10 @@ void Service::Init(util::AcceptServer* acceptor, std::vector<facade::Listener*>
    return true;
  });

+  config_registry.Register("dir", [](const absl::CommandLineFlag& flag) { return true; });


Perhaps we could add some default function that does nothing, or an overload that does so?

chakaz · 2023-08-02T12:33:59Z

src/facade/dragonfly_listener.cc


  if (setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &val, sizeof(val)) < 0) {
    LOG(WARNING) << "Could not set reuse addr on socket " << SafeErrorMessage(errno);
  }
-  bool success = ConfigureKeepAlive(fd, kInterval);
+  uint32_t interval = absl::GetFlag(FLAGS_tcp_keepalive);


Nit: I'd remove the var interval and call GetFlag() inline of ConfigureKeepAlive(), your call

Actually I need to do validation on this flag value, so register call back is needed.pushing now my changes

chakaz · 2023-08-02T12:36:33Z

src/server/command_registry.cc

+      string_view origin_name = AsciiStrToUpper(kv.first);
+      string_view new_name = AsciiStrToUpper(kv.second);


These are actually undefined behavior and could crash
AsciiStrToUpper()'s return type is string, and assigning such temporaries into a string_view is bad.
I wish we had something to catch such cases :(
Anyway, origin_name and new_name should be of type string

chakaz · 2023-08-02T12:38:46Z

src/server/command_registry.cc

+  auto apply_cb_on_all_commands = [](const std::vector<std::string>& commands,
+                                     std::function<bool(string_view, string_view)> cb) {
+    for (string command_data : commands) {
+      pair<string_view, string_view> kv = StrSplit(command_data, '=');


Here and above, this will be unexpected behavior if the new value is expected to have = inside the name.
Maybe use absl::MaxSplits('=', 1) instead of just '='?

src/server/command_registry.cc

romange · 2023-08-03T06:18:29Z

please note that cmd_stats_map relies on cid->name() be const char. Please double check if it still holds.

chakaz · 2023-08-03T08:29:25Z

please note that cmd_stats_map relies on cid->name() be const char. Please double check if it still holds.

@adiholden correct me if I'm wrong, but I think that specifically this string never changes. It's just how we get to the CommandId that's different.
A side effect of this would be that, even if we rename some command (like FLUSHDB -> NOOOOOOO), the stats will still be gathered around FLUSHDB.
(right?)

1. support config set for the following flags: rename_command,dir,requirepass,masterauth 2. more print details when failing on dbfilename flag validation Signed-off-by: adi_holden <adi@dragonflydb.io>

Signed-off-by: adi_holden <adi@dragonflydb.io>

chakaz · 2023-08-06T12:38:09Z

src/facade/dragonfly_listener.cc

@@ -139,6 +142,18 @@ Listener::Listener(Protocol protocol, ServiceInterface* si) : service_(si), prot
  http_base_.reset(new HttpListener<>);
  http_base_->set_resource_prefix("http://static.dragonflydb.io/data-plane");
  si->ConfigureHttpHandlers(http_base_.get());
+
+  tcp_keepalive = absl::GetFlag(FLAGS_tcp_keepalive);


This cache is not thread safe, in that an access to tcp_keepalive can occur in parallel to setting it.
However, are you sure it's something we need to cache? We read it upon receiving a new connection, it's definitely something that can be slowed down. I'd vote to reduce complexity and not cache.

I concur. connection creation is not on a hotpath.

So the reason here to use static variable is not to cache the flag but to enable the validation of the flag value i.e that it is bigger than 3. As when we call config set command we first set the flag value and then call the registered callback so I did the validation in the callback and if the validation passed I set the tcp_keepalive var. I spoke with @chakaz and he suggested to do this validation with using the absl custom flags inside the AbslParseFlag. I can do this but then the question raised do we really need this validation @romange ? I believe we have this validation today because we devide tcp_keepalive value by 3 when setting the interval between subsequent keep-alive probes. But I could just make it min value 1. Does this makes sense?

src/server/command_registry.cc

adiholden · 2023-08-07T07:10:27Z

please note that cmd_stats_map relies on cid->name() be const char. Please double check if it still holds.

Indead there is a problem here and also not only here but when we run find command we return a pointer to the map, so I can not change the command map .I will need to think on another solutions

adiholden · 2023-08-07T07:15:38Z

please note that cmd_stats_map relies on cid->name() be const char. Please double check if it still holds.

@adiholden correct me if I'm wrong, but I think that specifically this string never changes. It's just how we get to the CommandId that's different. A side effect of this would be that, even if we rename some command (like FLUSHDB -> NOOOOOOO), the stats will still be gathered around FLUSHDB. (right?)

The string in CommadId class does not change but we have another problem here. It is that we return a pointer to the map item CommandId when we execute a command and in the current implementation I move the object, so when executing a command the pointer maybe invalid after rename

romange · 2023-08-07T09:27:01Z

I did not understand the problematic scenario. Where exactly we keep the stale pointer?

chakaz · 2023-08-07T10:42:48Z

Adi and I just discussed this.
The issue is that CommandId* is returned from the registry, which is held directly in a flat_hash_map (i.e. no pointer stability).
It's easy to support command renaming via flat_hash_map<..., unique_ptr<CommandId>>. Supporting erasing of commands is a bit hacky if we want to support the case of erasing a command while it is invoked - we can just push the unique_ptr<CommandId> into some vector of zombie commands.
The real issue is that this registry is used across threads, but it is not protected by any means. I imagine we'll either duplicate it across all threads (like we do for the cluster config), or use some locking/atomics mechanism (but probably not because it's used in all hot paths essentially)

adiholden · 2023-08-08T20:04:52Z

Adi and I just discussed this. The issue is that CommandId* is returned from the registry, which is held directly in a flat_hash_map (i.e. no pointer stability). It's easy to support command renaming via flat_hash_map<..., unique_ptr<CommandId>>. Supporting erasing of commands is a bit hacky if we want to support the case of erasing a command while it is invoked - we can just push the unique_ptr<CommandId> into some vector of zombie commands. The real issue is that this registry is used across threads, but it is not protected by any means. I imagine we'll either duplicate it across all threads (like we do for the cluster config), or use some locking/atomics mechanism (but probably not because it's used in all hot paths essentially)

I am going to remove the support for config set rename_command in this PR as it looks to be a bigger change than what I thought and I will do it in a different PR

Signed-off-by: adi_holden <adi@dragonflydb.io>

romange · 2023-08-09T08:45:39Z

@adiholden let's not support rename via config. I do not see why we must do it dynamically, the intent is to have "hidden" commands that can be accessed internally. The whole concept of rename is weird (hacky) and should be handled by ACLs, imho.
So supporting ti via flags is more than enough.

romange · 2023-08-09T08:47:30Z

another thing: the renamee feature can be redesigned differently if we take into account the admin port.

romange · 2023-08-09T09:29:28Z

src/facade/dragonfly_listener.cc

@@ -27,6 +26,7 @@ ABSL_FLAG(string, tls_cert_file, "", "cert file for tls connections");
 ABSL_FLAG(string, tls_key_file, "", "key file for tls connections");
 ABSL_FLAG(string, tls_ca_cert_file, "", "ca signed certificate to validate tls connections");
 ABSL_FLAG(string, tls_ca_cert_dir, "", "ca signed certificates directory");
+ABSL_FLAG(uint32_t, tcp_keepalive, 300, "the period in seconds used to send ACKs to client");


I think the description of this flag is not accurate.

We start sending acks after the idle time that is bound to the value of this flag.,

We give up on the connection if we have not got any ack response during the additional period that equals to the same value.

So it's worth to mention that it's a period of inactivity after which keep-alives are triggerred and the total time until the dead connection is closed is twice the specified value.

Signed-off-by: adi_holden <adi@dragonflydb.io>

adiholden requested a review from chakaz August 2, 2023 12:10

chakaz reviewed Aug 2, 2023

View reviewed changes

adiholden added 4 commits August 6, 2023 14:08

feat(server): support config set for some flags

07bbc6e

1. support config set for the following flags: rename_command,dir,requirepass,masterauth 2. more print details when failing on dbfilename flag validation Signed-off-by: adi_holden <adi@dragonflydb.io>

add tcp_keepalive and add it to config set

87a9428

Signed-off-by: adi_holden <adi@dragonflydb.io>

fix pr

8a21890

Signed-off-by: adi_holden <adi@dragonflydb.io>

fix pr

25c82f1

Signed-off-by: adi_holden <adi@dragonflydb.io>

adiholden force-pushed the config_set_some_flags branch from ee525df to 25c82f1 Compare August 6, 2023 11:18

adiholden requested a review from chakaz August 6, 2023 11:19

chakaz reviewed Aug 6, 2023

View reviewed changes

adiholden added 6 commits August 8, 2023 23:16

remove rename command changes

37d3d26

Signed-off-by: adi_holden <adi@dragonflydb.io>

tcp_keepalive flag set fix

b0b2b5e

Signed-off-by: adi_holden <adi@dragonflydb.io>

fix comment on flag

f56a3ed

Signed-off-by: adi_holden <adi@dragonflydb.io>

remove redundant change

3e2777b

Signed-off-by: adi_holden <adi@dragonflydb.io>

small fix

27831d7

Signed-off-by: adi_holden <adi@dragonflydb.io>

small fix

a8cda31

Signed-off-by: adi_holden <adi@dragonflydb.io>

adiholden requested review from romange and chakaz August 8, 2023 20:40

romange previously approved these changes Aug 9, 2023

View reviewed changes

romange reviewed Aug 9, 2023

View reviewed changes

fix flag documentation

c4cf18d

Signed-off-by: adi_holden <adi@dragonflydb.io>

adiholden dismissed romange’s stale review via c4cf18d August 9, 2023 10:23

adiholden requested a review from romange August 9, 2023 10:23

chakaz approved these changes Aug 9, 2023

View reviewed changes

adiholden merged commit 6d84515 into main Aug 9, 2023
10 checks passed

adiholden deleted the config_set_some_flags branch August 9, 2023 11:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(server): support config set for some flags #1624

feat(server): support config set for some flags #1624

adiholden commented Aug 2, 2023 •

edited

Loading

chakaz Aug 2, 2023

chakaz Aug 2, 2023

adiholden Aug 3, 2023

chakaz Aug 2, 2023

chakaz Aug 2, 2023

romange commented Aug 3, 2023

chakaz commented Aug 3, 2023

chakaz Aug 6, 2023

romange Aug 6, 2023

adiholden Aug 8, 2023

adiholden commented Aug 7, 2023

adiholden commented Aug 7, 2023

romange commented Aug 7, 2023

chakaz commented Aug 7, 2023

adiholden commented Aug 8, 2023

romange commented Aug 9, 2023

romange commented Aug 9, 2023

romange Aug 9, 2023

		string_view origin_name = AsciiStrToUpper(kv.first);
		string_view new_name = AsciiStrToUpper(kv.second);

feat(server): support config set for some flags #1624

feat(server): support config set for some flags #1624

Conversation

adiholden commented Aug 2, 2023 • 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

romange commented Aug 3, 2023

chakaz commented Aug 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adiholden commented Aug 7, 2023

adiholden commented Aug 7, 2023

romange commented Aug 7, 2023

chakaz commented Aug 7, 2023

adiholden commented Aug 8, 2023

romange commented Aug 9, 2023

romange commented Aug 9, 2023

Choose a reason for hiding this comment

adiholden commented Aug 2, 2023 •

edited

Loading