1316183 - [gecko] Compress in-tree images with zstd

Status: RESOLVED FIXED

(Firefox Build System :: Task Configuration, task)

Comment 1

[Gregory Szorc [:gps]]

There are Python bindings at https://github.com/indygreg/python-zstandard. They support streaming compression, so it would be possible to e.g. stream into zstd and stream output into a network socket for upload. A benefit of doing it that way is you can set the compression level as high as your network can support, meaning you'll maximize compression without impacting wall time.

If you want any features in the Python bindings just ask - I'm the author :)

Comment 2

[Jonas Finnemann Jensen (:jonasfj)]

Attachment: Bug 1316183 - Compress docker images with zstd.

* Compress docker images with zstd
 * Removed need for context.tar from decision task
 * Index images by level rather than project

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

Comment 3

[Jonas Finnemann Jensen (:jonasfj)]

@gps, I can't stream over network as:
 1) I leave this to do docker-worker
 2) I need to support retries when uploading
 3) I really want to set content-length for S3

But, yeah in theory this might be a feature we could built into the worker someday.
And have it do compression level depending on it's previously measured network performance.
It could be interesting if we decided to use .zst instead of .gz/.bz2, and not do compression
inside the container.

In practice | zstd -3 | eats 60-65% of the image size, tweaking it anymore than that is likely overkill.

Note: In tc-worker where I also use zstd I generally prefer opening a subprocess, rather than using golang
      bindings as the unix principal of having one process do one thing well is solid. When streaming
      multiple GBs the overhead of starting a subprocess is negligible.

Comment 4

[Jonas Finnemann Jensen (:jonasfj)]

@dustin,
Let me know if you would like me to split the patch.
It pretty much needs everything to work, but I could perhaps split out the modifications to mach.

Note:
Once review is ok, I'll change:
  testing/docker/image_builder/VERSION -> 0.1.6
  testing/docker/image_builder/REGISTRY -> taskcluster
  + Build and push image_builder to taskcluster/image_builder:0.1.6.
and
  taskcluster/taskgraph/util/docker.py:INDEX_PREFIX -> 'docker.images.v2'
  + Grant queue:route:index.docker.images.v2.level-{level}.* to assume:moz-tree:level:{level}

### Scoping Change:
The proposed patch changes how images are index, as detailed below:
  Before:   docker.v1.<project>.<imageName>.hash.<hash>
  After:    docker.v2.level-<level>.<imageName>.hash.<hash>

Reasoning is that:
  A) We do caches by the level, so we should probably also do images by the level.
     After all these are just slightly more permanent caches.
  B) If we assign the scopes to:
       1) "repo:hg.mozilla.org/mozilla-central:*"
     Rather than
       2) "assume:moz-tree:level:3"
     then:
       i)  People won't be able to retrigger tasks
       ii) Actions tasks won't be able to trigger a task that depends on an image build
           (unless the image build is cached)
     Obviously:
       There is a downside, as attaching the indexing scopes (1) instead of (2) as proposed
       here. Makes it a lot harder to accidentally poison the image cache.
  C) If we reasonably trust the image builds, this shouldn't be a problem.
     In fact sharing caches across projects is likely more problematic than sharing images.
     (as we do hash the input for docker images builds)

Unrelated:
  Should we consider scoping caches by project? Or is that too many scopes...
  Maybe cache names should be: gecko-level-<level>-<project>
  then we can just give out the scope for gecko-level-<level>-*'
  (not that I often hear about cache poisoning problems)

Comment 5

[Jonas Finnemann Jensen (:jonasfj)]

Having just written down the arguments, I'm not 100% sure the scoping change is ideal.
Please give me a sanity check on that :)

Comment 6

[Gregory Szorc [:gps]]

Comment on attachment 8808850 [details]
Bug 1316183 - Compress docker images with zstd.

https://reviewboard.mozilla.org/r/91584/#review91624

So glad we're finally compressing Docker images.

::: taskcluster/taskgraph/docker.py:44
(Diff revision 1)
> +    subprocess.check_call(['curl', '-#', '-L', '-o', tempfilename, artifact_url])
> +    print("Decompressing")
> +    subprocess.check_call(['zstd', '-d', tempfilename, '-o', filename])
> +    print("Deleting temporary file")
> +    os.unlink(tempfilename)
>  
>      print("Determining image name")
>      tf = tarfile.open(filename)
>      repositories = json.load(tf.extractfile('repositories'))
>      name = repositories.keys()[0]
>      tag = repositories[name].keys()[0]
>      name = '{}:{}'.format(name, tag)
>      print("Image name:", name)
>  
>      print("Loading image into docker")
>      try:
>          subprocess.check_call(['docker', 'load', '-i', filename])

This patch changes behavior from:

1. Download tar archive to local disk
2. Load tar archive into Docker

To:

1. Download tar archive to local disk
2. Decompress archive, writing it to disk
3. Load tar archive into Docker

While the network transfer will be smaller and will likely take less time, we'll be incurring hundreds of megabytes of extra disk I/O to write 2 copies of the file. I would not at all be surprised if this makes things slower overall, especially on instances with less RAM and therefore less page cache to buffer the write before incurring a disk flush.

At the very least, we should stream from network the decompressed tar file without an intermediate disk write to preserve existing I/O properties.

Ideally, we'd avoid the disk write completely, as `docker load` can stream from stdin. That could be a bit complicated if Python needs to slurp the "repositories" file from within the archive. That file is only used for logging. Perhaps we could get the metadata from elsewhere?

::: testing/docker/image_builder/Dockerfile:1
(Diff revision 1)
> -FROM ubuntu:14.04
> +FROM ubuntu:16.04

Thank you, thank you, thank you. I can't remember what, but there was something annoying with the old OS/Docker running on this image that was causing us grief. Now to remember what it was...

::: testing/docker/image_builder/setup.sh:29
(Diff revision 1)
> +curl -L --retry 5 --connect-timeout 30 --fail https://github.com/facebook/zstd/archive/v1.0.0.tar.gz > /tmp/zstd.tar.gz
> +echo '197e6ef74da878cbf72844f38461bb18129d144fd5221b3598e973ecda6f5963 /tmp/zstd.tar.gz' | sha256sum -c

This should use tooltool, as we don't like run-time dependencies on github.com, even if rebuilding the image_builder image is not something that happens a lot.

Comment 7

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8808850 [details]
Bug 1316183 - Compress docker images with zstd.

https://reviewboard.mozilla.org/r/91584/#review91548

This is a great improvement!

Regarding levels, I think that's sufficient isolation.

::: taskcluster/mach_commands.py:277
(Diff revision 1)
> -    def build_image(self, image_name):
> -        from taskgraph.docker import build_image
> -
> +    @CommandArgument('--context-only',
> +                     help='Only build the image context.tar',
> +                     metavar='context.tar')

It looks like this can specify a filename?  That is probably worth mentioning in the help string

::: taskcluster/taskgraph/task/docker_image.py:115
(Diff revision 1)
>          return False, None
>  
>      @classmethod
>      def from_json(cls, task_dict):
>          # Generating index_paths for optimization
> -        routes = task_dict['task']['routes']
> +        imgMeta = task_dict['task']['extra']['imageMeta']

This is a nice workaround for some awkwarndess here.

::: testing/docker/image_builder/VERSION:1
(Diff revision 1)
> -0.1.5
> +0.1.6-alpha11

I'd say this deserves a major version bump since it's incompatible

::: testing/docker/image_builder/build-image.sh:31
(Diff revision 1)
> +  -- /home/worker/checkouts/gecko/mach \
> +  taskcluster-build-image \

switch the line breaks around here:

  --
  /home/worker/checkouts/gecko/mach taskcluster-build-image

Comment 8

[Jonas Finnemann Jensen (:jonasfj)]

Comment on attachment 8808850 [details]
Bug 1316183 - Compress docker images with zstd.

https://reviewboard.mozilla.org/r/91584/#review91548

> It looks like this can specify a filename?  That is probably worth mentioning in the help string

Good point

> I'd say this deserves a major version bump since it's incompatible

Good point

> switch the line breaks around here:
> 
>   --
>   /home/worker/checkouts/gecko/mach taskcluster-build-image

Will do

Comment 9

[Jonas Finnemann Jensen (:jonasfj)]

Comment on attachment 8808850 [details]
Bug 1316183 - Compress docker images with zstd.

https://reviewboard.mozilla.org/r/91584/#review91624

> This patch changes behavior from:
> 
> 1. Download tar archive to local disk
> 2. Load tar archive into Docker
> 
> To:
> 
> 1. Download tar archive to local disk
> 2. Decompress archive, writing it to disk
> 3. Load tar archive into Docker
> 
> While the network transfer will be smaller and will likely take less time, we'll be incurring hundreds of megabytes of extra disk I/O to write 2 copies of the file. I would not at all be surprised if this makes things slower overall, especially on instances with less RAM and therefore less page cache to buffer the write before incurring a disk flush.
> 
> At the very least, we should stream from network the decompressed tar file without an intermediate disk write to preserve existing I/O properties.
> 
> Ideally, we'd avoid the disk write completely, as `docker load` can stream from stdin. That could be a bit complicated if Python needs to slurp the "repositories" file from within the archive. That file is only used for logging. Perhaps we could get the metadata from elsewhere?

This is only used by developers locally.
I'm all for making this better, but really we should also be retrying downloads.

I'll file separate bug for improving this.

See bug 1316446

> This patch changes behavior from:
> 
> 1. Download tar archive to local disk
> 2. Load tar archive into Docker
> 
> To:
> 
> 1. Download tar archive to local disk
> 2. Decompress archive, writing it to disk
> 3. Load tar archive into Docker
> 
> While the network transfer will be smaller and will likely take less time, we'll be incurring hundreds of megabytes of extra disk I/O to write 2 copies of the file. I would not at all be surprised if this makes things slower overall, especially on instances with less RAM and therefore less page cache to buffer the write before incurring a disk flush.
> 
> At the very least, we should stream from network the decompressed tar file without an intermediate disk write to preserve existing I/O properties.
> 
> Ideally, we'd avoid the disk write completely, as `docker load` can stream from stdin. That could be a bit complicated if Python needs to slurp the "repositories" file from within the archive. That file is only used for logging. Perhaps we could get the metadata from elsewhere?

This is only used by developers locally.
I'm all for making this better, but really we should also be retrying downloads.

I'll file separate bug for improving this.

See bug 1316446

> This should use tooltool, as we don't like run-time dependencies on github.com, even if rebuilding the image_builder image is not something that happens a lot.

image_builder has to built manually, but I'm happy to move sources into tooltool.

Comment 10

[Jonas Finnemann Jensen (:jonasfj)]

Comment on attachment 8808850 [details]
Bug 1316183 - Compress docker images with zstd.

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2168cbd4b154
Compress docker images with zstd. r=dustin

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2168cbd4b154