From 02da69214f468de0fff584a02cc02bebb94c3056 Mon Sep 17 00:00:00 2001 From: sam marshall Date: Fri, 27 Sep 2013 18:16:21 +0100 Subject: [PATCH 1/7] MDL-42039 Restore: Prevent adding too many form fields at once This change relates to the Moodle forms used to select activities in the restore process. If you add too many form fields at once, it takes a very long time during which we can't report progress. But adding fields in smaller groups takes longer overall, so it's a tough balance. This change splits the adds into groups of 1,000, reporting progress between each. --- backup/util/ui/restore_ui_stage.class.php | 36 +++++++++++++++++++++-- 1 file changed, 34 insertions(+), 2 deletions(-) diff --git a/backup/util/ui/restore_ui_stage.class.php b/backup/util/ui/restore_ui_stage.class.php index dd58ca876ce..189d62753b8 100644 --- a/backup/util/ui/restore_ui_stage.class.php +++ b/backup/util/ui/restore_ui_stage.class.php @@ -486,6 +486,11 @@ class restore_ui_stage_settings extends restore_ui_stage { * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ class restore_ui_stage_schema extends restore_ui_stage { + /** + * @var int Maximum number of settings to add to form at once + */ + const MAX_SETTINGS_BATCH = 1000; + /** * Schema stage constructor * @param backup_moodleform $ui @@ -550,6 +555,12 @@ class restore_ui_stage_schema extends restore_ui_stage { $tasks = $this->ui->get_tasks(); $courseheading = false; + // Track progress through each stage. + $progress = $this->ui->get_progress_reporter(); + $progress->start_progress('Initialise schema stage form', 3); + + $progress->start_progress('', count($tasks)); + $done = 1; $allsettings = array(); foreach ($tasks as $task) { if (!($task instanceof restore_root_task)) { @@ -576,16 +587,37 @@ class restore_ui_stage_schema extends restore_ui_stage { } } } + // Update progress. + $progress->progress($done++); } + $progress->end_progress(); - // Actually add all the settings that we put in the array. - $form->add_settings($allsettings); + // Add settings for tasks in batches of up to 1000. Adding settings + // in larger batches improves performance, but if it takes too long, + // we won't be able to update the progress bar so the backup might + // time out. 1000 is chosen to balance this. + $numsettings = count($allsettings); + $progress->start_progress('', ceil($numsettings / self::MAX_SETTINGS_BATCH)); + $start = 0; + $done = 1; + while($start < $numsettings) { + $length = min(self::MAX_SETTINGS_BATCH, $numsettings - $start); + $form->add_settings(array_slice($allsettings, $start, $length)); + $start += $length; + $progress->progress($done++); + } + $progress->end_progress(); // Add the dependencies for all the settings. + $progress->start_progress('', count($allsettings)); + $done = 1; foreach ($allsettings as $settingtask) { $form->add_dependencies($settingtask[0]); + $progress->progress($done++); } + $progress->end_progress(); + $progress->end_progress(); $this->stageform = $form; } return $this->stageform; From 37ff843d7db758d942fad71c99a288ff5f759a4b Mon Sep 17 00:00:00 2001 From: sam marshall Date: Mon, 30 Sep 2013 11:31:21 +0100 Subject: [PATCH 2/7] MDL-42039 Restore: Add progress reporting between files in resource When a resource contains a very large number of (presumably small) files, we should display progress between each file so that it doesn't time out. --- backup/util/dbops/restore_dbops.class.php | 18 +++++++++++++++++- .../util/plan/restore_structure_step.class.php | 15 +++++++++++++-- .../progress/core_backup_progress.class.php | 14 ++++++++++++++ 3 files changed, 44 insertions(+), 3 deletions(-) diff --git a/backup/util/dbops/restore_dbops.class.php b/backup/util/dbops/restore_dbops.class.php index bdb3775b7a2..533553abe3f 100644 --- a/backup/util/dbops/restore_dbops.class.php +++ b/backup/util/dbops/restore_dbops.class.php @@ -848,6 +848,9 @@ abstract class restore_dbops { * optionally one source itemname to match itemids * put the corresponding files in the pool * + * If you specify a progress reporter, it will get called once per file with + * indeterminate progress. + * * @param string $basepath the full path to the root of unzipped backup file * @param string $restoreid the restore job's identification * @param string $component @@ -858,9 +861,13 @@ abstract class restore_dbops { * @param int|null $olditemid * @param int|null $forcenewcontextid explicit value for the new contextid (skip mapping) * @param bool $skipparentitemidctxmatch + * @param core_backup_progress $progress Optional progress reporter * @return array of result object */ - public static function send_files_to_pool($basepath, $restoreid, $component, $filearea, $oldcontextid, $dfltuserid, $itemname = null, $olditemid = null, $forcenewcontextid = null, $skipparentitemidctxmatch = false) { + public static function send_files_to_pool($basepath, $restoreid, $component, $filearea, + $oldcontextid, $dfltuserid, $itemname = null, $olditemid = null, + $forcenewcontextid = null, $skipparentitemidctxmatch = false, + core_backup_progress $progress = null) { global $DB, $CFG; $backupinfo = backup_general_helper::get_backup_information(basename($basepath)); @@ -924,8 +931,17 @@ abstract class restore_dbops { $fs = get_file_storage(); // Get moodle file storage $basepath = $basepath . '/files/';// Get backup file pool base + // Report progress before query. + if ($progress) { + $progress->progress(); + } $rs = $DB->get_recordset_sql($sql, $params); foreach ($rs as $rec) { + // Report progress each time around loop. + if ($progress) { + $progress->progress(); + } + $file = (object)backup_controller_dbops::decode_backup_temp_info($rec->info); // ignore root dirs (they are created automatically) diff --git a/backup/util/plan/restore_structure_step.class.php b/backup/util/plan/restore_structure_step.class.php index 822c71b493c..300f92d485d 100644 --- a/backup/util/plan/restore_structure_step.class.php +++ b/backup/util/plan/restore_structure_step.class.php @@ -108,10 +108,10 @@ abstract class restore_structure_step extends restore_step { // And process it, dispatch to target methods in step will start automatically $xmlparser->process(); - $progress->end_progress(); // Have finished, launch the after_execute method of all the processing objects $this->launch_after_execute_methods(); + $progress->end_progress(); } /** @@ -223,9 +223,20 @@ abstract class restore_structure_step extends restore_step { * Add all the existing file, given their component and filearea and one backup_ids itemname to match with */ public function add_related_files($component, $filearea, $mappingitemname, $filesctxid = null, $olditemid = null) { + // If the current progress object is set up and ready to receive + // indeterminate progress, then use it, otherwise don't. (This check is + // just in case this function is ever called from somewhere not within + // the execute() method here, which does set up progress like this.) + $progress = $this->get_task()->get_progress(); + if (!$progress->is_in_progress_section() || + $progress->get_current_max() !== core_backup_progress::INDETERMINATE) { + $progress = null; + } + $filesctxid = is_null($filesctxid) ? $this->task->get_old_contextid() : $filesctxid; $results = restore_dbops::send_files_to_pool($this->get_basepath(), $this->get_restoreid(), $component, - $filearea, $filesctxid, $this->task->get_userid(), $mappingitemname, $olditemid); + $filearea, $filesctxid, $this->task->get_userid(), $mappingitemname, $olditemid, null, false, + $progress); $resultstoadd = array(); foreach ($results as $result) { $this->log($result->message, $result->level); diff --git a/backup/util/progress/core_backup_progress.class.php b/backup/util/progress/core_backup_progress.class.php index 513b18f1ea8..d4a5012075e 100644 --- a/backup/util/progress/core_backup_progress.class.php +++ b/backup/util/progress/core_backup_progress.class.php @@ -227,6 +227,20 @@ abstract class core_backup_progress { return !empty($this->descriptions); } + /** + * Checks max value of current progress section. + * + * @return int Current max value (may be core_backup_progress::INDETERMINATE) + * @throws coding_exception If not in a progress section + */ + public function get_current_max() { + $max = end($this->maxes); + if ($max === false) { + throw new coding_exception('Not inside progress section'); + } + return $max; + } + /** * @return string Current progress section description */ From 1cd39657b5793977ec22872b92e7ecd5f3991d03 Mon Sep 17 00:00:00 2001 From: sam marshall Date: Mon, 30 Sep 2013 13:06:42 +0100 Subject: [PATCH 3/7] MDL-42039 Restore: Add progress reporting to directory deletion Deleting directories at the end of backup can take a long time if there are many directories (potentially up to 65536 file directories). This change reports progress while deleting directories to avoid timeout. --- backup/moodle2/backup_stepslib.php | 12 ++++-- backup/moodle2/restore_stepslib.php | 7 +++- backup/util/helper/backup_helper.class.php | 47 +++++++++++++++++----- 3 files changed, 52 insertions(+), 14 deletions(-) diff --git a/backup/moodle2/backup_stepslib.php b/backup/moodle2/backup_stepslib.php index 25392b59256..6dbe0731701 100644 --- a/backup/moodle2/backup_stepslib.php +++ b/backup/moodle2/backup_stepslib.php @@ -34,10 +34,13 @@ defined('MOODLE_INTERNAL') || die(); class create_and_clean_temp_stuff extends backup_execution_step { protected function define_execution() { + $progress = $this->task->get_progress(); + $progress->start_progress('Deleting backup directories'); backup_helper::check_and_create_backup_dir($this->get_backupid());// Create backup temp dir - backup_helper::clear_backup_dir($this->get_backupid()); // Empty temp dir, just in case - backup_helper::delete_old_backup_dirs(time() - (4 * 60 * 60)); // Delete > 4 hours temp dirs + backup_helper::clear_backup_dir($this->get_backupid(), $progress); // Empty temp dir, just in case + backup_helper::delete_old_backup_dirs(time() - (4 * 60 * 60), $progress); // Delete > 4 hours temp dirs backup_controller_dbops::create_backup_ids_temp_table($this->get_backupid()); // Create ids temp table + $progress->end_progress(); } } @@ -61,7 +64,10 @@ class drop_and_clean_temp_stuff extends backup_execution_step { // 1) If $CFG->keeptempdirectoriesonbackup is not enabled // 2) If backup temp dir deletion has been marked to be avoided if (empty($CFG->keeptempdirectoriesonbackup) && !$this->skipcleaningtempdir) { - backup_helper::delete_backup_dir($this->get_backupid()); // Empty backup dir + $progress = $this->task->get_progress(); + $progress->start_progress('Deleting backup dir'); + backup_helper::delete_backup_dir($this->get_backupid(), $progress); // Empty backup dir + $progress->end_progress(); } } diff --git a/backup/moodle2/restore_stepslib.php b/backup/moodle2/restore_stepslib.php index f9f78020ff8..c36f90afc6e 100644 --- a/backup/moodle2/restore_stepslib.php +++ b/backup/moodle2/restore_stepslib.php @@ -65,10 +65,13 @@ class restore_drop_and_clean_temp_stuff extends restore_execution_step { protected function define_execution() { global $CFG; restore_controller_dbops::drop_restore_temp_tables($this->get_restoreid()); // Drop ids temp table - backup_helper::delete_old_backup_dirs(time() - (4 * 60 * 60)); // Delete > 4 hours temp dirs + $progress = $this->task->get_progress(); + $progress->start_progress('Deleting backup dir'); + backup_helper::delete_old_backup_dirs(time() - (4 * 60 * 60), $progress); // Delete > 4 hours temp dirs if (empty($CFG->keeptempdirectoriesonbackup)) { // Conditionally - backup_helper::delete_backup_dir($this->task->get_tempdir()); // Empty restore dir + backup_helper::delete_backup_dir($this->task->get_tempdir(), $progress); // Empty restore dir } + $progress->end_progress(); } } diff --git a/backup/util/helper/backup_helper.class.php b/backup/util/helper/backup_helper.class.php index e1c9d1e9ae5..c2d59aae048 100644 --- a/backup/util/helper/backup_helper.class.php +++ b/backup/util/helper/backup_helper.class.php @@ -41,10 +41,16 @@ abstract class backup_helper { /** * Given one backupid, ensure its temp dir is completely empty + * + * If supplied, progress object should be ready to receive indeterminate + * progress reports. + * + * @param string $backupid Backup id + * @param core_backup_progress $progress Optional progress reporting object */ - static public function clear_backup_dir($backupid) { + static public function clear_backup_dir($backupid, core_backup_progress $progress = null) { global $CFG; - if (!self::delete_dir_contents($CFG->tempdir . '/backup/' . $backupid)) { + if (!self::delete_dir_contents($CFG->tempdir . '/backup/' . $backupid, '', $progress)) { throw new backup_helper_exception('cannot_empty_backup_temp_dir'); } return true; @@ -52,10 +58,16 @@ abstract class backup_helper { /** * Given one backupid, delete completely its temp dir + * + * If supplied, progress object should be ready to receive indeterminate + * progress reports. + * + * @param string $backupid Backup id + * @param core_backup_progress $progress Optional progress reporting object */ - static public function delete_backup_dir($backupid) { + static public function delete_backup_dir($backupid, core_backup_progress $progress = null) { global $CFG; - self::clear_backup_dir($backupid); + self::clear_backup_dir($backupid, $progress); return rmdir($CFG->tempdir . '/backup/' . $backupid); } @@ -63,10 +75,21 @@ abstract class backup_helper { * Given one fullpath to directory, delete its contents recursively * Copied originally from somewhere in the net. * TODO: Modernise this + * + * If supplied, progress object should be ready to receive indeterminate + * progress reports. + * + * @param string $dir Directory to delete + * @param string $excludedir Exclude this directory + * @param core_backup_progress $progress Optional progress reporting object */ - static public function delete_dir_contents($dir, $excludeddir='') { + static public function delete_dir_contents($dir, $excludeddir='', core_backup_progress $progress = null) { global $CFG; + if ($progress) { + $progress->progress(); + } + if (!is_dir($dir)) { // if we've been given a directory that doesn't exist yet, return true. // this happens when we're trying to clear out a course that has only just @@ -108,7 +131,7 @@ abstract class backup_helper { // Empty sub directories and then remove the directory for ($i=0; $idirectorypermissions); - if (self::delete_dir_contents($dir_subdirs[$i]) == false) { + if (self::delete_dir_contents($dir_subdirs[$i], '', $progress) == false) { return false; } else { if (remove_dir($dir_subdirs[$i]) == false) { @@ -125,9 +148,15 @@ abstract class backup_helper { } /** - * Delete all the temp dirs older than the time specified + * Delete all the temp dirs older than the time specified. + * + * If supplied, progress object should be ready to receive indeterminate + * progress reports. + * + * @param int $deletefrom Time to delete from + * @param core_backup_progress $progress Optional progress reporting object */ - static public function delete_old_backup_dirs($deletefrom) { + static public function delete_old_backup_dirs($deletefrom, core_backup_progress $progress = null) { global $CFG; $status = true; @@ -140,7 +169,7 @@ abstract class backup_helper { //If directory, recurse if (is_dir($file_path)) { // $file is really the backupid - $status = self::delete_backup_dir($file); + $status = self::delete_backup_dir($file, $progress); //If file } else { unlink($file_path); From a658b73cd04a076c8a09bdae2937b8cafd6a5235 Mon Sep 17 00:00:00 2001 From: sam marshall Date: Tue, 1 Oct 2013 13:14:04 +0100 Subject: [PATCH 4/7] MDL-42039 Restore: Avoid unnecessary archive file copy at start Change the start of restore process so that the archive file can be passed as a pathnamehash pointing to the Moodle file object, rather than only as a filename in the temporary directory. This avoids making an unnecessary copy of the archive file, which can take a long time (and potentially cause a timeout) in the case of multi-gigabyte backups. --- backup/restorefile.php | 21 +++++++-- backup/util/ui/restore_ui_stage.class.php | 57 +++++++++++++++++++---- 2 files changed, 64 insertions(+), 14 deletions(-) diff --git a/backup/restorefile.php b/backup/restorefile.php index 0c4095ba4c9..1d12d0dd18a 100644 --- a/backup/restorefile.php +++ b/backup/restorefile.php @@ -72,10 +72,23 @@ if (!check_dir_exists($tmpdir, true, true)) { // choose the backup file from backup files tree if ($action == 'choosebackupfile') { if ($fileinfo = $browser->get_file_info($filecontext, $component, $filearea, $itemid, $filepath, $filename)) { - $filename = restore_controller::get_tempdir_name($course->id, $USER->id); - $pathname = $tmpdir . '/' . $filename; - $fileinfo->copy_to_pathname($pathname); - $restore_url = new moodle_url('/backup/restore.php', array('contextid'=>$contextid, 'filename'=>$filename)); + if (is_a($fileinfo, 'file_info_stored')) { + // Use the contenthash rather than copying the file where possible, + // to improve performance and avoid timeouts with large files. + $fs = get_file_storage(); + $params = $fileinfo->get_params(); + $file = $fs->get_file($params['contextid'], $params['component'], $params['filearea'], + $params['itemid'], $params['filepath'], $params['filename']); + $restore_url = new moodle_url('/backup/restore.php', array('contextid' => $contextid, + 'pathnamehash' => $file->get_pathnamehash(), 'contenthash' => $file->get_contenthash())); + } else { + // If it's some weird other kind of file then use old code. + $filename = restore_controller::get_tempdir_name($course->id, $USER->id); + $pathname = $tmpdir . '/' . $filename; + $fileinfo->copy_to_pathname($pathname); + $restore_url = new moodle_url('/backup/restore.php', array( + 'contextid' => $contextid, 'filename' => $filename)); + } redirect($restore_url); } else { redirect($url, get_string('filenotfound', 'error')); diff --git a/backup/util/ui/restore_ui_stage.class.php b/backup/util/ui/restore_ui_stage.class.php index 189d62753b8..2cad0d7857a 100644 --- a/backup/util/ui/restore_ui_stage.class.php +++ b/backup/util/ui/restore_ui_stage.class.php @@ -185,6 +185,16 @@ class restore_ui_stage_confirm extends restore_ui_independent_stage implements f protected $contextid; protected $filename = null; protected $filepath = null; + + /** + * @var string Content hash of archive file to restore (if specified by hash) + */ + protected $contenthash = null; + /** + * @var string Pathname hash of stored_file object to restore + */ + protected $pathnamehash = null; + protected $details; /** @@ -194,27 +204,54 @@ class restore_ui_stage_confirm extends restore_ui_independent_stage implements f public function __construct($contextid) { $this->contextid = $contextid; - $this->filename = required_param('filename', PARAM_FILE); + $this->filename = optional_param('filename', null, PARAM_FILE); + if ($this->filename === null) { + // Identify file object by its pathname hash. + $this->pathnamehash = required_param('pathnamehash', PARAM_ALPHANUM); + + // The file content hash is also passed for security; users + // cannot guess the content hash (unless they know the file contents), + // so this guarantees that either the system generated this link or + // else the user has access to the restore archive anyhow. + $this->contenthash = required_param('contenthash', PARAM_ALPHANUM); + } } + public function process() { global $CFG; - if (!file_exists("$CFG->tempdir/backup/".$this->filename)) { - throw new restore_ui_exception('invalidrestorefile'); - } - $outcome = $this->extract_file_to_dir(); - if ($outcome) { - fulldelete($this->filename); + if ($this->filename) { + $archivepath = $CFG->tempdir . '/backup/' . $this->filename; + if (!file_exists($archivepath)) { + throw new restore_ui_exception('invalidrestorefile'); + } + $outcome = $this->extract_file_to_dir($archivepath); + if ($outcome) { + fulldelete($archivepath); + } + } else { + $fs = get_file_storage(); + $storedfile = $fs->get_file_by_hash($this->pathnamehash); + if (!$storedfile || $storedfile->get_contenthash() !== $this->contenthash) { + throw new restore_ui_exception('invalidrestorefile'); + } + $outcome = $this->extract_file_to_dir($storedfile); } return $outcome; } - protected function extract_file_to_dir() { + + /** + * Extracts the file. + * + * @param string|stored_file $source Archive file to extract + */ + protected function extract_file_to_dir($source) { global $CFG, $USER; $this->filepath = restore_controller::get_tempdir_name($this->contextid, $USER->id); $fb = get_file_packer('application/vnd.moodle.backup'); - $result = $fb->extract_to_pathname("$CFG->tempdir/backup/".$this->filename, - "$CFG->tempdir/backup/$this->filepath/", null, $this); + $result = $fb->extract_to_pathname($source, + $CFG->tempdir . '/backup/' . $this->filepath . '/', null, $this); // If any progress happened, end it. if ($this->startedprogress) { From bf2a039e9adc79e61a155aab5622e6fff34102fe Mon Sep 17 00:00:00 2001 From: sam marshall Date: Tue, 1 Oct 2013 13:46:28 +0100 Subject: [PATCH 5/7] MDL-42039 Restore: Add progress tracking in restore controller load In the case of a very large backup with many activities, simply loading the restore controller (specifically, building the plan) can take a long time and cause timeouts. This change adds progress reporting while the restore controller is being loaded. (This also reduces the time between first going to the page, and the progress bar appearing, making a slightly better user experience.) Conflicts: backup/restore.php --- .../controller/restore_controller.class.php | 23 ++++++++++--- backup/moodle2/restore_plan_builder.class.php | 7 ++++ backup/restore.php | 32 +++++++++++++++---- 3 files changed, 51 insertions(+), 11 deletions(-) diff --git a/backup/controller/restore_controller.class.php b/backup/controller/restore_controller.class.php index daa5aafe2fd..b2859f6c071 100644 --- a/backup/controller/restore_controller.class.php +++ b/backup/controller/restore_controller.class.php @@ -58,6 +58,11 @@ class restore_controller extends base_controller { protected $checksum; // Cache @checksumable results for lighter @is_checksum_correct() uses /** + * Constructor. + * + * If you specify a progress monitor, this will be used to report progress + * while loading the plan, as well as for future use. (You can change it + * for a different one later using set_progress.) * * @param string $tempdir Directory under tempdir/backup awaiting restore * @param int $courseid Course id where restore is going to happen @@ -65,8 +70,10 @@ class restore_controller extends base_controller { * @param int $mode backup::MODE_[ GENERAL | HUB | IMPORT | SAMESITE ] * @param int $userid * @param int $target backup::TARGET_[ NEW_COURSE | CURRENT_ADDING | CURRENT_DELETING | EXISTING_ADDING | EXISTING_DELETING ] + * @param core_backup_progress $progress Optional progress monitor */ - public function __construct($tempdir, $courseid, $interactive, $mode, $userid, $target){ + public function __construct($tempdir, $courseid, $interactive, $mode, $userid, $target, + core_backup_progress $progress = null) { $this->tempdir = $tempdir; $this->courseid = $courseid; $this->interactive = $interactive; @@ -99,9 +106,14 @@ class restore_controller extends base_controller { // Default logger chain (based on interactive/execution) $this->logger = backup_factory::get_logger_chain($this->interactive, $this->execution, $this->restoreid); - // By default there is no progress reporter. Interfaces that wish to - // display progress must set it. - $this->progress = new core_backup_null_progress(); + // By default there is no progress reporter unless you specify one so it + // can be used during loading of the plan. + if ($progress) { + $this->progress = $progress; + } else { + $this->progress = new core_backup_null_progress(); + } + $this->progress->start_progress('Constructing restore_controller'); // Instantiate the output_controller singleton and active it if interactive and inmediate $oc = output_controller::get_instance(); @@ -135,6 +147,9 @@ class restore_controller extends base_controller { $this->set_status(backup::STATUS_NEED_PRECHECK); } } + + // Tell progress monitor that we finished loading. + $this->progress->end_progress(); } /** diff --git a/backup/moodle2/restore_plan_builder.class.php b/backup/moodle2/restore_plan_builder.class.php index 1f830ac9728..b7c65ef29b0 100644 --- a/backup/moodle2/restore_plan_builder.class.php +++ b/backup/moodle2/restore_plan_builder.class.php @@ -97,6 +97,7 @@ abstract class restore_plan_builder { // preloading information to temp table // and other init tasks $plan->add_task(new restore_root_task('root_task')); + $controller->get_progress()->progress(); switch ($controller->get_type()) { case backup::TYPE_1ACTIVITY: @@ -115,6 +116,7 @@ abstract class restore_plan_builder { // conversion...) // and perform other various final actions. $plan->add_task(new restore_final_task('final_task')); + $controller->get_progress()->progress(); } @@ -134,6 +136,7 @@ abstract class restore_plan_builder { // as far as the module can be missing on restore if ($task = restore_factory::get_restore_activity_task($infoactivity)) { // can be missing $plan->add_task($task); + $controller->get_progress()->progress(); // For the given activity path, add as many block tasks as necessary // TODO: Add blocks, we need to introspect xml here @@ -141,6 +144,7 @@ abstract class restore_plan_builder { foreach ($blocks as $basepath => $name) { if ($task = restore_factory::get_restore_block_task($name, $basepath)) { $plan->add_task($task); + $controller->get_progress()->progress(); } else { // TODO: Debug information about block not supported } @@ -163,6 +167,7 @@ abstract class restore_plan_builder { // Add the section task, responsible for restoring // all the section related information $plan->add_task(restore_factory::get_restore_section_task($infosection)); + $controller->get_progress()->progress(); // For the given section, add as many activity tasks as necessary foreach ($info->activities as $activityid => $activity) { if ($activity->sectionid != $infosection->sectionid) { @@ -188,6 +193,7 @@ abstract class restore_plan_builder { // all the course related information $task = restore_factory::get_restore_course_task($info->course, $courseid); $plan->add_task($task); + $controller->get_progress()->progress(); // For the given course path, add as many block tasks as necessary // TODO: Add blocks, we need to introspect xml here @@ -195,6 +201,7 @@ abstract class restore_plan_builder { foreach ($blocks as $basepath => $name) { if ($task = restore_factory::get_restore_block_task($name, $basepath)) { $plan->add_task($task); + $controller->get_progress()->progress(); } else { // TODO: Debug information about block not supported } diff --git a/backup/restore.php b/backup/restore.php index 5b835e3468e..6e19ef903c2 100644 --- a/backup/restore.php +++ b/backup/restore.php @@ -17,6 +17,23 @@ $PAGE->set_pagelayout('standard'); require_login($course, null, $cm); require_capability('moodle/restore:restorecourse', $context); +// Show page header. +$PAGE->set_title($course->shortname . ': ' . get_string('restore')); +$PAGE->set_heading($course->fullname); + +$renderer = $PAGE->get_renderer('core','backup'); +echo $OUTPUT->header(); + +// Prepare a progress bar which can display optionally during long-running +// operations while setting up the UI. +$slowprogress = new core_backup_display_progress_if_slow(get_string('preparingui', 'backup')); + +// Overall, allow 10 units of progress. +$slowprogress->start_progress('', 10); + +// This progress section counts for loading the restore controller. +$slowprogress->start_progress('', 1, 1); + // Restore of large courses requires extra memory. Use the amount configured // in admin settings. raise_memory_limit(MEMORY_EXTRA); @@ -43,15 +60,12 @@ if ($stage & restore_ui::STAGE_CONFIRM + restore_ui::STAGE_DESTINATION) { } } -$PAGE->set_title($course->shortname . ': ' . get_string('restore')); -$PAGE->set_heading($course->fullname); +// End progress section for loading restore controller. +$slowprogress->end_progress(); -$renderer = $PAGE->get_renderer('core','backup'); -echo $OUTPUT->header(); +// This progress section is for the 'process' function below. +$slowprogress->start_progress('', 1, 9); -// Prepare a progress bar which can display optionally during long-running -// operations while setting up the UI. -$slowprogress = new core_backup_display_progress_if_slow(get_string('preparingui', 'backup')); // Depending on the code branch above, $restore may be a restore_ui or it may // be a restore_ui_independent_stage. Either way, this function exists. $restore->set_progress_reporter($slowprogress); @@ -62,6 +76,10 @@ if (!$restore->is_independent() && $restore->enforce_changed_dependencies()) { } $loghtml = ''; +// Finish the 'process' progress reporting section, and the overall count. +$slowprogress->end_progress(); +$slowprogress->end_progress(); + if (!$restore->is_independent()) { // Use a temporary (disappearing) progress bar to show the precheck progress if any. $precheckprogress = new core_backup_display_progress_if_slow(get_string('preparingdata', 'backup')); From 0e2637054ff7d5464988c10982b5ded5660acb0d Mon Sep 17 00:00:00 2001 From: sam marshall Date: Tue, 1 Oct 2013 18:01:31 +0100 Subject: [PATCH 6/7] MDL-42039 Restore: Add progress tracking while creating users When restoring a backup that contains a large number of users to a different server where those users don't already exist, creating users can take a significant time. This change adds progress reporting so that it doesn't time out and shows activity in the user interface while doing the creation. --- backup/moodle2/restore_stepslib.php | 3 ++- backup/util/dbops/restore_dbops.class.php | 22 ++++++++++++++++++---- 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/backup/moodle2/restore_stepslib.php b/backup/moodle2/restore_stepslib.php index c36f90afc6e..082f0bafdaf 100644 --- a/backup/moodle2/restore_stepslib.php +++ b/backup/moodle2/restore_stepslib.php @@ -729,7 +729,8 @@ class restore_create_included_users extends restore_execution_step { protected function define_execution() { - restore_dbops::create_included_users($this->get_basepath(), $this->get_restoreid(), $this->task->get_userid()); + restore_dbops::create_included_users($this->get_basepath(), $this->get_restoreid(), + $this->task->get_userid(), $this->task->get_progress()); } } diff --git a/backup/util/dbops/restore_dbops.class.php b/backup/util/dbops/restore_dbops.class.php index 533553abe3f..0d0155d64b9 100644 --- a/backup/util/dbops/restore_dbops.class.php +++ b/backup/util/dbops/restore_dbops.class.php @@ -1057,10 +1057,20 @@ abstract class restore_dbops { * in backup_ids having newitemid = 0, as far as * precheck_included_users() have left them there * ready to be created. Also, annotate their newids - * once created for later reference + * once created for later reference. + * + * This function will start and end a new progress section in the progress + * object. + * + * @param string $basepath Base path of unzipped backup + * @param string $restoreid Restore ID + * @param int $userid Default userid for files + * @param core_backup_progress $progress Object used for progress tracking */ - public static function create_included_users($basepath, $restoreid, $userid) { + public static function create_included_users($basepath, $restoreid, $userid, + core_backup_progress $progress) { global $CFG, $DB; + $progress->start_progress('Creating included users'); $authcache = array(); // Cache to get some bits from authentication plugins $languages = get_string_manager()->get_list_of_translations(); // Get languages for quick search later @@ -1069,6 +1079,7 @@ abstract class restore_dbops { // Iterate over all the included users with newitemid = 0, have to create them $rs = $DB->get_recordset('backup_ids_temp', array('backupid' => $restoreid, 'itemname' => 'user', 'newitemid' => 0), '', 'itemid, parentitemid, info'); foreach ($rs as $recuser) { + $progress->progress(); $user = (object)backup_controller_dbops::decode_backup_temp_info($recuser->info); // if user lang doesn't exist here, use site default @@ -1208,11 +1219,14 @@ abstract class restore_dbops { } // Create user files in pool (profile, icon, private) by context - restore_dbops::send_files_to_pool($basepath, $restoreid, 'user', 'icon', $recuser->parentitemid, $userid); - restore_dbops::send_files_to_pool($basepath, $restoreid, 'user', 'profile', $recuser->parentitemid, $userid); + restore_dbops::send_files_to_pool($basepath, $restoreid, 'user', 'icon', + $recuser->parentitemid, $userid, null, null, null, false, $progress); + restore_dbops::send_files_to_pool($basepath, $restoreid, 'user', 'profile', + $recuser->parentitemid, $userid, null, null, null, false, $progress); } } $rs->close(); + $progress->end_progress(); } /** From 93eddacc614d188b6212a901111ba111383933f6 Mon Sep 17 00:00:00 2001 From: sam marshall Date: Thu, 3 Oct 2013 17:50:47 +0100 Subject: [PATCH 7/7] MDL-42039 Restore: Add progress tracking while creating question files If a backup contains a large number of questions, the create_question_files function can take a very long time. This change adds progress reporting during that function. --- backup/moodle2/restore_stepslib.php | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/backup/moodle2/restore_stepslib.php b/backup/moodle2/restore_stepslib.php index 082f0bafdaf..9fa8a9b5a13 100644 --- a/backup/moodle2/restore_stepslib.php +++ b/backup/moodle2/restore_stepslib.php @@ -3551,6 +3551,10 @@ class restore_create_question_files extends restore_execution_step { protected function define_execution() { global $DB; + // Track progress, as this task can take a long time. + $progress = $this->task->get_progress(); + $progress->start_progress($this->get_name(), core_backup_progress::INDETERMINATE); + // Let's process only created questions $questionsrs = $DB->get_recordset_sql("SELECT bi.itemid, bi.newitemid, bi.parentitemid, q.qtype FROM {backup_ids_temp} bi @@ -3558,6 +3562,9 @@ class restore_create_question_files extends restore_execution_step { WHERE bi.backupid = ? AND bi.itemname = 'question_created'", array($this->get_restoreid())); foreach ($questionsrs as $question) { + // Report progress for each question. + $progress->progress(); + // Get question_category mapping, it contains the target context for the question if (!$qcatmapping = restore_dbops::get_backup_ids_record($this->get_restoreid(), 'question_category', $question->parentitemid)) { // Something went really wrong, cannot find the question_category for the question @@ -3570,21 +3577,22 @@ class restore_create_question_files extends restore_execution_step { // Add common question files (question and question_answer ones) restore_dbops::send_files_to_pool($this->get_basepath(), $this->get_restoreid(), 'question', 'questiontext', - $oldctxid, $this->task->get_userid(), 'question_created', $question->itemid, $newctxid, true); + $oldctxid, $this->task->get_userid(), 'question_created', $question->itemid, $newctxid, true, $progress); restore_dbops::send_files_to_pool($this->get_basepath(), $this->get_restoreid(), 'question', 'generalfeedback', - $oldctxid, $this->task->get_userid(), 'question_created', $question->itemid, $newctxid, true); + $oldctxid, $this->task->get_userid(), 'question_created', $question->itemid, $newctxid, true, $progress); restore_dbops::send_files_to_pool($this->get_basepath(), $this->get_restoreid(), 'question', 'answer', - $oldctxid, $this->task->get_userid(), 'question_answer', null, $newctxid, true); + $oldctxid, $this->task->get_userid(), 'question_answer', null, $newctxid, true, $progress); restore_dbops::send_files_to_pool($this->get_basepath(), $this->get_restoreid(), 'question', 'answerfeedback', - $oldctxid, $this->task->get_userid(), 'question_answer', null, $newctxid, true); + $oldctxid, $this->task->get_userid(), 'question_answer', null, $newctxid, true, $progress); restore_dbops::send_files_to_pool($this->get_basepath(), $this->get_restoreid(), 'question', 'hint', - $oldctxid, $this->task->get_userid(), 'question_hint', null, $newctxid, true); + $oldctxid, $this->task->get_userid(), 'question_hint', null, $newctxid, true, $progress); restore_dbops::send_files_to_pool($this->get_basepath(), $this->get_restoreid(), 'question', 'correctfeedback', - $oldctxid, $this->task->get_userid(), 'question_created', $question->itemid, $newctxid, true); + $oldctxid, $this->task->get_userid(), 'question_created', $question->itemid, $newctxid, true, $progress); restore_dbops::send_files_to_pool($this->get_basepath(), $this->get_restoreid(), 'question', 'partiallycorrectfeedback', - $oldctxid, $this->task->get_userid(), 'question_created', $question->itemid, $newctxid, true); + $oldctxid, $this->task->get_userid(), 'question_created', $question->itemid, $newctxid, true, $progress); restore_dbops::send_files_to_pool($this->get_basepath(), $this->get_restoreid(), 'question', 'incorrectfeedback', - $oldctxid, $this->task->get_userid(), 'question_created', $question->itemid, $newctxid, true); + $oldctxid, $this->task->get_userid(), 'question_created', $question->itemid, $newctxid, true, $progress); + // Add qtype dependent files $components = backup_qtype_plugin::get_components_and_fileareas($question->qtype); foreach ($components as $component => $fileareas) { @@ -3592,11 +3600,12 @@ class restore_create_question_files extends restore_execution_step { // Use itemid only if mapping is question_created $itemid = ($mapping == 'question_created') ? $question->itemid : null; restore_dbops::send_files_to_pool($this->get_basepath(), $this->get_restoreid(), $component, $filearea, - $oldctxid, $this->task->get_userid(), $mapping, $itemid, $newctxid, true); + $oldctxid, $this->task->get_userid(), $mapping, $itemid, $newctxid, true, $progress); } } } $questionsrs->close(); + $progress->end_progress(); } }