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.73%. Comparing base (3c32ee1) to head (fc31906).
Report is 47 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.73%   +0.07%     
============================================
  Files           114      116       +2     
  Lines         63158    63312     +154     
============================================
+ Hits          44624    44781     +157     
+ Misses        18534    18531       -3     

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

... and 33 files with indirect coverage changes

---- 🚨 Try these New Features:

• Flaky Tests Detection - Detect and resolve failed and flaky tests
• JS Bundle Analysis - Avoid shipping oversized bundles

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

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

[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) */

[hwware]

Sorry, it looks like on that time, my brain is offline. Yes, you are right, it is int, not pointer. The condition here is useless, I should remove it.

[zuiderkwast]

Check that ValkeyModule_UpdateRuntimeArgs returns VALKEYMODULE_OK here.

[hwware]

Update to: return ValkeyModule_UpdateRuntimeArgs(ctx, argv, argc);

[zuiderkwast]

Yes, or

Suggested change

	ValkeyModule_UpdateRuntimeArgs(ctx, argv, argc);
	int res = ValkeyModule_UpdateRuntimeArgs(ctx, argv, argc);
	assert(res == VALKEYMODULE_OK);

[zuiderkwast]

The command still needs to send a reply to the client, right?

[hwware]

I just have one statement as return ValkeyModule_ReplyWithSimpleString(ctx, "OK");

[zuiderkwast]

It should return OK on success, right?

Suggested change

	r testmoduleparameter.update.parameter 40 50 60 70
	assert_equal OK [r testmoduleparameter.update.parameter 40 50 60 70]

[zuiderkwast]

Or maybe it returns the string "Module runtime args test", the reply sent by this command in the previous commits.

Suggested change

	r testmoduleparameter.update.parameter 40 50 60 70
	assert_equal "Module runtime args test" [r testmoduleparameter.update.parameter 40 50 60 70]

... but OK seems more normal I think 😁

[hwware]

I update this part as assert_equal OK [r testmoduleparameter.update.parameter 40 50 60 70]

[hwware]

I update some logic in module.c, now it works well. @zuiderkwast