https://dev.arvados.org/https://dev.arvados.org/favicon.ico?15576888422014-03-14T14:23:11ZArvadosArvados - Task #2325: 2221-complete-docker code reviewhttps://dev.arvados.org/issues/2325?journal_id=86822014-03-14T14:23:11ZTom Cleggtom@curii.com
<ul></ul><p>Notes about <code>crunch-job</code> changes</p>
<ul>
<li><del>In collate_output, don't you have to close $child_in in order for "arv keep put" to know it has read all of its input?</del> fixed in <a class="changeset" title="Remove legacy 'thaw' code (refs #2221)." href="https://dev.arvados.org/projects/arvados/repository/arvados/revisions/3690225e29161191cc5daabe4a335a0ec5737d3d">arvados|3690225e29161191cc5daabe4a335a0ec5737d3d</a></li>
<li>L1074: <code>print $child_in "XXX fetch_block($output) failed XXX\n";</code> ← this should probably be sent to <code>Log()</code> instead of getting dumped into the output manifest. (Even though that's been happening so far.)</li>
<li>L1082: <code>$s->can_read(5);</code> should be longer than 5s. Maybe 120s before giving up?</li>
<li>L1082: Log some sort of "timed out" error message if that happens.</li>
</ul> Arvados - Task #2325: 2221-complete-docker code reviewhttps://dev.arvados.org/issues/2325?journal_id=86862014-03-14T17:20:16ZAnonymous
<ul><li><strong>Status</strong> changed from <i>New</i> to <i>Resolved</i></li><li><strong>Start date</strong> set to <i>03/14/2014</i></li><li><strong>% Done</strong> changed from <i>0</i> to <i>100</i></li><li><strong>Remaining (hours)</strong> changed from <i>2.0</i> to <i>0.0</i></li></ul><p>Applied in changeset arvados|commit:cf8b27749cf44aee74914622dc8bc2a9690204dc.</p> Arvados - Task #2325: 2221-complete-docker code reviewhttps://dev.arvados.org/issues/2325?journal_id=87042014-03-17T15:33:20ZTim Piercetwp@curoverse.com
<ul><li><strong>Status</strong> changed from <i>Resolved</i> to <i>In Progress</i></li></ul><p>[oops, should not have closed this until code review is actually complete]</p> Arvados - Task #2325: 2221-complete-docker code reviewhttps://dev.arvados.org/issues/2325?journal_id=91052014-03-28T16:52:46ZTom Cleggtom@curii.com
<ul><li><strong>Target version</strong> changed from <i>2014-03-26 Debt service and dev painkillers</i> to <i>2014-04-16 Dev tools and data/resource management</i></li></ul> Arvados - Task #2325: 2221-complete-docker code reviewhttps://dev.arvados.org/issues/2325?journal_id=91082014-03-28T17:20:03ZTom Cleggtom@curii.com
<ul><li><strong>Target version</strong> changed from <i>2014-04-16 Dev tools and data/resource management</i> to <i>2014-03-26 Debt service and dev painkillers</i></li><li><strong>Remaining (hours)</strong> changed from <i>0.0</i> to <i>2.0</i></li></ul> Arvados - Task #2325: 2221-complete-docker code reviewhttps://dev.arvados.org/issues/2325?journal_id=91952014-04-02T17:46:33ZBrett Smithbrett.smith@curii.com
<ul><li><strong>Assigned To</strong> changed from <i>Ward Vandewege</i> to <i>Tim Pierce</i></li></ul><p>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 <em>fantastic</em>.</p>
<p>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.</p>
<p>Other than that, I just have a couple of minor style points. Near the bottom of <code>build/config.rb</code> there's a comparison against nil; I think the <code>nil?</code> method is preferable. There's also a couple of whitespace bugs; try <code>git diff --check master...</code> for a report.</p>
<p>Thanks again.</p> Arvados - Task #2325: 2221-complete-docker code reviewhttps://dev.arvados.org/issues/2325?journal_id=92032014-04-03T14:20:08ZTim Piercetwp@curoverse.com
<ul><li><strong>Assigned To</strong> changed from <i>Tim Pierce</i> to <i>Brett Smith</i></li></ul><p>PTAL, comments below:</p>
<p>Brett Smith wrote:</p>
<blockquote>
<p>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 <em>fantastic</em>.</p>
</blockquote>
<p>Nice changes, I've merged them back into 2221-complete-docker along with my changes for your comments here.</p>
<blockquote>
<p>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.</p>
</blockquote>
<p>I guess we can explicitly close the filehandles, but what's the win? Won't the kernel reclaim them anyway when the process terminates?</p>
<p>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 <a class="issue tracker-6 status-3 priority-4 priority-default closed parent" title="Idea: Add tests for a few potential login/authentication attacks (Resolved)" href="https://dev.arvados.org/issues/2350">#2350</a> (which I have for this sprint).</p>
<blockquote>
<p>Other than that, I just have a couple of minor style points. Near the bottom of <code>build/config.rb</code> there's a comparison against nil; I think the <code>nil?</code> method is preferable. There's also a couple of whitespace bugs; try <code>git diff --check master...</code> for a report.</p>
</blockquote>
<p>Works for me, added. I also simplified the permission changes in <code>build_tools/config.rb</code> per our discussion on IRC.</p> Arvados - Task #2325: 2221-complete-docker code reviewhttps://dev.arvados.org/issues/2325?journal_id=92052014-04-03T15:05:12ZBrett Smithbrett.smith@curii.com
<ul><li><strong>Assigned To</strong> changed from <i>Brett Smith</i> to <i>Tim Pierce</i></li></ul><blockquote>
<p>I guess we can explicitly close the filehandles, but what's the win? Won't the kernel reclaim them anyway when the process terminates?</p>
</blockquote>
<p>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.</p>
<p>Per discussion on IRC, I suggested but did not implement the style changes, so go ahead with those. Thanks again.</p> Arvados - Task #2325: 2221-complete-docker code reviewhttps://dev.arvados.org/issues/2325?journal_id=92092014-04-03T15:30:16ZTim Piercetwp@curoverse.com
<ul><li><strong>Assigned To</strong> changed from <i>Tim Pierce</i> to <i>Brett Smith</i></li><li><strong>Remaining (hours)</strong> changed from <i>2.0</i> to <i>0.5</i></li></ul><p>Sorry for the misunderstanding. Addressed the style issues in commit bb511c0.</p>
<p>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 :-)</p>
<p>Brett Smith wrote:</p>
<blockquote><blockquote>
<p>I guess we can explicitly close the filehandles, but what's the win? Won't the kernel reclaim them anyway when the process terminates?</p>
</blockquote>
<p>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.</p>
<p>Per discussion on IRC, I suggested but did not implement the style changes, so go ahead with those. Thanks again.</p>
</blockquote> Arvados - Task #2325: 2221-complete-docker code reviewhttps://dev.arvados.org/issues/2325?journal_id=92122014-04-03T15:44:39ZBrett Smithbrett.smith@curii.com
<ul><li><strong>Assigned To</strong> changed from <i>Brett Smith</i> to <i>Tim Pierce</i></li></ul><p>The new open() call in fetch_blocks is missing its semicolon at the end of the line.</p>
<p>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. :)</p> Arvados - Task #2325: 2221-complete-docker code reviewhttps://dev.arvados.org/issues/2325?journal_id=92132014-04-03T15:47:29ZTim Piercetwp@curoverse.com
<ul></ul><p>Brett Smith wrote:</p>
<blockquote>
<p>The new open() call in fetch_blocks is missing its semicolon at the end of the line.</p>
<p>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. :)</p>
</blockquote>
<p>Embarrassing. Thanks, fixed.</p> Arvados - Task #2325: 2221-complete-docker code reviewhttps://dev.arvados.org/issues/2325?journal_id=92142014-04-03T15:55:18ZTim Piercetwp@curoverse.com
<ul><li><strong>Status</strong> changed from <i>In Progress</i> to <i>Resolved</i></li><li><strong>Remaining (hours)</strong> changed from <i>0.5</i> to <i>0.0</i></li></ul><p>Applied in changeset arvados|commit:6d73acc845a2dd7413fdde0742473d83bf3d0719.</p> Arvados - Task #2325: 2221-complete-docker code reviewhttps://dev.arvados.org/issues/2325?journal_id=92172014-04-03T16:24:58ZTim Piercetwp@curoverse.com
<ul><li><strong>Status</strong> changed from <i>Resolved</i> to <i>Closed</i></li><li><strong>Target version</strong> changed from <i>2014-03-26 Debt service and dev painkillers</i> to <i>2014-04-16 Dev tools and data/resource management</i></li></ul>