Details
Improvement
Closed
Minor
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.
Attachments
Issue Links
Activity
- All
- Comments
- History
- Activity
- Source
- Test Sessions
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
Dan 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
Dan attaching patch to add backup_add_to_log() calls to backuplib.php so I don't lose it!
Dan
Dan
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
Dan 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
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.