Add API UpdateRuntimeArgs to allow the module arguments during runtime and save the new arguments value into the conf file

[hwware]

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

[codecov]

Codecov Report

Attention: Patch coverage is 8.33333% with 11 lines in your changes missing coverage. Please review.

Project coverage is 70.69%. Comparing base (3c32ee1) to head (a117ce9).
Report is 9 commits behind head on unstable.

Files with missing lines	Patch %	Lines
src/module.c	8.33%	11 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #1041      +/-   ##
============================================
+ Coverage     70.65%   70.69%   +0.03%     
============================================
  Files           114      114              
  Lines         63158    63159       +1     
============================================
+ Hits          44624    44649      +25     
+ Misses        18534    18510      -24     

Files with missing lines	Coverage Δ
src/module.c	9.66% <8.33%> (-0.01%)	⬇️

... and 18 files with indirect coverage changes

[zuiderkwast]

Looks simple, but what's the use case? Why does a module need to update args in runtime?

A module can already have config that is updated using CONFIG SET, right?

[hwware]

@zuiderkwast I update the top description for this PR, i think this is a good way to access the updated module arguments. Pls take a look when you have time, Thanks a lot

[zuiderkwast]

You added a module API to access the argv. Interesting.

But, I am not convinced. :)

In a program written in C, the arguments are passed to main as int main(int argc, char **argv). It's not possible to change these args later, from outside the program, while the program is running.

The argv for MODULE LOAD is the same. If you set the arguments with MODULE SET-ARGUMENTS later, the module was still loaded with the old arguments and most modules (or new modules like valkey-bloom) will not notice the arguments have changed. But there will be a problem. In the log, it looks like the module is using the new arguments, even if it is using the old arguments.

MODULE UNLOAD + MODULE LOAD doesn't have this problem.

[hwware]

You added a module API to access the argv. Interesting.

But, I am not convinced. :)

In a program written in C, the arguments are passed to main as int main(int argc, char **argv). It's not possible to change these args later, from outside the program, while the program is running.

The argv for MODULE LOAD is the same. If you set the arguments with MODULE SET-ARGUMENTS later, the module was still loaded with the old arguments and most modules (or new modules like valkey-bloom) will not notice the arguments have changed. But there will be a problem. In the log, it looks like the module is using the new arguments, even if it is using the old arguments.

MODULE UNLOAD + MODULE LOAD doesn't have this problem.

Thanks for your comment, but some cons for operation MODULE UNLOAD + MODULE LOAD

1. some modules maybe not 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
4. through the way MODULE UNLOAD + MODULE LOAD to update module runtime args, the management plan and data plan software need to be updated as well.

I would like to create an issue to let community to discuss this problem. Thanks

[hwware]

@madolson As we discussed, I desgin the API as int VM_UpdateRuntimeArgs(ValkeyModuleCtx *ctx, int argc, ValkeyModuleString **argv), please take a look, Thanks

[zuiderkwast]

Looks good to me. Only documentation comment missing.

What about VM_GetRuntimeArgs, is it not needed?

[zuiderkwast]

Please add a comment. It is used for generated documentation. Without a comment, the function will not be included in the API documentation. The comment needs to use correct markdown syntax.

You can try the ruby script to generate the documentation markdown: utils/generate-module-api-doc.rb. (Later, we run the script and save the output in the valkey-doc repo in the file topics/module-api-ref.md.)

[hwware]

Addressed, add comments already

[zuiderkwast]

Good, but I have some small comments:

module->loadmod is something internal in Valkey. Module authors are not aware of this. Let's assume module authors are know about the API documentation and valkeymodule.h.

Usually we write commands in uppercase, like CONFIG REWRITE.

Dot is missing after "is called".

Is the example in src/modules/ or tests/modules/ ? We have both, so it's better to be specific. Also it can be good mention that that it is in the Valkey source code, because the documentation is published in other places and module authors are maybe not even looking at valkey source code.

[zuiderkwast]

Sorry for many small reviews. I was going to click Approve, but then I read the test case. more carefully and found some strange function and command names. :)

[zuiderkwast]

"myhello"?

Maybe "moduleparameter" is more descriptive?

[zuiderkwast]

Command name "hello.hi" and function name GET_HELLO are not perfect names. :)

Something like "updateargs", updateArgsCommand?

[hwware]

Thanks for your comments, it is updated

[madolson]

Suggested change

	ValkeyModule_UpdateRuntimeArgs(ctx, argv, argc);
	return ValkeyModule_ReplyWithSimpleString(ctx, "Module runtime args test");
	ValkeyModule_UpdateRuntimeArgs(ctx, argv, argc);
	return ValkeyModule_ReplyWithSimpleString(ctx, "Module runtime args test");

I guess we should probably apply the formatter on these files as well.

[madolson]

We don't normally reference the test modules since these are rendered here: https://valkey.io/topics/modules-api-ref/, which doesn't clearly map to the test files anymore. I'm not sure it's really needed to show an example.

[hwware]

It looks like no specific test file is mentioned in reference, I will remove it.

[madolson]

Why is this only allowed at startup? I thought the whole point was they were able to update these at any point in time.

[zuiderkwast]

Good point. I'm surprised the test case passes.

[hwware]

The condition "!ctx->module->onload" is checked here is not like what you thought it can be only allowed to update at startup, the goal is to check if ctx->module->onload is NULL to prevent NULL pointer exception. This API can be called at any time during runtime.

[zuiderkwast]

I think it means that onload is ongoing. If it doesn't, then there is something wrong.

I tried to follow the function calls that happen when a module is loaded. This is what I could find:

• MODULE LOAD -> moduleCommand -> moduleLoad.
• moduleLoad loads the module file using dlopen.
• moduleLoad finds the ValkeyModule_OnLoad function in the module using dlsym.
• moduleLoad calls the ValkeyModule_OnLoad in the module.
• The module's ValkeyModule_OnLoad function calls ValkeyModule_Init.
• ValkeyModule_Init (implemented in valkeymodule.h) calls ValkeyModule_SetModuleAttribs.
• VM_SetModuleAttribs creates the struct ctx->module and sets ctx->module->onload = 1.
• The ValkeyModule_OnLoad function calls other functions, such as ValkeyModule_CreateCommand
• The ValkeyModule_OnLoad function returns.
• moduleLoad logs serverLog(LL_NOTICE, "Module '%s' loaded from %s", ctx.module->name, path);
• moduleLoad sets ctx.module->onload = 0;
• This ctx is freed, but the module object is added to a dict.

There is no other place that onload is set. That means onload = 1 only when ValkeyModule_OnLoad is running.

[zuiderkwast]

The onload field in ValkeyModule is not a pointer. It is an int.

int onload;                           /* Flag to identify if the call is being made from Onload (0 or 1) */

[zuiderkwast]

The Redis module API is just for backward compatibilty. We don't add functions to that API.

Suggested change

#define RedisModule_UpdateRuntimeArgs ValkeyModule_UpdateRuntimeArgs

[zuiderkwast]

Check that ValkeyModule_UpdateRuntimeArgs returns VALKEYMODULE_OK here.

[zuiderkwast]

This is not an assert. It will silently fail. It just returns 0 and the return value is ignored here.