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

Automated backups with external storage fill trashdir

    Details

      Description

      The current backup code copies the temporary zip file to the file pool, and depending on external storage settings will then copy to this location and then potentially delete the file pool copy.

      Deleting the filepool backup copy means this is sent into trashdir, which on large sites where many backups run at once can cause a huge amount of additional disk usage before the trashdir is cleared. Additionally, the additional disk operations cause the whole backup process to take much longer than it should as there is twice the work involved.

      This should be changed, so that in the case where automated course backups should only be put in an external storage location they are put there directly instead of first running past the filepool and fillingtrashdir.

      Somewhat ugly patch for this incoming - there might be a better way to handle this?

        Gliffy Diagrams

          Issue Links

            Activity

            Show
            tlevi Tony Levi added a comment - https://github.com/tlevi/moodle/tree/mdl30725
            Hide
            salvetore Michael de Raadt added a comment -

            Thanks for reporting that and proposing a solution, Tony.

            Show
            salvetore Michael de Raadt added a comment - Thanks for reporting that and proposing a solution, Tony.
            Hide
            salvetore Michael de Raadt added a comment -

            I'm bumping this issue as it has now been duplicated.

            Show
            salvetore Michael de Raadt added a comment - I'm bumping this issue as it has now been duplicated.
            Hide
            tlevi Tony Levi added a comment -

            This change continues to operate successfully for us on multiple production environments.

            Show
            tlevi Tony Levi added a comment - This change continues to operate successfully for us on multiple production environments.
            Hide
            tsala Helen Foster added a comment -

            Just noting a discussion about this issue http://moodle.org/mod/forum/discuss.php?d=191275

            I've also made a note of it in the docs http://docs.moodle.org/22/en/Automated_course_backup

            Show
            tsala Helen Foster added a comment - Just noting a discussion about this issue http://moodle.org/mod/forum/discuss.php?d=191275 I've also made a note of it in the docs http://docs.moodle.org/22/en/Automated_course_backup
            Hide
            tlevi Tony Levi added a comment -

            Jesus this hasn't bean integrated yet ?!?

            We've by now forgotten about this problem thanks to this fix.

            Can somebody accelerate this, its just depressing; why bother filing bugs and patches if /critical/ disk consuming problems won't be fixed?!

            Show
            tlevi Tony Levi added a comment - Jesus this hasn't bean integrated yet ?!? We've by now forgotten about this problem thanks to this fix. Can somebody accelerate this, its just depressing; why bother filing bugs and patches if /critical/ disk consuming problems won't be fixed?!
            Hide
            poltawski Dan Poltawski added a comment -

            Hi Tony,

            Thanks for the patch and sorry that it has been missed until now. Please feel free to make some noise about it!

            I'm not really familiar with this area, so sending this for peer review.

            Show
            poltawski Dan Poltawski added a comment - Hi Tony, Thanks for the patch and sorry that it has been missed until now. Please feel free to make some noise about it! I'm not really familiar with this area, so sending this for peer review.
            Hide
            poltawski Dan Poltawski added a comment -

            Apu - you are more familiar with backup than me. Could you cast you eyes on this? thanks

            Show
            poltawski Dan Poltawski added a comment - Apu - you are more familiar with backup than me. Could you cast you eyes on this? thanks
            Hide
            mindyk Melinda Kraft added a comment -

            I would just like to cast my vote to fix this. When we turn on site wide backups I have to babysit our rather large installation to delete files out of the trashdir because we keep running out of space. I can't see a reason why trashed temp files should be held on the disk for later deletion by the cron job. It seems to be any benefit gained by the new file mgt system is negated by the exploding hard drive space caused by sitewide backups.

            Show
            mindyk Melinda Kraft added a comment - I would just like to cast my vote to fix this. When we turn on site wide backups I have to babysit our rather large installation to delete files out of the trashdir because we keep running out of space. I can't see a reason why trashed temp files should be held on the disk for later deletion by the cron job. It seems to be any benefit gained by the new file mgt system is negated by the exploding hard drive space caused by sitewide backups.
            Hide
            nebgor Aparup Banerjee added a comment -

            Hi, i've had a look and certainly this would solve the huge file io happening here.

            The patch does introduce a data loss risk (write errors/network errors etc) when simply using the copy function. The risk has so far been recoverable with the file in trash.

            The proper way here imo seems to be to allow a particular stored_file to be flagged as not as trash worthy . Perhaps the file in this case would have been verified to have been transferred successfully upon which the copy of the file can then be deleted and also flagged as not needing to be in trash at all. This however seems to be a larger more significant patch within files api, but it would be the better way imo.

            The implications of skipping trash and recovery features vs the risk of (re)doing db processing,data creation, all the associated io and the risk of backed up data loss itself should be weighed here.

            That said, perhaps its ok to have it as an option (at own data loss risk being explained here)

            Show
            nebgor Aparup Banerjee added a comment - Hi, i've had a look and certainly this would solve the huge file io happening here. The patch does introduce a data loss risk (write errors/network errors etc) when simply using the copy function. The risk has so far been recoverable with the file in trash. The proper way here imo seems to be to allow a particular stored_file to be flagged as not as trash worthy . Perhaps the file in this case would have been verified to have been transferred successfully upon which the copy of the file can then be deleted and also flagged as not needing to be in trash at all. This however seems to be a larger more significant patch within files api, but it would be the better way imo. The implications of skipping trash and recovery features vs the risk of (re)doing db processing,data creation, all the associated io and the risk of backed up data loss itself should be weighed here. That said, perhaps its ok to have it as an option (at own data loss risk being explained here)
            Hide
            tlevi Tony Levi added a comment -

            There is a dataloss 'risk' using external full stop; if the files go missing the trashdir will only save you for 24 hours anyway.

            Show
            tlevi Tony Levi added a comment - There is a dataloss 'risk' using external full stop; if the files go missing the trashdir will only save you for 24 hours anyway.
            Hide
            nebgor Aparup Banerjee added a comment -

            You are right Levi, the risk is there always with external storage, although I believe Koen noted the files stay in trash for 4 days in Helen's link to the discussion.

            But either way, as mentioned, i think we can improve this if either (1) the patch handles the risk (verify/errors) or (2) explains the risk (to users in the setting).

            Show
            nebgor Aparup Banerjee added a comment - You are right Levi, the risk is there always with external storage, although I believe Koen noted the files stay in trash for 4 days in Helen's link to the discussion. But either way, as mentioned, i think we can improve this if either (1) the patch handles the risk (verify/errors) or (2) explains the risk (to users in the setting).
            Hide
            skodak Petr Skoda added a comment -

            I like the patch and I think the implementation is ok, I do not see any major risk there. I personally do not like the idea of modifying the file api to alter file deleting because it is there to prevent race conditions in the first place.

            Show
            skodak Petr Skoda added a comment - I like the patch and I think the implementation is ok, I do not see any major risk there. I personally do not like the idea of modifying the file api to alter file deleting because it is there to prevent race conditions in the first place.
            Hide
            skodak Petr Skoda added a comment -

            I have rewritten the patch to make it hopefully a bit more reliable, it can be considered for backporting later. Thanks a lot for the report and patch!

            Show
            skodak Petr Skoda added a comment - I have rewritten the patch to make it hopefully a bit more reliable, it can be considered for backporting later. Thanks a lot for the report and patch!
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Integrated (21, 22 and master).

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Integrated (21, 22 and master).
            Hide
            tlevi Tony Levi added a comment -

            Guys, can I just ask when you base work heavily on an existing patch / commit, can we please preserve that existing authorship and history information?

            This would also really help to merge back for the contributors, as there won't be conflicts. It has happened a few times now across many issues so this is just a general note...

            Is there is some reason this isn't done?

            Show
            tlevi Tony Levi added a comment - Guys, can I just ask when you base work heavily on an existing patch / commit, can we please preserve that existing authorship and history information? This would also really help to merge back for the contributors, as there won't be conflicts. It has happened a few times now across many issues so this is just a general note... Is there is some reason this isn't done?
            Hide
            skodak Petr Skoda added a comment -

            Oh, I was not able to cherry pick your commit without conflicts, so I started from scratch, I have used your explanation comment only and gave you credit in the commit message for the original idea, the rest of the patch was written by me. I thought it is ok, but I will write my own comments next time to make the code authorship 100% clear, sorry.

            Show
            skodak Petr Skoda added a comment - Oh, I was not able to cherry pick your commit without conflicts, so I started from scratch, I have used your explanation comment only and gave you credit in the commit message for the original idea, the rest of the patch was written by me. I thought it is ok, but I will write my own comments next time to make the code authorship 100% clear, sorry.
            Hide
            tlevi Tony Levi added a comment -

            I don't think that is quite fair, the bulk of the code is the same. If there are conflicts you can ask for a rebase or fix conflicts only and commit will preserve the auth info.

            This is not just about authorship but for us it will be much easier to merge upstream later if the extra changes are a new commit. The extra history is important in other ways too, such as blame or bisect.

            Anyway, thanks for getting this one fixed in core.

            Show
            tlevi Tony Levi added a comment - I don't think that is quite fair, the bulk of the code is the same. If there are conflicts you can ask for a rebase or fix conflicts only and commit will preserve the auth info. This is not just about authorship but for us it will be much easier to merge upstream later if the extra changes are a new commit. The extra history is important in other ways too, such as blame or bisect. Anyway, thanks for getting this one fixed in core.
            Hide
            skodak Petr Skoda added a comment -

            I understand it would help your company, but others might be confused why I am changing the logic of your patch without long explanation (it does move instead of your copy with proper fallback to the old way, it changes api once again) and why I modified nearly every line of your code, I do not think this detour in git history would be very helpful for others especially when backporting to older versions or in annotate view. I agree that if patch needs just minor tweaks and the logic is not changed we should keep the complete commit history.

            Anyway thanks a lot for discovering this issue and proposing the solution, I hope it will work fine for everybody.

            Show
            skodak Petr Skoda added a comment - I understand it would help your company, but others might be confused why I am changing the logic of your patch without long explanation (it does move instead of your copy with proper fallback to the old way, it changes api once again) and why I modified nearly every line of your code, I do not think this detour in git history would be very helpful for others especially when backporting to older versions or in annotate view. I agree that if patch needs just minor tweaks and the logic is not changed we should keep the complete commit history. Anyway thanks a lot for discovering this issue and proposing the solution, I hope it will work fine for everybody.
            Hide
            phalacee Jason Fowler added a comment -

            all good

            Show
            phalacee Jason Fowler added a comment - all good
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            We could celebrate it today... but better if we perform a bigger party after releasing Moodle 2.3.

            Print this message and come to Perth that day, it's valid for one beer, wine, coke or... water, as you wish.

            Many thanks for your collaboration!

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - We could celebrate it today... but better if we perform a bigger party after releasing Moodle 2.3. Print this message and come to Perth that day, it's valid for one beer, wine, coke or... water, as you wish. Many thanks for your collaboration! Ciao
            Hide
            dougiamas Martin Dougiamas added a comment -

            Petr, thanks for working on this but please make your best efforts to preserve existing commits wherever possible.

            Show
            dougiamas Martin Dougiamas added a comment - Petr, thanks for working on this but please make your best efforts to preserve existing commits wherever possible.
            Hide
            lael Lael... added a comment -

            has this reverted in 2.6.1? I think we are having trouble with this problem over the last week.

            Show
            lael Lael... added a comment - has this reverted in 2.6.1? I think we are having trouble with this problem over the last week.
            Hide
            chadberg Chad Bergeron added a comment -

            Also seeing the trashdir fill up and not get cleared out. 2.5.1.

            Show
            chadberg Chad Bergeron added a comment - Also seeing the trashdir fill up and not get cleared out. 2.5.1.

              People

              • Votes:
                12 Vote for this issue
                Watchers:
                18 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  9/Jul/12