[Bug 28276] New: Do not fetch config ($KOHA_CONF) from memcached
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28276 Bug ID: 28276 Summary: Do not fetch config ($KOHA_CONF) from memcached Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: ASSIGNED Severity: normal Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: julian.maurice@biblibre.com Reporter: julian.maurice@biblibre.com QA Contact: testopia@bugs.koha-community.org memcached address and namespace are in $KOHA_CONF, so it is required to read it before being able to access the cache. And after that, configuration is kept in memory forever. Storing this in memcached is useless and even counter-productive, since Koha reads both the file and the cache. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28276 --- Comment #1 from Julian Maurice <julian.maurice@biblibre.com> --- Created attachment 120352 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=120352&action=edit Bug 28276: Do not fetch config ($KOHA_CONF) from memcached memcached address and namespace are in $KOHA_CONF, so it is required to read it before being able to access the cache. And after that, configuration is kept in memory forever. Storing this in memcached is useless and even counter-productive, since Koha reads both the file and the cache This patch addresses this issue by removing the cache-related code from C4::Context->new. It means that C4::Context->new will always read the configuration file, so this patch also replaces inappropriate calls to C4::Context->new->config by appropriate calls to C4::Context->config It also fixes a bug where C4::Context->new would ignore the filepath given in parameters if there was something in cache. It also removes a problematic call to Koha::Caches->get_instance. Because this call was outside of any subroutine, it would have happened before the initialization of $C4::Context::context (which happen in C4::Context::import) Test plan: 1. Do not apply the patch yet 2. Add the following line at the beginning of Koha::Config::read_from_file warn "read_from_file($file)"; This will allow you to check how many times the file is read. 3. Flush memcached and restart starman 4. Check the logs, you should see "read_from_file" a bunch of times 5. Apply the patch 6. Re-add the line from step 2 7. Flush memcached and restart starman 8. Check the logs, you should see "read_from_file" only once 9. Make sure the memcached config from $KOHA_CONF (memcached_servers, memcached_namespace) is taken into account by checking the About page -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28276 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28276 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28276 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |victor@tuxayo.net --- Comment #2 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Error: «Starting Z39.50/SRU daemon for kohadev:Global symbol "$file" requires explicit package name (did you forget to declare "my $file"?) at /kohadevbox/koha/Koha/Config.pm line 36.» I guess there should be simple quotes right? Test results: Is that what should be expected? (less reads but still multiple) ==================== Before patch ======================== ==================== restart ============================= root@kohadevbox:kohadevbox$ restart_all Restarting Apache httpd web server: apache2. Restarting Koha ILS: koha-commonStopping Plack daemon for kohadev:. Error: Z39.50/SRU not running for kohadev: failed! Stopping Koha worker daemon for kohadev:. Stopping Koha indexing daemon for kohadev:. Starting Plack daemon for kohadev:. Starting Z39.50/SRU daemon for kohadev:read_from_file($file)####################### at /kohadevbox/koha/Koha/Config.pm line 36. read_from_file($file)####################### at /kohadevbox/koha/Koha/Config.pm line 36. read_from_file($file)####################### at /kohadevbox/koha/Koha/Config.pm line 36. . Starting Koha worker daemon for kohadev:. Starting Koha indexing daemon for kohadev:. ==================== logs ======================== ==> /var/log/koha/kohadev/plack-error.log <== read_from_file($file)####################### at /kohadevbox/koha/Koha/Config.pm line 36. read_from_file($file)####################### at /kohadevbox/koha/Koha/Config.pm line 36. read_from_file($file)####################### at /kohadevbox/koha/Koha/Config.pm line 36. read_from_file($file)####################### at /kohadevbox/koha/Koha/Config.pm line 36. ==> /var/log/koha/kohadev/sip-output.log <== read_from_file($file)####################### at /kohadevbox/koha/Koha/Config.pm line 36. read_from_file($file)####################### at /kohadevbox/koha/Koha/Config.pm line 36. read_from_file($file)####################### at /kohadevbox/koha/Koha/Config.pm line 36. ==> /var/log/koha/kohadev/indexer-output.log <== read_from_file($file)####################### at /kohadevbox/koha/Koha/Config.pm line 36. read_from_file($file)####################### at /kohadevbox/koha/Koha/Config.pm line 36. ==> /var/log/koha/kohadev/worker-output.log <== read_from_file($file)####################### at /kohadevbox/koha/Koha/Config.pm line 36. read_from_file($file)####################### at /kohadevbox/koha/Koha/Config.pm line 36. ==> /var/log/koha/kohadev/indexer-output.log <== read_from_file($file)####################### at /kohadevbox/koha/Koha/Config.pm line 36. ==================== After patch ======================== ==================== restart ============================= root@kohadevbox:kohadevbox$ restart_all Restarting Apache httpd web server: apache2. Restarting Koha ILS: koha-commonStopping Plack daemon for kohadev:. Stopping Z39.50/SRU daemon for kohadev:. Stopping Koha worker daemon for kohadev:. Stopping Koha indexing daemon for kohadev:. Starting Plack daemon for kohadev:. Starting Z39.50/SRU daemon for kohadev:read_from_file($file)####################### at /kohadevbox/koha/Koha/Config.pm line 36. . Starting Koha worker daemon for kohadev:. Starting Koha indexing daemon for kohadev:. ==================== logs ======================== ==> /var/log/koha/kohadev/plack-error.log <== Process Backgrounded 2021/05/03-19:36:13 Starman::Server (type Net::Server::PreFork) starting! pid(3537) Binding to UNIX socket file "/var/run/koha/kohadev/plack.sock" Setting gid to "1000 1000" Setting uid to "1000" Starman: Accepting connections at unix://localhost:/var/run/koha/kohadev/plack.sock/ read_from_file($file)####################### at /kohadevbox/koha/Koha/Config.pm line 36. read_from_file($file)####################### at /kohadevbox/koha/Koha/Config.pm line 36. ==> /var/log/koha/kohadev/sip-output.log <== read_from_file($file)####################### at /kohadevbox/koha/Koha/Config.pm line 36. ==> /var/log/koha/kohadev/indexer-output.log <== read_from_file($file)####################### at /kohadevbox/koha/Koha/Config.pm line 36. ==> /var/log/koha/kohadev/worker-output.log <== read_from_file($file)####################### at /kohadevbox/koha/Koha/Config.pm line 36. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28276 --- Comment #3 from Julian Maurice <julian.maurice@biblibre.com> --- (In reply to Victor Grousset/tuxayo from comment #2)
Error: «Starting Z39.50/SRU daemon for kohadev:Global symbol "$file" requires explicit package name (did you forget to declare "my $file"?) at /kohadevbox/koha/Koha/Config.pm line 36.»
I guess there should be simple quotes right?
You probably added the warn too early. It should be just after getting the parameters: my ($class, $file) = @_; warn "read_from_file($file)";
Test results: Is that what should be expected? (less reads but still multiple)
You should see it once per worker. I see that there are two "read_from_file" in plack-error.log (after the patch). So if you have two workers that's normal. Please make sure there are no "read_from_file" after startup, when you use koha web interface. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28276 --- Comment #4 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Comment on attachment 120352 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=120352 Bug 28276: Do not fetch config ($KOHA_CONF) from memcached Review of attachment 120352: --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=28276&attachment=120352) ----------------------------------------------------------------- ::: Koha/Database.pm @@ +49,4 @@
require Koha::Schema;
+ my $db_driver = C4::Context::db_scheme2dbi(C4::Context->config('db_scheme'));;
This change looks reasonable to me, but is worth highlighting to whoever does QA as worth digging into.. I'm not 100% sure if there's some background reason this used to call $context->{db_driver} instead of passing db_scheme into db_scheme2dbi. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28276 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #120352|0 |1 is obsolete| | --- Comment #5 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 120428 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=120428&action=edit Bug 28276: Do not fetch config ($KOHA_CONF) from memcached memcached address and namespace are in $KOHA_CONF, so it is required to read it before being able to access the cache. And after that, configuration is kept in memory forever. Storing this in memcached is useless and even counter-productive, since Koha reads both the file and the cache This patch addresses this issue by removing the cache-related code from C4::Context->new. It means that C4::Context->new will always read the configuration file, so this patch also replaces inappropriate calls to C4::Context->new->config by appropriate calls to C4::Context->config It also fixes a bug where C4::Context->new would ignore the filepath given in parameters if there was something in cache. It also removes a problematic call to Koha::Caches->get_instance. Because this call was outside of any subroutine, it would have happened before the initialization of $C4::Context::context (which happen in C4::Context::import) Test plan: 1. Do not apply the patch yet 2. Add the following line at the beginning of Koha::Config::read_from_file warn "read_from_file($file)"; This will allow you to check how many times the file is read. 3. Flush memcached and restart starman 4. Check the logs, you should see "read_from_file" a bunch of times 5. Apply the patch 6. Re-add the line from step 2 7. Flush memcached and restart starman 8. Check the logs, you should see "read_from_file" only once 9. Make sure the memcached config from $KOHA_CONF (memcached_servers, memcached_namespace) is taken into account by checking the About page Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28276 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com Status|Needs Signoff |Signed Off --- Comment #6 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- This achieves the desired effect for me.. works well and is a nice bit of cleanup. Signing off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28276 --- Comment #7 from Victor Grousset/tuxayo <victor@tuxayo.net> --- (In reply to Julian Maurice from comment #3)
You probably added the warn too early. It should be just after getting the parameters: my ($class, $file) = @_; warn "read_from_file($file)";
oops, how careless, now it's obvious ^^" thanks :)
You should see it once per worker. I see that there are two "read_from_file" in plack-error.log (after the patch). So if you have two workers that's normal.
Potential issue found, I added the following: warn "######## read_from_file($file)" . "###" . int(rand(10000000)) . "######"; The rand to identify each unique call of the function. Output with the patch: ============================== restart command ====================== Starting Z39.50/SRU daemon for kohadev:################# read_from_file(/etc/koha/sites/kohadev/koha-conf.xml)###9584085###### at /kohadevbox/koha/Koha/Config.pm line 37. =============================== all logs ============================= ==> /var/log/koha/kohadev/plack-error.log <== Starman: Accepting connections at unix://localhost:/var/run/koha/kohadev/plack.sock/ ################# read_from_file(/etc/koha/sites/kohadev/koha-conf.xml)###6082019###### at /kohadevbox/koha/Koha/Config.pm line 37. ################# read_from_file(/etc/koha/sites/kohadev/koha-conf.xml)###9302336###### at /kohadevbox/koha/Koha/Config.pm line 37. ==> /var/log/koha/kohadev/sip-output.log <== ################# read_from_file(/etc/koha/sites/kohadev/koha-conf.xml)###3475587###### at /kohadevbox/koha/Koha/Config.pm line 37. ==> /var/log/koha/kohadev/indexer-output.log <== ################# read_from_file(/etc/koha/sites/kohadev/koha-conf.xml)###5666078###### at /kohadevbox/koha/Koha/Config.pm line 37. ==> /var/log/koha/kohadev/worker-output.log <== ################# read_from_file(/etc/koha/sites/kohadev/koha-conf.xml)###488901###### at /kohadevbox/koha/Koha/Config.pm line 37. There are two for plack, one per worker. But are the other calls expected? And the one in the restart_all output?
Please make sure there are no "read_from_file" after startup, when you use koha web interface.
Selenium tests didn't show calls to read_from_file in the logs. Though the call to prove did but I guess it's because the test code load Koha stuff right? https://paste.chapril.org/?39fdfda86050ed4c#8zBSp79ACPT6PRs29DKGzR4QbB3HWH9y... Then I messed a bit with the OPAC, I can't reproduce how that happended but I ended up doing this query: http://localhost:8080/opac-search.pl?idx=&q=test&weight_search=1 Which calls read_from_file. I guess it's not an issue since not having cgi-bin/koha is expected to change stuff. The real issue would how did I end up with such an URL, which I didn't find. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28276 --- Comment #8 from Julian Maurice <julian.maurice@biblibre.com> --- (In reply to Victor Grousset/tuxayo from comment #7)
There are two for plack, one per worker. But are the other calls expected? And the one in the restart_all output? Yes. Each service is started in a different process and they need to read $KOHA_CONF too.
Selenium tests didn't show calls to read_from_file in the logs. Though the call to prove did but I guess it's because the test code load Koha stuff right? I don't know how Selenium works, but I'm not too worried about those calls. Did you test this on master ?
-- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28276 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28276 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #120428|0 |1 is obsolete| | --- Comment #9 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 120693 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=120693&action=edit Bug 28276: Do not fetch config ($KOHA_CONF) from memcached memcached address and namespace are in $KOHA_CONF, so it is required to read it before being able to access the cache. And after that, configuration is kept in memory forever. Storing this in memcached is useless and even counter-productive, since Koha reads both the file and the cache This patch addresses this issue by removing the cache-related code from C4::Context->new. It means that C4::Context->new will always read the configuration file, so this patch also replaces inappropriate calls to C4::Context->new->config by appropriate calls to C4::Context->config It also fixes a bug where C4::Context->new would ignore the filepath given in parameters if there was something in cache. It also removes a problematic call to Koha::Caches->get_instance. Because this call was outside of any subroutine, it would have happened before the initialization of $C4::Context::context (which happen in C4::Context::import) Test plan: 1. Do not apply the patch yet 2. Add the following line at the beginning of Koha::Config::read_from_file warn "read_from_file($file)"; This will allow you to check how many times the file is read. 3. Flush memcached and restart starman 4. Check the logs, you should see "read_from_file" a bunch of times 5. Apply the patch 6. Re-add the line from step 2 7. Flush memcached and restart starman 8. Check the logs, you should see "read_from_file" only once 9. Make sure the memcached config from $KOHA_CONF (memcached_servers, memcached_namespace) is taken into account by checking the About page Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28276 --- Comment #10 from Victor Grousset/tuxayo <victor@tuxayo.net> --- (In reply to Julian Maurice from comment #8)
Yes. Each service is started in a different process and they need to read $KOHA_CONF too.
Good :)
I don't know how Selenium works, but I'm not too worried about those calls.
Sorry for the confusion, during the Selenium tests, the logs of Koha didn't show a call to the function. It was just a lazy way to do things in the UI ^^ The terminal where prove -r t/db_dependent/selenium was run did. But I'm not worried about that either.
Did you test this on master ?
Yes -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28276 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |28306 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28306 [Bug 28306] Allow to query database with minimal memory footprint -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28276 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |21.05.00 released in| | Status|Passed QA |Pushed to master -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28276 --- Comment #11 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Pushed to master for 21.05, thanks to everybody involved! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28276 --- Comment #12 from David Cook <dcook@prosentient.com.au> --- Yay! This should prevent many a headache... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28276 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m Status|Pushed to master |Pushed to stable Version(s)|21.05.00 |21.05.00,20.11.06 released in| | --- Comment #13 from Fridolin Somers <fridolin.somers@biblibre.com> --- Pushed to 20.11.x for 20.11.06 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28276 --- Comment #14 from Fridolin Somers <fridolin.somers@biblibre.com> --- (In reply to David Cook from comment #12)
Yay! This should prevent many a headache...
Indeed. Any configuration change whent with "Oh, I forgot to flush memcached, again". -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28276 --- Comment #15 from David Cook <dcook@prosentient.com.au> --- (In reply to Fridolin Somers from comment #14)
(In reply to David Cook from comment #12)
Yay! This should prevent many a headache...
Indeed. Any configuration change whent with "Oh, I forgot to flush memcached, again".
I'm sorry/glad that I'm not the only one who suffered this. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28276 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |andrew@bywatersolutions.com --- Comment #16 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Doesn't apply cleanly to 20.05, please rebase if needed -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org