1359965 - Produce .tar.gz test archive for WPT to work around 2**16 file limit

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[James Graham [:jgraham]]

After addign css tests and associated metadata to web-platform-tests I have a build error in package-tests:

 0:25.96 Traceback (most recent call last):
 0:25.96   File "/usr/lib/python2.7/runpy.py", line 174, in _run_module_as_main
 0:25.96     "__main__", fname, loader, pkg_name)
 0:25.96   File "/usr/lib/python2.7/runpy.py", line 72, in _run_code
 0:25.96     exec code in run_globals
 0:25.96   File "/home/james/develop/gecko/python/mozbuild/mozbuild/action/test_archive.py", line 642, in <module>
 0:25.96     sys.exit(main(sys.argv[1:]))
 0:25.96   File "/home/james/develop/gecko/python/mozbuild/mozbuild/action/test_archive.py", line 632, in main
 0:25.96     file_count += 1
 0:25.96   File "/home/james/develop/gecko/python/mozbuild/mozpack/mozjar.py", line 492, in __exit__
 0:25.96     self.finish()
 0:25.96   File "/home/james/develop/gecko/python/mozbuild/mozpack/mozjar.py", line 570, in finish
 0:25.96     self._data.write(end.serialize())
 0:25.96   File "/home/james/develop/gecko/python/mozbuild/mozpack/mozjar.py", line 146, in serialize
 0:25.96     serialized += struct.pack('<' + format, value)
 0:25.96 struct.error: ushort format requires 0 <= number <= USHRT_MAX
 0:26.29 /home/james/develop/gecko/testing/testsuite-targets.mk:161: recipe for target 'package-tests-web-platform' failed

I think this is a Zip32 limit of 2**16-1 files in an archive. Note that mochtitest-plain currently has 53775 files, which is 82% of the limit, so it seems likely to be affected in the near future.

mozjar ought to support Zip64.

Comment 1

[Gregory Szorc [:gps]]

The archives produced by mozjar need to be decoded by libjar in Gecko. So before we change this, we should verify libjar can decode zip64 archives. Alternatively (and this might be a good idea to reduce the size of omni.ja), we could have mozjar "upgrade" to zip64 when N>=2^16 entries are present. I think nearly all consumers of mozjar populate archives by first obtaining a list of entries. So this could be a run-time decision.

FWIW, Python's built-in zip implementation doesn't support zip64. mozjar's code either relies heavily on the Python implementation or it borrowed heavily from its code. So there may be a bit of work here to support zip64.

Comment 2

[Gregory Szorc [:gps]]

The other question I want to ask is: why are we putting this many files in a single archive? Compressing zip files is inefficient because each file is compressed in its own compression context. Contrast with tar files, where you put the raw content in a single container and compress that.

IMO we should be using tar + compression (preferably zstandard) everywhere in automation. An exception to this are where we absolutely need zip as an interchange format. Another exception is when we only extract a subset of files. But in the case where we extract a subset of files, I'd rather produce a standalone archive that can be extracted wholesale than a zip file because shipping around megabytes of unused content in zip files is just slow. Redundantly compressing files into multiple archives with zstandard will likely offset producer-side performance drawbacks from the redundant compression. And storage is cheap.

Given the question of "y u no tar," do we still need this?

Comment 3

[(not currently active) Ted Mielczarek]

We really should just fix bug 1286900 instead. Putting that many files into a zip so we can upload and then unzipping them is just silly.

Comment 4

[James Graham [:jgraham]]

The Python stdlib does support zip64 I think [1]

I agree that putting this many files in a zip is silly :) But I'm not clear how much work is involved in changing away from zip for packaging tests; bug 1286900 ran into a roadblock and I don't know if there's any specific dependencies on zip other than mozharness which I guess is not too hard to update. 

[1] https://docs.python.org/2/library/zipfile.html

Comment 5

[James Graham [:jgraham]]

So in https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4f9732692123863cd4bc10dbb46ff077e5211b6 I just replaced mozjar with zipfile for packaging tests. Anything else looked pretty daunting. So far nothing terrible seems to have happened (perf. looked similar, archive sizes looked similar).

Comment 6

[Mike Hommey [:glandium]]

For reference, the zip64 format is described here:
https://www.loc.gov/preservation/digital/formats/digformatspecs/APPNOTE%2820120901%29_Version_6.3.3.txt

The short version is that there needs to be two zip64-specific end-of-central-directory related sections, and zip64 extended info headers added to each file entries that require one.

