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

        1. backup_scheduled.txt
          1 kB
          Dan Marsden
        2. restorelib.txt
          2 kB
          Dan Marsden

          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