Moodle
  1. Moodle
  2. MDL-32148

Automated backups : Skip backup of courses that have remained unmodified since last backup

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.4
    • Fix Version/s: 2.4
    • Component/s: Backup
    • Labels:
    • Testing Instructions:
      Hide
      1. Go to "Site administration > Courses > Backups > Automated backup setup" and set Active dropdown list to Manual
      • Testing of "backup_auto_skip_hidden"
      1. Go to "Site administration > Courses > Backups > Automated backup setup" and check "Skip hidden courses"
      2. Create 2 new courses that are not hidden
      3. Run file admin/cli/automated_backups.php from cli (php admin/cli/automated_backups.php).
        -> You should see in the output that the 2 courses are backed up.
      4. Go see in the restore page of each courses that the backup is there.
      5. Edit the 2 courses to be hidden
      6. Run file admin/cli/automated_backups.php from cli again.
        -> You should see in the output that the 2 courses are skipped with message "Skipping Course name (Not visible)"
      7. Go see in the restore page of each courses that the backup hasn't changed (time).
        -> The 2 new courses should be skipped from backup.
      • Testing of "backup_auto_skip_not_modif_since_days"
      1. Go to "Site administration > Courses > Backups > Automated backup setup" and select "3 days" in "Skip courses not modified since previous backup"
      2. Create 2 new courses that are not hidden
      3. Run file admin/cli/automated_backups.php from cli (php admin/cli/automated_backups.php).
        -> You should see in the output that the 2 courses are backed up.
      4. Go see in the restore page of each courses that the backup is there.
      5. Directly in the database, change the timemodified and the logs of the courses to greater than 3 days.
      6. Run file admin/cli/automated_backups.php from cli again.
        -> You should see in the output that the 2 courses are skipped with message "Skipping Course name (Not modified since XX days)"
      7. Go see in the restore page of each courses that the backup hasn't changed (time).
        -> The 2 new courses should be skipped from backup.
      • Testing of "backup_auto_skip_not_modif_since_prev"
      1. Go to "Site administration > Courses > Backups > Automated backup setup" and check "Skip courses not modified since previous backup"
      2. Create 2 new courses that are not hidden
      3. Run file admin/cli/automated_backups.php from cli (php admin/cli/automated_backups.php).
        -> You should see in the output that the 2 courses are backed up.
      4. Go see in the restore page of each courses that the backup is there.
      5. Run file admin/cli/automated_backups.php from cli again.
        -> You should see in the output that the 2 courses are skipped (Not modified since previous backup)
      6. Go see in the restore page of each courses that the backup hasn't changed (time).
        -> The 2 new courses should be skipped from backup.
        Repeat for each resources and activities
      7. Modifiy one of the courses by adding any resouces or activities (File, folders, assignment, forum...)
      8. Run file admin/cli/automated_backups.php from cli again.
        -> You should see in the output that the course you modified is backed up and the other skipped.
      9. Go see in the restore page of the courses that one backup has changed (time) and the other not.
        End for
      • TEST DIFFERENT COMBINATIONS OF SETTINGS
      • Rule should be that if one of the three is true, then the course is skipped from backup
      Show
      Go to "Site administration > Courses > Backups > Automated backup setup" and set Active dropdown list to Manual Testing of "backup_auto_skip_hidden" Go to "Site administration > Courses > Backups > Automated backup setup" and check "Skip hidden courses" Create 2 new courses that are not hidden Run file admin/cli/automated_backups.php from cli (php admin/cli/automated_backups.php). -> You should see in the output that the 2 courses are backed up. Go see in the restore page of each courses that the backup is there. Edit the 2 courses to be hidden Run file admin/cli/automated_backups.php from cli again. -> You should see in the output that the 2 courses are skipped with message "Skipping Course name (Not visible)" Go see in the restore page of each courses that the backup hasn't changed (time). -> The 2 new courses should be skipped from backup. Testing of "backup_auto_skip_not_modif_since_days" Go to "Site administration > Courses > Backups > Automated backup setup" and select "3 days" in "Skip courses not modified since previous backup" Create 2 new courses that are not hidden Run file admin/cli/automated_backups.php from cli (php admin/cli/automated_backups.php). -> You should see in the output that the 2 courses are backed up. Go see in the restore page of each courses that the backup is there. Directly in the database, change the timemodified and the logs of the courses to greater than 3 days. Run file admin/cli/automated_backups.php from cli again. -> You should see in the output that the 2 courses are skipped with message "Skipping Course name (Not modified since XX days)" Go see in the restore page of each courses that the backup hasn't changed (time). -> The 2 new courses should be skipped from backup. Testing of "backup_auto_skip_not_modif_since_prev" Go to "Site administration > Courses > Backups > Automated backup setup" and check "Skip courses not modified since previous backup" Create 2 new courses that are not hidden Run file admin/cli/automated_backups.php from cli (php admin/cli/automated_backups.php). -> You should see in the output that the 2 courses are backed up. Go see in the restore page of each courses that the backup is there. Run file admin/cli/automated_backups.php from cli again. -> You should see in the output that the 2 courses are skipped (Not modified since previous backup) Go see in the restore page of each courses that the backup hasn't changed (time). -> The 2 new courses should be skipped from backup. Repeat for each resources and activities Modifiy one of the courses by adding any resouces or activities (File, folders, assignment, forum...) Run file admin/cli/automated_backups.php from cli again. -> You should see in the output that the course you modified is backed up and the other skipped. Go see in the restore page of the courses that one backup has changed (time) and the other not. End for TEST DIFFERENT COMBINATIONS OF SETTINGS Rule should be that if one of the three is true, then the course is skipped from backup
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
      git@github.com:StudiUM/moodle.git
    • Pull Master Branch:
      MDL-32148-master
    • Rank:
      38872

      Description

      Skip backup of courses that have remained unmodified since last backup instead of unmodified in a month.

      The current state causes problem where backups of courses are done when you don't need to because nothing has changed.

      Benefits of this improvement :

      • less time to execute automated backups (Optimization of server resources like CPU)
      • less overall disk usage
      • Backup files that are kept (backup_auto_keep rotation) really are useful (no duplicates)

      If you have a large number of courses and a major users activity like we do (about 1300 active courses per semester), this is a must before any activation of the automated backups procedure.

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Thanks for sharing this improvement.

          I've triaged this issue, but I have also promoted you as a developer. If you wish, you can now push this for peer review.

          I've assigned Eloy as peer reviewer as I think he would be keen to see this improvement.

          Show
          Michael de Raadt added a comment - Thanks for sharing this improvement. I've triaged this issue, but I have also promoted you as a developer. If you wish, you can now push this for peer review. I've assigned Eloy as peer reviewer as I think he would be keen to see this improvement.
          Hide
          Jean-Philippe Gaudreau added a comment -

          Thx Michael!

          I've just requested peer review from Eloy. I also did a little correction to remove the hidden course condition. Thx for putting issue links so I could see in http://tracker.moodle.org/browse/MDL-25454 that this would have cause a bug.

          Show
          Jean-Philippe Gaudreau added a comment - Thx Michael! I've just requested peer review from Eloy. I also did a little correction to remove the hidden course condition. Thx for putting issue links so I could see in http://tracker.moodle.org/browse/MDL-25454 that this would have cause a bug.
          Hide
          Dan Poltawski added a comment -

          Hi Jean-Phillippe,

          Makes a lot of sense to me, thanks!

          Are you able to rebase your branches aginst the lastest changes and supply testing instructions for this change?

          Once that has done I will submit this for integration.

          thanks again,
          dan

          Show
          Dan Poltawski added a comment - Hi Jean-Phillippe, Makes a lot of sense to me, thanks! Are you able to rebase your branches aginst the lastest changes and supply testing instructions for this change? Once that has done I will submit this for integration. thanks again, dan
          Hide
          Jean-Philippe Gaudreau added a comment -

          Hi Dan,

          I've rebase branches and supply testing instructions! Feel free to say if you think I should cover more cases in the tests.

          J-P

          Show
          Jean-Philippe Gaudreau added a comment - Hi Dan, I've rebase branches and supply testing instructions! Feel free to say if you think I should cover more cases in the tests. J-P
          Hide
          Dan Poltawski added a comment -

          I've submitted this for integration - thanks!

          Show
          Dan Poltawski added a comment - I've submitted this for integration - thanks!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi,

          while I can see this as a really good (and clever) plan to decide about when to backup one course... I think we shouldn't be taking rid of the old approach completely, as far as it is in use by production sites out there and perhaps changing to the new alternative could have some impact in their systems/organization.

          Trying to imagine the best way to support both worlds, I've come with this idea:

          1) Create a bunch of new settings for automated backups, namely:

          • backup_auto_skip_hidden (1 = yes, 0 = no)
          • backup_auto_skip_not_modif_since_days (X = number of days, 0 = no)
          • backup_auto_skip_not_modif_since_previous_backup (1 = yes, 0 = no)

          2) Make automated backup to evaluate all the enabled ones in order to decide if one backup can be skipped (logical AND).

          3) Upgrade existing sites to hidden + not_modif_since_days = 30 (old behavior).

          4) Decide the defaults for new sites. Surely we can continue with the same, but not problem if we decide to go with the new "not_modif_since_previous_backup" alternative.

          5) Done. Full BC and with organized support for new skipping strategies.

          Of course, this only can land into DEV version and not in stables at all, as far as it's a new feature.

          I'm going to reopen this for further discussion... thanks a lot for your contribution Jean-Philippe.

          Ciao

          PS: Personally, I think that, instead of having all those course backups, it's far better to use complete backups of the moodle site (bd + moodledata) in order to restore anything successfully, not to talk about the "extra" effort/resources/pressure that performing daily/weekly automated backups causes to the system.

          Show
          Eloy Lafuente (stronk7) added a comment - Hi, while I can see this as a really good (and clever) plan to decide about when to backup one course... I think we shouldn't be taking rid of the old approach completely, as far as it is in use by production sites out there and perhaps changing to the new alternative could have some impact in their systems/organization. Trying to imagine the best way to support both worlds, I've come with this idea: 1) Create a bunch of new settings for automated backups, namely: backup_auto_skip_hidden (1 = yes, 0 = no) backup_auto_skip_not_modif_since_days (X = number of days, 0 = no) backup_auto_skip_not_modif_since_previous_backup (1 = yes, 0 = no) 2) Make automated backup to evaluate all the enabled ones in order to decide if one backup can be skipped (logical AND). 3) Upgrade existing sites to hidden + not_modif_since_days = 30 (old behavior). 4) Decide the defaults for new sites. Surely we can continue with the same, but not problem if we decide to go with the new "not_modif_since_previous_backup" alternative. 5) Done. Full BC and with organized support for new skipping strategies. Of course, this only can land into DEV version and not in stables at all, as far as it's a new feature. I'm going to reopen this for further discussion... thanks a lot for your contribution Jean-Philippe. Ciao PS: Personally, I think that, instead of having all those course backups, it's far better to use complete backups of the moodle site (bd + moodledata) in order to restore anything successfully, not to talk about the "extra" effort/resources/pressure that performing daily/weekly automated backups causes to the system.
          Hide
          Jean-Philippe Gaudreau added a comment -

          Hi Eloy,

          Thx! Everything you said makes lot of sense to me. It would be the best way to offer this feature with flexibility and without impacting current production sites.

          Unfortunately, I don't have the time to work on those changes right now... I'll try to put this on our next sprint but can't garantee anything since we have a lot to do lol...

          Anyway thx for the solution!

          Just for information : We already have complete backups of moodle site but I think the best scenario is to have both complete backups (bd + moodledata) and moodle automated backups of modified courses because one gives total safety and the other makes it easier to restore a course (For example, A teacher accidently delete one assigment activity).

          To solve the "effort/resources/pressure that performing daily/weekly automated backups causes to the system", we built a dedicated server for everything that's running on CLI (cron, automated backups ...) so that helps a lot!

          Bye!

          Show
          Jean-Philippe Gaudreau added a comment - Hi Eloy, Thx! Everything you said makes lot of sense to me. It would be the best way to offer this feature with flexibility and without impacting current production sites. Unfortunately, I don't have the time to work on those changes right now... I'll try to put this on our next sprint but can't garantee anything since we have a lot to do lol... Anyway thx for the solution! Just for information : We already have complete backups of moodle site but I think the best scenario is to have both complete backups (bd + moodledata) and moodle automated backups of modified courses because one gives total safety and the other makes it easier to restore a course (For example, A teacher accidently delete one assigment activity). To solve the "effort/resources/pressure that performing daily/weekly automated backups causes to the system", we built a dedicated server for everything that's running on CLI (cron, automated backups ...) so that helps a lot! Bye!
          Hide
          Heiko Schach added a comment -

          Hello,

          I agree the best scenario is complete site backups plus automated backups of modified courses.

          Automated backups currently put unnecessary load on our servers. Skipping the backup of courses unmodified since last backup makes sense to me. Eloy's idea of making this configurable sounds great.

          How do you define unmodified? What are the modifications to consider?
          Teacher modifying course content is obvious, but how about students taking a quiz, posting to a forum, submitting a file or just browsing material?

          Show
          Heiko Schach added a comment - Hello, I agree the best scenario is complete site backups plus automated backups of modified courses. Automated backups currently put unnecessary load on our servers. Skipping the backup of courses unmodified since last backup makes sense to me. Eloy's idea of making this configurable sounds great. How do you define unmodified? What are the modifications to consider? Teacher modifying course content is obvious, but how about students taking a quiz, posting to a forum, submitting a file or just browsing material?
          Hide
          Heiko Schach added a comment -

          Thank you Jean-Philippe for this patch.

          On our site we have a lot of courses with little or no activity.
          We have automated backups running with your patch for some time now on our testing site (Moodle 2.2, PostgreSQL).

          Automated backups now usually complete in under one hour. Without the patch it would take more than one day to backup all the courses.

          Show
          Heiko Schach added a comment - Thank you Jean-Philippe for this patch. On our site we have a lot of courses with little or no activity. We have automated backups running with your patch for some time now on our testing site (Moodle 2.2, PostgreSQL). Automated backups now usually complete in under one hour. Without the patch it would take more than one day to backup all the courses.
          Hide
          Jean-Philippe Gaudreau added a comment -

          Hi Heiko,

          I'm glad to hear the patch was useful to you.

          I've just posted a new one with the new settings for automated backups :

          • backup_auto_skip_hidden (1 = yes, 0 = no)
          • backup_auto_skip_not_modif_since_days (X = number of days, 0 = no)
          • backup_auto_skip_not_modif_since_previous_backup (1 = yes, 0 = no)

          I'm putting Eloy as peer reviewer as he would the best to do so.

          Cheers!

          Show
          Jean-Philippe Gaudreau added a comment - Hi Heiko, I'm glad to hear the patch was useful to you. I've just posted a new one with the new settings for automated backups : backup_auto_skip_hidden (1 = yes, 0 = no) backup_auto_skip_not_modif_since_days (X = number of days, 0 = no) backup_auto_skip_not_modif_since_previous_backup (1 = yes, 0 = no) I'm putting Eloy as peer reviewer as he would the best to do so. Cheers!
          Hide
          Frédéric Massart added a comment -

          Hi Jean-Philippe, thank you for working on this. Before anyone can peer review that, it would be nice if you could rebase your branch as it conflicts with the current master (see MDL-28531). Cheers !

          Show
          Frédéric Massart added a comment - Hi Jean-Philippe, thank you for working on this. Before anyone can peer review that, it would be nice if you could rebase your branch as it conflicts with the current master (see MDL-28531 ). Cheers !
          Hide
          Jean-Philippe Gaudreau added a comment -

          Hi Frédéric,

          I've rebased the branch with the current master! Effectively, there was a conflict because I renamed the "$skipped" variable.

          Bye!

          Show
          Jean-Philippe Gaudreau added a comment - Hi Frédéric, I've rebased the branch with the current master! Effectively, there was a conflict because I renamed the "$skipped" variable. Bye!
          Hide
          Frédéric Massart added a comment -

          Hi Jean-Philippe, thanks a lot for working on this. This will definitely be a great improvement for a lot of people!

          I have reviewed your code, although there are still a couple of conflicts which you probably want to get rid of so that I will be able to finalise the review.

          • About the new settings, I'd personally try to have shorter setting code names and names. The description is the ideal place to be more verbose and describe in details what the setting actually does. It's not major, but backup_auto_skip_not_modif_since_prev is a bit long IMO.
          • It's not of a major importance either, but having different level of skipping makes the logic less understandable. If you are willing to, I'd be happy to have only on if (!$skipped) which would contain the actual backup logic instead of other skipping tests.
          • While you are at it, could you replace the hardcoded 2 with BACKUP_STATUS_UNFINISHED on line 189 if ($backupcourse->laststatus != 2)?
          • I'm a bit scared of using backup_auto_skip_not_modif_since_prev because that needs deeper checks on whether the course has been successfully backuped or not. In the present case, the backup would be skipped if the previous backup has failed, and we definitely don't want that. Also, for now the logic does not update laststarttime when the backup is skipped, but if for any reason we did it, we have to handle this case as well. Perhaps we could assume that any backup which is not BACKUP_STATUS_OK should be backuped after all, I don't know... please note that we recently introduced the status WARNING which appears when a file happens to be missing during a backup.
          • I just thought that outputting in the logs the reason of the skip could be nice as well, nothing fancy but something like Skipping MyCourse (notvisible).
          • You can use the constant DAYSECS instead of 24 * 60 * 60, not that I think days will be longer in the future, but as the constant exists...
          • In both if's not modif since I'd check for $course->timemodified <= $timenotmodifsincedays before doing the SQL query as it won't always be required.
          • There are a couple of white space issues (space missing before if and the brackets, empty line with spaces, ...), the comments you have introduced should end with a dot. http://docs.moodle.org/dev/Coding_style
          • And to make everything perfect, we could use the component backup in the commit message http://docs.moodle.org/dev/Commit_cheat_sheet#Provide_clear_commit_messages

          Thanks again for your work and your time given here. Please tell me if you do not agree with any of the above so that we can discuss it or push it for integration.

          Cheers!

          Show
          Frédéric Massart added a comment - Hi Jean-Philippe, thanks a lot for working on this. This will definitely be a great improvement for a lot of people! I have reviewed your code, although there are still a couple of conflicts which you probably want to get rid of so that I will be able to finalise the review. About the new settings, I'd personally try to have shorter setting code names and names. The description is the ideal place to be more verbose and describe in details what the setting actually does. It's not major, but backup_auto_skip_not_modif_since_prev is a bit long IMO. It's not of a major importance either, but having different level of skipping makes the logic less understandable. If you are willing to, I'd be happy to have only on if (!$skipped) which would contain the actual backup logic instead of other skipping tests. While you are at it, could you replace the hardcoded 2 with BACKUP_STATUS_UNFINISHED on line 189 if ($backupcourse->laststatus != 2) ? I'm a bit scared of using backup_auto_skip_not_modif_since_prev because that needs deeper checks on whether the course has been successfully backuped or not. In the present case, the backup would be skipped if the previous backup has failed, and we definitely don't want that. Also, for now the logic does not update laststarttime when the backup is skipped, but if for any reason we did it, we have to handle this case as well. Perhaps we could assume that any backup which is not BACKUP_STATUS_OK should be backuped after all, I don't know... please note that we recently introduced the status WARNING which appears when a file happens to be missing during a backup. I just thought that outputting in the logs the reason of the skip could be nice as well, nothing fancy but something like Skipping MyCourse (notvisible) . You can use the constant DAYSECS instead of 24 * 60 * 60, not that I think days will be longer in the future, but as the constant exists... In both if's not modif since I'd check for $course->timemodified <= $timenotmodifsincedays before doing the SQL query as it won't always be required. There are a couple of white space issues (space missing before if and the brackets, empty line with spaces, ...), the comments you have introduced should end with a dot. http://docs.moodle.org/dev/Coding_style And to make everything perfect, we could use the component backup in the commit message http://docs.moodle.org/dev/Commit_cheat_sheet#Provide_clear_commit_messages Thanks again for your work and your time given here. Please tell me if you do not agree with any of the above so that we can discuss it or push it for integration. Cheers!
          Hide
          Jean-Philippe Gaudreau added a comment -

          Hi frédéric,

          Thx for the review! I'm a bit busy right now but I'll try to integrate the modifications as soon as possible.

          Show
          Jean-Philippe Gaudreau added a comment - Hi frédéric, Thx for the review! I'm a bit busy right now but I'll try to integrate the modifications as soon as possible.
          Hide
          Jean-Philippe Gaudreau added a comment - - edited

          Hi Frédéric,

          First of all, thx for the detailed review!

          Here's what I've done for each of your feedback (text in red) :

          • About the new settings, I'd personally try to have shorter setting code names and names. The description is the ideal place to be more verbose and describe in details what the setting actually does. It's not major, but backup_auto_skip_not_modif_since_prev is a bit long IMO.

            I've shortened the variables name. They are still long but I wanted to keep a certain meaning without having to see the detailed description
            ** backup_auto_skip_not_modif_since_days -> backup_auto_skip_modif_days
            ** backup_auto_skip_not_modif_since_prev -> backup_auto_skip_modif_prev
          • It's not of a major importance either, but having different level of skipping makes the logic less understandable. If you are willing to, I'd be happy to have only on if (!$skipped) which would contain the actual backup logic instead of other skipping tests.

            Yes, after looking at it I think it's a better idea and makes the code more understandable. I've refactored to code to only use $skipped variable.
          • While you are at it, could you replace the hardcoded 2 with BACKUP_STATUS_UNFINISHED on line 189 if ($backupcourse->laststatus != 2)?

            I've made the change even though it's not related to the task . I've also changed "backup_cron_automated_helper::BACKUP_STATUS_SKIPPED;" on line 132 to "self::BACKUP_STATUS_SKIPPED;".
          • I'm a bit scared of using backup_auto_skip_not_modif_since_prev because that needs deeper checks on whether the course has been successfully backuped or not. In the present case, the backup would be skipped if the previous backup has failed, and we definitely don't want that. Also, for now the logic does not update laststarttime when the backup is skipped, but if for any reason we did it, we have to handle this case as well. Perhaps we could assume that any backup which is not BACKUP_STATUS_OK should be backuped after all, I don't know... please note that we recently introduced the status WARNING which appears when a file happens to be missing during a backup.

            I've added a condition to check if backup last status id BACKUP_STATUS_SKIPPED or BACKUP_STATUS_OK for the backup_auto_skip_modif_prev setting
          • I just thought that outputting in the logs the reason of the skip could be nice as well, nothing fancy but something like Skipping MyCourse (notvisible).

            I've added a variable with a description of why the course is skipped. There is four different messages :
            ** Skipping Course name (Do not yet need backup)
            ** Skipping Course name (Not visible)
            ** Skipping Course name (Not modified since XXdays)
            ** Skipping Course name (Not modified since previous backup)
          • You can use the constant DAYSECS instead of 24 * 60 * 60, not that I think days will be longer in the future, but as the constant exists...

            Done
          • In both if's not modif since I'd check for $course->timemodified <= $timenotmodifsincedays before doing the SQL query as it won't always be required.

            I think it's safer to leave it this way because I realized over time that $course->timemodified is not always modified when course is updated... Checking both makes the skipped condition safer IMO.
          • There are a couple of white space issues (space missing before if and the brackets, empty line with spaces, ...), the comments you have introduced should end with a dot.

            Thx for the link! I've tried my best to follow the coding style. Please tell me if I missed something.
          • And to make everything perfect, we could use the component backup in the commit message

            I've put "backup" in the commit message after the MDL-xxxxx. Is it what you meant?

          Feel free to give me some new feedbacks and/or argue about the $course->timemodified and log condition

          Thx again!

          Show
          Jean-Philippe Gaudreau added a comment - - edited Hi Frédéric, First of all, thx for the detailed review! Here's what I've done for each of your feedback (text in red) : About the new settings, I'd personally try to have shorter setting code names and names. The description is the ideal place to be more verbose and describe in details what the setting actually does. It's not major, but backup_auto_skip_not_modif_since_prev is a bit long IMO. I've shortened the variables name. They are still long but I wanted to keep a certain meaning without having to see the detailed description ** backup_auto_skip_not_modif_since_days -> backup_auto_skip_modif_days ** backup_auto_skip_not_modif_since_prev -> backup_auto_skip_modif_prev It's not of a major importance either, but having different level of skipping makes the logic less understandable. If you are willing to, I'd be happy to have only on if (!$skipped) which would contain the actual backup logic instead of other skipping tests. Yes, after looking at it I think it's a better idea and makes the code more understandable. I've refactored to code to only use $skipped variable. While you are at it, could you replace the hardcoded 2 with BACKUP_STATUS_UNFINISHED on line 189 if ($backupcourse->laststatus != 2)? I've made the change even though it's not related to the task . I've also changed "backup_cron_automated_helper::BACKUP_STATUS_SKIPPED;" on line 132 to "self::BACKUP_STATUS_SKIPPED;". I'm a bit scared of using backup_auto_skip_not_modif_since_prev because that needs deeper checks on whether the course has been successfully backuped or not. In the present case, the backup would be skipped if the previous backup has failed, and we definitely don't want that. Also, for now the logic does not update laststarttime when the backup is skipped, but if for any reason we did it, we have to handle this case as well. Perhaps we could assume that any backup which is not BACKUP_STATUS_OK should be backuped after all, I don't know... please note that we recently introduced the status WARNING which appears when a file happens to be missing during a backup. I've added a condition to check if backup last status id BACKUP_STATUS_SKIPPED or BACKUP_STATUS_OK for the backup_auto_skip_modif_prev setting I just thought that outputting in the logs the reason of the skip could be nice as well, nothing fancy but something like Skipping MyCourse (notvisible). I've added a variable with a description of why the course is skipped. There is four different messages : ** Skipping Course name (Do not yet need backup) ** Skipping Course name (Not visible) ** Skipping Course name (Not modified since XXdays) ** Skipping Course name (Not modified since previous backup) You can use the constant DAYSECS instead of 24 * 60 * 60, not that I think days will be longer in the future, but as the constant exists... Done In both if's not modif since I'd check for $course->timemodified <= $timenotmodifsincedays before doing the SQL query as it won't always be required. I think it's safer to leave it this way because I realized over time that $course->timemodified is not always modified when course is updated... Checking both makes the skipped condition safer IMO. There are a couple of white space issues (space missing before if and the brackets, empty line with spaces, ...), the comments you have introduced should end with a dot. Thx for the link! I've tried my best to follow the coding style. Please tell me if I missed something. And to make everything perfect, we could use the component backup in the commit message I've put "backup" in the commit message after the MDL-xxxxx. Is it what you meant? Feel free to give me some new feedbacks and/or argue about the $course->timemodified and log condition Thx again!
          Hide
          Frédéric Massart added a comment -

          Hi Jean-Philippe,

          that's great! Thanks for this.

          I thought of a few other comments but thought that it would be easier to patch them instead of going back and forth again. Mainly, I have simplified the code so that the different skipping checks are not encapsulated within each other. I have also removed the DB->update call for the first skip to use the one at the end only. Fixed a few things that were not related to your patch at all but as we were at it... and finally, I thought that checking that the last backup was successful when skipping after x days was a good idea as well.

          Please have a look at it and pull/fix it into your branch so that we can push that for integration later on.

          https://github.com/FMCorz/moodle/commit/1965e5ea4b1379d5e6f1c341545864196e01b21a
          https://github.com/FMCorz/moodle/compare/MDL-32148-master-test

          Show
          Frédéric Massart added a comment - Hi Jean-Philippe, that's great! Thanks for this. I thought of a few other comments but thought that it would be easier to patch them instead of going back and forth again. Mainly, I have simplified the code so that the different skipping checks are not encapsulated within each other. I have also removed the DB->update call for the first skip to use the one at the end only. Fixed a few things that were not related to your patch at all but as we were at it... and finally, I thought that checking that the last backup was successful when skipping after x days was a good idea as well. Please have a look at it and pull/fix it into your branch so that we can push that for integration later on. https://github.com/FMCorz/moodle/commit/1965e5ea4b1379d5e6f1c341545864196e01b21a https://github.com/FMCorz/moodle/compare/MDL-32148-master-test
          Hide
          Jean-Philippe Gaudreau added a comment -

          Hi Frédéric,

          Sorry for the delay I was out of office. I've merged your fixes to my current branch.

          Putting back in peer review! Thx!

          Show
          Jean-Philippe Gaudreau added a comment - Hi Frédéric, Sorry for the delay I was out of office. I've merged your fixes to my current branch. Putting back in peer review! Thx!
          Hide
          Frédéric Massart added a comment -

          Hi Jean-Philippe,

          I like your the current patch and I think it is ready to go for integration, however I'd like your opinion on the following.

          Assuming we have a course with laststatus set to SKIPPED, and the baackup of this course will be skipped because it has not been updated since x days. Should we check in that case that the laststarttime is valid to ensure that we are not skipping a course that was skipped but not backed up? I was thinking that we could add that condition in the $lastbackupwassuccessful variable.

          Let me know what you think of it, if you agree with that please update your branch and I will push that for integration.

          Merci beaucoup!

          Show
          Frédéric Massart added a comment - Hi Jean-Philippe, I like your the current patch and I think it is ready to go for integration, however I'd like your opinion on the following. Assuming we have a course with laststatus set to SKIPPED, and the baackup of this course will be skipped because it has not been updated since x days. Should we check in that case that the laststarttime is valid to ensure that we are not skipping a course that was skipped but not backed up? I was thinking that we could add that condition in the $lastbackupwassuccessful variable. Let me know what you think of it, if you agree with that please update your branch and I will push that for integration. Merci beaucoup!
          Hide
          Jean-Philippe Gaudreau added a comment -

          Hi Frédéric,

          I'm not sure I understand what you mean with the validation of laststarttime. The way I see it, laststarttime and lastendtime are only related to the backup being done (BACKUP_STATUS_OK).

          Can you provide me with a more detailed case of how this could create a problem?

          Merci à toi!

          Show
          Jean-Philippe Gaudreau added a comment - Hi Frédéric, I'm not sure I understand what you mean with the validation of laststarttime. The way I see it, laststarttime and lastendtime are only related to the backup being done (BACKUP_STATUS_OK). Can you provide me with a more detailed case of how this could create a problem? Merci à toi!
          Hide
          Frédéric Massart added a comment -

          Hi Jean-Philippe,

          Well, I am not entirely sure when this problem could occur, but I just thought that it might be wise to check that there was a previous backup done before skipping it. It's unlikely to happen considering the current logic, but let's assume this:

          • A course named 'Abc' has been skipped because the backup is not yet required
            • laststarttime = 0
            • laststatus = SKIPPED
          • A course named '123' is hidden since its creation
            • laststarttime = 0
            • laststatus = SKIPPED

          For some reason, the 'Abc' and '123' have not changed since the last backup (or in the last x days). Then their backup will be skipped even though we do not currently hold any backup of them, that would be an unexpected behavior. I think if we check for the last status, we should also ensure that the laststarttime AND lastendtime is not 0. That is just a security to make sure we don't skip what we are not supposed to skip.

                          // The last backup is considered as successful when OK or SKIPPED.
                          $lastbackupwassuccessful =  ($backupcourse->laststatus == self::BACKUP_STATUS_SKIPPED ||
                                                      $backupcourse->laststatus == self::BACKUP_STATUS_OK) && (
                                                      $backupcourse->laststarttime > 0 && $backupcourse->lastendtime > 0);
          

          Does this make sense to you?

          That makes me think that this whole skipping logic should not rely on the information in the database but on the real information contained in the backups, maybe a future improvement.

          Show
          Frédéric Massart added a comment - Hi Jean-Philippe, Well, I am not entirely sure when this problem could occur, but I just thought that it might be wise to check that there was a previous backup done before skipping it. It's unlikely to happen considering the current logic, but let's assume this: A course named 'Abc' has been skipped because the backup is not yet required laststarttime = 0 laststatus = SKIPPED A course named '123' is hidden since its creation laststarttime = 0 laststatus = SKIPPED For some reason, the 'Abc' and '123' have not changed since the last backup (or in the last x days). Then their backup will be skipped even though we do not currently hold any backup of them, that would be an unexpected behavior. I think if we check for the last status, we should also ensure that the laststarttime AND lastendtime is not 0. That is just a security to make sure we don't skip what we are not supposed to skip. // The last backup is considered as successful when OK or SKIPPED. $lastbackupwassuccessful = ($backupcourse->laststatus == self::BACKUP_STATUS_SKIPPED || $backupcourse->laststatus == self::BACKUP_STATUS_OK) && ( $backupcourse->laststarttime > 0 && $backupcourse->lastendtime > 0); Does this make sense to you? That makes me think that this whole skipping logic should not rely on the information in the database but on the real information contained in the backups, maybe a future improvement.
          Hide
          Jean-Philippe Gaudreau added a comment -

          Hi Frédéric,

          Yes I think you are right, this situation could happen and it would be safer to add those conditions. Just updated the branch with the code you provided!

          Thx for the explanations!

          Show
          Jean-Philippe Gaudreau added a comment - Hi Frédéric, Yes I think you are right, this situation could happen and it would be safer to add those conditions. Just updated the branch with the code you provided! Thx for the explanations!
          Hide
          Frédéric Massart added a comment -

          Thanks Jean-Philippe, I think we can now push this for integration.

          Massive thanks for your work, especially with all those back and forth, I know how annoying I can be sometime !

          Cheers!

          Show
          Frédéric Massart added a comment - Thanks Jean-Philippe, I think we can now push this for integration. Massive thanks for your work, especially with all those back and forth, I know how annoying I can be sometime ! Cheers!
          Hide
          Sam Hemelryk added a comment -

          Congratulations! this has now been integrated

          Show
          Sam Hemelryk added a comment - Congratulations! this has now been integrated
          Hide
          Michael de Raadt added a comment -

          Test result: Success!

          Tested in master only.

          I noted that the detection of course modification relies on a course view entry in the log table. This does not capture all possible activities that could be considered as modifications to a course and I will create a new issue to explore that.

          Show
          Michael de Raadt added a comment - Test result: Success! Tested in master only. I noted that the detection of course modification relies on a course view entry in the log table. This does not capture all possible activities that could be considered as modifications to a course and I will create a new issue to explore that.
          Hide
          Aparup Banerjee added a comment -

          Your issue has dug up some gold.
          It works great i've been told.
          Go forth, be brave, be bold.

          yay! "All your thoughts are belong to everyone."

          Thanks and ciao!

          Show
          Aparup Banerjee added a comment - Your issue has dug up some gold. It works great i've been told. Go forth, be brave, be bold. yay! "All your thoughts are belong to everyone." Thanks and ciao!
          Hide
          Mary Cooch added a comment -

          Removing docs_required label as this is now documented here http://docs.moodle.org/24/en/Automated_course_backup

          Show
          Mary Cooch added a comment - Removing docs_required label as this is now documented here http://docs.moodle.org/24/en/Automated_course_backup

            People

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

              Dates

              • Created:
                Updated:
                Resolved: