Details

    • Testing Instructions:
      Hide

      0. You will need a sample course to backup and restore. I used the 'M' test course created by the tool from MDL-38197.

      1. Go to the course. Do a backup with default settings. The screens should be unchanged except when you get to the end of step 3 and click to actually start the backup...

      EXPECTED:

      • Step 4 should display fairly soon (may be a bit of a pause while it loads the backup controller).
      • You should see a progress bar that moves across from left to right (albeit with some jerky parts) as the backup progresses.
      • There is no text next to the progress bar (I decided not to display the backup task names because they are not user-friendly) but there is a time remaining estimate which should update, as standard Moodle progress bar behaviour.
      • A thin bar below the progress bar should gradually change colour (from black to white and back) as the backup progresses. (This bar is intended to indicate that something is happening even when we can't necessarily measure progress numerically.)
      • Once the progress bar completes, the step 4 content should immediately disappear and step 5 - completed, with the continue button - should appear. (These two steps are actually on the same HTML page.)

      2. Continue to the list of backups. Click Restore against the backup you just made, and go through the steps with default options to restore it to a new course. This should be unchanged until you get to the point where it actually processes the restore.

      EXPECTED: The restore progress should be displayed in the same manner as for backup.

      3. Within a course, use the 'duplicate' feature to duplicate an activity.

      EXPECTED: The 'duplicate' feature should work exactly as before, without a progress bar. (Note: This test is because duplicating uses the backup/restore API, so we want to check it still works OK when there is no progress display.)

      4. You'll need another, probably smaller, course; I used the 'XS' test course. In your new course, choose the 'Import' option and select the smaller course. Accept default options and import everything.

      EXPECTED: When you click 'Process import', you should see a screen with a suitable stage heading and a progress bar that moves across as the import is carried out. As soon as it finishes, the progress bar should disappear and be replaced with the 'Continue' button (as was there before this change).

      Show
      0. You will need a sample course to backup and restore. I used the 'M' test course created by the tool from MDL-38197 . 1. Go to the course. Do a backup with default settings. The screens should be unchanged except when you get to the end of step 3 and click to actually start the backup... EXPECTED: Step 4 should display fairly soon (may be a bit of a pause while it loads the backup controller). You should see a progress bar that moves across from left to right (albeit with some jerky parts) as the backup progresses. There is no text next to the progress bar (I decided not to display the backup task names because they are not user-friendly) but there is a time remaining estimate which should update, as standard Moodle progress bar behaviour. A thin bar below the progress bar should gradually change colour (from black to white and back) as the backup progresses. (This bar is intended to indicate that something is happening even when we can't necessarily measure progress numerically.) Once the progress bar completes, the step 4 content should immediately disappear and step 5 - completed, with the continue button - should appear. (These two steps are actually on the same HTML page.) 2. Continue to the list of backups. Click Restore against the backup you just made, and go through the steps with default options to restore it to a new course. This should be unchanged until you get to the point where it actually processes the restore. EXPECTED: The restore progress should be displayed in the same manner as for backup. 3. Within a course, use the 'duplicate' feature to duplicate an activity. EXPECTED: The 'duplicate' feature should work exactly as before, without a progress bar. (Note: This test is because duplicating uses the backup/restore API, so we want to check it still works OK when there is no progress display.) 4. You'll need another, probably smaller, course; I used the 'XS' test course. In your new course, choose the 'Import' option and select the smaller course. Accept default options and import everything. EXPECTED: When you click 'Process import', you should see a screen with a suitable stage heading and a progress bar that moves across as the import is carried out. As soon as it finishes, the progress bar should disappear and be replaced with the 'Continue' button (as was there before this change).
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE
    • Pull Master Branch:
      MDL-38190-master
    • Rank:
      48025

      Description

      We would like backup and restore operations to display progress.

      • Backup and restore operations can take a very long time.
      • Nothing at all is displayed during the operation.

      For all Moodle users, not just us, this is unsatisfactory as it can cause users to become unsure about whether the operation is continuing or has failed.

      • Our front-end server (a NetScaler) has a time limit on requests. If nothing is output from the server, requests are cancelled after 6 minutes. As a result, backup and restore operations will fail if they take longer than 6 minutes (they nearly always do).
      • Other insititutions are likely to use similar front-end servers.

      Progress information could consist of any of the following:

      • A progress bar.
      • Dots printed periodically.
      • Anything else that causes a visible update via data sent as part of the HTTP response.

      Detail 1: The progress information must be output as part of the main HTTP request (the one that is taking a long time to complete) and not via any associated request such as an AJAX request.

      • This is because if there is no output as part of the main request for 6 minutes, then that request will fail on our systems, regardless of what other requests may be going on.
      • Note: This may mean it is not possible to see progress information on Internet Explorer 9 until it finishes, due to technical limitations on displaying partially-loaded web pages in that browser. This would be acceptable to us and probably most people...

      Detail 2: Progress information should be provided frequently so that there are no long pauses without progress information.

      • For example, we achieve some of this in internal code using a potential_dot function which displays a dot if it has been at least 1 second since the last dot was displayed, and then calling it in lots of places. After displaying the dot, it also resets the PHP time limit to a suitable time.
      • Implementing this will require potentially displaying progress in the following locations:
        • When each backup/restore task starts.
        • Before or after copying each file, when copying files.
        • Before or after zipping / unzipping each file (or a certain number of files).

      Note: We have previously achieved this by using the command-line zip and unzip tool, set to verbose mode, and reading its output buffer, displaying progress at each chunk of data read.

      Detail 3: As part of the backup/restore API it should be possible for plugins such as modules to themselves indicate that progress has occurred.

      • For example, when backing up user data from a forum, this could take a very long time if there are many discussions. The forum should be able to indicate progress between each discussion or each post.

      Detail 4: When an instance of the resource module contains a large number of files (e.g. 10,000) progress should be indicated between files.

      • Without looking at the code, I’m not sure whether this should happen in a generic way (like there is some ‘add file’ method that should be trapped) or it has to be specific to the resource module.
      • These large resources can take a very long time (> 100s) even in their standard backup task.

      Detail 5: The Moodle time limit used during backup/restore should be configurable in admin settings (similar to extramemorylimit), or fixed to a reasonable (low) value e.g. 2 minutes. The limit should apply from the last progress update.

      • Currently the time limit is fixed to 1 hour. This is not very useful in our system where the NetScaler cancels the request well before a PHP timeout occurs. It would be more helpful to see the timeout error.
      • Resetting the time limit each time progress is updated means that as long as progress is continually updated, the backup and restore can take hours if required. However the backup/restore will time out if there is a long time without a progress report, which indicates a system bug (the system is not reporting progress frequently enough).

      Detail 6: When using the code API to do backup and restore, it should be possible to hook into this progress system and control progress display, for example via a callback function which is called regularly with progress information.

      • Our customised roll forward system uses the code API and may need to display progress in a different way.
      • This will also be necessary for other non-OU users of the backup/restore API, such as the standard duplicate feature.

        Issue Links

          Activity

          Hide
          Chris Follin added a comment -

          Sam's description is very thorough so I don't have a lot to add other than we are using NetScaler load balancers and having the same problem when the backup process runs longer than 6 minutes. This is a major problem and although it may be specific to our type of equipment, it is not the equipment's problem. This is an application problem. Moodle should output some sort of progress indicator to keep the connection alive and inform the user that something is happening.

          Show
          Chris Follin added a comment - Sam's description is very thorough so I don't have a lot to add other than we are using NetScaler load balancers and having the same problem when the backup process runs longer than 6 minutes. This is a major problem and although it may be specific to our type of equipment, it is not the equipment's problem. This is an application problem. Moodle should output some sort of progress indicator to keep the connection alive and inform the user that something is happening.
          Hide
          Sam Marshall added a comment -

          For info - I'm intending to actually start development on this feature soon.

          Show
          Sam Marshall added a comment - For info - I'm intending to actually start development on this feature soon.
          Hide
          Sam Marshall added a comment -

          Starting work on this now (it might take me some days, I should think).

          Show
          Sam Marshall added a comment - Starting work on this now (it might take me some days, I should think).
          Hide
          Chris Follin added a comment -

          Thank you, Sam. We very much appreciate your work on this.

          Show
          Chris Follin added a comment - Thank you, Sam. We very much appreciate your work on this.
          Hide
          Sam Marshall added a comment -

          I've marked MDL-38191 as blocking this because without the larger memory limit, it's difficult to test using my test sites (even though I'm using a smaller one).

          Show
          Sam Marshall added a comment - I've marked MDL-38191 as blocking this because without the larger memory limit, it's difficult to test using my test sites (even though I'm using a smaller one).
          Hide
          Sam Marshall added a comment -

          Adding MDL-41050 as a 'blocker'. It isn't really a blocker but the progress display looks a bit rubbish without it.

          Show
          Sam Marshall added a comment - Adding MDL-41050 as a 'blocker'. It isn't really a blocker but the progress display looks a bit rubbish without it.
          Hide
          Sam Marshall added a comment -

          I have split the work of this task - this task now refers to the API and the basic indication of progress, whereas I have moved the task of adding progress to 'stranger' areas like zipping and specific modules into separate issues that can go in after this one.

          I've written some testing instructions.

          Am now doing final checks on this code before I submit it for review.

          Show
          Sam Marshall added a comment - I have split the work of this task - this task now refers to the API and the basic indication of progress, whereas I have moved the task of adding progress to 'stranger' areas like zipping and specific modules into separate issues that can go in after this one. I've written some testing instructions. Am now doing final checks on this code before I submit it for review.
          Hide
          Sam Marshall added a comment -

          Now submitting for peer review as it passes the test and I didn't spot anything wrong when I looked through the code again...

          Note: There are three logically separate commits; looking at them separately should make things clearer. The first is the most significant.

          Show
          Sam Marshall added a comment - Now submitting for peer review as it passes the test and I didn't spot anything wrong when I looked through the code again... Note: There are three logically separate commits; looking at them separately should make things clearer. The first is the most significant.
          Hide
          Sam Marshall added a comment -

          Continuing for development - I forgot to do the import feature. The progress bar would also be useful in import. Am going to add this now if it's easy.

          Show
          Sam Marshall added a comment - Continuing for development - I forgot to do the import feature. The progress bar would also be useful in import. Am going to add this now if it's easy.
          Hide
          Sam Marshall added a comment -

          Putting up for review again - I have added support for the 'import' screen which I had previously forgotten.

          Show
          Sam Marshall added a comment - Putting up for review again - I have added support for the 'import' screen which I had previously forgotten.
          Hide
          Dan Poltawski added a comment -

          Hi Sam,

          I've played with this a bit and it seems really nice.

          I almost certainly know less about the whole overall backup architecture and how this fits in than you so I can't really comment on that (and i'll be happy to see Eloy Lafuente (stronk7) involved before integration).

          backup/util/progress/core_backup_display_progress.class.php
          1. The code looks mostly good to me, pleased to see unit tests too. The colours in the php code seems like a no-no to me - surely they should be styles which can be altered by themes?

          theme/base/style/core.css
          2. These styles need to make their way into bootstrap base too (and tested should identify issues with both these items).

          backup/util/plan/tests/fixtures/plan_fixtures.php
          3. Trailing whitespace is introduced.

          util/progress/tests/progress_test.php
          4. I wasn't overly sold on the method name assert_range(), it indicated something more generic than it is to me. (trivial comment)

          Show
          Dan Poltawski added a comment - Hi Sam, I've played with this a bit and it seems really nice. I almost certainly know less about the whole overall backup architecture and how this fits in than you so I can't really comment on that (and i'll be happy to see Eloy Lafuente (stronk7) involved before integration). backup/util/progress/core_backup_display_progress.class.php 1. The code looks mostly good to me, pleased to see unit tests too. The colours in the php code seems like a no-no to me - surely they should be styles which can be altered by themes? theme/base/style/core.css 2. These styles need to make their way into bootstrap base too (and tested should identify issues with both these items). backup/util/plan/tests/fixtures/plan_fixtures.php 3. Trailing whitespace is introduced. util/progress/tests/progress_test.php 4. I wasn't overly sold on the method name assert_range(), it indicated something more generic than it is to me. (trivial comment)
          Hide
          Sam Marshall added a comment -

          Thanks Dan!

          I have addressed these as follows:

          1. I did wonder that but it was going to involve creating some pretty stupid CSS classes... Stupid classes now created!

          2. Oh gods, I didn't realise that was happening. What a mess. OK, I have added the same styles to the bootstrap_base theme. (Good news, at least it has a specific file for backup/restore styles.)

          3. Fixed, sorry.

          4. I have renamed the method assert_min_max since that's more closely what it does.

          Show
          Sam Marshall added a comment - Thanks Dan! I have addressed these as follows: 1. I did wonder that but it was going to involve creating some pretty stupid CSS classes... Stupid classes now created! 2. Oh gods, I didn't realise that was happening. What a mess. OK, I have added the same styles to the bootstrap_base theme. (Good news, at least it has a specific file for backup/restore styles.) 3. Fixed, sorry. 4. I have renamed the method assert_min_max since that's more closely what it does.
          Hide
          Sam Marshall added a comment -

          Correcting diff url.

          Note: I have changed the branch so it only includes the 3 commits for this change and not for blockers - anybody who wants to actually try it out will need to manually add those too (until they've already been integrated).

          I have also rerun the phpunit tests and done a brief manual retest to make sure it still seems to work.

          Show
          Sam Marshall added a comment - Correcting diff url. Note: I have changed the branch so it only includes the 3 commits for this change and not for blockers - anybody who wants to actually try it out will need to manually add those too (until they've already been integrated). I have also rerun the phpunit tests and done a brief manual retest to make sure it still seems to work.
          Hide
          Sam Marshall added a comment -

          Dan: I agree it would be nice if Eloy Lafuente (stronk7) reviewed this change before integration. I've added him as peer reviewer and am setting this back to waiting for peer review (even though you have already done a peer review, thank you!) - is that correct?

          Show
          Sam Marshall added a comment - Dan: I agree it would be nice if Eloy Lafuente (stronk7) reviewed this change before integration. I've added him as peer reviewer and am setting this back to waiting for peer review (even though you have already done a peer review, thank you!) - is that correct?
          Hide
          Sam Marshall added a comment -

          Note: While working on MDL-41167, I discovered a need to improve the progress bar API that I created in this issue. As a result, I've updated the third commit on this branch slightly and re-pushed.

          Show
          Sam Marshall added a comment - Note: While working on MDL-41167 , I discovered a need to improve the progress bar API that I created in this issue. As a result, I've updated the third commit on this branch slightly and re-pushed.
          Hide
          Dan Poltawski added a comment -

          Hi Sam,

          I think its better if we send this for integration, and I can subtly hint to the integrator that Eloy should take this

          Show
          Dan Poltawski added a comment - Hi Sam, I think its better if we send this for integration, and I can subtly hint to the integrator that Eloy should take this
          Hide
          Dan Poltawski added a comment -

          Since I think Sam is away, i'm pushing this to integration on his behalf.

          TO INTEGRATOR: I think we should make Eloy see this

          Show
          Dan Poltawski added a comment - Since I think Sam is away, i'm pushing this to integration on his behalf. TO INTEGRATOR: I think we should make Eloy see this
          Hide
          Damyon Wiese added a comment -

          Checked this and it looks OK to me. The code looks perfect - I would have made the progress bar a little bigger, and not disappear when it's finished - but that is more of a preference.

          Putting this back in the queue for Eloy - but if he doesn't get to it this gets a +1 from me for this week.

          Show
          Damyon Wiese added a comment - Checked this and it looks OK to me. The code looks perfect - I would have made the progress bar a little bigger, and not disappear when it's finished - but that is more of a preference. Putting this back in the queue for Eloy - but if he doesn't get to it this gets a +1 from me for this week.
          Hide
          Sam Marshall added a comment -

          Thanks for review! Good timing, I'm back from holiday today & should be able to do changes etc if required.

          Regarding the comments, to explain why I did it that way:

          1) Progress bar size - I didn't really think about this, so it just uses the default, same as everywhere else. (I don't have a strong opinion about it!)

          2) Disappearing when it finishes - it isn't very much use after it's finished, also this keeps the screen looking the same as before. And it's not in this commit, but looking ahead, there might be cases where we have more than one progress bar on some of these pages, i.e. a progress bar for one thing then a progress bar for another thing (I know that's unhelpful but it's still better than no progress / timing out) so having them disappear after completion makes it a little bit more sane.

          Show
          Sam Marshall added a comment - Thanks for review! Good timing, I'm back from holiday today & should be able to do changes etc if required. Regarding the comments, to explain why I did it that way: 1) Progress bar size - I didn't really think about this, so it just uses the default, same as everywhere else. (I don't have a strong opinion about it!) 2) Disappearing when it finishes - it isn't very much use after it's finished, also this keeps the screen looking the same as before. And it's not in this commit, but looking ahead, there might be cases where we have more than one progress bar on some of these pages, i.e. a progress bar for one thing then a progress bar for another thing (I know that's unhelpful but it's still better than no progress / timing out) so having them disappear after completion makes it a little bit more sane.
          Hide
          Sam Marshall added a comment -

          Ugh, I forgot the main reason for making it disappear when finished. The progress bar div actually includes not just the progress bar, but also the, er, other progress bar, the one that says like

          1. Settings 2. Something else 3. Something else 4. Processing backup 5. Complete

          As part of this change it actually prints this twice - first time it prints, 'Processing backup' will be highlighted, second time 'Complete' will be. It's confusing if it has two of those strips visible at once.

          (By the way, prior to this change, the 'Processing backup' step 4 was shown on that list, but never became highlighted, which was in itself a bit weird!)

          Show
          Sam Marshall added a comment - Ugh, I forgot the main reason for making it disappear when finished. The progress bar div actually includes not just the progress bar, but also the, er, other progress bar, the one that says like 1. Settings 2. Something else 3. Something else 4. Processing backup 5. Complete As part of this change it actually prints this twice - first time it prints, 'Processing backup' will be highlighted, second time 'Complete' will be. It's confusing if it has two of those strips visible at once. (By the way, prior to this change, the 'Processing backup' step 4 was shown on that list, but never became highlighted, which was in itself a bit weird!)
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Looks perfect. I got a bit confused with your backup_progress and the backup progress bars (I did not remember they were there, lol).

          Apart from any style consideration that can come later, once people see the bars in action, and a little conflict that I found and solved, nice one.

          So, integrating...

          Show
          Eloy Lafuente (stronk7) added a comment - Looks perfect. I got a bit confused with your backup_progress and the backup progress bars (I did not remember they were there, lol). Apart from any style consideration that can come later, once people see the bars in action, and a little conflict that I found and solved, nice one. So, integrating...
          Hide
          Eloy Lafuente (stronk7) added a comment -

          … and integrated (master only), thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - … and integrated (master only), thanks!
          Hide
          David Monllaó added a comment -

          It passes, all working as described. Thanks for the detailed instructions.

          When restoring the course, at the beginning, the progress bar was stopped for about 2 minutes without changing the percentage nor the lower bar colour, after that it continued as usual. Same behaviour when backing up but not that long. When importing it stops a while just before the 50% but it is not a specially long stop.

          Show
          David Monllaó added a comment - It passes, all working as described. Thanks for the detailed instructions. When restoring the course, at the beginning, the progress bar was stopped for about 2 minutes without changing the percentage nor the lower bar colour, after that it continued as usual. Same behaviour when backing up but not that long. When importing it stops a while just before the 50% but it is not a specially long stop.
          Hide
          Sam Marshall added a comment -

          Thanks David!

          Regarding the long pauses in the progress bar - I wanted to get this framework in place initially, then it should be relatively easy to improve the progress bar afterward in small areas as individual tasks when we identify a long-running part. There are some issues I already filed about that, MDL-41088 (avoid a pause when copying files) and MDL-41089 (avoid a pause when backing up/restoring a forum that has a large number of posts).

          Show
          Sam Marshall added a comment - Thanks David! Regarding the long pauses in the progress bar - I wanted to get this framework in place initially, then it should be relatively easy to improve the progress bar afterward in small areas as individual tasks when we identify a long-running part. There are some issues I already filed about that, MDL-41088 (avoid a pause when copying files) and MDL-41089 (avoid a pause when backing up/restoring a forum that has a large number of posts).
          Hide
          Eloy Lafuente (stronk7) added a comment -

          also, unziping is extremely slow (with our current PHP impl). I've some courses taking hours, when the OS is able to uncompress the same file in a hand of seconds.

          Show
          Eloy Lafuente (stronk7) added a comment - also, unziping is extremely slow (with our current PHP impl). I've some courses taking hours, when the OS is able to uncompress the same file in a hand of seconds.
          Hide
          Kris Stokking added a comment -

          Eloy - we have the same problems with move and copy. Not to hijack this thread, but I wonder if certain functions would be better executed as system calls instead of handled by the application, or at least configurable to do so for performance reasons.

          Show
          Kris Stokking added a comment - Eloy - we have the same problems with move and copy. Not to hijack this thread, but I wonder if certain functions would be better executed as system calls instead of handled by the application, or at least configurable to do so for performance reasons.
          Hide
          Sam Marshall added a comment -

          Kris/Eloy - I think step 1 is to put progress bars in the current implementation. One advantage is that the internal implementations are easier to show progress for With the external ones, because PHP doesn't multi-thread, you have to choose the 'verbose' option and read stdout/stderr from the process, then you can display indeterminate progress every 1024 bytes or something like that. We've done that, but it's more of a nuisance than just adding a 'progress happened' call inside a loop.

          I have code written for unzip progress bar which I am going to submit for next week hopefully. (It depends on this change, obviously.) MDL-41147. There is also one for zip MDL-41087 which I haven't coded yet.

          Show
          Sam Marshall added a comment - Kris/Eloy - I think step 1 is to put progress bars in the current implementation. One advantage is that the internal implementations are easier to show progress for With the external ones, because PHP doesn't multi-thread, you have to choose the 'verbose' option and read stdout/stderr from the process, then you can display indeterminate progress every 1024 bytes or something like that. We've done that, but it's more of a nuisance than just adding a 'progress happened' call inside a loop. I have code written for unzip progress bar which I am going to submit for next week hopefully. (It depends on this change, obviously.) MDL-41147 . There is also one for zip MDL-41087 which I haven't coded yet.
          Hide
          Dan Poltawski added a comment -

          Congratulations! This change has been integrated upstream and is now available from our git and download mirrors. To celebrate, here is a joke:

          A SQL query goes into a bar, walks up to two tables and asks, "Can I join you?"

          Show
          Dan Poltawski added a comment - Congratulations! This change has been integrated upstream and is now available from our git and download mirrors. To celebrate, here is a joke: A SQL query goes into a bar, walks up to two tables and asks, "Can I join you?"
          Hide
          Martin Dougiamas added a comment -

          This really should have been backported see MDL-42211

          Show
          Martin Dougiamas added a comment - This really should have been backported see MDL-42211
          Hide
          Sam Marshall added a comment -

          Regarding backport, this is an API change which is why it was done on master only to begin with. There are many other changes you need to make it fully work; a slightly out of date diagram is shown on MDL-38189, just backporting this one issue won't fix it.

          I've commented on MDL-42211 and can supply a patch for Moodle 2.5 at some point soonish.

          Show
          Sam Marshall added a comment - Regarding backport, this is an API change which is why it was done on master only to begin with. There are many other changes you need to make it fully work; a slightly out of date diagram is shown on MDL-38189 , just backporting this one issue won't fix it. I've commented on MDL-42211 and can supply a patch for Moodle 2.5 at some point soonish.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: