Arvados

Applied in changeset arvados|commit:cf8b27749cf44aee74914622dc8bc2a9690204dc.

[oops, should not have closed this until code review is actually complete]

I just pushed 2221-complete-docker-brett. It includes a few small bugfixes, mostly aimed at ensuring a more consistent environment in the Docker images, accounting for more differences in the local environment. With these changes, I can follow the README to get the complete set of Docker images. Which is fantastic.

I like the changes to crunch-job; this is a little more comfortable for mere mortal Perl hackers, I think. Is it feasible to expressly close the filehandles we get from open2? I think this is a case where that could actually matter, depending on how much we end up collating and how often we call fetch_block. It would also be helpful to use \Q…\E (a.k.a. quotemeta) on variable arguments in `system calls`, to help prevent exploits.

Other than that, I just have a couple of minor style points. Near the bottom of build/config.rb there's a comparison against nil; I think the nil? method is preferable. There's also a couple of whitespace bugs; try git diff --check master... for a report.

Thanks again.

PTAL, comments below:

Brett Smith wrote:

I just pushed 2221-complete-docker-brett. It includes a few small bugfixes, mostly aimed at ensuring a more consistent environment in the Docker images, accounting for more differences in the local environment. With these changes, I can follow the README to get the complete set of Docker images. Which is fantastic.

Nice changes, I've merged them back into 2221-complete-docker along with my changes for your comments here.

I like the changes to crunch-job; this is a little more comfortable for mere mortal Perl hackers, I think. Is it feasible to expressly close the filehandles we get from open2? I think this is a case where that could actually matter, depending on how much we end up collating and how often we call fetch_block. It would also be helpful to use \Q…\E (a.k.a. quotemeta) on variable arguments in `system calls`, to help prevent exploits.

I guess we can explicitly close the filehandles, but what's the win? Won't the kernel reclaim them anyway when the process terminates?

Re quotemeta: excellent suggestion, I've made this change for the code that I touched in this branch. More generally crunch-job's shell escapes should be more thoroughly audited, probably in issue #2350 (which I have for this sprint).

Other than that, I just have a couple of minor style points. Near the bottom of build/config.rb there's a comparison against nil; I think the nil? method is preferable. There's also a couple of whitespace bugs; try git diff --check master... for a report.

Works for me, added. I also simplified the permission changes in build_tools/config.rb per our discussion on IRC.

I guess we can explicitly close the filehandles, but what's the win? Won't the kernel reclaim them anyway when the process terminates?

Yes, but is it possible that we'll hit the limit before then? It's only 1024 by default, so I'm concerned that, especially on a job with a lot of subtasks where we call fetch_block a lot, we could use them all up before we finish the work.

Per discussion on IRC, I suggested but did not implement the style changes, so go ahead with those. Thanks again.

Sorry for the misunderstanding. Addressed the style issues in commit bb511c0.

commit b1b9ffbb replaces the open2() call in fetch_block() with a simple open(). In collate_output(), we're already closing the input filehandle as soon as we know we're done with it, and terminating the process as soon as we're done collecting output, so I don't think there's a lot more optimization to do productively there. (Also, I sincerely hope that we'll replace this code with a native Keep SDK before we ever get close to running out of filehandles :-)

Brett Smith wrote:

I guess we can explicitly close the filehandles, but what's the win? Won't the kernel reclaim them anyway when the process terminates?

Yes, but is it possible that we'll hit the limit before then? It's only 1024 by default, so I'm concerned that, especially on a job with a lot of subtasks where we call fetch_block a lot, we could use them all up before we finish the work.

Per discussion on IRC, I suggested but did not implement the style changes, so go ahead with those. Thanks again.

The new open() call in fetch_blocks is missing its semicolon at the end of the line.

With that bugfix, I think it's good to merge. Thanks very much; I know we're both looking forward to building on this base. :)

Applied in changeset arvados|commit:6d73acc845a2dd7413fdde0742473d83bf3d0719.