Moodle
  1. Moodle
  2. MDL-33531

Courses automated backup does not delete old backups from filesystem when backup_shortname is set

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.2.3, 2.3
    • Fix Version/s: 2.2.5, 2.3.2
    • Component/s: Backup
    • Labels:
    • Database:
      Any
    • Testing Instructions:
      Hide

      These testing instructions includes the execution of automated backups, so less courses = faster automated backup

      1.- Enable automated backups with this setting:
      .backup_auto_active to 'manual'
      .backup_auto_storage to 'Specified directory for automated backups'
      .set backup_auto_destination wherever you want
      .backup_auto_keep to '1'
      .check backup_shortname
      2.- Execute "sudo -u www-data /usr/bin/php admin/cli/automated_backups.php" from the command line
      2.- Execute again "sudo -u www-data /usr/bin/php admin/cli/automated_backups.php" from the command line
      3.- Check backup_auto_directory folder, there SHOULD be just one backup file per course

      Show
      These testing instructions includes the execution of automated backups, so less courses = faster automated backup 1.- Enable automated backups with this setting: .backup_auto_active to 'manual' .backup_auto_storage to 'Specified directory for automated backups' .set backup_auto_destination wherever you want .backup_auto_keep to '1' .check backup_shortname 2.- Execute "sudo -u www-data /usr/bin/php admin/cli/automated_backups.php" from the command line 2.- Execute again "sudo -u www-data /usr/bin/php admin/cli/automated_backups.php" from the command line 3.- Check backup_auto_directory folder, there SHOULD be just one backup file per course
    • Workaround:
      Hide

      New if() to add the id or the shortname to the regexp, the same process backup_plan_dbops::get_default_backup_filename() does, by Lorenzo Nicora. Another way to get the regexp could be calling backup_plan_dbops::get_default_backup_filename() and hacking it's return to remove the last -*, but IMO this alternative is more tricky

      Show
      New if() to add the id or the shortname to the regexp, the same process backup_plan_dbops::get_default_backup_filename() does, by Lorenzo Nicora. Another way to get the regexp could be calling backup_plan_dbops::get_default_backup_filename() and hacking it's return to remove the last -*, but IMO this alternative is more tricky
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-33531_master
    • Rank:
      41454

      Description

      In my moodle, automated backups are set as follow:

      • Storage: specified directory of local filesystem
      • backup_auto_keep: 1
      • Course names are used in backup filename
      • Backups include everything, except logs and histories
      • Backup logs lifetime: 7 days

      After installed version 2.2.3, and set to use course names in filename and backuplog lifetime, old backups files are not deleted from filesystem, although keep is 1, old backups stay there forever.

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Hi, Riccardo.

          Could you explain what you mean by installing 2.2.3? Do you mean you upgraded or was this a new install? Did the problem present itself after an upgrade to 2.2.3? Are recently created backups being deleted after a new backup is made?

          Are you completing backups through cron or CLI? In other words, what is your setting for backup_auto_active?

          Also, what is your setting for backup_auto_storage and could this have changed recently?

          Show
          Michael de Raadt added a comment - Hi, Riccardo. Could you explain what you mean by installing 2.2.3? Do you mean you upgraded or was this a new install? Did the problem present itself after an upgrade to 2.2.3? Are recently created backups being deleted after a new backup is made? Are you completing backups through cron or CLI? In other words, what is your setting for backup_auto_active? Also, what is your setting for backup_auto_storage and could this have changed recently?
          Hide
          Riccardo Mazza added a comment -

          Hi Michael,
          the issue started as soon as I upgraded to last MOODLE_22_STABLE about a week ago. Recently created backups are NOT deleted after a new backup is made. Each day I have to delete previous backup files via unix shell rm command. We never had this problem until version 2.2.2+ of Moodle. I have this issue in several servers running multiple Moodle installations.

          backup_auto_active is set to "Enabled", cron is executed via CLI

          backup_auto_storage is set to "specified directory for automatic backup", we have not changed that since the initial Moodle 2.1 installation we made 6 months ago.

          Doesn't seems to be a permissions problem, since owner of backups files is the user that runs the cron via CLI.

          Reports -> backup doesn't report any issue.

          Thanks for your support.

          Riccardo

          Show
          Riccardo Mazza added a comment - Hi Michael, the issue started as soon as I upgraded to last MOODLE_22_STABLE about a week ago. Recently created backups are NOT deleted after a new backup is made. Each day I have to delete previous backup files via unix shell rm command. We never had this problem until version 2.2.2+ of Moodle. I have this issue in several servers running multiple Moodle installations. backup_auto_active is set to "Enabled", cron is executed via CLI backup_auto_storage is set to "specified directory for automatic backup", we have not changed that since the initial Moodle 2.1 installation we made 6 months ago. Doesn't seems to be a permissions problem, since owner of backups files is the user that runs the cron via CLI. Reports -> backup doesn't report any issue. Thanks for your support. Riccardo
          Hide
          Michael de Raadt added a comment -

          Thanks for the additional information.

          Show
          Michael de Raadt added a comment - Thanks for the additional information.
          Hide
          Lorenzo Nicora added a comment -

          I'm experiencing the same issue on 2.2.3+ (build: 20120601)
          My configuration is similar, except that I run backup by cron (cron run by CLI) with backup_auto_weekdays= 2 days a week.

          I also manually applied the released patch for #MDL-30725 but nothing changes.

          Show
          Lorenzo Nicora added a comment - I'm experiencing the same issue on 2.2.3+ (build: 20120601) My configuration is similar, except that I run backup by cron (cron run by CLI) with backup_auto_weekdays= 2 days a week. I also manually applied the released patch for # MDL-30725 but nothing changes.
          Hide
          Lorenzo Nicora added a comment -

          I think I found the problem.

          The bug occurs with automated backups when hese options are set:

          • backup_auto_storage = 2 (external dir)
          • backup_shortname = true
          • backup_auto_keep > 0

          In backup/util/helper/backup_cron_helper.class.php at lines 504, the regexp used to find old backup files always uses $course->id regardless of backup_shortname setting.
          (I'm looking both at MOODLE_22_STABLE and master: these lines are the same in both)

          Here is a quick fix:

          in backup/util/helper/backup_cron_helper.class.php
          replace line (504), currently

          $filename = $backupword . '-' . backup::FORMAT_MOODLE . '-' . backup::TYPE_1COURSE . '-' .$course->id . '-';
          

          ...with these:

          // Fix for MDL-33531
          if ( $config->backup_shortname ) {
          	$courseref = $course->shortname;
          	$courseref = str_replace(' ', '_', $courseref);
          	$courseref = moodle_strtolower(trim(clean_filename($courseref), '_'));
          } else {
          	$courseref = $course->id;
          }
          $filename = $backupword . '-' . backup::FORMAT_MOODLE . '-' . backup::TYPE_1COURSE . '-' . $courseref . '-';
          

          I think this is not very elegant as it duplicates code from get_default_backup_filename(...) in backup_plan_dbops.class.php for building filename, but I found no cleaner solution.

          Show
          Lorenzo Nicora added a comment - I think I found the problem. The bug occurs with automated backups when hese options are set: backup_auto_storage = 2 (external dir) backup_shortname = true backup_auto_keep > 0 In backup/util/helper/backup_cron_helper.class.php at lines 504, the regexp used to find old backup files always uses $course->id regardless of backup_shortname setting. (I'm looking both at MOODLE_22_STABLE and master: these lines are the same in both) Here is a quick fix: in backup/util/helper/backup_cron_helper.class.php replace line (504), currently $filename = $backupword . '-' . backup::FORMAT_MOODLE . '-' . backup::TYPE_1COURSE . '-' .$course->id . '-'; ...with these: // Fix for MDL-33531 if ( $config->backup_shortname ) { $courseref = $course->shortname; $courseref = str_replace(' ', '_', $courseref); $courseref = moodle_strtolower(trim(clean_filename($courseref), '_')); } else { $courseref = $course->id; } $filename = $backupword . '-' . backup::FORMAT_MOODLE . '-' . backup::TYPE_1COURSE . '-' . $courseref . '-'; I think this is not very elegant as it duplicates code from get_default_backup_filename(...) in backup_plan_dbops.class.php for building filename, but I found no cleaner solution.
          Hide
          Lorenzo Nicora added a comment -

          Updated the issue name to describe the issue more correctly

          Show
          Lorenzo Nicora added a comment - Updated the issue name to describe the issue more correctly
          Hide
          Lorenzo Nicora added a comment -

          Added link fixes on github, either for master and 2.2 branches (there is a little difference in fixes)

          I hope I did it correctly

          Show
          Lorenzo Nicora added a comment - Added link fixes on github, either for master and 2.2 branches (there is a little difference in fixes) I hope I did it correctly
          Hide
          Chad Outten added a comment -

          i can confirm same behaviour - old backups not deleted. clean 2.3.2 install. specified directory for automated backups and backup_shortname set to yes.

          Show
          Chad Outten added a comment - i can confirm same behaviour - old backups not deleted. clean 2.3.2 install. specified directory for automated backups and backup_shortname set to yes.
          Hide
          David Monllaó added a comment -

          Thanks for the report Riccardo and for the patch Lorenzo.

          Adding testing instructions, updating pull branches (commits marked as coming from nicusX) and changing priority as there are no crashes, loss of data or severe memory leaks.

          Assigning Ankit as a peer reviewer

          Show
          David Monllaó added a comment - Thanks for the report Riccardo and for the patch Lorenzo. Adding testing instructions, updating pull branches (commits marked as coming from nicusX) and changing priority as there are no crashes, loss of data or severe memory leaks. Assigning Ankit as a peer reviewer
          Hide
          Ankit Agarwal added a comment -

          Looks good David.
          +1 for integration.
          Thanks

          Show
          Ankit Agarwal added a comment - Looks good David. +1 for integration. Thanks
          Hide
          Dan Poltawski added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          I'm sending this back this week sorry.
          The changes look 99% spot on, however there is one gotcha in there still.

          https://github.com/dmonllao/moodle/blob/62396592bcb91543452b74c4b9e9110ffa96784f/backup/util/helper/backup_cron_helper.class.php#L513

          Its important that $course->shortname gets passed through format_string like happens in get_default_backup_filename.

          https://github.com/dmonllao/moodle/blob/62396592bcb91543452b74c4b9e9110ffa96784f/backup/util/dbops/backup_plan_dbops.class.php#L218

          The is particularly important in ensuring this changes works with courses where things like the multilang filter have been used for the shortname.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, I'm sending this back this week sorry. The changes look 99% spot on, however there is one gotcha in there still. https://github.com/dmonllao/moodle/blob/62396592bcb91543452b74c4b9e9110ffa96784f/backup/util/helper/backup_cron_helper.class.php#L513 Its important that $course->shortname gets passed through format_string like happens in get_default_backup_filename. https://github.com/dmonllao/moodle/blob/62396592bcb91543452b74c4b9e9110ffa96784f/backup/util/dbops/backup_plan_dbops.class.php#L218 The is particularly important in ensuring this changes works with courses where things like the multilang filter have been used for the shortname. Cheers Sam
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          David Monllaó added a comment -

          Sorry Sam, applied the same behavior of get_default_backup_filename(). Repository updated and rebased against upstream.

          Requesting peer review

          Show
          David Monllaó added a comment - Sorry Sam, applied the same behavior of get_default_backup_filename(). Repository updated and rebased against upstream. Requesting peer review
          Hide
          Ankit Agarwal added a comment -

          This looks good David.
          Thanks

          Show
          Ankit Agarwal added a comment - This looks good David. Thanks
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          David Monllaó added a comment -

          Pull branches rebased

          Show
          David Monllaó added a comment - Pull branches rebased
          Hide
          Sam Hemelryk added a comment -

          Thanks guys this has been integrated now

          Show
          Sam Hemelryk added a comment - Thanks guys this has been integrated now
          Hide
          Adrian Greeve added a comment -

          YES! It works.
          Tested pre and post patch.
          Thanks,

          Show
          Adrian Greeve added a comment - YES! It works. Tested pre and post patch. Thanks,
          Hide
          Eloy Lafuente (stronk7) added a comment -

          For the good and the bad... this is now part of Moodle and people around the world will start using it immediately, what a responsibility!

          Many thanks for your collaboration, yay!

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - For the good and the bad... this is now part of Moodle and people around the world will start using it immediately, what a responsibility! Many thanks for your collaboration, yay! Closing, ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Note: this is being reverted by MDL-33812, where we are going to start using "id+shortname" as part of the file name, so "id" will be always there, so no potential conflicts can happen (imagine one shortname being a number, coinciding with some id or so).

          Show
          Eloy Lafuente (stronk7) added a comment - Note: this is being reverted by MDL-33812 , where we are going to start using "id+shortname" as part of the file name, so "id" will be always there, so no potential conflicts can happen (imagine one shortname being a number, coinciding with some id or so).

            People

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

              Dates

              • Created:
                Updated:
                Resolved: