Moodle
  1. Moodle
  2. MDL-43794

Temp dir cleaned before restore finished with new backup format

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.6.1
    • Fix Version/s: 2.6.2
    • Component/s: Backup, Files API
    • Labels:

      Description

      When using the new .tar.gz backup format, the extracted files retain their original creation time. This means that when restoring a backup older than a week, cron wipes out all the files.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Eric Merrill added a comment - - edited

            The 'flaw' happens in lib/filestorage/tgz_extractor.php, line 278:

            $mtime = octdec(substr($block, 136, 11));

            By far the easiest fix is to change the line to:

            $mtime = time();

            In our testing, that does seem to fix the problem.

            It appears that the old zip system does not maintain file times, so the question becomes, should tgz? If yes, then either the cleanup routine needs to change, or we need to exclude restore extractions from the filetime preservation.

            I'm happy to make the patch, just need to know what people (particularly Sam) think.

            Show
            Eric Merrill added a comment - - edited The 'flaw' happens in lib/filestorage/tgz_extractor.php, line 278: $mtime = octdec(substr($block, 136, 11)); By far the easiest fix is to change the line to: $mtime = time(); In our testing, that does seem to fix the problem. It appears that the old zip system does not maintain file times, so the question becomes, should tgz? If yes, then either the cleanup routine needs to change, or we need to exclude restore extractions from the filetime preservation. I'm happy to make the patch, just need to know what people (particularly Sam) think.
            Hide
            Mark Nelson added a comment -

            Thanks for the report Eric.

            Could you create a patch that I could review and push for integration? That way you get the credit for the fix.

            Show
            Mark Nelson added a comment - Thanks for the report Eric. Could you create a patch that I could review and push for integration? That way you get the credit for the fix.
            Hide
            CiBoT added a comment -

            Results for MDL-43794

            • Remote repository: git@github.com:merrill-oakland/moodle.git
            Show
            CiBoT added a comment - Results for MDL-43794 Remote repository: git@github.com:merrill-oakland/moodle.git Remote branch MDL-43794 _m26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/713 Error: Unable to fetch information from MDL-43794 _m26 branch at git@github.com:merrill-oakland/moodle.git. Remote branch MDL-43794 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/714 Error: Unable to fetch information from MDL-43794 branch at git@github.com:merrill-oakland/moodle.git.
            Show
            CiBoT added a comment - Results for MDL-43794 Remote repository: https://github.com/merrill-oakland/moodle.git Remote branch MDL-43794 _m26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/715 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/715/artifact/work/smurf.html Remote branch MDL-43794 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/716 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/716/artifact/work/smurf.html
            Hide
            Eric Merrill added a comment -

            I believe this is ready for review.

            Show
            Eric Merrill added a comment - I believe this is ready for review.
            Show
            CiBoT added a comment - Results for MDL-43794 Remote repository: https://github.com/merrill-oakland/moodle.git Remote branch MDL-43794 _m26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/717 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/717/artifact/work/smurf.html Remote branch MDL-43794 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/718 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/718/artifact/work/smurf.html
            Hide
            Sam Marshall added a comment -

            I agree reluctantly with the general approach - I don't like it, as really I think it's a bug that zip/tgz does not preserve modified time when extracting, but there seems to be no sensible way to keep this behaviour given how cron works to delete old files - but would it be possible to put the code in a different location?

            I'd suggest that we should KEEP the mtime when making a list of files and the only place it should not be used is when actually writing out files. In other words, if you just list the archive, that should include mtimes still - no reason to lose that functionality.

            So instead of your change, could be this one:

            tgz_extractor line 459:

            // Update modified time.
            touch($this->currentfile, $this->currentmtime);

            Replace with:

            // At this point we should set the file modified time to
            // $this->currentmtime. However, when extracting to the temp
            // directory, cron will delete files more than a week old, so
            // to avoid problems we leave all files set to current time.

            That might make a difference to the unit test changes (hopefully there can be fewer of those).

            What do you think?

            Show
            Sam Marshall added a comment - I agree reluctantly with the general approach - I don't like it, as really I think it's a bug that zip/tgz does not preserve modified time when extracting, but there seems to be no sensible way to keep this behaviour given how cron works to delete old files - but would it be possible to put the code in a different location? I'd suggest that we should KEEP the mtime when making a list of files and the only place it should not be used is when actually writing out files. In other words, if you just list the archive, that should include mtimes still - no reason to lose that functionality. So instead of your change, could be this one: tgz_extractor line 459: // Update modified time. touch($this->currentfile, $this->currentmtime); Replace with: // At this point we should set the file modified time to // $this->currentmtime. However, when extracting to the temp // directory, cron will delete files more than a week old, so // to avoid problems we leave all files set to current time. That might make a difference to the unit test changes (hopefully there can be fewer of those). What do you think?
            Hide
            Eric Merrill added a comment -

            I modified your comment a little bit, but should be all set.

            Show
            Eric Merrill added a comment - I modified your comment a little bit, but should be all set.
            Hide
            Sam Marshall added a comment -

            +1 to submit for integration.

            Trivia: new comment wording is good but you might like to correct punctuation, should be 'its' (no apostrophe). Don't think this would block integration though if you can't be bothered.

            Show
            Sam Marshall added a comment - +1 to submit for integration. Trivia: new comment wording is good but you might like to correct punctuation, should be 'its' (no apostrophe). Don't think this would block integration though if you can't be bothered.
            Hide
            Eric Merrill added a comment -

            I can be bothered in fact. Fixed.

            Thanks!

            Show
            Eric Merrill added a comment - I can be bothered in fact. Fixed. Thanks!
            Hide
            Mark Nelson added a comment -

            Thanks guys, submitting to integration.

            Show
            Mark Nelson added a comment - Thanks guys, submitting to integration.
            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
            CiBoT added a comment -

            Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

            Show
            CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            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
            Andrew Davis added a comment -

            Works as described. Passing.

            Show
            Andrew Davis added a comment - Works as described. Passing.
            Hide
            Marina Glancy added a comment -

            Thanks for your hard work. Your code has now become a part of Moodle!

            Show
            Marina Glancy added a comment - Thanks for your hard work. Your code has now become a part of Moodle!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: