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

[NEW]Update the module args during runtime and save the new args value into the conf file #1177

Closed
hwware opened this issue Oct 16, 2024 · 4 comments · Fixed by #1041
Closed
Labels
major-decision-approved Major decision approved by TSC team

Comments

@hwware
Copy link
Member

hwware commented Oct 16, 2024

The problem/use-case that the feature addresses
Before Redis OSS 7, if we load a module with some args during runtime,
and run the command "config rewrite", the module information will not be saved into the
conf file.

Since Redis OSS 7 and Valkey 7.2, if we load a module with some args during runtime,
the module information (path, args number, and args value) can be saved into the conf file after config rewrite command is called.
Thus the module will be loaded automatically when the server startup next time.

Following is one example:

bind 172.25.0.58
port 7000
protected-mode no
enable-module-command yes

Generated by CONFIG REWRITE
latency-tracking-info-percentiles 50 99 99.9
dir "/home/ubuntu/valkey"
save 3600 1 300 100 60 10000
user default on nopass sanitize-payload ~* &* +@ALL
loadmodule tests/modules/datatype.so 10 20

However there is one problem.
If developers write a module, and update the running args by someway, the updated args can not be saved into the conf file even "config rewrite" is called.
The reason comes from the following function
rewriteConfigLoadmoduleOption (src/config.c)

void rewriteConfigLoadmoduleOption(struct rewriteConfigState *state) {
..........
struct ValkeyModule *module = dictGetVal(de);
line = sdsnew("loadmodule ");
line = sdscatsds(line, module->loadmod->path);
for (int i = 0; i < module->loadmod->argc; i++) {
line = sdscatlen(line, " ", 1);
line = sdscatsds(line, module->loadmod->argv[i]->ptr);
}
rewriteConfigRewriteLine(state, "loadmodule", line, 1);
.......
}

The function only save the initial args information (module->loadmod) into the conf file.

Description of the feature

There are 2 ways to solve the problem:

Solution 1: It needs to take 2 steps. (This option is not what we choose)

step 1: client calls command: MODULE SET-ARGUMENT <module_name> [args...] to update the args (module->loadmod) during runtime
step 2: client gets the updated args (module->loadmod) in the module through the new API ValkeyModule_GetRunTimeArgs(ctx);
Reference: #1041

Solution 2: it only takes 1 step

Call int VM_UpdateRunTimeArgs(ValkeyModuleCtx *ctx, int index, char *value) API in the module and update the specific args according to the index (module->loadmod).
Reference: hwware#8

Thus, I think the best way is to add a Module API to implement the above 2 features.

Alternatives you've considered

Administrators or developer could update the module args by the following way:

MODULE UNLOAD
Update module args in the conf file
MODULE LOAD

But I think there are following disadvantages:

  1. some modules can not be unloaded. Such as the example module datatype.so, which is tests/modules/datatype.so
  2. it is not atomic operation for MODULE UNLOAD + MODULE LOAD
  3. sometimes, if we just run the module unload, the client business could be interrupted

Additional information

Any additional information that is relevant to the feature request.

@hwware hwware added the major-decision-pending Major decision pending by TSC team label Oct 16, 2024
@hwware
Copy link
Member Author

hwware commented Oct 16, 2024

@wuranxx
Copy link

wuranxx commented Oct 16, 2024

In Redis7, a new module configuration mode is added. This mode seems to be more readable and elegant than adding parameters after loadmodule. In addition, this mode can control module configuration like the native Redis configuration.redis/redis#10285

Can this feature replace the way that add parameters after loadmodules? Do we recommend adding module parameters in a new way in valkey?

@hwware
Copy link
Member Author

hwware commented Oct 17, 2024

one of solutions is to add a module API to allow module developer to update the args as below:

ValkeyModule_UpdateRunTimeArgs(ValkeyModuleCtx *ctx, int index, char *value)
index is the arg index, and value is new args value

The example is here hwware#8

@hwware hwware changed the title [NEW]Add a Module API to update the module args and save the new value into the conf file [NEW]Update the module args during runtime and save the new args value into the conf file Oct 24, 2024
@madolson
Copy link
Member

Core meeting notes:
We all agree about solution 2, letting the module update it's arguments, making the most sense. There was a discussion about the API, whether or not implement get APIs as well as have a single API to replace all of them.

Original proposal:

int VM_UpdateRunTimeArgs(ValkeyModuleCtx *ctx, int index, char *value);

Updated proposal:

RedisModuleString **values VM_GetRuntimeArgs(ValkeyModuleCtx *ctx);
int VM_UpdateRuntimeArgs(ValkeyModuleCtx *ctx, int argc, RedisModuleString **values);

Either option is OK. We'll decide in the PR which one to take.

