Moodle
  1. Moodle
  2. MDL-30725

Automated backups with external storage fill trashdir

    Details

    • Rank:
      33601

      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?

        Issue Links

          Activity

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

          Thanks for reporting that and proposing a solution, Tony.

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

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

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

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

          Show
          Tony Levi added a comment - This change continues to operate successfully for us on multiple production environments.
          Hide
          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
          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
          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
          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
          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
          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
          Dan Poltawski added a comment -

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

          Show
          Dan Poltawski added a comment - Apu - you are more familiar with backup than me. Could you cast you eyes on this? thanks
          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          Petr Škoda 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
          Petr Škoda 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
          Petr Škoda 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
          Petr Škoda 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
          Eloy Lafuente (stronk7) added a comment -

          Integrated (21, 22 and master).

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (21, 22 and master).
          Hide
          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
          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
          Petr Škoda 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
          Petr Škoda 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
          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
          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
          Petr Škoda 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
          Petr Škoda 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
          Jason Fowler added a comment -

          all good

          Show
          Jason Fowler added a comment - all good
          Hide
          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
          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
          Martin Dougiamas added a comment -

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

          Show
          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... 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... added a comment - has this reverted in 2.6.1? I think we are having trouble with this problem over the last week.
          Hide
          Chad Bergeron added a comment -

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

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