Skip to content

Modules: rewrite loadmodule and extend LIST reply #4848

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.

Sign up for GitHub

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

Jump to bottom
Merged
oranagra merged 4 commits into from
Jun 1, 2021
Merged

Modules: rewrite loadmodule and extend LIST reply #4848

oranagra merged 4 commits into from
Jun 1, 2021

Conversation

@soloestoy
Copy link
Contributor

@soloestoy soloestoy commented Apr 16, 2018 •
edited
Loading

This PR contains three changes:

  1. CONFIG REWRITE command will rewrite modules as loadmodule options into config file, in case someone uses MODULE LOAD or UNLOAD commands in runtime.
  2. free elements in server.loadmodule_queue after module loaded from config file or UNLOAD to save memory.
  3. show path and args in MODULE LIST reply.

charsyam
charsyam suggested changes Apr 19, 2018
View reviewed changes
@soloestoy
Copy link
Contributor Author

soloestoy commented Apr 20, 2018

This PR contains two features:

  1. After load modules from queue generated by config "loadmodule" option, we can empty the queue to free unnecessary memory.

  2. Rewrite config "loadmodule" option is necessary I think, thus we can load all modules after reboot.

What do you think about it @antirez ?

@soloestoy soloestoy changed the title Modules: free node after module loaded from server.loadmodule_queue Modules: free and rewrite loadmodule Apr 20, 2018
@antirez
Copy link
Contributor

antirez commented May 31, 2018

Thank you, PR checked, everything looks sane but I've to think a bit about the fact of rewriting the config. Probably a good idea but just taking a few days to consider this. Going to merge for RC2 for sure. Thanks.

@soloestoy soloestoy mentioned this pull request Sep 30, 2018
Closed
@oranagra
Copy link
Member

oranagra commented May 4, 2021

IIUC, the aim here is that if someone uses the MODULE LOAD or MODULE UNLOAD commands, and then CONFIG REWRITE, the new module list will be reflected in the generated config file, right?
i suppose that's logical (same thing happens with ACL), since it's a behaviour change, we need to introduce it in 7.0.
@soloestoy can you please rebase / refresh this, and update the top comment?
@yossigo @madolson do you see any problem with that?

@yossigo
Copy link
Collaborator

yossigo commented May 4, 2021

@oranagra No, I think that makes sense.

@madolson
Copy link
Contributor

madolson commented May 4, 2021

Sounds good to me as well.

@soloestoy soloestoy force-pushed the free-loadmodule-queue branch from 96ea2bf to f389a02 Compare May 10, 2021 11:57
@soloestoy
Copy link
Contributor Author

soloestoy commented May 10, 2021

I just rebased this PR.

BTW, do you think we should show the path and args in MODULE LIST command's reply? I didn't push the original commit, if you think it's necessary I can push it.

@oranagra oranagra added release-notes indication that this issue needs to be mentioned in the release notes state:major-decision Requires core team consensus labels May 10, 2021
@oranagra
Copy link
Member

oranagra commented May 10, 2021

i think it can be nice to add the full path and args to the MODULE LIST response with more fields.
it can be part of this PR, or a different ones (both are major decision anyway).

@soloestoy soloestoy changed the title Modules: free and rewrite loadmodule Modules: rewrite loadmodule and extend LIST reply May 12, 2021
oranagra
oranagra previously approved these changes May 12, 2021
View reviewed changes
@oranagra oranagra added the approval-needed Waiting for core team approval to be merged label May 12, 2021
madolson
madolson previously approved these changes May 13, 2021
View reviewed changes
itamarhaber
itamarhaber previously approved these changes May 19, 2021
View reviewed changes
@oranagra oranagra added the state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten label May 19, 2021
@oranagra
Copy link
Member

oranagra commented May 27, 2021

@soloestoy shall we merge this with a rebase-merge or a squash merge?
i see 3 individual commits and 3 distinct topics, so if you can confirm they're indeed completely independent, we can use rebase-merge.

also, i'd like to add a test for the CONFIG REWRITE part.
do you wanna do that as part of this PR, or shall we ask for some volunteer after it is merged?

