1316446 - Improve robustness of ./mach taskcluster-load-image

Status: RESOLVED FIXED

(Taskcluster Graveyard :: Docker Images, defect)

Description

[Jonas Finnemann Jensen (:jonasfj)]

./mach taskcluster-load-image

Loads a docker image from taskId or index.

We should:
 - retry downloads
 - stream directly: curl URL | zstd -d | docker load -
 - verify that client has zstd installed
   (ideally also verifying the zstd version)

Comment 1

[Jonas Finnemann Jensen (:jonasfj)]

Piping from "zstd -d" to "docker load" could be hard as we need to decode and read the repositories file.

Ideally, we should support rewriting the repositories file, so the tag to load it as can be given as command line argument.

Comment 2

[Jonas Finnemann Jensen (:jonasfj)]

Attachment: Bug 1316446 - Improve mach taskcluster-load-image

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

Comment 3

[Jonas Finnemann Jensen (:jonasfj)]

@gps,
This adds the nice streaming, granted the error handling isn't as nice.

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 8809612 [details]
Bug 1316446 - Improve mach taskcluster-load-image

https://reviewboard.mozilla.org/r/92160/#review92144

Nice hacks with Python and the `tarfile` class. This code isn't as bad as I thought it would be.

This is pretty much an r+. The change from `mozilla-central` to `level-3` needs explaining though.

::: taskcluster/taskgraph/docker.py:28
(Diff revision 1)
>  IMAGE_DIR = os.path.join(GECKO, 'testing', 'docker')
>  INDEX_URL = 'https://index.taskcluster.net/v1/task/' + docker.INDEX_PREFIX + '.{}.{}.hash.{}'
>  ARTIFACT_URL = 'https://queue.taskcluster.net/v1/task/{}/artifacts/{}'
>  
>  
> -def load_image_by_name(image_name):
> +def load_image_by_name(image_name, tag = None):

Nit: drop spaces around `=`.

::: taskcluster/taskgraph/docker.py:32
(Diff revision 1)
>  
> -def load_image_by_name(image_name):
> +def load_image_by_name(image_name, tag = None):
>      context_path = os.path.join(GECKO, 'testing', 'docker', image_name)
>      context_hash = docker.generate_context_hash(GECKO, context_path, image_name)
>  
> -    image_index_url = INDEX_URL.format('mozilla-central', image_name, context_hash)
> +    image_index_url = INDEX_URL.format('level-3', image_name, context_hash)

This change seems unrelated?

::: taskcluster/taskgraph/docker.py:39
(Diff revision 1)
>      task = json.load(urllib2.urlopen(image_index_url))
>  
> -    return load_image_by_task_id(task['taskId'])
> +    return load_image_by_task_id(task['taskId'], tag)
>  
>  
> -def load_image_by_task_id(task_id):
> +def load_image_by_task_id(task_id, tag = None):

Nit: drop spacees.

::: taskcluster/taskgraph/docker.py:118
(Diff revision 1)
>          print('a VERSION file in the image directory.')
>          print('*' * 50)
> +
> +
> +
> +def load_image(url, imageName = None, imageTag = None):

Nit: drop spaces.

::: taskcluster/taskgraph/docker.py:170
(Diff revision 1)
> +            tag = repos[image].keys()[0]
> +            layer = repos[image][tag]
> +
> +            # Rewrite the repositories file
> +            data = json.dumps({imageName or image: {imageTag or tag: layer}})
> +            reader = StringIO(data)

This should be an `io.BytesIO` since it is bytes, not unicode. Although `StringIO.StringIO` accepts both on Python 2. It's better to use the `io` types because they enforce types and make the eventual Python 3 porting easier.

Comment 5

[Jonas Finnemann Jensen (:jonasfj)]

Comment on attachment 8809612 [details]
Bug 1316446 - Improve mach taskcluster-load-image

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

Comment 6

[Jonas Finnemann Jensen (:jonasfj)]

Comment on attachment 8809612 [details]
Bug 1316446 - Improve mach taskcluster-load-image

https://reviewboard.mozilla.org/r/92160/#review92144

> This change seems unrelated?

yeah, we switched from project to level scoping for indexing, this one was missed in last review.

Comment 7

[Gregory Szorc [:gps]]

Comment on attachment 8809612 [details]
Bug 1316446 - Improve mach taskcluster-load-image

https://reviewboard.mozilla.org/r/92160/#review92186

::: taskcluster/taskgraph/docker.py:174
(Diff revisions 1 - 2)
>  
> -        # Add member and reader
> +            # Add member and reader
> -        tarout.addfile(member, reader)
> +            tarout.addfile(member, reader)
> -        reader.close()
> +            reader.close()
> -    tarout.close()
> +        tarout.close()
> +    except:

bareword `except` in Python is evil because it also catches `SystemExit` and `KeyboardInterrupt` (which don't inherit from `Exception`). This means your program can ignore signals, which is bad.

Speaking of exception, I forget what subprocess.Popen does when it goes out of scope and gets garbage collections. I'm guessing it sends a kill signal to the process? Hopefully it doesn't wait gracefully, otherwise we may have a starvation problem with these process pipes...

Comment 8

[Jonas Finnemann Jensen (:jonasfj)]

Comment on attachment 8809612 [details]
Bug 1316446 - Improve mach taskcluster-load-image

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

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 8809612 [details]
Bug 1316446 - Improve mach taskcluster-load-image

https://reviewboard.mozilla.org/r/92160/#review92194

::: taskcluster/taskgraph/docker.py:184
(Diff revision 3)
> +        if curl.wait() != 0:
> +            trykill(zstd)
> +            trykill(docker)
> +            raise Exception('failed to download from url: {}'.format(url))
> +        if zstd.wait() != 0:
> +            trykill(docker)
> +            raise Exception('zstd decompression failed')
> +        docker.stdin.close()
> +        if docker.wait() != 0:
> +            raise Exception('loading into docker failed')

You need to guard access to `curl`, `zstd`, and `docker` because they may be `None` and raise `AttributeError`. Simple `if X:` blocks will do.

Comment 10

[Jonas Finnemann Jensen (:jonasfj)]

Comment on attachment 8809612 [details]
Bug 1316446 - Improve mach taskcluster-load-image

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

Comment 11

[Gregory Szorc [:gps]]

Comment on attachment 8809612 [details]
Bug 1316446 - Improve mach taskcluster-load-image

https://reviewboard.mozilla.org/r/92160/#review93216

Niiiice. Can't wait to see what improvements this has on image load performance. Although, I wouldn't be surprised if Docker is doing some buffering of its own. At least we did all we can.

Comment 12

[Jonas Finnemann Jensen (:jonasfj)]

If someone would ship this through autoland that would awesome :)

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b660931ce41d
Improve mach taskcluster-load-image r=gps

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b660931ce41d