@madolson madolson added major-decision-approved Major decision approved by TSC team and removed major-decision-pending Major decision pending by TSC team labels Oct 28, 2024
hwware added a commit that referenced this issue Dec 5, 2024
…ntime (#1041)

Before Redis OSS 7, if we load a module with some arguments during
runtime,
and run the command "config rewrite", the module information will not be
saved into the
config file.

Since Redis OSS 7 and Valkey 7.2, if we load a module with some
arguments during runtime,
the module information (path, arguments number, and arguments value) can
be saved into the config file after config rewrite command is called.
Thus, the module will be loaded automatically when the server startup
next time.

Following is one example:

bind 172.25.0.58
port 7000
protected-mode no
enable-module-command yes

Generated by CONFIG REWRITE
latency-tracking-info-percentiles 50 99 99.9
dir "/home/ubuntu/valkey"
save 3600 1 300 100 60 10000
user default on nopass sanitize-payload ~* &* +https://github.com/ALL
loadmodule tests/modules/datatype.so 10 20

However, there is one problem.
If developers write a module, and update the running arguments by
someway, the updated arguments can not be saved into the config file
even "config rewrite" is called.
The reason comes from the following function
rewriteConfigLoadmoduleOption (src/config.c)

void rewriteConfigLoadmoduleOption(struct rewriteConfigState *state) {
..........
struct ValkeyModule *module = dictGetVal(de);
line = sdsnew("loadmodule ");
line = sdscatsds(line, module->loadmod->path);
for (int i = 0; i < module->loadmod->argc; i++) {
line = sdscatlen(line, " ", 1);
line = sdscatsds(line, module->loadmod->argv[i]->ptr);
}
rewriteConfigRewriteLine(state, "loadmodule", line, 1);
.......
}

The function only save the initial arguments information
(module->loadmod) into the configfile.

After core members discuss, ref
#1177


We decide add the following API to implement this feature:

Original proposal:

int VM_UpdateRunTimeArgs(ValkeyModuleCtx *ctx, int index, char *value);


Updated proposal:

ValkeyModuleString **values VM_GetRuntimeArgs(ValkeyModuleCtx *ctx);
**int VM_UpdateRuntimeArgs(ValkeyModuleCtx *ctx, int argc,
ValkeyModuleString **values);



Why we do not recommend the following way: 


MODULE UNLOAD
Update module args in the conf file
MODULE LOAD

I think there are the following disadvantages:

1. Some modules can not be unloaded. Such as the example module
datatype.so, which is tests/modules/datatype.so
2. it is not atomic operation for MODULE UNLOAD + MODULE LOAD
3. sometimes, if we just run the module unload, the client business
could be interrupted

---------

Signed-off-by: hwware <[email protected]>
vudiep411 pushed a commit to Autxmaton/valkey that referenced this issue Dec 15, 2024
…ntime (valkey-io#1041)

Before Redis OSS 7, if we load a module with some arguments during
runtime,
and run the command "config rewrite", the module information will not be
saved into the
config file.

Since Redis OSS 7 and Valkey 7.2, if we load a module with some
arguments during runtime,
the module information (path, arguments number, and arguments value) can
be saved into the config file after config rewrite command is called.
Thus, the module will be loaded automatically when the server startup
next time.

Following is one example:

bind 172.25.0.58
port 7000
protected-mode no
enable-module-command yes

Generated by CONFIG REWRITE
latency-tracking-info-percentiles 50 99 99.9
dir "/home/ubuntu/valkey"
save 3600 1 300 100 60 10000
user default on nopass sanitize-payload ~* &* +https://github.com/ALL
loadmodule tests/modules/datatype.so 10 20

However, there is one problem.
If developers write a module, and update the running arguments by
someway, the updated arguments can not be saved into the config file
even "config rewrite" is called.
The reason comes from the following function
rewriteConfigLoadmoduleOption (src/config.c)

void rewriteConfigLoadmoduleOption(struct rewriteConfigState *state) {
..........
struct ValkeyModule *module = dictGetVal(de);
line = sdsnew("loadmodule ");
line = sdscatsds(line, module->loadmod->path);
for (int i = 0; i < module->loadmod->argc; i++) {
line = sdscatlen(line, " ", 1);
line = sdscatsds(line, module->loadmod->argv[i]->ptr);
}
rewriteConfigRewriteLine(state, "loadmodule", line, 1);
.......
}

The function only save the initial arguments information
(module->loadmod) into the configfile.

After core members discuss, ref
valkey-io#1177


We decide add the following API to implement this feature:

Original proposal:

int VM_UpdateRunTimeArgs(ValkeyModuleCtx *ctx, int index, char *value);


Updated proposal:

ValkeyModuleString **values VM_GetRuntimeArgs(ValkeyModuleCtx *ctx);
**int VM_UpdateRuntimeArgs(ValkeyModuleCtx *ctx, int argc,
ValkeyModuleString **values);



Why we do not recommend the following way: 


MODULE UNLOAD
Update module args in the conf file
MODULE LOAD

I think there are the following disadvantages:

1. Some modules can not be unloaded. Such as the example module
datatype.so, which is tests/modules/datatype.so
2. it is not atomic operation for MODULE UNLOAD + MODULE LOAD
3. sometimes, if we just run the module unload, the client business
could be interrupted

---------

Signed-off-by: hwware <[email protected]>
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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants