A way for in-place upgrade of a module (old version pass state to new version)

[sundb]

Now, if we need to upgrade a module, we need to module unload first, and then module load.
But module unload will have some edge cases, which will cause crashes or illegal memory address references.
In-place upgrades may solve this set of problems.
Ref: #4826, #10259

We will disable in-place upgrades under the following conditions, just as module unload does.

• module exports one or more module-side data types.
• module exports APIs used by other modules.
• module has blocked clients.
• module holds timer that is not fired.

Another question, if any command has been modified, do we need to disable the in-place upgrade.

• Add a new command
• Delete an old command
• Modify the parameters of any old command

[oranagra]

maybe we should allow upgrades for modules with registered data types,
maybe the old module should have a way to pass some struct with it's global pointers to the new module?

[sundb]

@oranagra But what if the data struct of data types have been modified(i.g add new filed)?
At this point, reading the value of the old data type will be out of memory.

[oranagra]

I think we should let the modules handle it (in many cases there's a bug fix in the code, but no changes in the data structures).
And even if there is a change in the data structure, maybe the new module can handle both the new and old types.
Maybe allow it only for modules that declare they can handle it.
I.e. either ones that set some "option" flag, or ones the implement the globals hand-off API i suggested.

[chenyang8094]

IIUC, we are talking about modules hot upgrade?

[sundb]

@chenyang8094 Yes.

[chenyang8094]

I'm not sure what scenarios require it, but in our production environment, both module and redis images are bundled and released, and we don't use moudle unload and module load to upgrade modules individually. Instead, we will use the new version redis+module as the old version redis' replica, and do a switch over after waiting for the data synchronization to complete.

For the module hot upgrade of the running redis, I think there will be many problems (such as address resolution, reconstruction of variables in memory, causing RT jitter, etc.). Currently, we also do not allow unloading of a module that exports data types.

So, I don't think it's a strong requirement (maybe only for those modules that don't export data types), of course it would be best if it could be supported, but I think it would cost a lot.

[sundb]

For the module hot upgrade of the running redis, I think there will be many problems (such as address resolution, reconstruction of variables in memory, causing RT jitter, etc.). Currently, we also do not allow unloading of a module that exports data types.

Yep, This is the most problematic part of it, so we need to limit the use of this feature.
For example, Restrictions on modifying the data structure of an old module, but require the module developer to know
about it, otherwise it would be dangerous, which is exactly what @oranagra mentioned about only allowing modules with
options like REDISMODULE_OPTIONS_ALLOW_RELOAD to be upgraded.

[chenyang8094]

This may be a paradox, usually when a module can use REDISMODULE_OPTIONS_ALLOW_RELOAD it means that it is not very complicated(so there is very little chance it needs a hot update), conversely, a slightly more complicated module is unlikely to use REDISMODULE_OPTIONS_ALLOW_RELOAD. I think REDISMODULE_OPTIONS_ALLOW_RELOAD will bring a lot of challenges to developers(use or not), and it can easily lead to online disasters and be difficult to debug.

[sundb]

@chenyang8094 Agree with you.
There are still some uses for this feature, such as fixing a bug in a small piece of code, when using hot-upgrade does not require such a costly data migration as repl.
Need more people's opinions.

[oranagra]

yes, hot upgrade is complicated, and could in some cases be useful.
UNLOAD, turns out to be complicated too (i could argue that even more complicated than RELOAD as we've seen in #10259).

maybe we should drop UNLOAD support...

[yossigo]

Modules vary greatly, so it's hard to refer to them as one thing.

For a module with no data types, an in-place upgrade using UNLOAD followed by LOAD makes sense to me.
For modules with data types, I think I'd avoid that anyway and take the Redis way of upgrading by failing over to a replica, etc.

Because of that, I'm not in favor of RELOAD which will end up more complex (both in terms of API, and surely giving way too much rope for module developers).

[oranagra]

so when UNLOAD was created, it meant to reject cases where redis has pointers to the module's allocations.
registered commands turns out to be one of them, so rejecting this case, means rejecting all UNLOAD attempts.