Improve robustness of ./mach taskcluster-load-image

RESOLVED FIXED in mozilla53

Status

Taskcluster
Docker Images
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jonasfj, Assigned: jonasfj)

Tracking

unspecified
mozilla53

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

a year ago
./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)
(Assignee)

Comment 1

a year ago
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.
Component: Task Configuration → Docker Images
(Assignee)

Updated

a year ago
Assignee: nobody → jopsen
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8809612 - Flags: review?(gps)
(Assignee)

Comment 3

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

Comment 4

a year ago
mozreview-review
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.
Attachment #8809612 - Flags: review?(gps) → review-
Comment hidden (mozreview-request)
(Assignee)

Comment 6

a year ago
mozreview-review-reply
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

a year ago
mozreview-review
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...
Attachment #8809612 - Flags: review?(gps) → review-
Comment hidden (mozreview-request)

Comment 9

a year ago
mozreview-review
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.
Attachment #8809612 - Flags: review?(gps) → review-
Comment hidden (mozreview-request)

Comment 11

a year ago
mozreview-review
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.
Attachment #8809612 - Flags: review?(gps) → review+
(Assignee)

Comment 12

a year ago
If someone would ship this through autoland that would awesome :)
Keywords: checkin-needed

Comment 13

a year ago
Pushed by gszorc@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b660931ce41d
Improve mach taskcluster-load-image r=gps
Keywords: checkin-needed

Comment 14

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b660931ce41d
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.