@oranagra oranagra dismissed stale reviews from itamarhaber, madolson, and themself via 39e91fb June 1, 2021 09:52
@oranagra oranagra merged commit 7cb42c9 into redis:unstable Jun 1, 2021
@oranagra oranagra mentioned this pull request Oct 10, 2021
Closed
@oranagra oranagra mentioned this pull request May 23, 2022
Merged
@vikram-krishna-s
Copy link

vikram-krishna-s commented Apr 4, 2023

Hi @soloestoy!

I maintain a secondary.confconfiguration file for loadmodule and other configurations and include them it in the primary.conf file which I use to start the redis-server (Redis version=7.0.10).

The primary.conf and secondary.conf look like this,

primary.conf

...
include /home/vikram/redis7/secondary.conf
...

secondary.conf

...
loadmodule /home/vikram/redis7/aclcheck.so
...
  1. I did not encounter any errors when I first started the redis-server withprimary.conf.

redis7>./redis-server ./primary.conf

10755:C 04 Apr 2023 15:16:09.690 # oO0OoO0OoO0Oo Redis is starting oO0OoO0OoO0Oo
10755:C 04 Apr 2023 15:16:09.690 # Redis version=7.0.10, bits=64, commit=00000000, modified=0, pid=10755, just started
10755:C 04 Apr 2023 15:16:09.690 # Configuration loaded
10755:M 04 Apr 2023 15:16:09.692 * monotonic clock: POSIX clock_gettime
10755:M 04 Apr 2023 15:16:09.693 # Server initialized
10755:M 04 Apr 2023 15:16:09.694 * Module 'aclcheck' loaded from /home/vikram/redis7/aclcheck.so
10755:M 04 Apr 2023 15:16:09.695 * Ready to accept connections
  1. Then after executing CONFIG REWRITE , the primary.conf file was appended with 'loadmodule /home/vikram/redis7/aclcheck.so' as said in Modules: rewrite loadmodule and extend LIST reply #4848.

redis7>./redis-cli CONFIG REWRITE

primary.conf file after CONFIG REWRITE

include /home/vikram/redis7/secondary.conf

# Generated by CONFIG REWRITE
...
loadmodule /home/vikram/redis7/aclcheck.so
...
  1. Now on redis-server restart, I am facing the below error because during startup there are two loadmodule directives for the same module, one from secondary.conf and one from primary.conf.
redis7> ./redis-server ./primary.conf

11671:C 04 Apr 2023 15:17:37.469 # oO0OoO0OoO0Oo Redis is starting oO0OoO0OoO0Oo
11671:C 04 Apr 2023 15:17:37.469 # Redis version=7.0.10, bits=64, commit=00000000, modified=0, pid=11671, just started
11671:C 04 Apr 2023 15:17:37.469 # Configuration loaded
11671:M 04 Apr 2023 15:17:37.470 * monotonic clock: POSIX clock_gettime
11671:M 04 Apr 2023 15:17:37.472 # Server initialized
11671:M 04 Apr 2023 15:17:37.473 * Module 'aclcheck' loaded from /home/vikram/redis7/aclcheck.so
11671:M 04 Apr 2023 15:17:37.473 # Module /home/vikram/redis7/aclcheck.so initialization failed. Module not loaded
11671:M 04 Apr 2023 15:17:37.473 # Can't load module from /home/vikram/redis7/aclcheck.so: server aborting

@vikram-krishna-s vikram-krishna-s mentioned this pull request Apr 7, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oranagra oranagra oranagra approved these changes

@itamarhaber itamarhaber itamarhaber left review comments

+2 more reviewers

@charsyam charsyam charsyam requested changes

@madolson madolson madolson left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

approval-needed Waiting for core team approval to be merged release-notes indication that this issue needs to be mentioned in the release notes state:major-decision Requires core team consensus state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten

Projects

Redis 7.0
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@soloestoy @antirez @oranagra @yossigo @madolson @vikram-krishna-s @charsyam @itamarhaber