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

issue deleting temp dirs in backup

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 1.8.3, 1.9
    • Fix Version/s: 1.9.5, 2.0
    • Component/s: Backup
    • Labels:
      None
    • Affected Branches:
      MOODLE_18_STABLE, MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE

      Description

      Hi Eloy,

      this is related to the user post here:
      http://moodle.org/mod/forum/discuss.php?d=95768#p426822

      it seems to happen in backup_execute() in backuplib.php

      obviously for some reason, backup_delete_old_data() is failing and not deleting the directories inside directories completely - I haven't looked closely - but is this a bug in backup_delete_old_dirs() ? - the user is using an older version of moodle so it's potentially fixed.....

      I suggest we change the call in backup_execute:
      if (!defined('BACKUP_SILENTLY'))

      { error ("An error occurred deleting old backup data"); }

      ... to use a debugging call instead of the error() function.. - the fact that it hasn't deleted all the old dirs in temp doesn't actually affect the backup itself, which should succeed fine, and simply displaying a warning should suffice IMO

      thanks,

      Dan

        Gliffy Diagrams

          Attachments

            Activity

            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Well spotted, 100% agree, Dan...

            that shouldn't be an stop-error. debugging sounds ok here... although perhaps I also will notify() the user about that. Note that the BACKUP_SILENTLY part (some lines below...) is also returning false and perhaps should allow to continue. And same could be happening in scheduled_backups and restore (both use that function).

            Can you handle this? TIA!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Well spotted, 100% agree, Dan... that shouldn't be an stop-error. debugging sounds ok here... although perhaps I also will notify() the user about that. Note that the BACKUP_SILENTLY part (some lines below...) is also returning false and perhaps should allow to continue. And same could be happening in scheduled_backups and restore (both use that function). Can you handle this? TIA!
            Hide
            danmarsden Dan Marsden added a comment -

            will do

            thanks,

            Dan

            Show
            danmarsden Dan Marsden added a comment - will do thanks, Dan
            Hide
            danmarsden Dan Marsden added a comment -

            now in 1.9stable and HEAD - not sure about 1.8stable though.... that function is new in 1.9 and I can't seem to find the string mentioned in the forum posting anywhere in 1.8 .... is this an issue in 1.8stable?

            Dan

            Show
            danmarsden Dan Marsden added a comment - now in 1.9stable and HEAD - not sure about 1.8stable though.... that function is new in 1.9 and I can't seem to find the string mentioned in the forum posting anywhere in 1.8 .... is this an issue in 1.8stable? Dan
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Hi Dan,

            if I'm not wrong... under 1.8 that is executed by backup_execute.html (in 1.9 it was repackaged to the backup_execute() function you've changed).

            Also... backup_scheduled.php and restorelib.php seem to be using that function with similar behaviour: stop the process.

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Hi Dan, if I'm not wrong... under 1.8 that is executed by backup_execute.html (in 1.9 it was repackaged to the backup_execute() function you've changed). Also... backup_scheduled.php and restorelib.php seem to be using that function with similar behaviour: stop the process. Ciao
            Hide
            danmarsden Dan Marsden added a comment -

            thanks for the details about 1.8 - will look at patching that too sometime soon (I thought those other files did something similar too, but was playing with cvs a bit and wanted to test some small commits!)

            ...looking at the way those errors are presented - the silent backup errors don't go anywhere! - I'm keen to insert them into the backup_logs table, and can't see much in scheduled backups that relys on the data in backup_logs except for this call to see if it hasn't completed - which we might be able to alter slightly....

            $numofrec = count_records_select ("backup_log","time > $timeafter")

            In fact I'm thinking that we could add a new field to backup_log that defines what process/function wrote to the log eg (manual backup, Scheduled backup, Incremental backup, Silient backup - or individual module backup scripts?)

            does that sound ok?

            Dan

            Show
            danmarsden Dan Marsden added a comment - thanks for the details about 1.8 - will look at patching that too sometime soon (I thought those other files did something similar too, but was playing with cvs a bit and wanted to test some small commits!) ...looking at the way those errors are presented - the silent backup errors don't go anywhere! - I'm keen to insert them into the backup_logs table, and can't see much in scheduled backups that relys on the data in backup_logs except for this call to see if it hasn't completed - which we might be able to alter slightly.... $numofrec = count_records_select ("backup_log","time > $timeafter") In fact I'm thinking that we could add a new field to backup_log that defines what process/function wrote to the log eg (manual backup, Scheduled backup, Incremental backup, Silient backup - or individual module backup scripts?) does that sound ok? Dan
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment - - edited

            Ah, yes! I read this yesterday and forgot to comment.

            Sounds ok for me (HEAD only) + potential mini add_to_backup_log() function to control all them together.

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - - edited Ah, yes! I read this yesterday and forgot to comment. Sounds ok for me (HEAD only) + potential mini add_to_backup_log() function to control all them together. Ciao
            Hide
            danmarsden Dan Marsden added a comment -

            yes - I was thinking Head only - (can't make db changes to Stable branches)

            Dan

            Show
            danmarsden Dan Marsden added a comment - yes - I was thinking Head only - (can't make db changes to Stable branches) Dan
            Hide
            danmarsden Dan Marsden added a comment -

            attaching patches for restorelib and backup_scheduled.php - we really need somewhere to store all those $errorstr when calling restore_silently, backup_silently!

            Dan

            Show
            danmarsden Dan Marsden added a comment - attaching patches for restorelib and backup_scheduled.php - we really need somewhere to store all those $errorstr when calling restore_silently, backup_silently! Dan
            Hide
            danmarsden Dan Marsden added a comment -

            fix now in HEAD

            Show
            danmarsden Dan Marsden added a comment - fix now in HEAD
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Adding 2.0 as fixfor version (partially) and closing... thanks Dan!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Adding 2.0 as fixfor version (partially) and closing... thanks Dan!

              People

              • Votes:
                0 Vote for this issue
                Watchers:
                3 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  13/May/09