-
Notifications
You must be signed in to change notification settings - Fork 583
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement retrying on cleanup #265
Conversation
…buted filesystem errors.
Hah, ok - interesting that this happens for the log file but not for the temporary directory used for report generation? Perhaps instead of this, I could just create a new configuration option to disable logging to a file? Then if you / others have problems, you can just disable that piece of code altogether? Also avoids adding another dependency as a bonus. Best of all, we both get our desired behaviour 😉 Phil |
After a bit of googling and help from stack overflow, it sounds like it could be because logger.handlers[0].close()
logger.removeHandler(logger.handlers[0])
shutil.rmtree(log_tmp_dir) |
Version someone else wrote for the same reason with no dependencies: https://github.com/hashdist/hashdist/pull/116/files (no exponential back off, but would be easy to add). |
Oh, gotcha, forgot to add those links (actually Oliver told me about them first, doh)... anyway will put that |
@@ -6,11 +6,9 @@ | |||
|
|||
import logging | |||
import os | |||
import shutil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still need shutil
for copying files..
Rebased, the last commit/comment. I think both commits ( |
@@ -7,9 +7,31 @@ | |||
import json | |||
import os | |||
import yaml | |||
import time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to add shutil
here too.. Good spot Travis!
…t package, robust_rmtree. Thanks @ewels
Thanks! 👍 |
I leave the office for a few hours and you fix everything? Thanks, appreciate it -- off to give it a try :) The logger as the source makes sense and might explain why I am not seeing the error with any of the rmtrree() calls that bcbio issues. |
Yup, agreed - MultiQC has other |
Awesome, thanks folks. I saw this as well occasionally so it is awesome to have a fix in. |
Hmm, apparently it is not enough with
|
What about if you try my idea above for closing the logging handler? Sorry I can't be more help here, difficult when I can't reproduce the error myself. |
@ewels, That too, but do you think it'll then remove the NFS stalled file? |
Wow, there's even an errno dedicated to NFS:
|
Tjena Phil!
This is related to the following backtrace seen on a new cluster with NFS mounts.
Seems that my mantra on "Filesystems are evil and logs should be streamed" is paying dividends in this particular instance, @ewels ;P
Bits that deserve some attention before merging this up:
@ohofmann, perhaps you can apply this to your cluster "manually" as a "Hotfix" for now, in the interest of time.
@chapmanb