Closed Bug 1316446 Opened 3 years ago Closed 3 years ago

Improve robustness of ./mach taskcluster-load-image

Categories

(Taskcluster Graveyard :: Docker Images, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla53

People

(Reporter: jonasfj, Assigned: jonasfj)

Details

Attachments

(1 file)

./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)
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: nobody → jopsen
Status: NEW → ASSIGNED
Attachment #8809612 - Flags: review?(gps)
@gps,
This adds the nice streaming, granted the error handling isn't as nice.
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 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 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 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 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+
If someone would ship this through autoland that would awesome :)
Keywords: checkin-needed
Pushed by gszorc@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b660931ce41d
Improve mach taskcluster-load-image r=gps
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b660931ce41d
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Product: Taskcluster → Taskcluster Graveyard
You need to log in before you can comment on or make changes to this bug.