-
Notifications
You must be signed in to change notification settings - Fork 1
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
[simulator util] Ignore error during sim log directory deletion. #1
Conversation
Similarly here, please give more context about this fix + any associated issues that support how we came to this conclusion. |
@jverkoey description has been updated with an issue and context. |
@@ -158,7 +158,7 @@ def Delete(self): | |||
preexec_fn=os.setpgrp) | |||
# The delete command won't delete the simulator log directory. | |||
if os.path.exists(self.simulator_log_root_dir): | |||
shutil.rmtree(self.simulator_log_root_dir) | |||
shutil.rmtree(self.simulator_log_root_dir, ignore_errors=True) |
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.
Does ignore_errors force the directory to be deleted, even if there are errors? Or does this flag simply allow execution to continue, even though the directory may not have been deleted?
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.
Good catch. I think it only ignores the error. This function accepts a onError handler. Do you think if we need to handle it in that way? Or maybe leaving an empty directory is acceptable.
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.
I changed the description to reflect it.
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.
I believe we'll need some sort of equivalent to a rm -f
solution.
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.
Might be worth testing a small python script locally to see if ignore_errors works like -f
or not.
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.
Good suggestion. I tried to reproduce it locally with a python script and got more data. This issue is not easy to reproduce. The documentation of shutil.rmtree
doesn't say that when it would throw "Directory not empty" error. It could be likely caused by race condition mentioned in reframe-hpc/reframe#291.
I found this issue when a Booting simulator solution was used originally for #2, which caused the deletion process to be slow and rmtree
can't be finished in time. It doesn't seem to happen for the time being.
I am going to close this PR because ignore_errors
is not appropriate to be used under this situation.
Closes #4.
Because simulator is deleted asynchronously, the log directory can be non-empty when being deleted. This change ignores the error when it happens.