I don't expect this to be a lot of work for minimal support, but I'm not entirely convinced we should do it either. Greg, please assign this bug to me if you feel we should do it.

Comment 7

[Gregory Szorc [:gps]]

Like Ted, I feel we should do bug 1286900. However, I/O on Windows testers is still horrible and I'm afraid this slows down tests too much.

I'm going to spend ~30 minutes trying to make tar.gz test archives work. The code in mozharness should automagically handle different archive extensions. So as long as things aren't hard coding references to .zip, it should "just work." I'll take the conservative approach and only convert the web platform archive by default. If it doesn't pan out, I'm fine with jgraham's patch to use zipfile directly. Although we may want to conditionally do that if there are >2^16 files (because I like mozjar's determinism).

Comment 8

[Gregory Szorc [:gps]]

Attachment: Bug 1359965 - Support mozpack.file.BaseFile in create_tar_from_files;

This allows us to write files coming from a finder or other source
that isn't directly the filesystem.

Review commit: https://reviewboard.mozilla.org/r/137332/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/137332/

Comment 9

[Gregory Szorc [:gps]]

Attachment: Bug 1359965 - Support and generate tar.gz WPT archive;

Several years ago there was a single zip file for all test files. Clients
would only extract the files they needed. Thus, zip was a reasonable
archive format because it allowed direct access to members without
having to decompress the entirety of the stream.

We have since split up that monolithic archive into separate,
domain-specific archives. e.g. 1 archive for mochitests and one
for xpcshell tests. This drastically cut down on network I/O
required on testers because they only fetched archives/data that
was relevant. It also enabled parallel generation of test archives,
we shaved dozens of seconds off builds due to compression being
a long pole.

Despite the architectural changes to test archive management, we
still used zip files. This is not ideal because we no longer access
specific files in test archives and thus don't care about single/partial
member access performance.

This commit implements support for generating tar.gz test archives.
And it switches the web-platform archive to a tar.gz file.

The performance implications for archive generation are significant:

before: 48,321,250 bytes; 6.05s
after:  31,844,267 bytes; 4.57s

The size is reduced because we have a single compression context
so data from 1 file can benefit compression in a subsequent file.
CPU usage is reduced because the compressor has to work less with
1 context than it does with N. While I didn't measure it, decompression
performance should also be improved for the same reasons. And of course
network I/O will be reduced.

mozharness consumers use a generic method for handling unarchiving.
This method automagically handles multiple file extensions. So as long
as downstream consumers aren't hard coding ".zip" this change should
"just work."

Review commit: https://reviewboard.mozilla.org/r/137334/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/137334/

Comment 10

[Gregory Szorc [:gps]]

Comment on attachment 8865705 [details]
Bug 1359965 - Support and generate tar.gz WPT archive;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137334/diff/1-2/

Comment 11

[Mike Hommey [:glandium]]

Comment on attachment 8865704 [details]
Bug 1359965 - Support mozpack.file.BaseFile in create_tar_from_files;

https://reviewboard.mozilla.org/r/137332/#review140840

::: python/mozbuild/mozpack/archive.py:38
(Diff revision 1)
> +                ti.mode = f.mode or (stat.S_IRUSR | stat.S_IWUSR |
> +                                     stat.S_IRGRP | stat.S_IROTH)

stat.S_IRUSR | stat.S_IWUSR | stat.S_IRGRP | stat.S_IROTH

needs a lot of mental effort to figure out what it means, while 0644 or 0o644 is the same thing and plain obvious to most people.

::: python/mozbuild/mozpack/archive.py:65
(Diff revision 1)
>  
>              # Set mtime to a constant value.
>              ti.mtime = DEFAULT_MTIME
>  
> -            with open(fs_path, 'rb') as fh:
> +            if isinstance(f, BaseFile):
> +                data = f.read()

f.open() should always return a file handle that tf.addfile should be able to take.

::: python/mozbuild/mozpack/files.py:500
(Diff revision 1)
> +    def read(self):
> +        return self.content

This effectively fixes bug 1170329, but you actually don't need it here.

Comment 12

[Mike Hommey [:glandium]]

Comment on attachment 8865704 [details]
Bug 1359965 - Support mozpack.file.BaseFile in create_tar_from_files;

https://reviewboard.mozilla.org/r/137332/#review140840

> This effectively fixes bug 1170329, but you actually don't need it here.

Err no, it doesn't, it's GeneratedFile, not BaseFile...

Comment 13

[Mike Hommey [:glandium]]

Comment on attachment 8865705 [details]
Bug 1359965 - Support and generate tar.gz WPT archive;

https://reviewboard.mozilla.org/r/137334/#review140852

Comment 14

[Gregory Szorc [:gps]]

Comment on attachment 8865704 [details]
Bug 1359965 - Support mozpack.file.BaseFile in create_tar_from_files;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137332/diff/1-2/

Comment 15

[Gregory Szorc [:gps]]

Comment on attachment 8865705 [details]
Bug 1359965 - Support and generate tar.gz WPT archive;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137334/diff/2-3/

Comment 16

[Mike Hommey [:glandium]]

Comment on attachment 8865704 [details]
Bug 1359965 - Support mozpack.file.BaseFile in create_tar_from_files;

https://reviewboard.mozilla.org/r/137332/#review143952

::: python/mozbuild/mozpack/archive.py:64
(Diff revision 2)
>  
>              # Set mtime to a constant value.
>              ti.mtime = DEFAULT_MTIME
>  
> -            with open(fs_path, 'rb') as fh:
> +            if isinstance(f, BaseFile):
> +                ti.size = f.read()

f.read() is not going to return a size. It's a bummer that TarInfo.addfile wants a size in self.size :-/

Can we add a size() method to BaseFile instead of reading the thing? File could use stat(), GeneratedFile could use len(self.content)

Comment 17

[Gregory Szorc [:gps]]

Comment on attachment 8865704 [details]
Bug 1359965 - Support mozpack.file.BaseFile in create_tar_from_files;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137332/diff/2-3/

Comment 18

[Gregory Szorc [:gps]]

Comment on attachment 8865705 [details]
Bug 1359965 - Support and generate tar.gz WPT archive;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137334/diff/3-4/

Comment 19

[Mike Hommey [:glandium]]

Comment on attachment 8865704 [details]
Bug 1359965 - Support mozpack.file.BaseFile in create_tar_from_files;

https://reviewboard.mozilla.org/r/137332/#review145298

Comment 20

[Gregory Szorc [:gps]]

More accurate summary.

Comment 21

[Gregory Szorc [:gps]]

Comment on attachment 8865704 [details]
Bug 1359965 - Support mozpack.file.BaseFile in create_tar_from_files;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137332/diff/3-4/

Comment 22

[Gregory Szorc [:gps]]

Comment on attachment 8865705 [details]
Bug 1359965 - Support and generate tar.gz WPT archive;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137334/diff/4-5/

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ae8bce278626
Support mozpack.file.BaseFile in create_tar_from_files; r=glandium
https://hg.mozilla.org/integration/autoland/rev/a0e257e346cc
Support and generate tar.gz WPT archive; r=glandium

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ae8bce278626
https://hg.mozilla.org/mozilla-central/rev/a0e257e346cc

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out from m-c in https://hg.mozilla.org/mozilla-central/rev/1ce55499e8193d53aeff91546b16e0c580ebd6f1 at callek's request for likely breaking tc nightlies.

Comment 26

[Justin Wood (:Callek)]

From IRC:

3:45 PM <Callek_cloud9> KWierso: gps: ping -- so I think we need to backout https://bugzilla.mozilla.org/show_bug.cgi?id=1359965
3:45 PM <firebot> Bug 1359965 — FIXED, [e-mail] — Produce .tar.gz test archive for WPT to work around 2**16 file limit
3:46 PM <Callek_cloud9> https://hg.mozilla.org/mozilla-central/rev/a0e257e346cc#l2.2 --> https://dxr.mozilla.org/mozilla-central/source/taskcluster/taskgraph/transforms/beetmover.py#18
3:46 PM <Callek_cloud9> so two issues, one is the beetmover script code won't support the new artifact, second is that its likely not `.tar.zip`

Comment 27

[Gregory Szorc [:gps]]

Looks like it was a trivial typo of "tar.zip" instead of "tar.gz". I'll reland.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a872303fd084
Support mozpack.file.BaseFile in create_tar_from_files; r=glandium
https://hg.mozilla.org/integration/autoland/rev/d10f5ccd882b
Support and generate tar.gz WPT archive; r=glandium

Comment 29

[Gregory Szorc [:gps]]

I missed the bit about having to update beetmover script code before this lands. I just submitted https://github.com/mozilla-releng/beetmoverscript/pull/63. If that gets deployed before we trigger nightlies with the reland, we should be OK. If not, we'll get the same failure.

I should have read comment #26 in more detail before relanding. In my defense, I wasn't expecting there to be an out-of-tree reference to this filename because most work in the past few years has been about consolidating references in tree so these types of failures don't occur :/

I won't be offended if I'm backed out because beetmover can't be deployed in time.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a872303fd084
https://hg.mozilla.org/mozilla-central/rev/d10f5ccd882b

Comment 31

[Mike Conley (:mconley) (:⚙️)]

https://hg.mozilla.org/projects/cedar/rev/ae8bce278626bc84914063f93292ac5e825eec36
Bug 1359965 - Support mozpack.file.BaseFile in create_tar_from_files; r=glandium

https://hg.mozilla.org/projects/cedar/rev/a0e257e346ccf3c1db332ec5903241f4eeb9a7ee
Bug 1359965 - Support and generate tar.gz WPT archive; r=glandium

https://hg.mozilla.org/projects/cedar/rev/a872303fd08497bbde0e3b4cea09a88a4182810e
Bug 1359965 - Support mozpack.file.BaseFile in create_tar_from_files; r=glandium

https://hg.mozilla.org/projects/cedar/rev/d10f5ccd882b965fcad39914f7c3c930d1301a41
Bug 1359965 - Support and generate tar.gz WPT archive; r=glandium

Comment 32

[Justin Wood (:Callek)]

Backed out from central again --  https://hg.mozilla.org/mozilla-central/rev/ab0dcc11686cfaa56aab1c7b879fecbe8c1f0d0c
 https://hg.mozilla.org/mozilla-central/rev/23a341e9b53d04f80ea6a66ced2d72cdc17afffb

Comment 33

[Mihai Tabara [:mtabara]⌚️GMT]

https://github.com/mozilla-releng/beetmoverscript/pull/63 lgtm 

Comment 34

[Mihai Tabara [:mtabara]⌚️GMT]

Copy-pasting comment from PR, for verbosity and FYI: 

But please do keep in mind that changing here is only half-way to solving the problem. You'd also need to change the artifact in the beetmover in-tree task, specifically, all occurrences of target.web-platform.tests in https://hg.mozilla.org/mozilla-central/file/tip/taskcluster/taskgraph/transforms/beetmover.py and land that to inbound/central/beta.

Until we have those patches landed, I can't land this as it will break the current existing nightly beetmover jobs.

Comment 35

[Gregory Szorc [:gps]]

RyanVM: are you the right person to ask for help about coordinating an uplift? A try push with backported patches is at https://treeherder.mozilla.org/#/jobs?repo=try&revision=a47888d0b6b206bf98e1a66b0ebfb724d79e5c5b.

There should be no impact to a Firefox distribution: either automation is happy or it isn't. Biggest risk is we screw up beetmover on beta and jeopardize a beta build.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

Sure, I can uplift it later today with some other approvals if you get the beetmover stuff taken care of.

Comment 37

[Gregory Szorc [:gps]]

AFAICT the original patches already modified beetmover in tree. We just need to coordinate the deployment of upstream beetmover with the patches in this bug.

Comment 38

[Mihai Tabara [:mtabara]⌚️GMT]

If we want to modify anything related to the artifacts beetmover is supposed to upload, path is the following:

0. land in-tree + uplift all build changes that are changing the format of the web-platform.tests from `zip` to `tar.gz`
1. change, land + uplift beetmover artifacts that are being downloaded. That is changing all occurrrences from this[1] file of `target.web-platform.tests.zip` to `target.web-platform.tests.tar.gz`
2. land https://github.com/mozilla-releng/beetmoverscript/pull/63/ and roll-out a new beetmoverscript package to the beetmoverworkers

I know it's not ideal that we have to change in two different places for (1) and (2). We've gotten bug 1331141 to single-source that. I will take care of (2) once (1) is landed + uplifted. I can help with patches for (1) if you want me to.

[1]: https://hg.mozilla.org/mozilla-central/file/tip/taskcluster/taskgraph/transforms/beetmover.py

Comment 39

[Justin Wood (:Callek)]

(In reply to Mihai Tabara [:mtabara]⌚️GMT from comment #38)
> If we want to modify anything related to the artifacts beetmover is supposed
> to upload, path is the following:
> 
> 0. land in-tree + uplift all build changes that are changing the format of
> the web-platform.tests from `zip` to `tar.gz`
> 1. change, land + uplift beetmover artifacts that are being downloaded. That
> is changing all occurrrences from this[1] file of
> `target.web-platform.tests.zip` to `target.web-platform.tests.tar.gz`
> 2. land https://github.com/mozilla-releng/beetmoverscript/pull/63/ and
> roll-out a new beetmoverscript package to the beetmoverworkers
> 
> I know it's not ideal that we have to change in two different places for (1)
> and (2). We've gotten bug 1331141 to single-source that. I will take care of
> (2) once (1) is landed + uplifted. I can help with patches for (1) if you
> want me to.
> 
> [1]:
> https://hg.mozilla.org/mozilla-central/file/tip/taskcluster/taskgraph/
> transforms/beetmover.py

Ok, heres my comprehension on things, and is why I'm also still confused, mihai can you confirm/deny this more optimal workflow:

0. land to beetmover + deploy the *addition* of the new format of an artifact that should get uploaded. (similar to the PR 63, but without removing the old).
1. Land to in-tree code to change the filename and specify the new filename in taskcluster.
2. Once patch from 1 is on all trees where we expect to beetmove that prior artifact name (e.g. central and project branches where that format is to be used on Nightlies, and is in place for beta/release/esr stuff where we also beetmove) we can delete the old version of the filename from beetmover on github, and deploy.

The reason I come to these steps as a plan is that it's my understanding that if your step 0 and 1 happens then we have the issue that beetmover would reject those filenames, and thus nightlies would fail.

if we go with my listed plan, then we have success with beetmover (and much less of a chicken and egg) since it won't care if a file is missing from its template transforms.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

We're going to hold off on landing this on Beta today to avoid adding risk to the 54b13 gtb.

Comment 41

[Mihai Tabara [:mtabara]⌚️GMT]

Bug 1359965 - Update web platform test archive name
https://github.com/mozilla-releng/beetmoverscript/commit/5e651f54c99f827ee16cb12912c9e999b21bcc56

Bug 1359965 - bump beetmoverscript version in beetmoverworkers. r=versionbump
https://hg.mozilla.org/build/puppet/rev/793eed2dc811

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0a94b9164a64
Support mozpack.file.BaseFile in create_tar_from_files; r=glandium
https://hg.mozilla.org/integration/autoland/rev/11ba6213be5a
Support and generate tar.gz WPT archive; r=glandium

Comment 43

[Mihai Tabara [:mtabara]⌚️GMT]

(In reply to Justin Wood (:Callek) from comment #39)
> (In reply to Mihai Tabara [:mtabara]⌚️GMT from comment #38)
> > If we want to modify anything related to the artifacts beetmover is supposed
> > to upload, path is the following:
> > 
> > 0. land in-tree + uplift all build changes that are changing the format of
> > the web-platform.tests from `zip` to `tar.gz`
> > 1. change, land + uplift beetmover artifacts that are being downloaded. That
> > is changing all occurrrences from this[1] file of
> > `target.web-platform.tests.zip` to `target.web-platform.tests.tar.gz`
> > 2. land https://github.com/mozilla-releng/beetmoverscript/pull/63/ and
> > roll-out a new beetmoverscript package to the beetmoverworkers
> > 
> > I know it's not ideal that we have to change in two different places for (1)
> > and (2). We've gotten bug 1331141 to single-source that. I will take care of
> > (2) once (1) is landed + uplifted. I can help with patches for (1) if you
> > want me to.
> > 
> > [1]:
> > https://hg.mozilla.org/mozilla-central/file/tip/taskcluster/taskgraph/
> > transforms/beetmover.py
> 
> Ok, heres my comprehension on things, and is why I'm also still confused,
> mihai can you confirm/deny this more optimal workflow:
> 
> 0. land to beetmover + deploy the *addition* of the new format of an
> artifact that should get uploaded. (similar to the PR 63, but without
> removing the old).
> 1. Land to in-tree code to change the filename and specify the new filename
> in taskcluster.
> 2. Once patch from 1 is on all trees where we expect to beetmove that prior
> artifact name (e.g. central and project branches where that format is to be
> used on Nightlies, and is in place for beta/release/esr stuff where we also
> beetmove) we can delete the old version of the filename from beetmover on
> github, and deploy.
> 
> The reason I come to these steps as a plan is that it's my understanding
> that if your step 0 and 1 happens then we have the issue that beetmover
> would reject those filenames, and thus nightlies would fail.
> 
> if we go with my listed plan, then we have success with beetmover (and much
> less of a chicken and egg) since it won't care if a file is missing from its
> template transforms.

You're absolutely right. This is the better aproach by far and we're currently following that.

Comment 44

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0a94b9164a64
https://hg.mozilla.org/mozilla-central/rev/11ba6213be5a