Moodle
  1. Moodle
  2. MDL-28346

Performing a backup fails when a file is missing

    Details

    • Testing Instructions:
      Hide

      Test pre-requisites

      • A course
      • All file used must be different
      • A forum activity with several posts
        • One post with an image in description (call the file forum_desc.jpg)
        • One post with a file as attachment (call the file forum_attach.txt)
      • A file resource activity with a file in it (call the file file_attach.txt)
      • You can receive emails sent to the admin
      • The tests have to be done one after the other

      Test #1

      1. Navigate to Home ► Courses ► YOUR CATEGORY ► YOUR COURSE ► Backup
      2. Create a backup of the course (leave settings as default)
      3. Make sure the backup finishes without warnings
      4. Save the new backup file for later use, I will call it test1.mbz

      Test #2

      1. Delete the files from moodledata (see procedure below)
      2. Backup your course
      3. Make sure the backup does not fail
      4. Make sure a warning message is printed out to tell you that some files were missing
      5. Save the new backup file for later use, I will call it test2.mbz

      Test #3

      1. Run the `php admin/cli/automated_backups.php`
      2. Make sure the backup ends without unexpected errors
      3. Make sure the console output display information about the missing files
      4. Navigate to Home ► Site administration ► Reports ► Backups
      5. Make sure the course backup status is 'Warning'
      6. Make sure the email sent to the admin displays that a backup had warnings

      Test #4

      1. Navigate to Home ► Courses ► YOUR CATEGORY ► YOUR COURSE ► Restore
      2. Use test2.mbz to restore the course into a new course
      3. Make sure the restore succeeded
      4. Make sure a warning message told you that some files were missing

      Test #5

      1. Navigate to Home ► Courses ► YOUR CATEGORY ► YOUR COURSE ► Restore
      2. Use test1.mbz to restore the course into a new course
      3. Make sure the restore succeeded without any warning

      Delete the files from moodledata

      1. Browse your table mdl_files ordered by ID DESC
      2. Look for the rows matching the file names as specified the pre-requisites (forum_attach, forum_desc, file_attach)
      3. Note down the contenthash for each of them, ie.: da39a3ee5e6b4b0d3255bfef95601890afd80709
      4. Browse to the directories $CFG->dataroot/filedir/da/39 (using the 4 first letters of the contenthash)
      5. Rename the file named as the contenthash. ie.: da39a3ee5e6b4b0d3255bfef95601890afd80709.DELETED
      6. You're done!
      Show
      Test pre-requisites A course All file used must be different A forum activity with several posts One post with an image in description (call the file forum_desc .jpg) One post with a file as attachment (call the file forum_attach .txt) A file resource activity with a file in it (call the file file_attach .txt) You can receive emails sent to the admin The tests have to be done one after the other Test #1 Navigate to Home ► Courses ► YOUR CATEGORY ► YOUR COURSE ► Backup Create a backup of the course (leave settings as default) Make sure the backup finishes without warnings Save the new backup file for later use, I will call it test1.mbz Test #2 Delete the files from moodledata (see procedure below) Backup your course Make sure the backup does not fail Make sure a warning message is printed out to tell you that some files were missing Save the new backup file for later use, I will call it test2.mbz Test #3 Run the `php admin/cli/automated_backups.php` Make sure the backup ends without unexpected errors Make sure the console output display information about the missing files Navigate to Home ► Site administration ► Reports ► Backups Make sure the course backup status is 'Warning' Make sure the email sent to the admin displays that a backup had warnings Test #4 Navigate to Home ► Courses ► YOUR CATEGORY ► YOUR COURSE ► Restore Use test2.mbz to restore the course into a new course Make sure the restore succeeded Make sure a warning message told you that some files were missing Test #5 Navigate to Home ► Courses ► YOUR CATEGORY ► YOUR COURSE ► Restore Use test1.mbz to restore the course into a new course Make sure the restore succeeded without any warning Delete the files from moodledata Browse your table mdl_files ordered by ID DESC Look for the rows matching the file names as specified the pre-requisites ( forum_attach , forum_desc , file_attach ) Note down the contenthash for each of them, ie.: da39a3ee5e6b4b0d3255bfef95601890afd80709 Browse to the directories $CFG->dataroot/filedir/da/39 (using the 4 first letters of the contenthash) Rename the file named as the contenthash. ie.: da39a3ee5e6b4b0d3255bfef95601890afd80709.DELETED You're done!
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-28346-master
    • Rank:
      17990

      Description

      My language is Portuguese of Brazil, or will post in both languages.
      I have problems to build up some courses.
      I tried every way possible before resorting to you, and I saw that I can no longer solve this problem.
      I have the moodle distribution of fully intact, without code modifications, or were talking to a native installation files.
      Summarizing the problem, for some courses I run the backup normally, but for others I see that the directory "/ moodledata / temp / backup" files it generates, but it seems he can not direct the file "*. nu.mbz" to the database by stopping the process.
      I made the migration from version 1.9.7 to 2.0.2
      I use postgres 8.3, Moodle 2.0.2, php 5.3.8
      Below lay the screen printing, which are the errors in detail with the debugger enabled function

      Please ask for special attention, because I saw some post in the forums related solutions fancy, and using Moodle for teaching instuição over 3000 subjects and we have created this solution as soon as possible.

      Regards

      --------------------------------------------------

      Meu idioma é português do Brasil, ou seja irei postar nas duas línguas.
      Estou com problemas para criar backup de alguns cursos.
      Tentei todas as formas possíveis antes de recorrer para vocês, e acabei vendo que não consigo mais resolver este problema.
      Tenho a distribuição do moodle totalmente intacta, sem modificações no código, ou seja estavamos falando de uma instalação com arquivos nativos.
      Resumindo o problema, para alguns cursos consigo gerar o backup normalmente, porém para outros vejo que no diretório "/moodledata/temp/backup" ele gera os arquivos, porém parece que ele não consegue direcionar o arquivo "*.nu.mbz" para o banco de dados, parando o processo.
      Fiz a migração da versão 1.9.7 para 2.0.2
      Utilizo postgres 8.3, moodle 2.0.2, php 5.3.8
      Abaixo coloco as impressão da tela, na qual estão os erros detalhadamente com a função de debbug ativada

      Por favor peço uma atenção especial, pois vi alguns post relacionados nos fóruns com soluções mirabolantes, sendo que utilizamos o moodle para instuição de ensino com mais de 3000 disciplinas criadas e precisamos desta solução o quanto antes.

      Abraço,

        Issue Links

          Activity

          Hide
          Andreas Grabs added a comment -

          +1 the error is thrown on the backup process if one or more files are missing in the filedir directory.
          This is very awful if there was a crash and not all files could be restored.
          If you have a course with xx activity instances using files and one of the linked files is broken so the whole course can not be backuped.
          The only chance you have is to look at every activity and remove the link to the broken file.
          I think a better way would be to do the backup even if one file is broken. There could be printed a message so we are aware that there is an error, but the backup should be go forward.
          Thanks
          Andreas

          Show
          Andreas Grabs added a comment - +1 the error is thrown on the backup process if one or more files are missing in the filedir directory. This is very awful if there was a crash and not all files could be restored. If you have a course with xx activity instances using files and one of the linked files is broken so the whole course can not be backuped. The only chance you have is to look at every activity and remove the link to the broken file. I think a better way would be to do the backup even if one file is broken. There could be printed a message so we are aware that there is an error, but the backup should be go forward. Thanks Andreas
          Hide
          Bill Stone added a comment -

          Any progress/resolution?

          We experience (what seems to be) the same issue when backing up a course:

          Moodle 2.2.1+ (Build: 20120119)

          Potential coding error - existing temptables found when disposing database. Must be dropped!

          Can not read file, either file does not exist or there are permission problems

          Stack trace:
          line 174 of /lib/filestorage/stored_file.php: file_exception thrown
          line 90 of /backup/util/helper/backup_file_manager.class.php: call to stored_file->copy_content_to()
          line 101 of /backup/moodle2/backup_custom_fields.php: call to backup_file_manager::copy_file_moodle2backup()
          line 70 of /backup/util/structure/backup_nested_element.class.php: call to file_nested_element->fill_values()
          line 94 of /backup/moodle2/backup_custom_fields.php: call to backup_nested_element->process()
          line 95 of /backup/util/structure/backup_nested_element.class.php: call to file_nested_element->process()
          line 95 of /backup/util/plan/backup_structure_step.class.php: call to backup_nested_element->process()
          line 153 of /backup/util/plan/base_task.class.php: call to backup_structure_step->execute()
          line 148 of /backup/util/plan/base_plan.class.php: call to base_task->execute()
          line 106 of /backup/util/plan/backup_plan.class.php: call to base_plan->execute()
          line 304 of /backup/controller/backup_controller.class.php: call to backup_plan->execute()
          line 111 of /backup/util/ui/backup_ui.class.php: call to backup_controller->execute_plan()
          line 89 of /backup/backup.php: call to backup_ui->execute()

          Show
          Bill Stone added a comment - Any progress/resolution? We experience (what seems to be) the same issue when backing up a course: Moodle 2.2.1+ (Build: 20120119) Potential coding error - existing temptables found when disposing database. Must be dropped! Can not read file, either file does not exist or there are permission problems Stack trace: line 174 of /lib/filestorage/stored_file.php: file_exception thrown line 90 of /backup/util/helper/backup_file_manager.class.php: call to stored_file->copy_content_to() line 101 of /backup/moodle2/backup_custom_fields.php: call to backup_file_manager::copy_file_moodle2backup() line 70 of /backup/util/structure/backup_nested_element.class.php: call to file_nested_element->fill_values() line 94 of /backup/moodle2/backup_custom_fields.php: call to backup_nested_element->process() line 95 of /backup/util/structure/backup_nested_element.class.php: call to file_nested_element->process() line 95 of /backup/util/plan/backup_structure_step.class.php: call to backup_nested_element->process() line 153 of /backup/util/plan/base_task.class.php: call to backup_structure_step->execute() line 148 of /backup/util/plan/base_plan.class.php: call to base_task->execute() line 106 of /backup/util/plan/backup_plan.class.php: call to base_plan->execute() line 304 of /backup/controller/backup_controller.class.php: call to backup_plan->execute() line 111 of /backup/util/ui/backup_ui.class.php: call to backup_controller->execute_plan() line 89 of /backup/backup.php: call to backup_ui->execute()
          Hide
          Greg added a comment -

          I have the same issue using:
          Moodle 2.0.8+ (Build: 20120504)
          All Server Checks are OK

          [09-May-2012 22:48:30] Default exception handler: Can not read file, either file does not exist or there are permission problems Debug:

          • line 174 of /lib/filestorage/stored_file.php: file_exception thrown
          • line 90 of /backup/util/helper/backup_file_manager.class.php: call to stored_file->copy_content_to()
          • line 101 of /backup/moodle2/backup_custom_fields.php: call to backup_file_manager::copy_file_moodle2backup()
          • line 70 of /backup/util/structure/backup_nested_element.class.php: call to file_nested_element->fill_values()
          • line 94 of /backup/moodle2/backup_custom_fields.php: call to backup_nested_element->process()
          • line 95 of /backup/util/structure/backup_nested_element.class.php: call to file_nested_element->process()
          • line 95 of /backup/util/plan/backup_structure_step.class.php: call to backup_nested_element->process()
          • line 153 of /backup/util/plan/base_task.class.php: call to backup_structure_step->execute()
          • line 148 of /backup/util/plan/base_plan.class.php: call to base_task->execute()
          • line 106 of /backup/util/plan/backup_plan.class.php: call to base_plan->execute()
          • line 304 of /backup/controller/backup_controller.class.php: call to backup_plan->execute()
          • line 111 of /backup/util/ui/backup_ui.class.php: call to backup_controller->execute_plan()
          • line 87 of /backup/backup.php: call to backup_ui->execute()

          [09-May-2012 22:48:31] Potential coding error - existing temptables found when disposing database. Must be dropped!

          Show
          Greg added a comment - I have the same issue using: Moodle 2.0.8+ (Build: 20120504) All Server Checks are OK [09-May-2012 22:48:30] Default exception handler: Can not read file, either file does not exist or there are permission problems Debug: line 174 of /lib/filestorage/stored_file.php: file_exception thrown line 90 of /backup/util/helper/backup_file_manager.class.php: call to stored_file->copy_content_to() line 101 of /backup/moodle2/backup_custom_fields.php: call to backup_file_manager::copy_file_moodle2backup() line 70 of /backup/util/structure/backup_nested_element.class.php: call to file_nested_element->fill_values() line 94 of /backup/moodle2/backup_custom_fields.php: call to backup_nested_element->process() line 95 of /backup/util/structure/backup_nested_element.class.php: call to file_nested_element->process() line 95 of /backup/util/plan/backup_structure_step.class.php: call to backup_nested_element->process() line 153 of /backup/util/plan/base_task.class.php: call to backup_structure_step->execute() line 148 of /backup/util/plan/base_plan.class.php: call to base_task->execute() line 106 of /backup/util/plan/backup_plan.class.php: call to base_plan->execute() line 304 of /backup/controller/backup_controller.class.php: call to backup_plan->execute() line 111 of /backup/util/ui/backup_ui.class.php: call to backup_controller->execute_plan() line 87 of /backup/backup.php: call to backup_ui->execute() [09-May-2012 22:48:31] Potential coding error - existing temptables found when disposing database. Must be dropped!
          Hide
          Samuel Witzig added a comment -

          We have the same issue while backing up some courses on Moodle 2.2.3 (upgraded from Moodle 1.9.18+ to 2.2.2, then Update to 2.2.3. Debug info below. Important: This happens only in maybe 50% of the courses.

          Debug info: /<server>/<filedirectory>/filedir/bc/d6/bcd62f6fb5e564daf6858d419e098706c12f2921
          Stack trace:
          line 174 of /lib/filestorage/stored_file.php: file_exception thrown
          line 90 of /backup/util/helper/backup_file_manager.class.php: call to stored_file->copy_content_to()
          line 101 of /backup/moodle2/backup_custom_fields.php: call to backup_file_manager::copy_file_moodle2backup()
          line 70 of /backup/util/structure/backup_nested_element.class.php: call to file_nested_element->fill_values()
          line 94 of /backup/moodle2/backup_custom_fields.php: call to backup_nested_element->process()
          line 95 of /backup/util/structure/backup_nested_element.class.php: call to file_nested_element->process()
          line 95 of /backup/util/plan/backup_structure_step.class.php: call to backup_nested_element->process()
          line 153 of /backup/util/plan/base_task.class.php: call to backup_structure_step->execute()
          line 148 of /backup/util/plan/base_plan.class.php: call to base_task->execute()
          line 106 of /backup/util/plan/backup_plan.class.php: call to base_plan->execute()
          line 309 of /backup/controller/backup_controller.class.php: call to backup_plan->execute()
          line 111 of /backup/util/ui/backup_ui.class.php: call to backup_controller->execute_plan()
          line 89 of /backup/backup.php: call to backup_ui->execute()

          Show
          Samuel Witzig added a comment - We have the same issue while backing up some courses on Moodle 2.2.3 (upgraded from Moodle 1.9.18+ to 2.2.2, then Update to 2.2.3. Debug info below. Important: This happens only in maybe 50% of the courses. Debug info: /<server>/<filedirectory>/filedir/bc/d6/bcd62f6fb5e564daf6858d419e098706c12f2921 Stack trace: line 174 of /lib/filestorage/stored_file.php: file_exception thrown line 90 of /backup/util/helper/backup_file_manager.class.php: call to stored_file->copy_content_to() line 101 of /backup/moodle2/backup_custom_fields.php: call to backup_file_manager::copy_file_moodle2backup() line 70 of /backup/util/structure/backup_nested_element.class.php: call to file_nested_element->fill_values() line 94 of /backup/moodle2/backup_custom_fields.php: call to backup_nested_element->process() line 95 of /backup/util/structure/backup_nested_element.class.php: call to file_nested_element->process() line 95 of /backup/util/plan/backup_structure_step.class.php: call to backup_nested_element->process() line 153 of /backup/util/plan/base_task.class.php: call to backup_structure_step->execute() line 148 of /backup/util/plan/base_plan.class.php: call to base_task->execute() line 106 of /backup/util/plan/backup_plan.class.php: call to base_plan->execute() line 309 of /backup/controller/backup_controller.class.php: call to backup_plan->execute() line 111 of /backup/util/ui/backup_ui.class.php: call to backup_controller->execute_plan() line 89 of /backup/backup.php: call to backup_ui->execute()
          Hide
          Frédéric Massart added a comment - - edited

          Time estimated: 7 hours (revised during Scrum)

          Show
          Frédéric Massart added a comment - - edited Time estimated: 7 hours (revised during Scrum)
          Hide
          Frédéric Massart added a comment -

          I have tested and reproduced this on master (2.3). Follows a quick patch which solves the issue, but is not the best approach the deal with it. My main concerns are:

          • How to nicely warn the user that the backup is done but incomplete (for automated backups too). I know there is a logging system, did not figure out how to use it from there though.
          • Shouldn't we handle the missing files while restoring as well?

          Here is the current patch:

          diff --git a/backup/util/helper/backup_file_manager.class.php b/backup/util/helper/backup_file_manager.class.php
          index 9941af7..7c97915 100644
          --- a/backup/util/helper/backup_file_manager.class.php
          +++ b/backup/util/helper/backup_file_manager.class.php
          @@ -91,7 +91,16 @@ class backup_file_manager {
           
                   // And copy the file (if doesn't exist already)
                   if (!file_exists($targetfilepath)) {
          -            $file->copy_content_to($targetfilepath);
          +            try {
          +                $file->copy_content_to($targetfilepath);
          +            } catch (file_exception $e) {
          +                // An error occurred while copying the file, but to ensure the backup
          +                // can be restored, we create an empty file instead of the original one
          +                file_put_contents($targetfilepath, '');
          +
          +                // throw new backup_helper_exception($e->errorcode, $e->a, $e->debuginfo);
          +                debugging($e->errorcode);
          +            }
                   }
               }
           }
          

          Here is the stacktrace in master

              line 102 of /backup/util/helper/backup_file_manager.class.php: call to debugging()
              line 106 of /backup/moodle2/backup_custom_fields.php: call to backup_file_manager::copy_file_moodle2backup()
              line 70 of /backup/util/structure/backup_nested_element.class.php: call to file_nested_element->fill_values()
              line 99 of /backup/moodle2/backup_custom_fields.php: call to backup_nested_element->process()
              line 95 of /backup/util/structure/backup_nested_element.class.php: call to file_nested_element->process()
              line 95 of /backup/util/plan/backup_structure_step.class.php: call to backup_nested_element->process()
              line 153 of /backup/util/plan/base_task.class.php: call to backup_structure_step->execute()
              line 163 of /backup/util/plan/base_plan.class.php: call to base_task->execute()
              line 110 of /backup/util/plan/backup_plan.class.php: call to base_plan->execute()
              line 309 of /backup/controller/backup_controller.class.php: call to backup_plan->execute()
              line 111 of /backup/util/ui/backup_ui.class.php: call to backup_controller->execute_plan()
              line 89 of /backup/backup.php: call to backup_ui->execute()
          
          Show
          Frédéric Massart added a comment - I have tested and reproduced this on master (2.3). Follows a quick patch which solves the issue, but is not the best approach the deal with it. My main concerns are: How to nicely warn the user that the backup is done but incomplete (for automated backups too). I know there is a logging system, did not figure out how to use it from there though. Shouldn't we handle the missing files while restoring as well? Here is the current patch: diff --git a/backup/util/helper/backup_file_manager.class.php b/backup/util/helper/backup_file_manager.class.php index 9941af7..7c97915 100644 --- a/backup/util/helper/backup_file_manager.class.php +++ b/backup/util/helper/backup_file_manager.class.php @@ -91,7 +91,16 @@ class backup_file_manager { // And copy the file (if doesn't exist already) if (!file_exists($targetfilepath)) { - $file->copy_content_to($targetfilepath); + try { + $file->copy_content_to($targetfilepath); + } catch (file_exception $e) { + // An error occurred while copying the file, but to ensure the backup + // can be restored, we create an empty file instead of the original one + file_put_contents($targetfilepath, ''); + + // throw new backup_helper_exception($e->errorcode, $e->a, $e->debuginfo); + debugging($e->errorcode); + } } } } Here is the stacktrace in master line 102 of /backup/util/helper/backup_file_manager.class.php: call to debugging() line 106 of /backup/moodle2/backup_custom_fields.php: call to backup_file_manager::copy_file_moodle2backup() line 70 of /backup/util/structure/backup_nested_element.class.php: call to file_nested_element->fill_values() line 99 of /backup/moodle2/backup_custom_fields.php: call to backup_nested_element->process() line 95 of /backup/util/structure/backup_nested_element.class.php: call to file_nested_element->process() line 95 of /backup/util/plan/backup_structure_step.class.php: call to backup_nested_element->process() line 153 of /backup/util/plan/base_task.class.php: call to backup_structure_step->execute() line 163 of /backup/util/plan/base_plan.class.php: call to base_task->execute() line 110 of /backup/util/plan/backup_plan.class.php: call to base_plan->execute() line 309 of /backup/controller/backup_controller.class.php: call to backup_plan->execute() line 111 of /backup/util/ui/backup_ui.class.php: call to backup_controller->execute_plan() line 89 of /backup/backup.php: call to backup_ui->execute()
          Hide
          Frédéric Massart added a comment -

          Adding Eloy here to have his feedback.

          Show
          Frédéric Massart added a comment - Adding Eloy here to have his feedback.
          Hide
          Brian King added a comment - - edited

          @ Frédéric: It should definitely be handled when restoring too ... I tried a similar approach to let the backup complete ... but restoring such a backup fails.

          Eloy's wisdom is asked here.

          By the way, for the case of the comment of Samuel above, this problem was caused by about 1% of files that went missing during an upgrade from 1.9 to 2.2. Why they went missing is unfortunately not clear, but we were able to fix the problem by copying the files in from the still existant Moodle 1.9 data directory.

          Show
          Brian King added a comment - - edited @ Frédéric: It should definitely be handled when restoring too ... I tried a similar approach to let the backup complete ... but restoring such a backup fails. Eloy's wisdom is asked here. By the way, for the case of the comment of Samuel above, this problem was caused by about 1% of files that went missing during an upgrade from 1.9 to 2.2. Why they went missing is unfortunately not clear, but we were able to fix the problem by copying the files in from the still existant Moodle 1.9 data directory.
          Hide
          Frédéric Massart added a comment -

          @ Eloy: Any thoughts about this?
          @ Brian: The patch I posted before allowed me to restore a backup without failing. Creating the empty file did the trick. Were you doing this too?

          Show
          Frédéric Massart added a comment - @ Eloy: Any thoughts about this? @ Brian: The patch I posted before allowed me to restore a backup without failing. Creating the empty file did the trick. Were you doing this too?
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi, first, one question:

          1) Fred, for curiosity, how did you manage to reproduce the problem? By manually deleting one file from the moodledat/filedir ?

          And them, some thoughts:

          1) We need to confirm if everybody here is getting those problems with 1.9.x upgrade sites, or if it also affects sites originally installed with 2.x. And this should lead to a new issue in the tracker to check if there is any 1.9 upgrade problem we can fix or no.

          2) In backup, for sure, we need to allow those operations to finish, much like has been done in the patch above. But I'm not sure if I like the idea of creating an empty file. Surely I would not include any file at all instead (see next points). And also I would log as much information as possible in backup logs with level LOG_WARNING, surely.

          3) In interactive (via UI) backups... at the final screen, I'd inform about any LOG_WARNING logged along the process. Always observing $CFG->debug levels, so the later takes precedence.

          4) In automated backups, I'd include support for a new backup status BACKUP_STATUS_WARNINGS, meaning that the backup ended but some warnings happened along the process. And, it would cause the email to admins to be sent, so they can go and look for problems @ backup logs (via the backups report).

          With those points we'll had backup covered. And those backups will be inconsistent because some files will be recorded in the files.xml and/or inforef.xml files, leading to a non-existing file). So then:

          5) In restore...simply, we'll skip any non-existing file, surely WARNING about that also in logs, and surely for interactive (via UI) operations, also informing in a similar way than in backup operations. As far as we have decided to skip those files completely... they will be lead to 404s, than IMO is better than "faking" with an empty file.

          Finally, note that both backup and restore steps.... do support the ability to return "results" at the end of the execution and later those results are (should be) shown at the end of any interactive (UI) execution (see controller->get_results() uses). So perhaps the way to implement the display of information in points 3 and 5 above is to use those results to transmit information to the UI. I think they have been used that way recently to show details about the backup and restore of file references, we just would need to return a new results element. Take a look to backup_ui_stage_complete and restore_ui_stage_complete classes.

          And that's it it, IMO. In summary:

          a) we always log any problem, both in backup and restore.
          b) we always generate and return one new 'missing_files_in_pool' (for backup) and 'missing_files_in_backup' (for restore) results array and make it to be displayed by the UI.
          c) in automated backups, we use the logs to decide about the new BACKUP_STATUS_WARNINGS and inform about it via mail.
          d) we need to know what's leading to those missing files and try to fix it if possible (another issue).
          e) the new status in automated backups and the information shown by the UI operations must be documented somewhere in Docs.

          Hope it helps...ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi, first, one question: 1) Fred, for curiosity, how did you manage to reproduce the problem? By manually deleting one file from the moodledat/filedir ? And them, some thoughts: 1) We need to confirm if everybody here is getting those problems with 1.9.x upgrade sites, or if it also affects sites originally installed with 2.x. And this should lead to a new issue in the tracker to check if there is any 1.9 upgrade problem we can fix or no. 2) In backup, for sure, we need to allow those operations to finish, much like has been done in the patch above. But I'm not sure if I like the idea of creating an empty file. Surely I would not include any file at all instead (see next points). And also I would log as much information as possible in backup logs with level LOG_WARNING, surely. 3) In interactive (via UI) backups... at the final screen, I'd inform about any LOG_WARNING logged along the process. Always observing $CFG->debug levels, so the later takes precedence. 4) In automated backups, I'd include support for a new backup status BACKUP_STATUS_WARNINGS, meaning that the backup ended but some warnings happened along the process. And, it would cause the email to admins to be sent, so they can go and look for problems @ backup logs (via the backups report). With those points we'll had backup covered. And those backups will be inconsistent because some files will be recorded in the files.xml and/or inforef.xml files, leading to a non-existing file). So then: 5) In restore...simply, we'll skip any non-existing file, surely WARNING about that also in logs, and surely for interactive (via UI) operations, also informing in a similar way than in backup operations. As far as we have decided to skip those files completely... they will be lead to 404s, than IMO is better than "faking" with an empty file. Finally, note that both backup and restore steps.... do support the ability to return "results" at the end of the execution and later those results are (should be) shown at the end of any interactive (UI) execution (see controller->get_results() uses). So perhaps the way to implement the display of information in points 3 and 5 above is to use those results to transmit information to the UI. I think they have been used that way recently to show details about the backup and restore of file references, we just would need to return a new results element. Take a look to backup_ui_stage_complete and restore_ui_stage_complete classes. And that's it it, IMO. In summary: a) we always log any problem, both in backup and restore. b) we always generate and return one new 'missing_files_in_pool' (for backup) and 'missing_files_in_backup' (for restore) results array and make it to be displayed by the UI. c) in automated backups, we use the logs to decide about the new BACKUP_STATUS_WARNINGS and inform about it via mail. d) we need to know what's leading to those missing files and try to fix it if possible (another issue). e) the new status in automated backups and the information shown by the UI operations must be documented somewhere in Docs. Hope it helps...ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          PS, surely this could be split into subtasks, for your consideration

          Show
          Eloy Lafuente (stronk7) added a comment - PS, surely this could be split into subtasks, for your consideration
          Hide
          Frédéric Massart added a comment -

          Hi Eloy,

          Thanks for your feedback. Yes, I had to manually delete the file to reproduce the issue.

          I agree with you, creating an empty file is not a solution. I hacked it like that in the patch so that the restore worked. Although I am having trouble passing information from where the exception is raised. I could load (a copy) of backup_controller to add some logging information but that does not help for the $results.

          Catching the exception in a caller class does not help much as we are skipping some XML writing which causes other exceptions to be raised. Plus, most of the steps do not return any $result at all, as far as I've seen the only one returning some is backup_store_backup_file (backup_structure_step even override execute() which does not call define_execution() any more).

          Anyway I am a bit confused lol! As you know the design of the backup system, that'd be nice of you if you could help me going in the right direction.

          Thanks!

          Show
          Frédéric Massart added a comment - Hi Eloy, Thanks for your feedback. Yes, I had to manually delete the file to reproduce the issue. I agree with you, creating an empty file is not a solution. I hacked it like that in the patch so that the restore worked. Although I am having trouble passing information from where the exception is raised. I could load (a copy) of backup_controller to add some logging information but that does not help for the $results. Catching the exception in a caller class does not help much as we are skipping some XML writing which causes other exceptions to be raised. Plus, most of the steps do not return any $result at all, as far as I've seen the only one returning some is backup_store_backup_file (backup_structure_step even override execute() which does not call define_execution() any more). Anyway I am a bit confused lol! As you know the design of the backup system, that'd be nice of you if you could help me going in the right direction. Thanks!
          Hide
          Chris Fryer added a comment -

          I'm experiencing this problem. We upgraded from 1.9 to 2.2, but the problematic file was attached to a forum post that was made after the upgrade was complete.

          I experience this with the interactive backup and the scheduled backup. What's worrying is that the problem course stops all the remaining courses from being backed up. I see the following in the PHP error log:

          Jul  3 21:27:59 moodle-file php: backup_auto_failed_on_course AA101
          Jul  3 21:27:59 moodle-file php: Exception: storedfilecannotread
          Jul  3 21:27:59 moodle-file php: backup_auto_failed_on_course BB202
          Jul  3 21:27:59 moodle-file php: Exception: ddltablealreadyexists backup_ids_temp
          Jul  3 21:28:00 moodle-file php: backup_auto_failed_on_course CC303
          Jul  3 21:28:00 moodle-file php: Exception: ddltablealreadyexists backup_ids_temp
          ...
          

          After the "storedfilecannotread" exception occurs, backup_ids_temp is not truncated or dropped, so the remaining courses cannot back up.

          Show
          Chris Fryer added a comment - I'm experiencing this problem. We upgraded from 1.9 to 2.2, but the problematic file was attached to a forum post that was made after the upgrade was complete. I experience this with the interactive backup and the scheduled backup. What's worrying is that the problem course stops all the remaining courses from being backed up. I see the following in the PHP error log: Jul 3 21:27:59 moodle-file php: backup_auto_failed_on_course AA101 Jul 3 21:27:59 moodle-file php: Exception: storedfilecannotread Jul 3 21:27:59 moodle-file php: backup_auto_failed_on_course BB202 Jul 3 21:27:59 moodle-file php: Exception: ddltablealreadyexists backup_ids_temp Jul 3 21:28:00 moodle-file php: backup_auto_failed_on_course CC303 Jul 3 21:28:00 moodle-file php: Exception: ddltablealreadyexists backup_ids_temp ... After the "storedfilecannotread" exception occurs, backup_ids_temp is not truncated or dropped, so the remaining courses cannot back up.
          Hide
          Frédéric Massart added a comment -

          Ok, here is a patch that will prevent the backup/restore to fail. If some of you experiencing problems could test it that'd be much appreciated. I haven't implemented any error reporting because I was waiting for Eloy's feedback, which he didn't have time to give, and is now on holiday. Unfortunately, this issue won't be fixed during this sprint.

          Show
          Frédéric Massart added a comment - Ok, here is a patch that will prevent the backup/restore to fail. If some of you experiencing problems could test it that'd be much appreciated. I haven't implemented any error reporting because I was waiting for Eloy's feedback, which he didn't have time to give, and is now on holiday. Unfortunately, this issue won't be fixed during this sprint.
          Hide
          Hartmut Scherer added a comment -

          Thanks a lot, Frédéric, for your patch. Before I applied the patch today, I got the usual error message when I tried to back up a course (Ethics). After I applied your patch, I was able to back up the course. This is excellent.
          Then, I tried to restore the course. Now I received the following error message:

          Debug info: Got a packet bigger than 'max_allowed_packet' bytes
          INSERT INTO mdl_backup_controllers (backupid,operation,type,itemid,format,interactive,purpose,userid,status,execution,executiontime,checksum,controller,timecreated,timemodified) VALUES(?,?,?,?,?,?,?,?,?,?,?,?,?,?,?)
          [array (
          0 => '63ad15fa7032143a87304268e318f19d',
          1 => 'restore',
          2 => 'course',
          3 => 177,
          4 => 'moodle2',
          5 => 1,
          6 => 10,
          7 => '3',
          8 => 500,
          9 => 1,
          10 => 0,
          11 => '8285ebdea632bac372a62e4123e12b1f',
          12 =>
          'TzoxODoicmVzdG9yZV9jb250cm9sbGVyIjoxOTp7czoxMDoiACoAdGVtcGRpciI7czozMjoiMGFhY2QyNDg3ZjQxZGMyYTA5ZDBmODk3Y2ZiZTA1ODIiO3M6MTI6IgAqAHJlc3RvcmVpZCI7czozMjoiNjNhZDE1ZmE3MDMyMTQzYTg3MzA0MjY4ZTMxOGYxOWQiO3M6MTE6IgAqAGNvdXJzZWlkIjtpOjE3Nzt....
          13 => 1341804946,
          14 => 0,
          )]

          Stack trace:

          line 397 of /lib/dml/moodle_database.php: dml_write_exception thrown
          line 893 of /lib/dml/mysqli_native_moodle_database.php: call to moodle_database->query_end()
          line 935 of /lib/dml/mysqli_native_moodle_database.php: call to mysqli_native_moodle_database->insert_record_raw()
          line 69 of /backup/util/dbops/restore_controller_dbops.class.php: call to mysqli_native_moodle_database->insert_record()
          line 371 of /backup/controller/restore_controller.class.php: call to restore_controller_dbops::save_controller()
          line 137 of /backup/util/ui/base_ui.class.php: call to restore_controller->save_controller()
          line 52 of /backup/restore.php: call to base_ui->save_controller()

          Skip Navigation
          Skip Settings

          Moodle Logo

          Please note that I was unable to copy the complete line 12.

          Show
          Hartmut Scherer added a comment - Thanks a lot, Frédéric, for your patch. Before I applied the patch today, I got the usual error message when I tried to back up a course (Ethics). After I applied your patch, I was able to back up the course. This is excellent. Then, I tried to restore the course. Now I received the following error message: Debug info: Got a packet bigger than 'max_allowed_packet' bytes INSERT INTO mdl_backup_controllers (backupid,operation,type,itemid,format,interactive,purpose,userid,status,execution,executiontime,checksum,controller,timecreated,timemodified) VALUES(?,?,?,?,?,?,?,?,?,?,?,?,?,?,?) [array ( 0 => '63ad15fa7032143a87304268e318f19d', 1 => 'restore', 2 => 'course', 3 => 177, 4 => 'moodle2', 5 => 1, 6 => 10, 7 => '3', 8 => 500, 9 => 1, 10 => 0, 11 => '8285ebdea632bac372a62e4123e12b1f', 12 => 'TzoxODoicmVzdG9yZV9jb250cm9sbGVyIjoxOTp7czoxMDoiACoAdGVtcGRpciI7czozMjoiMGFhY2QyNDg3ZjQxZGMyYTA5ZDBmODk3Y2ZiZTA1ODIiO3M6MTI6IgAqAHJlc3RvcmVpZCI7czozMjoiNjNhZDE1ZmE3MDMyMTQzYTg3MzA0MjY4ZTMxOGYxOWQiO3M6MTE6IgAqAGNvdXJzZWlkIjtpOjE3Nzt.... 13 => 1341804946, 14 => 0, )] Stack trace: line 397 of /lib/dml/moodle_database.php: dml_write_exception thrown line 893 of /lib/dml/mysqli_native_moodle_database.php: call to moodle_database->query_end() line 935 of /lib/dml/mysqli_native_moodle_database.php: call to mysqli_native_moodle_database->insert_record_raw() line 69 of /backup/util/dbops/restore_controller_dbops.class.php: call to mysqli_native_moodle_database->insert_record() line 371 of /backup/controller/restore_controller.class.php: call to restore_controller_dbops::save_controller() line 137 of /backup/util/ui/base_ui.class.php: call to restore_controller->save_controller() line 52 of /backup/restore.php: call to base_ui->save_controller() Skip Navigation Skip Settings Moodle Logo Please note that I was unable to copy the complete line 12.
          Hide
          Frédéric Massart added a comment -

          Hi Hartmut, thanks for testing this patch and giving feedback. I am afraid that the issue you are facing is a known one and, as for now, most likely to be fixed on your side, see MDL-30954.

          That'd be great if you could provide information about that error on MDL-30954, including your system details, Moodle/MySQL/PHP version, and anything that could help us.

          Thank you!

          Show
          Frédéric Massart added a comment - Hi Hartmut, thanks for testing this patch and giving feedback. I am afraid that the issue you are facing is a known one and, as for now, most likely to be fixed on your side, see MDL-30954 . That'd be great if you could provide information about that error on MDL-30954 , including your system details, Moodle/MySQL/PHP version, and anything that could help us. Thank you!
          Hide
          Jason Bennett added a comment -

          We're experiencing this issue on a Moodle 2.2 instance that was not upgraded from 1.9.
          We haven't tried Frédéric's patch but plan to try it right away.

          Error message and stacktrace:

          Can not read file, either file does not exist or there are permission problems
          Stack trace:
          line 174 of /lib/filestorage/stored_file.php: file_exception thrown
          line 90 of /backup/util/helper/backup_file_manager.class.php: call to stored_file->copy_content_to()
          line 101 of /backup/moodle2/backup_custom_fields.php: call to backup_file_manager::copy_file_moodle2backup()
          line 70 of /backup/util/structure/backup_nested_element.class.php: call to file_nested_element->fill_values()
          line 94 of /backup/moodle2/backup_custom_fields.php: call to backup_nested_element->process()
          line 95 of /backup/util/structure/backup_nested_element.class.php: call to file_nested_element->process()
          line 95 of /backup/util/plan/backup_structure_step.class.php: call to backup_nested_element->process()
          line 153 of /backup/util/plan/base_task.class.php: call to backup_structure_step->execute()
          line 148 of /backup/util/plan/base_plan.class.php: call to base_task->execute()
          line 105 of /backup/util/plan/backup_plan.class.php: call to base_plan->execute()
          line 296 of /backup/controller/backup_controller.class.php: call to backup_plan->execute()
          line 111 of /backup/util/ui/backup_ui.class.php: call to backup_controller->execute_plan()
          line 89 of /backup/backup.php: call to backup_ui->execute()

          Show
          Jason Bennett added a comment - We're experiencing this issue on a Moodle 2.2 instance that was not upgraded from 1.9. We haven't tried Frédéric's patch but plan to try it right away. Error message and stacktrace: Can not read file, either file does not exist or there are permission problems Stack trace: line 174 of /lib/filestorage/stored_file.php: file_exception thrown line 90 of /backup/util/helper/backup_file_manager.class.php: call to stored_file->copy_content_to() line 101 of /backup/moodle2/backup_custom_fields.php: call to backup_file_manager::copy_file_moodle2backup() line 70 of /backup/util/structure/backup_nested_element.class.php: call to file_nested_element->fill_values() line 94 of /backup/moodle2/backup_custom_fields.php: call to backup_nested_element->process() line 95 of /backup/util/structure/backup_nested_element.class.php: call to file_nested_element->process() line 95 of /backup/util/plan/backup_structure_step.class.php: call to backup_nested_element->process() line 153 of /backup/util/plan/base_task.class.php: call to backup_structure_step->execute() line 148 of /backup/util/plan/base_plan.class.php: call to base_task->execute() line 105 of /backup/util/plan/backup_plan.class.php: call to base_plan->execute() line 296 of /backup/controller/backup_controller.class.php: call to backup_plan->execute() line 111 of /backup/util/ui/backup_ui.class.php: call to backup_controller->execute_plan() line 89 of /backup/backup.php: call to backup_ui->execute()
          Hide
          Fred Weiss added a comment -

          I have experieinced this problem on Moodle 2.3.1 today.

          Can not read file, either file does not exist or there are permission problems

          More information about this error

          Debug info: S:\moodledata/filedir/eb/bb/ebbb6a820139a3858e9f26c3309adbc37fde789a
          Error code: storedfilecannotread
          Stack trace:
          line 387 of \lib\filestorage\stored_file.php: file_exception thrown
          line 94 of \backup\util\helper\backup_file_manager.class.php: call to stored_file->copy_content_to()
          line 106 of \backup\moodle2\backup_custom_fields.php: call to backup_file_manager::copy_file_moodle2backup()
          line 70 of \backup\util\structure\backup_nested_element.class.php: call to file_nested_element->fill_values()
          line 99 of \backup\moodle2\backup_custom_fields.php: call to backup_nested_element->process()
          line 95 of \backup\util\structure\backup_nested_element.class.php: call to file_nested_element->process()
          line 95 of \backup\util\plan\backup_structure_step.class.php: call to backup_nested_element->process()
          line 153 of \backup\util\plan\base_task.class.php: call to backup_structure_step->execute()
          line 163 of \backup\util\plan\base_plan.class.php: call to base_task->execute()
          line 110 of \backup\util\plan\backup_plan.class.php: call to base_plan->execute()
          line 309 of \backup\controller\backup_controller.class.php: call to backup_plan->execute()
          line 111 of \backup\util\ui\backup_ui.class.php: call to backup_controller->execute_plan()
          line 89 of \backup\backup.php: call to backup_ui->execute()

          Show
          Fred Weiss added a comment - I have experieinced this problem on Moodle 2.3.1 today. Can not read file, either file does not exist or there are permission problems More information about this error Debug info: S:\moodledata/filedir/eb/bb/ebbb6a820139a3858e9f26c3309adbc37fde789a Error code: storedfilecannotread Stack trace: line 387 of \lib\filestorage\stored_file.php: file_exception thrown line 94 of \backup\util\helper\backup_file_manager.class.php: call to stored_file->copy_content_to() line 106 of \backup\moodle2\backup_custom_fields.php: call to backup_file_manager::copy_file_moodle2backup() line 70 of \backup\util\structure\backup_nested_element.class.php: call to file_nested_element->fill_values() line 99 of \backup\moodle2\backup_custom_fields.php: call to backup_nested_element->process() line 95 of \backup\util\structure\backup_nested_element.class.php: call to file_nested_element->process() line 95 of \backup\util\plan\backup_structure_step.class.php: call to backup_nested_element->process() line 153 of \backup\util\plan\base_task.class.php: call to backup_structure_step->execute() line 163 of \backup\util\plan\base_plan.class.php: call to base_task->execute() line 110 of \backup\util\plan\backup_plan.class.php: call to base_plan->execute() line 309 of \backup\controller\backup_controller.class.php: call to backup_plan->execute() line 111 of \backup\util\ui\backup_ui.class.php: call to backup_controller->execute_plan() line 89 of \backup\backup.php: call to backup_ui->execute()
          Hide
          Michael de Raadt added a comment -

          Carrying over to the next sprint.

          Show
          Michael de Raadt added a comment - Carrying over to the next sprint.
          Hide
          Frédéric Massart added a comment -

          Sprint planning.
          Estimated remaining development difficulty: M

          Show
          Frédéric Massart added a comment - Sprint planning. Estimated remaining development difficulty: M
          Hide
          Jason Bennett added a comment -

          Frédéric's patch worked for us.

          Show
          Jason Bennett added a comment - Frédéric's patch worked for us.
          Hide
          Hartmut Scherer added a comment -

          Jason, just a question. Could you back up AND restore the file?

          Show
          Hartmut Scherer added a comment - Jason, just a question. Could you back up AND restore the file?
          Hide
          Jason Bennett added a comment -

          Yes, but to be exact, I backed up a course on Moodle 2.2 and restored it to Moodle 2.3.

          Show
          Jason Bennett added a comment - Yes, but to be exact, I backed up a course on Moodle 2.2 and restored it to Moodle 2.3.
          Hide
          Frédéric Massart added a comment -

          Hi Eloy, here the work I have done on this issue. I don't consider it 'done' but I'd like to have your feedback now to know if I am going towards the right direction.

          You will notice that I have 5 commits, I will merge them together later. I will eventually create subtasks to make it easier to integrate, review and test.

          I am not convinced on the way to pass arguments back from nested elements, especially in the restore part. Well, up to you .

          Thanks!

          Show
          Frédéric Massart added a comment - Hi Eloy, here the work I have done on this issue. I don't consider it 'done' but I'd like to have your feedback now to know if I am going towards the right direction. You will notice that I have 5 commits, I will merge them together later. I will eventually create subtasks to make it easier to integrate, review and test. I am not convinced on the way to pass arguments back from nested elements, especially in the restore part. Well, up to you . Thanks!
          Hide
          Martin Dougiamas added a comment -

          Thanks for working on this Fred!

          Show
          Martin Dougiamas added a comment - Thanks for working on this Fred!
          Hide
          Michael de Raadt added a comment -

          Carrying over to the next sprint.

          Show
          Michael de Raadt added a comment - Carrying over to the next sprint.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Apologizes for the delay, this had disappeared from my list completely. Getting on it now.

          Show
          Eloy Lafuente (stronk7) added a comment - Apologizes for the delay, this had disappeared from my list completely. Getting on it now.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Fréd, overall, I like the code a lot, but some points commented below:

          A) First of all, I'd suggest you to spread the code into 2-3 commits. One for backup, another for restore (this 2 can be together) and another for the automated backups. Just for readability. I started looking at code commit by commit, and it was confusing (some commits undoing work from previous ones).

          B) No problem with restore about how results are handled. You create the results array @ send_files_to_pool(), returning the array and spreading logs ok from the structure step. The only tiny point is, perhaps, that you don't need to have the "$this->task->add_result" call within the loop but, simply, if there are results, add the info once out from the loop.

          C) The part that is trickier is backup. While the plan/tasks/steps there support logging, the structure elements (nested...) do not support logging nor results handling. And I'm not sure if your implementation is 100% correct, because of the nested nature of those structures. It seems that you are always merging both the logs and the results level by level, so at the end those logs and results are in multiple elements. Instead... surely, I'd implement both as sort of parent->add_log|result() that, recursively would "send" the logs and results up-up-up, until the root element (without parent) is found. Of course this requires testing (backup/util/structure, surely), imagine this case: 1 forum with 3 discussions, each with 2 posts. On each discussion, one post produces one log and one result. At the end, the root element (the "structure" one) will have the 3 logs and the 3 results, and the rest of elements won't have any log nor result (because they have, simply, pushed them to the root. I this that case does not work with your current code.

          D) The outcome_from_results() implementation is really strange. I think $outcome == self::BACKUP_STATUS_ERROR is 100% unreachable there.

          E) While reviewing this just discovered that there are a bunch of backup_cron_automated_helper::BACKUP_STATUS_[UNFINISHED|WARNING|ERROR...] in the backup_cron_automated_helper.php file. Surely it would a good moment to change all them to "self::"

          F) There are some minor mistakes in phpdocs like: (warning) Comments must end with dot, (error) Incorrect use of the inline

          {@see}

          tag. Must be

          {@link}

          instead. The former cannot be used inline, some missing @return... surely running the phpdoc checker will tell you better.

          G) Ideally, we should be able to, with phpunit, create one course, some content (forum) with some file... delete it... and run backup & restore, to check everything runs and work as expected (that apart from the "structure" tests commented in C) above. And also, test the same course, but without deleting the file. That will help backporting it to stable branches for sure.

          I) In backup... it seems that we are sending to logs the contenthash or so. I think it's better to send always the path, like in restore logs. Not 100% sure, though. Also, I think we don't need the original exception code there (file_not_found or storedfilecannotread), just the "missing file xx/yy/zz in pool".

          And that's all. As said, the list of points is big, but the code is really near from perfection, only the backup part needs a bit more of love.

          Finally, and apart from all this... I'm really worried about how people is "losing" files. I can imagine some problem with contents coming from 1.9, but there are comments about contents generated in 2.x leading to this error, and we should investigate a bit about that, seriously.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Fréd, overall, I like the code a lot, but some points commented below: A) First of all, I'd suggest you to spread the code into 2-3 commits. One for backup, another for restore (this 2 can be together) and another for the automated backups. Just for readability. I started looking at code commit by commit, and it was confusing (some commits undoing work from previous ones). B) No problem with restore about how results are handled. You create the results array @ send_files_to_pool(), returning the array and spreading logs ok from the structure step. The only tiny point is, perhaps, that you don't need to have the "$this->task->add_result" call within the loop but, simply, if there are results, add the info once out from the loop. C) The part that is trickier is backup. While the plan/tasks/steps there support logging, the structure elements (nested...) do not support logging nor results handling. And I'm not sure if your implementation is 100% correct, because of the nested nature of those structures. It seems that you are always merging both the logs and the results level by level, so at the end those logs and results are in multiple elements. Instead... surely, I'd implement both as sort of parent->add_log|result() that, recursively would "send" the logs and results up-up-up, until the root element (without parent) is found. Of course this requires testing (backup/util/structure, surely), imagine this case: 1 forum with 3 discussions, each with 2 posts. On each discussion, one post produces one log and one result. At the end, the root element (the "structure" one) will have the 3 logs and the 3 results, and the rest of elements won't have any log nor result (because they have, simply, pushed them to the root. I this that case does not work with your current code. D) The outcome_from_results() implementation is really strange. I think $outcome == self::BACKUP_STATUS_ERROR is 100% unreachable there. E) While reviewing this just discovered that there are a bunch of backup_cron_automated_helper::BACKUP_STATUS_ [UNFINISHED|WARNING|ERROR...] in the backup_cron_automated_helper.php file. Surely it would a good moment to change all them to "self::" F) There are some minor mistakes in phpdocs like: (warning) Comments must end with dot, (error) Incorrect use of the inline {@see} tag. Must be {@link} instead. The former cannot be used inline, some missing @return... surely running the phpdoc checker will tell you better. G) Ideally, we should be able to, with phpunit, create one course, some content (forum) with some file... delete it... and run backup & restore, to check everything runs and work as expected (that apart from the "structure" tests commented in C) above. And also, test the same course, but without deleting the file. That will help backporting it to stable branches for sure. I) In backup... it seems that we are sending to logs the contenthash or so. I think it's better to send always the path, like in restore logs. Not 100% sure, though. Also, I think we don't need the original exception code there (file_not_found or storedfilecannotread), just the "missing file xx/yy/zz in pool". And that's all. As said, the list of points is big, but the code is really near from perfection, only the backup part needs a bit more of love. Finally, and apart from all this... I'm really worried about how people is "losing" files. I can imagine some problem with contents coming from 1.9, but there are comments about contents generated in 2.x leading to this error, and we should investigate a bit about that, seriously. Ciao
          Hide
          Frédéric Massart added a comment - - edited

          Thanks a lot Eloy. I have updated the branch, and my comments follow.


          A) First of all, I'd suggest you to spread the code into 2-3 commits. One for backup, another for restore (this 2 can be together) and another for the automated backups. Just for readability. I started looking at code commit by commit, and it was confusing (some commits undoing work from previous ones).

          Done.


          B) No problem with restore about how results are handled. You create the results array @ send_files_to_pool(), returning the array and spreading logs ok from the structure step. The only tiny point is, perhaps, that you don't need to have the "$this->task->add_result" call within the loop but, simply, if there are results, add the info once out from the loop.

          Done, add_result is out of the loop.


          C) The part that is trickier is backup. While the plan/tasks/steps there support logging, the structure elements (nested...) do not support logging nor results handling. And I'm not sure if your implementation is 100% correct, because of the nested nature of those structures. It seems that you are always merging both the logs and the results level by level, so at the end those logs and results are in multiple elements. Instead... surely, I'd implement both as sort of parent->add_log|result() that, recursively would "send" the logs and results up-up-up, until the root element (without parent) is found. Of course this requires testing (backup/util/structure, surely), imagine this case: 1 forum with 3 discussions, each with 2 posts. On each discussion, one post produces one log and one result. At the end, the root element (the "structure" one) will have the 3 logs and the 3 results, and the rest of elements won't have any log nor result (because they have, simply, pushed them to the root. I this that case does not work with your current code.

          I don't think I can multiply results/logs with the current code. I managed to send back the results/logs so that the last parent to work with them is the backup_nested_element. However, it makes it a lot neater to set them in the root parent directly in add_result(). Regarding multiple elements, results are supposed to be an array where the key is the error message, if it not the case, than yes there could be some multiple errors, but they would be useless because the backup won't recognize them. Regarding the logs, it's a bit different, there could be similar ones but I think that it is a good option as several files could be missing and file_nested_element should log each of them.

          All in all, I am not sure if I got the situation here, that's the trickiest bit of the whole patch and the place where your wisdom was highly required. I have updated add_result and add_log, please tell me what you think about it.


          D) The outcome_from_results() implementation is really strange. I think $outcome == self::BACKUP_STATUS_ERROR is 100% unreachable there.

          And you are right. The reason I added the test for BACKUP_STATUS_ERROR is because I wanted this case to be already handled for future implementations. I've changed the method so that it's using `switch` because it makes more sense.


          E) While reviewing this just discovered that there are a bunch of backup_cron_automated_helper::BACKUP_STATUS_[UNFINISHED|WARNING|ERROR...] in the backup_cron_automated_helper.php file. Surely it would a good moment to change all them to "self::"

          Done.


          F) There are some minor mistakes in phpdocs like: (warning) Comments must end with dot, (error) Incorrect use of the inline {@see} tag. Must be {@link} instead. The former cannot be used inline, some missing @return... surely running the phpdoc checker will tell you better.

          Done.


          G) Ideally, we should be able to, with phpunit, create one course, some content (forum) with some file... delete it... and run backup & restore, to check everything runs and work as expected (that apart from the "structure" tests commented in C) above. And also, test the same course, but without deleting the file. That will help backporting it to stable branches for sure.

          Could this be files as second issue? AFAIK that'd be a massive work, I could not find existing backup tests to start from.


          I) In backup... it seems that we are sending to logs the contenthash or so. I think it's better to send always the path, like in restore logs. Not 100% sure, though. Also, I think we don't need the original exception code there (file_not_found or storedfilecannotread), just the "missing file xx/yy/zz in pool".

          Done.

          For 2 files missing in 2 different forum posts, and 1 file missing in a file resource, the log displays:

          missing file in pool: /home/fred/www/repositories/stable_master/moodledata/filedir/97/b9/97b93956bb1a2dd84f50593995eb2edca49926ef
          missing file in pool: /home/fred/www/repositories/stable_master/moodledata/filedir/ad/f4/adf4691481921f133e1391338db1bc995e84689b
          missing file in pool: /home/fred/www/repositories/stable_master/moodledata/filedir/da/39/da39a3ee5e6b4b0d3255bfef95601890afd80709
          

          Regarding the backport, I'd say that backporting to 2.3 is pretty safe. I don't know about 2.2, has a lot of change happened in backups between 2.2 and 2.3?

          I think the cause of this issue could come from a lack of permissions or something like that. Someone mentioned that they copied the moodledata folder over after backuping without the files, and that it went all good, meaning that the file exist at that location. I don't know the history from 1.9, but let's assume that an admin ran the upgrade script not using `sudo -u www-data` or even as root, some permissions might have been altered. From now on, the logs will display the path to the file, which will make it easier for them to tell us the permissions that were set on those, and to eventually find the origin of the problem.

          Show
          Frédéric Massart added a comment - - edited Thanks a lot Eloy. I have updated the branch, and my comments follow. A) First of all, I'd suggest you to spread the code into 2-3 commits. One for backup, another for restore (this 2 can be together) and another for the automated backups. Just for readability. I started looking at code commit by commit, and it was confusing (some commits undoing work from previous ones). Done. B) No problem with restore about how results are handled. You create the results array @ send_files_to_pool(), returning the array and spreading logs ok from the structure step. The only tiny point is, perhaps, that you don't need to have the "$this->task->add_result" call within the loop but, simply, if there are results, add the info once out from the loop. Done, add_result is out of the loop. C) The part that is trickier is backup. While the plan/tasks/steps there support logging, the structure elements (nested...) do not support logging nor results handling. And I'm not sure if your implementation is 100% correct, because of the nested nature of those structures. It seems that you are always merging both the logs and the results level by level, so at the end those logs and results are in multiple elements. Instead... surely, I'd implement both as sort of parent->add_log|result() that, recursively would "send" the logs and results up-up-up, until the root element (without parent) is found. Of course this requires testing (backup/util/structure, surely), imagine this case: 1 forum with 3 discussions, each with 2 posts. On each discussion, one post produces one log and one result. At the end, the root element (the "structure" one) will have the 3 logs and the 3 results, and the rest of elements won't have any log nor result (because they have, simply, pushed them to the root. I this that case does not work with your current code. I don't think I can multiply results/logs with the current code. I managed to send back the results/logs so that the last parent to work with them is the backup_nested_element. However, it makes it a lot neater to set them in the root parent directly in add_result(). Regarding multiple elements, results are supposed to be an array where the key is the error message, if it not the case, than yes there could be some multiple errors, but they would be useless because the backup won't recognize them. Regarding the logs, it's a bit different, there could be similar ones but I think that it is a good option as several files could be missing and file_nested_element should log each of them. All in all, I am not sure if I got the situation here, that's the trickiest bit of the whole patch and the place where your wisdom was highly required. I have updated add_result and add_log, please tell me what you think about it. D) The outcome_from_results() implementation is really strange. I think $outcome == self::BACKUP_STATUS_ERROR is 100% unreachable there. And you are right. The reason I added the test for BACKUP_STATUS_ERROR is because I wanted this case to be already handled for future implementations. I've changed the method so that it's using `switch` because it makes more sense. E) While reviewing this just discovered that there are a bunch of backup_cron_automated_helper::BACKUP_STATUS_ [UNFINISHED|WARNING|ERROR...] in the backup_cron_automated_helper.php file. Surely it would a good moment to change all them to "self::" Done. F) There are some minor mistakes in phpdocs like: (warning) Comments must end with dot, (error) Incorrect use of the inline {@see} tag. Must be {@link} instead. The former cannot be used inline, some missing @return... surely running the phpdoc checker will tell you better. Done. G) Ideally, we should be able to, with phpunit, create one course, some content (forum) with some file... delete it... and run backup & restore, to check everything runs and work as expected (that apart from the "structure" tests commented in C) above. And also, test the same course, but without deleting the file. That will help backporting it to stable branches for sure. Could this be files as second issue? AFAIK that'd be a massive work, I could not find existing backup tests to start from. I) In backup... it seems that we are sending to logs the contenthash or so. I think it's better to send always the path, like in restore logs. Not 100% sure, though. Also, I think we don't need the original exception code there (file_not_found or storedfilecannotread), just the "missing file xx/yy/zz in pool". Done. – For 2 files missing in 2 different forum posts, and 1 file missing in a file resource, the log displays: missing file in pool: /home/fred/www/repositories/stable_master/moodledata/filedir/97/b9/97b93956bb1a2dd84f50593995eb2edca49926ef missing file in pool: /home/fred/www/repositories/stable_master/moodledata/filedir/ad/f4/adf4691481921f133e1391338db1bc995e84689b missing file in pool: /home/fred/www/repositories/stable_master/moodledata/filedir/da/39/da39a3ee5e6b4b0d3255bfef95601890afd80709 Regarding the backport, I'd say that backporting to 2.3 is pretty safe. I don't know about 2.2, has a lot of change happened in backups between 2.2 and 2.3? I think the cause of this issue could come from a lack of permissions or something like that. Someone mentioned that they copied the moodledata folder over after backuping without the files, and that it went all good, meaning that the file exist at that location. I don't know the history from 1.9, but let's assume that an admin ran the upgrade script not using `sudo -u www-data` or even as root, some permissions might have been altered. From now on, the logs will display the path to the file, which will make it easier for them to tell us the permissions that were set on those, and to eventually find the origin of the problem.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Fred, I think it's looking 99% ok. Some minor comments:

          0) Right now it does not merge ok but conflicting. Easy to fix.

          1) With your code, only backup_nested_element is able to both ->add_log() and ->add_result(), so it does not have sense to call ->after_processing_element() for final_element (aka, backup/util/structure/backup_nested_element.class.php #96 seems unnecessary). That will save zillions of calls.

          2) Given 1) the conditions @ after_processing_element() are also unnecessary. If we ensure that the function is only called for backup_nested_element, we can take rid of them. As much we could be checking if the passed object is instance of or so (really, really cheaper than method_exists).

          3) Note I still don't understand why, at some point, you don't get duplicate log entries. In my mind that should be happening clearly, but I'm getting old and surely I'm wrong, so let's ignore that. In any case, it's better to have the problems logged twice than zero. No problem with results (key-based array).

          4) The add_log(), add_result() and after_processing_element() methods, as far as are only used by backup_nested_element and descendants can be, better, be declared protected, to avoid using them for custom logins or other crazy-devs ideas.

          5) I'm not still completely convinced about the outcome_from_results() implementation, but it does its work for now, so let's leave it that way... perhaps it wouldn't be a bad idea to add some comments before closing the switch() statement about adding more warning/error $code cases there in the future. Just to clarify its use.

          6) If possible, please, create a new issue linked with this about to introduce some backup/restore operations in phpunit tests, surely small courses, forced to produce some well-known results (like this file missing and friends). TIA.

          ZZ) If you agree with the points above and they don't break anything obvious... +1 for sending this to integration once done. Great work!

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Fred, I think it's looking 99% ok. Some minor comments: 0) Right now it does not merge ok but conflicting. Easy to fix. 1) With your code, only backup_nested_element is able to both ->add_log() and ->add_result(), so it does not have sense to call ->after_processing_element() for final_element (aka, backup/util/structure/backup_nested_element.class.php #96 seems unnecessary). That will save zillions of calls. 2) Given 1) the conditions @ after_processing_element() are also unnecessary. If we ensure that the function is only called for backup_nested_element, we can take rid of them. As much we could be checking if the passed object is instance of or so (really, really cheaper than method_exists). 3) Note I still don't understand why, at some point, you don't get duplicate log entries. In my mind that should be happening clearly, but I'm getting old and surely I'm wrong, so let's ignore that. In any case, it's better to have the problems logged twice than zero. No problem with results (key-based array). 4) The add_log(), add_result() and after_processing_element() methods, as far as are only used by backup_nested_element and descendants can be, better, be declared protected, to avoid using them for custom logins or other crazy-devs ideas. 5) I'm not still completely convinced about the outcome_from_results() implementation, but it does its work for now, so let's leave it that way... perhaps it wouldn't be a bad idea to add some comments before closing the switch() statement about adding more warning/error $code cases there in the future. Just to clarify its use. 6) If possible, please, create a new issue linked with this about to introduce some backup/restore operations in phpunit tests, surely small courses, forced to produce some well-known results (like this file missing and friends). TIA. ZZ) If you agree with the points above and they don't break anything obvious... +1 for sending this to integration once done. Great work!
          Hide
          Frédéric Massart added a comment -

          0) Rebased.

          1) Something weird happened. I got rid of after_processing_element and added support for the grand parent. I've somehow messed up my branches because it was not included in this one. So well, please have a quick look at this commit (link).

          2) Method removed. See #1

          3) See #1 for use of grand parent, which should prevent (handle properly) this.

          4) Methods set as protected.

          5) I've added a comment there.

          6) Done in MDL-35075.

          ZZ) I do, and I don't think I've killed anything, but I'd just like you to have a quick look at the commit in #1, as it reflects your initial comments that were missing in my github branch. Sorry about that back and forth.

          I've also created branches for 2.2 and 2.3. We should backport this right?

          Show
          Frédéric Massart added a comment - 0) Rebased. 1) Something weird happened. I got rid of after_processing_element and added support for the grand parent. I've somehow messed up my branches because it was not included in this one. So well, please have a quick look at this commit (link) . 2) Method removed. See #1 3) See #1 for use of grand parent, which should prevent (handle properly) this. 4) Methods set as protected. 5) I've added a comment there. 6) Done in MDL-35075 . ZZ) I do, and I don't think I've killed anything, but I'd just like you to have a quick look at the commit in #1, as it reflects your initial comments that were missing in my github branch. Sorry about that back and forth. I've also created branches for 2.2 and 2.3. We should backport this right?
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Sending to integration...

          Show
          Eloy Lafuente (stronk7) added a comment - Sending to integration...
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Added labels about to document the new backup warnings, both on execution and logs/email.

          Show
          Eloy Lafuente (stronk7) added a comment - Added labels about to document the new backup warnings, both on execution and logs/email.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated (22, 23 & master), thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (22, 23 & master), thanks!
          Hide
          Adrian Greeve added a comment -

          Tested on 2.2, 2.3 and master.
          Everything works and there were no errors.
          Test passed.

          Show
          Adrian Greeve added a comment - Tested on 2.2, 2.3 and master. Everything works and there were no errors. Test passed.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Many thanks for the hard work.

          These changes have been spread upstream and are already available in the git and cvs repositories.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Many thanks for the hard work. These changes have been spread upstream and are already available in the git and cvs repositories. Ciao
          Hide
          Helen Foster added a comment -

          Fred (or anyone else!), regarding docs_required, please could you advise on exactly what information needs to be added to Moodle Docs.

          Eloy mentions "document the new backup warnings, both on execution and logs/email." Do the warning pages include links to Moodle Docs and if so, what are the links?

          Show
          Helen Foster added a comment - Fred (or anyone else!), regarding docs_required, please could you advise on exactly what information needs to be added to Moodle Docs. Eloy mentions "document the new backup warnings, both on execution and logs/email." Do the warning pages include links to Moodle Docs and if so, what are the links?
          Hide
          Frédéric Massart added a comment -

          Hi Helen,

          the warning does not include links to any documentation. It is a new status that a backup can have, and it will be displayed in the cron logs, the backup report page and the email sent to the administrator.
          This issue also includes a message to the user when some files were missing which is displayed exactly like the one warning the user about file references in a backup.

          Ping me if you need more information!

          Did I miss something Eloy?

          Show
          Frédéric Massart added a comment - Hi Helen, the warning does not include links to any documentation. It is a new status that a backup can have, and it will be displayed in the cron logs, the backup report page and the email sent to the administrator. This issue also includes a message to the user when some files were missing which is displayed exactly like the one warning the user about file references in a backup. Ping me if you need more information! Did I miss something Eloy?
          Hide
          Mary Cooch added a comment -

          Removing docs_required label as my understanding from the comments is that this does not require user docs. Please let me know if I'm mistaken.

          Show
          Mary Cooch added a comment - Removing docs_required label as my understanding from the comments is that this does not require user docs. Please let me know if I'm mistaken.

            People

            • Votes:
              11 Vote for this issue
              Watchers:
              17 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: