Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-33531

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

    Details

    • Type: Bug
    • Status: Closed
    • Priority: 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

      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.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            salvetore 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
            salvetore 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
            mazzar 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
            mazzar 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
            salvetore Michael de Raadt added a comment -

            Thanks for the additional information.

            Show
            salvetore Michael de Raadt added a comment - Thanks for the additional information.
            Hide
            nicus 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
            nicus 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
            nicus 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
            nicus 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
            nicus Lorenzo Nicora added a comment -

            Updated the issue name to describe the issue more correctly

            Show
            nicus Lorenzo Nicora added a comment - Updated the issue name to describe the issue more correctly
            Hide
            nicus 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
            nicus 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
            chadsta 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
            chadsta 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
            dmonllao 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
            dmonllao 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_frenz Ankit Agarwal added a comment -

            Looks good David.
            +1 for integration.
            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - Looks good David. +1 for integration. Thanks
            Hide
            poltawski 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
            poltawski 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
            samhemelryk 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
            samhemelryk 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 CiBoT added a comment -

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

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            dmonllao 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
            dmonllao 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_frenz Ankit Agarwal added a comment -

            This looks good David.
            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - This looks good David. Thanks
            Hide
            stronk7 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
            stronk7 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
            dmonllao David Monllaó added a comment -

            Pull branches rebased

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

            Thanks guys this has been integrated now

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

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

            Show
            abgreeve Adrian Greeve added a comment - YES! It works. Tested pre and post patch. Thanks,
            Hide
            stronk7 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
            stronk7 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
            stronk7 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
            stronk7 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:
                  Fix Release Date:
                  10/Sep/12