Added a check for whether files can be created and executed within the configured tmp-dir #8951
Conversation
…o not be executable, to help troubleshooting if that causes issues.
|
Github actions tests reported job failures from actions build 10373907397
|
There was a problem hiding this comment.
@KevinCLydon I've reviewed this. I have a few comments. I like this approach, it seems practical and pretty robust. Have you tested that it actually warns on a system configured this way?
|
|
||
| // Warn if there's anything that prevents execution in the tmp dir because some tools need that | ||
| // This test relies on the file system supporting posix file permissions | ||
| if(p.getFileSystem().supportedFileAttributeViews().contains("posix")) { |
There was a problem hiding this comment.
So I would pull this whole bit out into a new method called something stupid like "tryToWriteAnExecutableFileAndWarnOnFailure". There are multiple levels of try / catch here and it's kind of confusing at the moment.
There was a problem hiding this comment.
I'll move this into a separate function but I'm not sure there's a great way to fix the nested try / catch issue. I want the inner one because it specifically catches issues setting exec permissions on the temp file and I don't think that function throws granular-enough exceptions for me to easily be able to distinguish between an exception from that and an exception from like creating the file or getting the posix permissions. And I need the outer try / catch even in a new function so I can make sure the temp file gets deleted.
There was a problem hiding this comment.
I see your point and it makes sense. I'm not sure we care that much about treating those cases differently though? Can we give meaningfully different advice if getting permissions failed vs if setting exec failed? Maybe can. If you think that's a useful distinction I'm not against having it split out. I do like it all encapsulated in a function that won't explode in any case though.
There was a problem hiding this comment.
Yeah, I think maybe it's not a worthwhile distinction
|
|
||
| // Warn if there's anything that prevents execution in the tmp dir because some tools need that | ||
| // This test relies on the file system supporting posix file permissions | ||
| if(p.getFileSystem().supportedFileAttributeViews().contains("posix")) { |
There was a problem hiding this comment.
A small and annoying caveat here, the HttpFileSystem throw UnsupportedOperation if you ask it this. (Which it probably should and we should fix).
There was a problem hiding this comment.
What should be the course of action if that happens? Catch it and move on?
There was a problem hiding this comment.
Well, we won't even get to here in that case, since we've already exploded on the check above due to not supporting write access. Just a weird edge case.
There was a problem hiding this comment.
I think catch and continue was the right choice.
| } | ||
| } | ||
| catch(IOException e) { | ||
| logger.warn( |
There was a problem hiding this comment.
We should probably attach the exception here. Since we want a sane human readable warning we might want to put an additional logger.debug(e) beneath this instead of logging it with the user warning.
| "directory using --tmp-dir" | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
I always prefer braces styled like
} catch () {
...
} finally {
instead of
}
catch () {
...
}
finally {
It's not big deal either way and we definitely have examples of both, but if you're not ideologically I'd say use braces on the same line.
| // This test relies on the file system supporting posix file permissions | ||
| if(p.getFileSystem().supportedFileAttributeViews().contains("posix")) { | ||
| // Write an empty file to the tempdir | ||
| tempFilePath = Files.createTempFile(p, "gatk_exec_test", null); |
There was a problem hiding this comment.
Lets move this and the permissions getting into the try block. I think we don't want additional potential exceptions propagating up here. This is a warning only situation.
| "This can cause issues for some GATK tools. You can specify a different directory using " + | ||
| "--tmp-dir" | ||
| ); | ||
| } |
There was a problem hiding this comment.
Lets add a logger.debug() line in the case where posix permissions aren't available.
| finally { | ||
| // Make sure we clean up the test file | ||
| try { | ||
| if (tempFilePath != null) |
| ); | ||
| } | ||
| } | ||
| catch(IOException e) { |
There was a problem hiding this comment.
We might want to catch general "Exception" here. That's generally bad practice, but in this case we really want this to be only a warning I think and not crash GATK if something weird gets thrown.
| @@ -177,6 +214,16 @@ public Object instanceMainPostParseArgs() { | |||
| // other exceptions with the tmp directory | |||
| throw new UserException.BadTempDir(p, e.getMessage(), e); | |||
| } | |||
| finally { | |||
There was a problem hiding this comment.
I think we can move this finally block and attack it to the try where we are writing the file.
|
@lbergelson Okay, I think I have a better version now that hopefully addresses all your comments 🤞 |
|
@KevinCLydon Loos good to me. If you think we should have a different warning in the case where we get an error while checking permissions then I support that, but I'm not sure what we'd tell people exactly. Good to either implement that or merge . |
Adds a couple warnings in case the user can write to the configured tmp-dir but can't set the files as executable or execute them, with the assumption in the latter case that the cause is likely that the directory is mounted using the "noexec" mount option.
I'm not sure if a test makes sense for this, because it would probably require mounting a directory for testing, and the user running the tests may not have permissions to do that. I think only root can mount with options in Ubuntu? Do we require root for other tests?
Fixes #8453