search

kbase / kb_sdk

Build and test new apps for the KBase platform
http://kbase.github.io/kb_sdk_docs
MIT License
26 stars 32 forks source link

remove -user $(id -u) setting which causes issues running local tests #320

Closed JamesJeffryes closed 6 years ago

JamesJeffryes commented 6 years ago

This removes the -user setting so the containers run as root. As noted, this might generate files in the test_local dir that the user then does not own but only on Linux. Furthermore, these files are perged on each test run by this code here: https://github.com/kbase/kb_sdk/blob/2c5a6cd346256b28e4b202d4465665e64857f2d4/src/java/us/kbase/mobu/tester/ModuleTester.java#L165-L171

JamesJeffryes commented 6 years ago

@jayrbolton @scanon Can I get some review on this?

MrCreosote commented 6 years ago

But will those deleteRecursively() calls work? At this point the java process isn't running as root, right, so I'd guess that they'd fail with a permissions error (or maybe silently depending on the implementation).

MrCreosote commented 6 years ago

Maybe that's what the root problem was - kb-sdk test would try and delete the files and fail, so kb-sdk test was blocked.

That'd be a showstopper if someone was running the sdk remotely on one of our systems, which some people have done.

JamesJeffryes commented 6 years ago

I'll try to replicate this on a linux box

MrCreosote commented 6 years ago

Add a line to test script run in docker container to remove any linge…

That's great until someone writes a test with a 10 GB file...

JamesJeffryes commented 6 years ago

@scanon @MrCreosote OK, here's what I settled on. The deleteRecursively() takes care of almost everything (including the subjobs dir) but not nested files in the scratch space on Linux boxes. Those get taken care of by a new command in the test script executed in docker (with root privs) that wipes out anything left in the scratch. Note that for existing repos, these changes will have no effect as the scripts are only generated if missing.

JamesJeffryes commented 6 years ago

So yes, if someone, on linux, writes a big file, to a nested subdirectory all that build context will have to go to the docker build but as it was, these files were not even being deleted till after the image was already built. We should not lose sight of the fact that dockerized kb-sdk is currently DOA for Mac developers

jayrbolton commented 6 years ago

The code changes look good to me, but I need to test locally

jayrbolton commented 6 years ago

I just had a couple minor comments above. When I make the Makefile modifications, things work well.

I think it's not ideal that this won't work with any existing apps without modifying some of the scripts and the makefile. But I guess that's more of a general downside to the whole templated code approach.. not much we can do about it here.

For the issue of creating/deleting large file sizes, I think many of those large files would be refdata, and it would be nice if refdata was handled by doing a read-only network mount, which would make the cleanup here less of an issue.

JamesJeffryes commented 6 years ago

Added in a little more logging per Jay's suggestion. BTW, I looked at rebuilding the makefile on each test run to ensure that these changes get propagated to existing modules but decided against it because I'd like the ability to tweak the makefile in a given repo if I need to.

@scanon Ready for your final review