Moodle
  1. Moodle
  2. MDL-36466

Ballooning memory on install when dataroot not writeable

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.4
    • Fix Version/s: 2.4
    • Component/s: Caching
    • Labels:
    • Testing Instructions:
      Hide
      1. Perform a fresh install through your browser.
      2. Perform a fresh install through CLI.
      3. Upgrade a 2.3 site in your browser.
      4. Run unit tests.
      5. Prepare to install a site (but don't run installation yet)
      6. Create the directory moodledata/muc and remove all permissions (sudo chmod -R 000 muc)
      7. Attempt the install.
      8. Check you get an error and not an timeout/max memory exceeded/max recursion error.
      Show
      Perform a fresh install through your browser. Perform a fresh install through CLI. Upgrade a 2.3 site in your browser. Run unit tests. Prepare to install a site (but don't run installation yet) Create the directory moodledata/muc and remove all permissions (sudo chmod -R 000 muc) Attempt the install. Check you get an error and not an timeout/max memory exceeded/max recursion error.
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull Master Branch:
      wip-MDL-36466-m24

      Description

      On Barbaras machine trying to do a fresh install:

      11:15 Fatal error: Allowed memory size of 4294967296 bytes exhausted (tried to allocate 71 bytes) in /Users/barbararamiro/Sites/integration/cache/locallib.php on line 68

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Sam Hemelryk added a comment -

            Caused because dataroot was not writable. Still very much a blocker, I'll work on it shortly.

            Show
            Sam Hemelryk added a comment - Caused because dataroot was not writable. Still very much a blocker, I'll work on it shortly.
            Hide
            Barbara Ramiro added a comment -

            As Sam suggested, the problem came from a wrong permission in dataroot. For some reason the 'muc' directory had no write access. I did not create it manually so I am not sure of the reason...

            drwxrwsrwx  7 barbararamiro  staff  238  9 Nov 11:02 .
            drwxrwxrwx  4 barbararamiro  staff  136  8 Nov 17:06 ..
            -rw-rw-rw-  1 _www           staff  128  9 Nov 11:02 .htaccess
            drwxrwxrwx  2 _www           staff   68  9 Nov 11:02 cache
            drwxrwxrwx  2 _www           staff   68  9 Nov 11:02 lang
            d---------  2 _www           staff   68  8 Nov 17:14 muc
            drwxrwxrwx  2 _www           staff   68  9 Nov 11:02 temp
            

            Fred (using Barbie's computer)

            Show
            Barbara Ramiro added a comment - As Sam suggested, the problem came from a wrong permission in dataroot. For some reason the 'muc' directory had no write access. I did not create it manually so I am not sure of the reason... drwxrwsrwx 7 barbararamiro staff 238 9 Nov 11:02 . drwxrwxrwx 4 barbararamiro staff 136 8 Nov 17:06 .. -rw-rw-rw- 1 _www staff 128 9 Nov 11:02 .htaccess drwxrwxrwx 2 _www staff 68 9 Nov 11:02 cache drwxrwxrwx 2 _www staff 68 9 Nov 11:02 lang d--------- 2 _www staff 68 8 Nov 17:14 muc drwxrwxrwx 2 _www staff 68 9 Nov 11:02 temp Fred (using Barbie's computer)
            Hide
            Frédéric Massart added a comment -

            Actually, that was my mistake. Or pretty sure it was!

            When there was this issue with cache/lib.php not being included. I manually hacked the code and added a require_once() within the function that created the cache object. Which means the CFG was not set. I had then set the main variables manually and try again. What would have happened is that the directory had been created using $CFG->dirpermissions which was 0. Hence this weird permission...

            Sorry for wasting your time...

            Show
            Frédéric Massart added a comment - Actually, that was my mistake. Or pretty sure it was! When there was this issue with cache/lib.php not being included. I manually hacked the code and added a require_once() within the function that created the cache object. Which means the CFG was not set. I had then set the main variables manually and try again. What would have happened is that the directory had been created using $CFG->dirpermissions which was 0. Hence this weird permission... Sorry for wasting your time...
            Hide
            Sam Hemelryk added a comment -

            No probs Fred, it turned up exception handling issues in the cache API that we may not have found before release otherwise!

            I'll be implementing a two part fix on this issue:

            1. Better exception handling within cache initialisation.
            2. Disable cache during installation.
            Show
            Sam Hemelryk added a comment - No probs Fred, it turned up exception handling issues in the cache API that we may not have found before release otherwise! I'll be implementing a two part fix on this issue: Better exception handling within cache initialisation. Disable cache during installation.
            Hide
            Michael de Raadt added a comment -

            Closing this as it was a bug that arose when trying to overcome another bug.

            Show
            Michael de Raadt added a comment - Closing this as it was a bug that arose when trying to overcome another bug.
            Hide
            Aparup Banerjee added a comment -

            MDr: i don't think the linked bug will actually resolve this.

            Show
            Aparup Banerjee added a comment - MDr: i don't think the linked bug will actually resolve this.
            Hide
            Dan Poltawski added a comment -

            There is still a bug here!

            Show
            Dan Poltawski added a comment - There is still a bug here!
            Hide
            Sam Hemelryk added a comment -

            Hehe was just about to reopen to that effect with a comment explaining it

            While this issue came about because of another issue it has turned up an entirely new issue to do with the exception handling within cache.
            The new issue is leading to an infinite loop situation:

            1. Cache starts initialisation.
            2. Cache throws exception.
            3. Exception handler calls get_string for the message.
            4. get_string call the cache to initialise.
            5. Goto 1.
            Show
            Sam Hemelryk added a comment - Hehe was just about to reopen to that effect with a comment explaining it While this issue came about because of another issue it has turned up an entirely new issue to do with the exception handling within cache. The new issue is leading to an infinite loop situation: Cache starts initialisation. Cache throws exception. Exception handler calls get_string for the message. get_string call the cache to initialise. Goto 1.
            Hide
            Sam Hemelryk added a comment -

            Putting this up for peer-review.
            There are five commits making the following changes:

            1. improved handling of exceptions during initialisation.
            2. renamed define NO_CACHE_STORES to CACHE_DISABLE_STORES
            3. renamed disable to disable_stores and added tests
            4. implemented functionality to disable the bulk of the cache API
            5. disabled caching during installation and upgrade

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Putting this up for peer-review. There are five commits making the following changes: improved handling of exceptions during initialisation. renamed define NO_CACHE_STORES to CACHE_DISABLE_STORES renamed disable to disable_stores and added tests implemented functionality to disable the bulk of the cache API disabled caching during installation and upgrade Cheers Sam
            Hide
            Martin Dougiamas added a comment -

            Petr can you please peer review this?

            Show
            Martin Dougiamas added a comment - Petr can you please peer review this?
            Hide
            Petr Skoda added a comment -

            starting...

            Show
            Petr Skoda added a comment - starting...
            Hide
            Petr Skoda added a comment -

            Hi, I do not understand what you are trying to do with cache_factory::disable() in installer and upgrade code because I think we should use constants like CACHE_DISABLE_STORES before we require config.php or any other libraries - I can not see any other safe way to disable everything. cache_factory::disable() does not kill/disable existing instances, right?

            I expected to see 3 options:
            1/ CACHE_DISABLE_STORES which is used only before config.php, expected to be used in CLI installer, CLI upgrade scripts and first part of web installer
            2/ cache_factory::disable_all_stores() - disable all already created cache instances and new instances will be disabled
            3/ cache_factory::enable_all_stores() - enable all previously disabled stores and create any new stores the normal way

            The admin/index.php is tricky because it does installation, upgrade and normal UI at the same time. Maybe it is time to split the upgrade and installation to separate scripts (with redirects). For now cache_factory::disable_all_stores() could do the trick, but I think it would be better to execute lib/setup.php and friends without any caching once we start hacking the config tables.

            I am puzzled by the persistent stores, that requirement seems to collide with any disabling of such stores. Or is it all or nothing guaranteed to be stored? Another potential problem is request stores, previously we kept them working even in upgrade and install scripts for performance reasons. Maybe there could be some extra info in definition describing what should be done when disabling stores to let developers decide.

            Sorry for this general feedback, I hope it is not totally wrong or off-topic, I guess I need more time to understand all the potential problems here and the current design. I want to spend all time on this today and tomorrow if necessary. Please ping me once you get online on Monday, ciao.

            Show
            Petr Skoda added a comment - Hi, I do not understand what you are trying to do with cache_factory::disable() in installer and upgrade code because I think we should use constants like CACHE_DISABLE_STORES before we require config.php or any other libraries - I can not see any other safe way to disable everything. cache_factory::disable() does not kill/disable existing instances, right? I expected to see 3 options: 1/ CACHE_DISABLE_STORES which is used only before config.php, expected to be used in CLI installer, CLI upgrade scripts and first part of web installer 2/ cache_factory::disable_all_stores() - disable all already created cache instances and new instances will be disabled 3/ cache_factory::enable_all_stores() - enable all previously disabled stores and create any new stores the normal way The admin/index.php is tricky because it does installation, upgrade and normal UI at the same time. Maybe it is time to split the upgrade and installation to separate scripts (with redirects). For now cache_factory::disable_all_stores() could do the trick, but I think it would be better to execute lib/setup.php and friends without any caching once we start hacking the config tables. I am puzzled by the persistent stores, that requirement seems to collide with any disabling of such stores. Or is it all or nothing guaranteed to be stored? Another potential problem is request stores, previously we kept them working even in upgrade and install scripts for performance reasons. Maybe there could be some extra info in definition describing what should be done when disabling stores to let developers decide. Sorry for this general feedback, I hope it is not totally wrong or off-topic, I guess I need more time to understand all the potential problems here and the current design. I want to spend all time on this today and tomorrow if necessary. Please ping me once you get online on Monday, ciao.
            Hide
            Sam Hemelryk added a comment -

            Thanks Petr.

            We talked this morning I'm going to action the discussed points and get this ready for your review and testing again, hopefully in the next 24 hours.

            Many thanks
            Sam

            Show
            Sam Hemelryk added a comment - Thanks Petr. We talked this morning I'm going to action the discussed points and get this ready for your review and testing again, hopefully in the next 24 hours. Many thanks Sam
            Hide
            Sam Hemelryk added a comment -

            Hi Petr,
            I've an updated branch available now.
            The following changes have been made:

            1. disable method is now protected and used only for unit tests.
            2. new define CACHE_DISABLE_ALL, disables everything, unused presently
            3. install and upgrade methods now disable stores before they start and then reset things once they're done.

            Keen to see what you think.

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Hi Petr, I've an updated branch available now. The following changes have been made: disable method is now protected and used only for unit tests. new define CACHE_DISABLE_ALL, disables everything, unused presently install and upgrade methods now disable stores before they start and then reset things once they're done. Keen to see what you think. Cheers Sam
            Hide
            Petr Skoda added a comment -

            Hi, I like the API much better now, thanks a lot. I was also playing with the installer and upgrades, borking permissions, deleting things randomly, etc. - so far works fine for me.

            +1

            Show
            Petr Skoda added a comment - Hi, I like the API much better now, thanks a lot. I was also playing with the installer and upgrades, borking permissions, deleting things randomly, etc. - so far works fine for me. +1
            Hide
            Sam Hemelryk added a comment -

            Awesome, thanks for looking at that Petr its much appreciated.
            Putting this up for integration now.

            Show
            Sam Hemelryk added a comment - Awesome, thanks for looking at that Petr its much appreciated. Putting this up for integration now.
            Hide
            Dan Poltawski added a comment -

            Integrated, thanks Sam.

            Show
            Dan Poltawski added a comment - Integrated, thanks Sam.
            Hide
            Rossiani Wijaya added a comment -

            Running Unit test for the site returned the following failures:

            There were 3 failures:
             
            1) dml_testcase::test_unique_index_collation_trouble
            Unique index is accent insensitive, this may cause problems for non-ascii languages. This is usually caused by accent insensitive default collation.
             
            /integration/cli/master/lib/dml/tests/dml_test.php:3633
             
            To re-run:
             /usr/bin/phpunit dml_testcase lib/dml/tests/dml_test.php
             
            2) dml_testcase::test_sql_binary_equal
            SQL operator "=" is expected to be case sensitive
            Failed asserting that 1 matches expected 2.
             
            /integration/cli/master/lib/dml/tests/dml_test.php:3661
             
            To re-run:
             /usr/bin/phpunit dml_testcase lib/dml/tests/dml_test.php
             
            3) grading_manager_testcase::test_tokenize
            A test using UTF-8 characters has failed. Consider updating PHP and PHP's PCRE or INTL extensions (MDL-30494)
            Failed asserting that false is true.
             
            /integration/cli/master/grade/grading/tests/lib_test.php:105
            /integration/cli/master/lib/phpunit/classes/advanced_testcase.php:76
             
            To re-run:
             /usr/bin/phpunit grading_manager_testcase grade/grading/tests/lib_test.php
             
            FAILURES!
            Tests: 1510, Assertions: 28313, Failures: 3.
            
            

            Show
            Rossiani Wijaya added a comment - Running Unit test for the site returned the following failures: There were 3 failures:   1) dml_testcase::test_unique_index_collation_trouble Unique index is accent insensitive, this may cause problems for non-ascii languages. This is usually caused by accent insensitive default collation.   /integration/cli/master/lib/dml/tests/dml_test.php:3633   To re-run: /usr/bin/phpunit dml_testcase lib/dml/tests/dml_test.php   2) dml_testcase::test_sql_binary_equal SQL operator "=" is expected to be case sensitive Failed asserting that 1 matches expected 2.   /integration/cli/master/lib/dml/tests/dml_test.php:3661   To re-run: /usr/bin/phpunit dml_testcase lib/dml/tests/dml_test.php   3) grading_manager_testcase::test_tokenize A test using UTF-8 characters has failed. Consider updating PHP and PHP's PCRE or INTL extensions (MDL-30494) Failed asserting that false is true.   /integration/cli/master/grade/grading/tests/lib_test.php:105 /integration/cli/master/lib/phpunit/classes/advanced_testcase.php:76   To re-run: /usr/bin/phpunit grading_manager_testcase grade/grading/tests/lib_test.php   FAILURES! Tests: 1510, Assertions: 28313, Failures: 3.
            Hide
            Rossiani Wijaya added a comment -

            For step 8: I received "Unable to save the cache config to file." message.

            I think that is the correct behavior.

            Hi Sam,

            Could you verify the error for phpunit test and let me know if it should be fixed on different issue?

            Thanks

            Show
            Rossiani Wijaya added a comment - For step 8: I received "Unable to save the cache config to file." message. I think that is the correct behavior. Hi Sam, Could you verify the error for phpunit test and let me know if it should be fixed on different issue? Thanks
            Hide
            Matteo Scaramuccia added a comment -

            Hi,
            IMHO those errors come from using a MySQL database with a collation like utf8_unicode_ci.

            HTH,
            Matteo

            Show
            Matteo Scaramuccia added a comment - Hi, IMHO those errors come from using a MySQL database with a collation like utf8_unicode_ci . HTH, Matteo
            Hide
            Dan Poltawski added a comment -

            Agreed about unit tests, please see: http://docs.moodle.org/dev/Common_unit_test_problems

            (The CI servers checked the tests and all looking good there).

            Show
            Dan Poltawski added a comment - Agreed about unit tests, please see: http://docs.moodle.org/dev/Common_unit_test_problems (The CI servers checked the tests and all looking good there).
            Hide
            Sam Hemelryk added a comment -

            Hi Rosie,

            That is the expected result for step 8.
            Certainly those unit tests are as Matteo and Dan have pointed out due to other factors.

            Many thanks
            Sam

            Show
            Sam Hemelryk added a comment - Hi Rosie, That is the expected result for step 8. Certainly those unit tests are as Matteo and Dan have pointed out due to other factors. Many thanks Sam
            Hide
            Rossiani Wijaya added a comment -

            Thank you for the feedback guys.

            I'm passing this test.

            Test passed.

            Show
            Rossiani Wijaya added a comment - Thank you for the feedback guys. I'm passing this test. Test passed.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Y E S !

            Closing as fixed, many thanks!

            Show
            Eloy Lafuente (stronk7) added a comment - Y E S ! Closing as fixed, many thanks!

              People

              • Votes:
                0 Vote for this issue
                Watchers:
                3 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: