luci-py/isolateserver.py: Add support for tar archives when downloading.

luci-py/isolateserver.py: Add support for tar archives when downloading.

Currently the isolateserver.py only supports extracting ar archive which
contain many small files grouped together. This CL extends the support
to also include tar archive format.

This support will exist while we evaluate and compare the ar and tar
file formats with real workloads on real servers.

It is expected that one format will be removed in the future.

 * https://github.com/luci/luci-go/issues/9
 * https://crbug.com/598990

BUG=598990
Committed: https://github.com/luci/luci-py/commit/ef55a41b26e19ca89d9b9b6c993469ab58bfaff1

[mithro, 2016-11-08 07:32:39]

tansell@chromium.org changed reviewers:
+ djd@chromium.org, maruel@google.com, mcgreevy@golang.org, nodir@chromium.org

[mithro, 2016-11-08 07:32:40]

Hi everyone,

This CL extends my previous addition of ar archive support to also include tar
archive support. This will also give us a good baseline of how things are
actually being improved.

This is from a discussion here in the Sydney office about the best path towards
getting the small file archive stuff working. The plan is to use the inbuilt
tarfile support in Go at first and then swap in the arfile implementation once
that is shown to be better performing.

Most of the hard effort was done when adding the ar format, hence adding a new
file format was pretty easy.

I've added Marc+Nodir who reviewed the original CL and Dave+Michael who I had
the discussion with.

Marc, I've forgotten how to update the swarming bots once this patch lands?

Thanks!

Tim 'mithro' Ansell

[M-A Ruel, 2016-11-08 14:33:43]

maruel@chromium.org changed reviewers:
+ maruel@chromium.org

[M-A Ruel, 2016-11-08 14:33:45]

I'm fine with experimenting live to see the difference in practice so lgtm.

On 2016/11/08 07:32:40, mithro wrote:
> Marc, I've forgotten how to update the swarming bots once this patch lands?

./tools/gae upl -A ...
./tools/gae swi -A ...

https://codereview.chromium.org/2484133002/diff/1/client/isolateserver.py
File client/isolateserver.py (right):

https://codereview.chromium.org/2484133002/diff/1/client/isolateserver.py#new...
client/isolateserver.py:8: __version__ = '0.6.0'
bump

https://codereview.chromium.org/2484133002/diff/1/client/isolateserver.py#new...
client/isolateserver.py:2118: extractor = tarfile.TarFile(fileobj=srcfileobj)
with tarfile.TarFile(fileobj=srcfileobj) as t:
  for ti in t:
...

https://codereview.chromium.org/2484133002/diff/1/client/isolateserver.py#new...
client/isolateserver.py:2125: fp = os.path.normpath(os.path.join(basedir,
ti.name))
if not fp.startswith(basedir):
  logging.error('tar file with path outside root directory')
  raise an error

https://codereview.chromium.org/2484133002/diff/1/client/tests/isolateserver_...
File client/tests/isolateserver_test.py (right):

https://codereview.chromium.org/2484133002/diff/1/client/tests/isolateserver_...
client/tests/isolateserver_test.py:1136: maxDiff = None
remove

https://codereview.chromium.org/2484133002/diff/1/client/tests/isolateserver_...
client/tests/isolateserver_test.py:1157: f1 = tarfile.TarInfo()
indent at +2

https://codereview.chromium.org/2484133002/diff/1/client/tests/isolateserver_...
client/tests/isolateserver_test.py:1226: 
only 2 lines between file level symbols

[mithro, 2016-11-09 00:19:36]

Fixed.

Will wait until the Go side is closer to being ready before landing.

https://codereview.chromium.org/2484133002/diff/1/client/isolateserver.py
File client/isolateserver.py (right):

https://codereview.chromium.org/2484133002/diff/1/client/isolateserver.py#new...
client/isolateserver.py:8: __version__ = '0.6.0'
On 2016/11/08 14:33:44, M-A Ruel wrote:
> bump

Done.

https://codereview.chromium.org/2484133002/diff/1/client/isolateserver.py#new...
client/isolateserver.py:2118: extractor = tarfile.TarFile(fileobj=srcfileobj)
On 2016/11/08 14:33:44, M-A Ruel wrote:
> with tarfile.TarFile(fileobj=srcfileobj) as t:
>   for ti in t:
> ...

Done.

https://codereview.chromium.org/2484133002/diff/1/client/isolateserver.py#new...
client/isolateserver.py:2125: fp = os.path.normpath(os.path.join(basedir,
ti.name))
On 2016/11/08 14:33:44, M-A Ruel wrote:
> if not fp.startswith(basedir):
>   logging.error('tar file with path outside root directory')
>   raise an error

Done.

https://codereview.chromium.org/2484133002/diff/1/client/tests/isolateserver_...
File client/tests/isolateserver_test.py (right):

https://codereview.chromium.org/2484133002/diff/1/client/tests/isolateserver_...
client/tests/isolateserver_test.py:1136: maxDiff = None
On 2016/11/08 14:33:44, M-A Ruel wrote:
> remove

Sorry, left over from debugging.

https://codereview.chromium.org/2484133002/diff/1/client/tests/isolateserver_...
client/tests/isolateserver_test.py:1157: f1 = tarfile.TarInfo()
On 2016/11/08 14:33:44, M-A Ruel wrote:
> indent at +2

Done.

https://codereview.chromium.org/2484133002/diff/1/client/tests/isolateserver_...
client/tests/isolateserver_test.py:1226: 
On 2016/11/08 14:33:44, M-A Ruel wrote:
> only 2 lines between file level symbols

Done.

[mithro, 2016-11-11 03:59:05]

The CQ bit was checked by tansell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-11 03:59:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-11 04:01:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-11 04:01:40]

Dry run: This issue passed the CQ dry run.

[mithro, 2016-11-14 02:38:19]

tansell@chromium.org changed reviewers:
+ ddoman@chromium.org

[mithro, 2016-11-14 02:38:19]

I'm going to submit and then deploy this now.

I have tested this on the dev cluster and it has successfully extracted the
isolate generated by djd's updated go client.

You can see a mostly successful run of the layout tests at
https://chromium-swarm-dev.appspot.com/task?id=326aa397db42bb10&refresh=10&sh...

Tim 'mithro' Ansell

[mithro, 2016-11-14 02:38:28]

The CQ bit was checked by tansell@chromium.org

[mithro, 2016-11-14 02:38:29]

The patchset sent to the CQ was uploaded after l-g-t-m from maruel@chromium.org
Link to the patchset: https://codereview.chromium.org/2484133002/#ps60001
(title: "Rebase onto master.")

[commit-bot: I haz the power, 2016-11-14 02:38:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-14 02:41:13]

Description was changed from

==========
luci-py/isolateserver.py: Add support for tar archives when downloading.

Currently the isolateserver.py only supports extracting ar archive which
contain many small files grouped together. This CL extends the support
to also include tar archive format.

This support will exist while we evaluate and compare the ar and tar
file formats with real workloads on real servers.

It is expected that one format will be removed in the future.

 * https://github.com/luci/luci-go/issues/9
 * https://crbug.com/598990

BUG=598990
==========

to

==========
luci-py/isolateserver.py: Add support for tar archives when downloading.

Currently the isolateserver.py only supports extracting ar archive which
contain many small files grouped together. This CL extends the support
to also include tar archive format.

This support will exist while we evaluate and compare the ar and tar
file formats with real workloads on real servers.

It is expected that one format will be removed in the future.

 * https://github.com/luci/luci-go/issues/9
 * https://crbug.com/598990

BUG=598990

Committed:
https://github.com/luci/luci-py/commit/ef55a41b26e19ca89d9b9b6c993469ab58bfaff1
==========

[commit-bot: I haz the power, 2016-11-14 02:41:14]

Committed patchset #4 (id:60001) as
https://github.com/luci/luci-py/commit/ef55a41b26e19ca89d9b9b6c993469ab58bfaff1