Moodle
  1. Moodle
  2. MDL-25454

Scheduled backups skip hidden courses, even if they have been modified

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 1.9.13, 2.0.4, 2.1.1, 2.2
    • Fix Version/s: 1.9.14, 2.0.5, 2.1.2
    • Component/s: Backup
    • Labels:
    • Database:
      Any
    • Testing Instructions:
      Hide

      Important: This must be tested in 19_STABLE and 21_STABLE (20 and master are 100% equivalent to 21)

      1. Hide a course
      2. In the database, change mdl_course.timemodified to a Unix timestamp older than 31 days, e.g. UPDATE mdl_course SET timemodified = UNIX_TIMESTAMP() - 32 * 86400
      3. Make some changes to the course, e.g. add some resources. It would be better to make changes in the resources or activities
      4. Trigger a scheduled backup

      Show
      Important: This must be tested in 19_STABLE and 21_STABLE (20 and master are 100% equivalent to 21) 1. Hide a course 2. In the database, change mdl_course.timemodified to a Unix timestamp older than 31 days, e.g. UPDATE mdl_course SET timemodified = UNIX_TIMESTAMP() - 32 * 86400 3. Make some changes to the course, e.g. add some resources. It would be better to make changes in the resources or activities 4. Trigger a scheduled backup
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE, MOODLE_21_STABLE
    • Pull Master Branch:
      wip-MDL-25454-master
    • Rank:
      2802

      Description

      backup/backup_scheduled.php contains code that prevents hidden courses from being backed up if they have not been modified in the last month:

      // Skip backup of unavailable courses that have remained unmodified in a month
      $skipped = false;
      if (!$course->visible && ($now - $course->timemodified) > 31*24*60*60) {  //Hidden + unmodified last month
          mtrace("            SKIPPING - hidden+unmodified");
          $DB->set_field("backup_courses","laststatus","3", array("courseid"=>$backup_course->courseid));
          $skipped = true;
      }
      

      However, $course->timemodified is only changed when the function "update_course" is called. This only happens when the course's "settings" page (course/edit.php) has been submitted with changes.

      If a user is editing a course while it is hidden from students, it will not be backed up unless they change a setting.

      Steps to reproduce:

      1. Hide a course
      2. In the database, change mdl_course.timemodified to a Unix timestamp older than 31 days, e.g. UPDATE mdl_course SET timemodified = UNIX_TIMESTAMP() - 32 * 86400
      3. Make some changes to the course, e.g. add some resources.
      4. Trigger a scheduled backup

      Expected outcome: The course is backed up

      Observed outcome: The course is skipped during scheduled backup, and no backup file is created.

      I'm not sure what the best fix is. Something that queries mdl_log, perhaps, e.g.

      SELECT MAX(time) from mdl_log WHERE course = <course id> AND action IN (<list of "update"-type actions>)

      Alternatively, perhaps there should be a mechanism for updating timemodified in mdl_course when such changes are made.

        Issue Links

          Activity

          Hide
          Chris Fryer added a comment -

          Regression caused by this fix

          Show
          Chris Fryer added a comment - Regression caused by this fix
          Hide
          Chris Follin added a comment -

          Any progress on this issue? Many instructors develop courses in advance of a semester and leave them hidden until the semester starts. These courses should be backed up as content is added and instructors believe the courses are being backed up but in reality they're not.

          Show
          Chris Follin added a comment - Any progress on this issue? Many instructors develop courses in advance of a semester and leave them hidden until the semester starts. These courses should be backed up as content is added and instructors believe the courses are being backed up but in reality they're not.
          Hide
          Michael Blake added a comment -

          Please give this issue priority. It has been reported as causing problems on MP sites.

          Show
          Michael Blake added a comment - Please give this issue priority. It has been reported as causing problems on MP sites.
          Hide
          Michael de Raadt added a comment -

          Just a note to say that we are planning to give this some attention after Moodle 2.1 is released.

          Michael;

          Show
          Michael de Raadt added a comment - Just a note to say that we are planning to give this some attention after Moodle 2.1 is released. Michael;
          Hide
          Marina Glancy added a comment -

          During the backup we check log for any non-view entries on the hidden course during the last month.
          This check is made only at the time of backup (not every time the cron is run, like it was before).
          In 2.0+ the field backup_courses.nextstarttime was updated only if the course was backed up. Since the log query is time&resource consuming and I do not want to run it more often than necessary I update the field each time now (as it is in 1.9).

          Show
          Marina Glancy added a comment - During the backup we check log for any non-view entries on the hidden course during the last month. This check is made only at the time of backup (not every time the cron is run, like it was before). In 2.0+ the field backup_courses.nextstarttime was updated only if the course was backed up. Since the log query is time&resource consuming and I do not want to run it more often than necessary I update the field each time now (as it is in 1.9).
          Hide
          Sam Hemelryk added a comment -

          Hi Marina,

          I've just been having a quick look at this now.
          The general logic for your changes makes good sense, however there is a couple of things to take into mind:

          1. The function get_logs first counts all of the matching entries and then returns a set of matching entries based upon $limitfrom and $limitnum. The way the code is presently it is still going to run the count over the logs table before fetching just the one row you are looking for.
            As well as this the select for the row is going the user table.
            I'm thinking that by writing your own query you will be able to get much better performance simply by returning just the id, avoiding the user table join, and avoiding the additional count query.
          1. Is %view% required? A right only search would be much faster if that is applicable. We can always ask Jordan to power up the moodle.org test vm and have a look at what is in that field if that is of help.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Marina, I've just been having a quick look at this now. The general logic for your changes makes good sense, however there is a couple of things to take into mind: The function get_logs first counts all of the matching entries and then returns a set of matching entries based upon $limitfrom and $limitnum. The way the code is presently it is still going to run the count over the logs table before fetching just the one row you are looking for. As well as this the select for the row is going the user table. I'm thinking that by writing your own query you will be able to get much better performance simply by returning just the id, avoiding the user table join, and avoiding the additional count query. Is %view% required? A right only search would be much faster if that is applicable. We can always ask Jordan to power up the moodle.org test vm and have a look at what is in that field if that is of help. Cheers Sam
          Hide
          Marina Glancy added a comment -

          Sam, I agree that using get_logs may slow the process. I replaced it with the direct query.
          Unfortunately, there are some actions like 'template view' and 'preview', so I would stick with %view% as well as report log on changes does

          Show
          Marina Glancy added a comment - Sam, I agree that using get_logs may slow the process. I replaced it with the direct query. Unfortunately, there are some actions like 'template view' and 'preview', so I would stick with %view% as well as report log on changes does
          Hide
          Marina Glancy added a comment - - edited

          During working on this issue I have noticed some differences between 1.9 and 2.0+

          In 1.9 the field backup_course.nextstarttime is updated each time the cron runs. In 2.0 it is updated only when the backup is performed. Probably because of this the check backup_course.nextstarttime>0 is replaced in 2.0 with backup_course.nextstarttime>=0.

          What it means (in 2.0+):

          • if course was hidden, the nextstarttime may never be updated. This was changed in this issue and it will most likely resolve MDL-28242
          • after the automatic backup is turned on for the first time, the backup is performed for all courses on the next cron run, which easily may be in the middle of the day slowing the work of the big website (quite unexpectedly for admin)
          • if the automatic backup schedule was changed, the change will only be applied after the next run which is already scheduled (again, admin may be unhappy)

          Probably it causes the problems for Emma who created issue MDL-25994 (if 1/10/2011 is the 1st of October in her schedule)

          The fix for it is really small. This is the patch for 2.0+ (applied after my changes):

          --- a/backup/util/helper/backup_cron_helper.class.php
          +++ b/backup/util/helper/backup_cron_helper.class.php
          @@ -124,7 +124,12 @@ abstract class backup_cron_automated_helper {
                           }
           
                           // Skip courses that do not yet need backup
          -                $skipped = !(($backupcourse->nextstarttime >= 0 && $backupcourse->nextstarttime < $now) || $rundirective == self::RUN_IMMEDIATELY);
          +                $skipped = !(($backupcourse->nextstarttime > 0 && $backupcourse->nextstarttime < $now) || $rundirective == self::RUN_IMMEDIATELY);
          +                if ($skipped && $backupcourse->nextstarttime != $nextstarttime) {
          +                    // Update nextstarttime in DB if the schedule has been changed but this time backup is skipped
          +                    $backupcourse->nextstarttime = $nextstarttime;
          +                    $DB->update_record('backup_courses', $backupcourse);
          +                }
                           // Skip backup of unavailable courses that have remained unmodified in a month
                           if (!$skipped && empty($course->visible) && ($now - $course->timemodified) > 31*24*60*60) {  //Hidden + settings were unmodified last month
                               //Check log if there were any modifications to the course content
          

          Eloy, I guess, we leave this task until you come back from vacation

          Show
          Marina Glancy added a comment - - edited During working on this issue I have noticed some differences between 1.9 and 2.0+ In 1.9 the field backup_course.nextstarttime is updated each time the cron runs. In 2.0 it is updated only when the backup is performed. Probably because of this the check backup_course.nextstarttime>0 is replaced in 2.0 with backup_course.nextstarttime>=0. What it means (in 2.0+): if course was hidden, the nextstarttime may never be updated. This was changed in this issue and it will most likely resolve MDL-28242 after the automatic backup is turned on for the first time, the backup is performed for all courses on the next cron run, which easily may be in the middle of the day slowing the work of the big website (quite unexpectedly for admin) if the automatic backup schedule was changed, the change will only be applied after the next run which is already scheduled (again, admin may be unhappy) Probably it causes the problems for Emma who created issue MDL-25994 (if 1/10/2011 is the 1st of October in her schedule) The fix for it is really small. This is the patch for 2.0+ (applied after my changes): --- a/backup/util/helper/backup_cron_helper.class.php +++ b/backup/util/helper/backup_cron_helper.class.php @@ -124,7 +124,12 @@ abstract class backup_cron_automated_helper { } // Skip courses that do not yet need backup - $skipped = !(($backupcourse->nextstarttime >= 0 && $backupcourse->nextstarttime < $now) || $rundirective == self::RUN_IMMEDIATELY); + $skipped = !(($backupcourse->nextstarttime > 0 && $backupcourse->nextstarttime < $now) || $rundirective == self::RUN_IMMEDIATELY); + if ($skipped && $backupcourse->nextstarttime != $nextstarttime) { + // Update nextstarttime in DB if the schedule has been changed but this time backup is skipped + $backupcourse->nextstarttime = $nextstarttime; + $DB->update_record('backup_courses', $backupcourse); + } // Skip backup of unavailable courses that have remained unmodified in a month if (!$skipped && empty($course->visible) && ($now - $course->timemodified) > 31*24*60*60) { //Hidden + settings were unmodified last month //Check log if there were any modifications to the course content Eloy, I guess, we leave this task until you come back from vacation
          Hide
          Marina Glancy added a comment -

          Eloy, please check also MDL-28531 - there are bugs in calculating of the next automatic backup time. Patch provided

          Show
          Marina Glancy added a comment - Eloy, please check also MDL-28531 - there are bugs in calculating of the next automatic backup time. Patch provided
          Hide
          Sam Hemelryk added a comment -

          Hmmm just been having a very close look at this, I really think this needs to wait until Eloy is back as he is certainly the best person to give this the go ahead.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hmmm just been having a very close look at this, I really think this needs to wait until Eloy is back as he is certainly the best person to give this the go ahead. Cheers Sam
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Marina (et all),

          I'm reopening this in order to have some more time to check it properly. It is in my TOP-5 TODO list so I hope to review and comment here before next week.

          Thanks for the hard word! Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Marina (et all), I'm reopening this in order to have some more time to check it properly. It is in my TOP-5 TODO list so I hope to review and comment here before next week. Thanks for the hard word! Ciao
          Hide
          Sam Hemelryk added a comment -

          ...ping...

          Show
          Sam Hemelryk added a comment - ...ping...
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Ho,

          before this to land we need, for sure to:

          1) mandatory: change that get_records() call to one count_records_select() or record_exists_select(). We don't need to fetch 1 zillion of ids to know if there are records there.
          2) optional (could be done after integrating this): See MDL-28531. We need clarification and agreement about how (and when) each type of automated backups run (scheduled, immediate) and how they calculate/set next execution times or no. Right now both are awfully mixed.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Ho, before this to land we need, for sure to: 1) mandatory: change that get_records() call to one count_records_select() or record_exists_select(). We don't need to fetch 1 zillion of ids to know if there are records there. 2) optional (could be done after integrating this): See MDL-28531 . We need clarification and agreement about how (and when) each type of automated backups run (scheduled, immediate) and how they calculate/set next execution times or no. Right now both are awfully mixed. Ciao
          Hide
          James Cracknell added a comment -

          Is MDL-28162 related to this?

          Show
          James Cracknell added a comment - Is MDL-28162 related to this?
          Hide
          moodle.com added a comment -

          This issue has been left behind from a few sprints ago. I'm bringing this into the current sprint and I'll ask Marina to look at it when she returns from holidays. I think resolving this may also resolve some other issues (linked); these should be checked after this issue is closed.

          Show
          moodle.com added a comment - This issue has been left behind from a few sprints ago. I'm bringing this into the current sprint and I'll ask Marina to look at it when she returns from holidays. I think resolving this may also resolve some other issues (linked); these should be checked after this issue is closed.
          Hide
          Marina Glancy added a comment -

          Eloy,

          1) I use

          $logs = $DB->get_records_sql($sql, $params, 0, 1); 
          

          which is SELECT ... LIMIT 0,1, and limiting is done within DB, so DB stops searching after finding the first entry.
          Besides, this is the same approach that is used in $DB->record_exists.
          Aggregate functions like COUNT will be definitely longer, because in this case DB will need to look through the whole table.
          I don't see how I can change the code to make the query faster.

          2) as we discussed yesterday with you, calculation of the nextstarttime will be changed (if needed) in MDL-28531. I copied my comment from 28/Jul there

          The branch is rebased. I am still sure it is ready for integration.

          Show
          Marina Glancy added a comment - Eloy, 1) I use $logs = $DB->get_records_sql($sql, $params, 0, 1); which is SELECT ... LIMIT 0,1 , and limiting is done within DB, so DB stops searching after finding the first entry. Besides, this is the same approach that is used in $DB->record_exists. Aggregate functions like COUNT will be definitely longer, because in this case DB will need to look through the whole table. I don't see how I can change the code to make the query faster. 2) as we discussed yesterday with you, calculation of the nextstarttime will be changed (if needed) in MDL-28531 . I copied my comment from 28/Jul there The branch is rebased. I am still sure it is ready for integration.
          Hide
          Marina Glancy added a comment -

          changed checking the log to record_exists

          Show
          Marina Glancy added a comment - changed checking the log to record_exists
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          (switching roles here between Sam and me)

          I think it looks perfect now, Marina! Many thanks! I'll try to do some runs today.

          Note that surely it needs some more specific testing instructions (expected results).

          Show
          Eloy Lafuente (stronk7) added a comment - - edited (switching roles here between Sam and me) I think it looks perfect now, Marina! Many thanks! I'll try to do some runs today. Note that surely it needs some more specific testing instructions (expected results).
          Hide
          Sam Hemelryk added a comment -

          Thanks guys - this has landed now

          Show
          Sam Hemelryk added a comment - Thanks guys - this has landed now
          Hide
          Michael Gaab added a comment -

          It would also be very useful to be able to force a full backup of a site at will. This would be very usefull at the end of every semester prior to downloading and removing last semester's courses. This would raise the level of confidence one has prior to permanently removing a course.

          Is it possible to add such a flag/switch that would be accessible on the current backup configuration page?

          I've been watching this item for some time. Sorry about the late suggestion.

          Mike

          Show
          Michael Gaab added a comment - It would also be very useful to be able to force a full backup of a site at will. This would be very usefull at the end of every semester prior to downloading and removing last semester's courses. This would raise the level of confidence one has prior to permanently removing a course. Is it possible to add such a flag/switch that would be accessible on the current backup configuration page? I've been watching this item for some time. Sorry about the late suggestion. Mike
          Hide
          Rajesh Taneja added a comment -

          Thanks for fixing this Marina
          Works like a charm.

          Show
          Rajesh Taneja added a comment - Thanks for fixing this Marina Works like a charm.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          git repositories have been updated with your awesome changes, thanks! Closing.

          Show
          Eloy Lafuente (stronk7) added a comment - git repositories have been updated with your awesome changes, thanks! Closing.
          Hide
          Michael Gaab added a comment -

          Is this fix functional in Moodle 1.9.15+ (Build: 20111201)?

          Show
          Michael Gaab added a comment - Is this fix functional in Moodle 1.9.15+ (Build: 20111201)?
          Hide
          Michael Gaab added a comment - - edited

          I'll just assume that it is functional as I did compare our previous build with the our current build and as far as I can tell the fix is in Moodle 1.9.15+ (Build: 20111201).

          We have been testing this fix as it is very important to our institution. We have found some issues with it. Namely, that if I if simply add a resourse or activity, the course will still be skipped, assuming that the other criteria are satisfied: timemodified > 31 days and the course is hidden. There may be other issues. We are continuing to test.

          _________________________________
          Michael Gaab
          Blackboard/Moodle Administrator
          School of Extended & Lifelong Learning
          The University of Montana
          Missoula, MT 59812
          406.243.6373

          Show
          Michael Gaab added a comment - - edited I'll just assume that it is functional as I did compare our previous build with the our current build and as far as I can tell the fix is in Moodle 1.9.15+ (Build: 20111201). We have been testing this fix as it is very important to our institution. We have found some issues with it. Namely, that if I if simply add a resourse or activity, the course will still be skipped, assuming that the other criteria are satisfied: timemodified > 31 days and the course is hidden. There may be other issues. We are continuing to test. _________________________________ Michael Gaab Blackboard/Moodle Administrator School of Extended & Lifelong Learning The University of Montana Missoula, MT 59812 406.243.6373
          Hide
          Rajesh Taneja added a comment -

          Thanks for testing this Michael. Support for 1.9 is now for security issues only, if you feel similar issue exists on 2.x version, then please feel free to open another bug.

          Show
          Rajesh Taneja added a comment - Thanks for testing this Michael. Support for 1.9 is now for security issues only, if you feel similar issue exists on 2.x version, then please feel free to open another bug.

            People

            • Votes:
              27 Vote for this issue
              Watchers:
              11 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: