Moodle
  1. Moodle
  2. MDL-33364

Automated backups always returning "ok" for all courses, ignoring any error.

    Details

    • Testing Instructions:
      Hide

      Testing this requires:

      • terminal access.
      • config.php edition.
      • hacking code.
      • database access.
      • one site with a few courses.
      • Have a correct email address as main admin.

      0/ Set debugging to DEBUG DEVELOPER level.

      1/ Comment $CFG->noemailever = true; if it's set (testing includes receiving one email).

      2/ Setup this also in config.php (the path below must be any one where the web server has write access):

      $CFG->backup_file_logger_extra = '/tmp/restorelogger.txt';
      $CFG->backup_file_logger_extra_level = 50;
      

      3/ Go to admin -> courses -> backups -> automated backup settings and set:

      1. Active: manual
      2. Automated backup storage: Specified directory for automated backups
      3. Save to: /tmp (or any other dir where the web server has write access).

      4/ With the web server user execute: admin/cli/automated_backups.php

      5/ Ignore any output in the execution window. At the time of writing these test, there are some notices still pending to be fixed related to stored files.

      6/ Look to the backup_courses table with phpadmin or similar tool.

      7/ TEST: There is one record for each course.

      8/ TEST: Apart from any skipped (value=3) laststatus, all the courses have laststatus = 1 (ok).

      9/ TEST: The admin email have received one email with all courses ok (and skipped if there were some). With title containing "Automated backup status" (no "error" at all in title).

      10/ TEST: Look to the file configured in 2/ ('/tmp/restorelogger.txt' or similar) and verify that there is not any '[error]' occurrence.

      Now, we are going to hack source code to force one exception to happen for some course.

      11/ From the backup_courses table select some of the "courseid" values which laststatus is = 1. Let's call it 'XX' in the following steps.

      12/ Edit backup/util/helper/backup_cron_helper.class.php and in the launch_automated_backup() method, exactly after this line:

      $bc->execute_plan();
      

      add these lines:

      if ($course->id == XX) {
          throw new backup_plan_exception('testexception', array($course->id, $course->shortname), 'debug information here');
      }
      

      (don't forget to change XX by the courseid picked in 11/.
      Save the changes to the file. This will cause course XX automated backup to fail, so we'll be able to check everything goes as expected.

      13/ Execute again, with the web server user: admin/cli/automated_backups.php

      14/ Ignore any output in the execution window. At the time of writing these test, there are some notices still pending to be fixed related to stored files.

      15/ Look to the backup_courses table with phpadmin or similar tool.

      16/ TEST: There is one record for each course.

      17/ TEST: Apart from any skipped (value=3) laststatus, all the courses have laststatus = 1 (ok) BUT the course XX has laststatus = 0 (error).

      18/ TEST: The admin email have received one email with all courses ok (and skipped if there were some) AND ONE error. With high priority and title containing "[ERROR] Automated backup status", and the body providing one link to the backup report where the failed course can be seen.

      19/ TEST: Look to the file configured in 2/ ('/tmp/restorelogger.txt' or similar) and verify that there are two '[error]' occurrences (followed by a '[debug]' one) looking like these:

      [date] [error] backup_auto_failed_on_course course->shortname
      [date] [error]   Exception: testexception XX, course-shortname
      [date] [debug]   Debug: debug information here
      

      That's all. Now failed courses are being informed correctly everywhere.

      Ciao

      Show
      Testing this requires: terminal access. config.php edition. hacking code. database access. one site with a few courses. Have a correct email address as main admin. 0/ Set debugging to DEBUG DEVELOPER level. 1/ Comment $CFG->noemailever = true; if it's set (testing includes receiving one email). 2/ Setup this also in config.php (the path below must be any one where the web server has write access): $CFG->backup_file_logger_extra = '/tmp/restorelogger.txt'; $CFG->backup_file_logger_extra_level = 50; 3/ Go to admin -> courses -> backups -> automated backup settings and set: Active: manual Automated backup storage: Specified directory for automated backups Save to: /tmp (or any other dir where the web server has write access). 4/ With the web server user execute: admin/cli/automated_backups.php 5/ Ignore any output in the execution window. At the time of writing these test, there are some notices still pending to be fixed related to stored files. 6/ Look to the backup_courses table with phpadmin or similar tool. 7/ TEST: There is one record for each course. 8/ TEST: Apart from any skipped (value=3) laststatus, all the courses have laststatus = 1 (ok). 9/ TEST: The admin email have received one email with all courses ok (and skipped if there were some). With title containing "Automated backup status" (no "error" at all in title). 10/ TEST: Look to the file configured in 2/ ('/tmp/restorelogger.txt' or similar) and verify that there is not any ' [error] ' occurrence. Now, we are going to hack source code to force one exception to happen for some course. 11/ From the backup_courses table select some of the "courseid" values which laststatus is = 1. Let's call it 'XX' in the following steps. 12/ Edit backup/util/helper/backup_cron_helper.class.php and in the launch_automated_backup() method, exactly after this line: $bc->execute_plan(); add these lines: if ($course->id == XX) { throw new backup_plan_exception('testexception', array($course->id, $course->shortname), 'debug information here'); } (don't forget to change XX by the courseid picked in 11/. Save the changes to the file. This will cause course XX automated backup to fail, so we'll be able to check everything goes as expected. 13/ Execute again, with the web server user: admin/cli/automated_backups.php 14/ Ignore any output in the execution window. At the time of writing these test, there are some notices still pending to be fixed related to stored files. 15/ Look to the backup_courses table with phpadmin or similar tool. 16/ TEST: There is one record for each course. 17/ TEST: Apart from any skipped (value=3) laststatus, all the courses have laststatus = 1 (ok) BUT the course XX has laststatus = 0 (error). 18/ TEST: The admin email have received one email with all courses ok (and skipped if there were some) AND ONE error. With high priority and title containing " [ERROR] Automated backup status", and the body providing one link to the backup report where the failed course can be seen. 19/ TEST: Look to the file configured in 2/ ('/tmp/restorelogger.txt' or similar) and verify that there are two ' [error] ' occurrences (followed by a ' [debug] ' one) looking like these: [date] [error] backup_auto_failed_on_course course->shortname [date] [error] Exception: testexception XX, course-shortname [date] [debug] Debug: debug information here That's all. Now failed courses are being informed correctly everywhere. Ciao
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      41229

      Description

      Credit for this goes to Kris Stokking & MoodleRooms.

      It seems that in ca1292e014de76f5d8ed79017eb1c16d00c47096 one important error was introduced leading to the backup_cron_helper::launch_automated_backup() method so any backup is always returned as "ok", completely ignoring any error that could have happened in the process.

      It has some bad consequences:

      • email to admins is misleading, reporting all courses as backup ok when they can be incorrect.
      • deletion of old (correct) backups and storage of new (incorrect) backups.

      So this needs to be fixed ASAP in order to:

      1) Fix the method to return the real outcome of the execution.
      2) Raise the logging to ERROR level (currently it's warning only).
      3) Test it works as expected.

        Activity

        Hide
        Eloy Lafuente (stronk7) added a comment -

        Sending to integration. Testing instructions are a bit complex, but needed. Testing under 21_STABLE and master should be enough.

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Sending to integration. Testing instructions are a bit complex, but needed. Testing under 21_STABLE and master should be enough. Ciao
        Hide
        Sam Hemelryk added a comment -

        Thanks Eloy, changes look like a really good improvement.
        Has been integrated now and is ready for testing.

        Show
        Sam Hemelryk added a comment - Thanks Eloy, changes look like a really good improvement. Has been integrated now and is ready for testing.
        Hide
        Rossiani Wijaya added a comment -

        This looks good.

        Thanks for fixing.

        Test passed.

        Show
        Rossiani Wijaya added a comment - This looks good. Thanks for fixing. Test passed.
        Hide
        Dan Poltawski added a comment -

        Congratulations!

        Your work has made into the latest Moodle release!

        You are only authorised to celebrate after testing 15 Moodle 2.3 QA tests, thanks!

        Show
        Dan Poltawski added a comment - Congratulations! Your work has made into the latest Moodle release! You are only authorised to celebrate after testing 15 Moodle 2.3 QA tests, thanks!

          People

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

            Dates

            • Created:
              Updated:
              Resolved: