Moodle
  1. Moodle
  2. MDL-12037

Backup system ignores errors - we'd like it to report them

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.0
    • Component/s: Backup
    • Labels:
      None
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE
    • Rank:
      36717

      Description

      Currently the backup system tends to ignore errors return values. It calls lots of functions, which may return false, but does not always take action as a result of the return value.

      For 2.0 HEAD, we would like to change it as follows:

      1) When any function called during backup returns false, an error should be printed at that point. The error should make clear which activity or part of the data caused the error, so that [for example] the user could retry the backup without that activity.

      2) The system should continue with the backup as at present, but track whether an error occurred. At the end of a backup a clear message should display ('Backup completed succesfully' or 'Backup completed with errors').

      3) Depending on a config option, admins can set up the system so that when a backup has errors, it is aborted. (The abort would happen either at the point where the error occurs, or at the final stage, to be determined. But either way it wouldn't make the zip file.) This would be an option because sometimes there are backup problems that are not fatal and, for sites that don't have a development team, a partial backup is better than none.

      4) This config option should be configurable in the admin pages in the same place as any existing backup-related options.

      5) Moodle has a system for running scheduled backups. When a scheduled backup is running, this should always be continued (not aborted) even if errors occur, regardless of the config option.

      1. backup_log_patch.txt
        23 kB
        Dan Marsden
      2. backup_log_table.patch
        7 kB
        Dan Marsden
      3. backup_log_table.patch
        7 kB
        Dan Marsden
      4. error.patch
        24 kB
        Dan Marsden

        Issue Links

          Activity

          Hide
          Colin Chambers added a comment -

          I just checked with sam to get a little more detail. Here is what he explained

          an example is function backup_module (line 2278 in backuplib.php in current MOODLE_19).

          foreach ($preferences->mods[$module]->instances as $instance => $object) {
          if (!empty($object->backup))

          { $status = $onemodbackup($bf,$preferences,$instance); }

          }

          As you can see, this will set the final 'status' value to whatever the last module returned. So if modules 1, 2, 3, and 4 fail, but module 5 passes, this function returns success. This is clearly wrong. In addition, this function also does not output any status information - which should be visible always-displayed status information, not just
          debugging() - indicating which module instance has actually failed.

          The way this was written (status value getting overwritten, not displaying information about which module caused a failure) appeared to me to be endemic across backup/restore code - that's what I wanted sorting out.

          Show
          Colin Chambers added a comment - I just checked with sam to get a little more detail. Here is what he explained an example is function backup_module (line 2278 in backuplib.php in current MOODLE_19). foreach ($preferences->mods [$module] ->instances as $instance => $object) { if (!empty($object->backup)) { $status = $onemodbackup($bf,$preferences,$instance); } } As you can see, this will set the final 'status' value to whatever the last module returned. So if modules 1, 2, 3, and 4 fail, but module 5 passes, this function returns success. This is clearly wrong. In addition, this function also does not output any status information - which should be visible always-displayed status information, not just debugging() - indicating which module instance has actually failed. The way this was written (status value getting overwritten, not displaying information about which module caused a failure) appeared to me to be endemic across backup/restore code - that's what I wanted sorting out.
          Hide
          Dan Marsden added a comment -
          • should change to something like this (untested!)
            foreach ($preferences->mods[$module]->instances as $instance => $object) {
            if (!empty($object->backup))
            Unknown macro: { $statusm = $onemodbackup($bf,$preferences,$instance); if (!$statusm) { notify('backup of:'.$module.'-'.$instance.' failed.'); $status = false; } }

            }

          --should also check to see if backup is silent and only notify when not silent, and should log an entry to backup_log

          Show
          Dan Marsden added a comment - should change to something like this (untested!) foreach ($preferences->mods [$module] ->instances as $instance => $object) { if (!empty($object->backup)) Unknown macro: { $statusm = $onemodbackup($bf,$preferences,$instance); if (!$statusm) { notify('backup of:'.$module.'-'.$instance.' failed.'); $status = false; } } } --should also check to see if backup is silent and only notify when not silent, and should log an entry to backup_log
          Hide
          Dan Marsden added a comment -

          here's a patch that works - will commit something close to this in 1.9 and HEAD

          if (!empty($object->backup)) {
          $statusm = $onemodbackup($bf,$preferences,$instance);
          if (!$statusm) {
          if (!defined('BACKUP_SILENTLY'))

          { notify('backup of '.$module.'-'.$object->name.' failed.'); }

          $status = false;
          }
          }

          the $status=false for silent backup imo isn't enough either... - as part of MDL-14736 I plan to add a new field to the backup_log table in HEAD which allows us to push errors like this into it.

          Dan

          Show
          Dan Marsden added a comment - here's a patch that works - will commit something close to this in 1.9 and HEAD if (!empty($object->backup)) { $statusm = $onemodbackup($bf,$preferences,$instance); if (!$statusm) { if (!defined('BACKUP_SILENTLY')) { notify('backup of '.$module.'-'.$object->name.' failed.'); } $status = false; } } the $status=false for silent backup imo isn't enough either... - as part of MDL-14736 I plan to add a new field to the backup_log table in HEAD which allows us to push errors like this into it. Dan
          Hide
          Dan Marsden added a comment -

          patch now in 1.9stable and Head

          Show
          Dan Marsden added a comment - patch now in 1.9stable and Head
          Hide
          Dan Marsden added a comment -

          first version of adding new field to backup_log table. - Note: this doesn't provide any extra logging. - just adds the field to backup_log - if this is ok I will commit to HEAD only, and then we can work on improving error reporting in other areas of the backup/restore.

          Dan

          Show
          Dan Marsden added a comment - first version of adding new field to backup_log table. - Note: this doesn't provide any extra logging. - just adds the field to backup_log - if this is ok I will commit to HEAD only, and then we can work on improving error reporting in other areas of the backup/restore. Dan
          Hide
          Dan Marsden added a comment -

          attaching patch to add backup_add_to_log() calls to backuplib.php so I don't lose it!

          Dan

          Show
          Dan Marsden added a comment - attaching patch to add backup_add_to_log() calls to backuplib.php so I don't lose it! Dan
          Hide
          Dan Marsden added a comment -

          new patch for HEAD to modify backup_log table to include a "backuptype" field - this means we can use it for other backup related logs. (eg silent backups)

          Show
          Dan Marsden added a comment - new patch for HEAD to modify backup_log table to include a "backuptype" field - this means we can use it for other backup related logs. (eg silent backups)
          Hide
          Dan Marsden added a comment -

          updating patch - missed a global call

          Show
          Dan Marsden added a comment - updating patch - missed a global call
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Dan,

          I think backup_log_table.patch (7 kB) looks ok. The older backup_log_patch.txt (23 kB) won't work anymore (backup tables were transferred to core lib/db some weeks ago).

          As far as current scheduled logs are written with 'scheduledbackup' and retrieved in that way, it seems ok... although what happens with "old" records? It seems that the new field isn't filled ever for those old records...

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Dan, I think backup_log_table.patch (7 kB) looks ok. The older backup_log_patch.txt (23 kB) won't work anymore (backup tables were transferred to core lib/db some weeks ago). As far as current scheduled logs are written with 'scheduledbackup' and retrieved in that way, it seems ok... although what happens with "old" records? It seems that the new field isn't filled ever for those old records...
          Hide
          Dan Marsden added a comment -

          thanks Eloy - yeah - the other files are old, I'm only planning on committing the backup_log_table.patch file now, I'll work on the other stuff later once the db changes are in. - good point about old records! - I'll add a check to add 'scheduledbackup' to the backuptype field of all old records. - thanks!

          Show
          Dan Marsden added a comment - thanks Eloy - yeah - the other files are old, I'm only planning on committing the backup_log_table.patch file now, I'll work on the other stuff later once the db changes are in. - good point about old records! - I'll add a check to add 'scheduledbackup' to the backuptype field of all old records. - thanks!
          Hide
          Dan Marsden added a comment -

          Changes now in HEAD

          1. - new field in backup_log table to allow manual backups to report errors there
          2. - improved backuplib.php backup_execute() to write errors to the backup_log table. (especially useful for silent backups)

          There are probably further areas for improvement, but closing this issue as fixed for now - We can use other Tracker issues for further improvements! - if anyone disagrees, feel free to re-open!

          Dan

          Show
          Dan Marsden added a comment - Changes now in HEAD 1. - new field in backup_log table to allow manual backups to report errors there 2. - improved backuplib.php backup_execute() to write errors to the backup_log table. (especially useful for silent backups) There are probably further areas for improvement, but closing this issue as fixed for now - We can use other Tracker issues for further improvements! - if anyone disagrees, feel free to re-open! Dan
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Looks perfect.

          My only comment is if, instead of executing that:

          $DB->execute("UPDATE

          {backup_log}

          SET backuptype='scheduledbackup'");

          we should use one simpler $DB->set_field() or, perhaps, if using 'scheduledbackup' is better for some databases (fill on creation instead of update).

          But I guess it isn't really important.

          Good work. Closing... ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Looks perfect. My only comment is if, instead of executing that: $DB->execute("UPDATE {backup_log} SET backuptype='scheduledbackup'"); we should use one simpler $DB->set_field() or, perhaps, if using 'scheduledbackup' is better for some databases (fill on creation instead of update). But I guess it isn't really important. Good work. Closing... ciao

            People

            • Votes:
              4 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: