Moodle
  1. Moodle
  2. MDL-14736

issue deleting temp dirs in backup

    Details

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

      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

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

        Activity

        Hide
        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
        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
        Dan Marsden added a comment -

        will do

        thanks,

        Dan

        Show
        Dan Marsden added a comment - will do thanks, Dan
        Hide
        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
        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
        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
        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
        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
        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
        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
        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
        Dan Marsden added a comment -

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

        Dan

        Show
        Dan Marsden added a comment - yes - I was thinking Head only - (can't make db changes to Stable branches) Dan
        Hide
        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
        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
        Dan Marsden added a comment -

        fix now in HEAD

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

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

        Show
        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: