Closed Bug 1226413 Opened 9 years ago Closed 9 years ago

Images should be built per push if context directory changed

Categories

(Taskcluster :: Services, defect)

Product:
Taskcluster
Component:
Services
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla45

People

(Reporter: garndt, Assigned: garndt)

Details

Attachments

(1 file)

MozReview Request: Bug 1226413 - Allow task images to be built upon push r=wcosta
40 bytes, text/x-review-board-request
wcosta
: review+
Details 
Currently our images are built manually and pushed to a docker registry making it a very cumbersome and error prone process.  It's also hard to iterate on an image and push to try.

Context directories for an image could be hashed and images built within tasks on demand.  These images should be indexed so they can be used for other tasks as well.   This will replace our reliance on the manual process and dependency of a docker registry.
Bug 1226413 - Allow task images to be built upon push r=wcosta
Attachment #8689791 - Flags: review?(wcosta)
(In reply to Greg Arndt [:garndt] from comment #1)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=643882a6bed4

Ignore this push as not all the worker types have the new docker-worker ami.

Try pushes like these show that images will be built and used (I had special worker types for it)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bdf6d052c297


Here's a push where the image was already created and indexed, so it didn't have to build a new one:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=27f948e1be6f
Comment on attachment 8689791 [details]
MozReview Request: Bug 1226413 - Allow task images to be built upon push r=wcosta

https://reviewboard.mozilla.org/r/25727/#review23255

A bunch of nits, but lgtm

::: testing/docker/README.md:8
(Diff revision 1)
> -Each folder describes a single docker image.
> +Each folder describes a single docker image.  We have two types of image sthat can be defined:

typo: s/image sthat/images that/

::: testing/docker/README.md:22
(Diff revision 1)
> +a given namespace, and used in a task by referencing the task ID.

Do you mean "used in another task?"

::: testing/docker/README.md:34
(Diff revision 1)
> +that were built from the same context.

I didn't get this paragraph

::: testing/docker/README.md:70
(Diff revision 1)
> +* docker.images.v1.{project}.{image_name}.pushdate.{year}.{month}-{day}-{pushtime}

Any special reason to not make month day and push time in their own namespaces?

::: testing/docker/build.sh:59
(Diff revision 1)
> +    test -n "$version" || usage_err "$folder_ver is empty aborting..."

Maybe also abort if the version file contains a character that's neither a dot not a number.

::: testing/docker/build.sh:91
(Diff revision 1)
> +    echo "to running 'docker push'."

nit: use "************************" before and after the message to try catching user's attention.

::: testing/docker/image_builder/bin/build_image.sh:3
(Diff revision 1)
> +set -x -e -v

nit, put a comment about what this line does. Rule of thumb for dark bash stuff.

::: testing/docker/image_builder/bin/build_image.sh:13
(Diff revision 1)
> +    CONTEXT_PATH=/home/worker/workspace/src/$CONTEXT_PATH

nit: add a "test -n $CONTEXT_PATH" in the else branch.

::: testing/docker/image_builder/bin/build_image.sh:16
(Diff revision 1)
> +docker build -t $PROJECT:$HASH $CONTEXT_PATH

nit: "test -n $PROJECT" and "test -n $CONTEXT_PATH"

::: testing/taskcluster/mach_commands.py:366
(Diff revision 1)
> -            for env in TREEHERDER_ROUTES:
> +            for env in routes_transform.TREEHERDER_ROUTES:

I guess TREEHERDER_ROUTES is a dict, so should be better using TREEHERDER_ROUTES.itervalues().

::: testing/taskcluster/taskcluster_graph/image_builder.py:71
(Diff revision 1)
> +    task ID will be used.  The reasoning behind his is that eventually everything ends

nit s/his/this/

::: testing/taskcluster/taskcluster_graph/image_builder.py:72
(Diff revision 1)
> +    up on mozilla-central at some point if if most tasks use this as a common image

nit: s/if//

::: testing/taskcluster/taskcluster_graph/image_builder.py:84
(Diff revision 1)
> +                raise Exception('Artifact does not exist')

Why not just return None here?

::: testing/taskcluster/taskcluster_graph/image_builder.py:86
(Diff revision 1)
> +            return task['taskId']

This could be just "return task['taskId'] if artifact_exists else None

::: testing/taskcluster/taskcluster_graph/image_builder.py:87
(Diff revision 1)
> +        except:

This is dangerous, if someone makes a mistake causing a parser error in the code, the function will silently return None.

::: testing/taskcluster/taskcluster_graph/image_builder.py:154
(Diff revision 1)
> +    for name, details in seen_images.items():

nit: s/items/iteritems/

::: testing/taskcluster/tasks/decision/try.yml:92
(Diff revision 1)
> +            # graph.json and context directories for now, so nothign that needs

typo: s/nothign/nothing/
Attachment #8689791 - Flags: review?(wcosta)
https://reviewboard.mozilla.org/r/25727/#review23351

::: testing/docker/README.md:22
(Diff revision 1)
> +a given namespace, and used in a task by referencing the task ID.

Yup. Fixed.

::: testing/docker/README.md:34
(Diff revision 1)
> +that were built from the same context.

Fixed, thanks for helping me clarify it.

::: testing/docker/README.md:70
(Diff revision 1)
> +* docker.images.v1.{project}.{image_name}.pushdate.{year}.{month}-{day}-{pushtime}

I originally had it this way, but then Jonas convinced me that because these images wouldn't be built often, that having buckets for month and day probably would just end up with one image there.

::: testing/docker/image_builder/bin/build_image.sh:13
(Diff revision 1)
> +    CONTEXT_PATH=/home/worker/workspace/src/$CONTEXT_PATH

Ah, good call, thanks.

::: testing/taskcluster/taskcluster_graph/image_builder.py:84
(Diff revision 1)
> +                raise Exception('Artifact does not exist')

Ah, I can't recall anymore why I was throwing an exception, so I'll make the change unless I remember why.

::: testing/taskcluster/taskcluster_graph/image_builder.py:154
(Diff revision 1)
> +    for name, details in seen_images.items():

As discused on IRC, because seen_images is small, and iteritems does not exist in python3, we'll keep using items()

Made changes, thanks so much for the helpful comments.  I'm going to push to try and see how it goes.
Comment on attachment 8689791 [details]
MozReview Request: Bug 1226413 - Allow task images to be built upon push r=wcosta

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25727/diff/1-2/
Attachment #8689791 - Flags: review?(wcosta)
Comment on attachment 8689791 [details]
MozReview Request: Bug 1226413 - Allow task images to be built upon push r=wcosta

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25727/diff/2-3/
Here is a push to build an image that doesn't exist for try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9fd779d8c2d1

The image was built and the b2g desktop tasks are using it.

Here is a push again to make sure that an image is NOT built since it was already indexed under the previous push.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f691f17384af

You can look at the b2g desktop builds and see that they are using the same task ID for the image as the other jobs in the previous try push.  Also, I included some tasks that use a docker image rather than task image just for the sake of it.
https://reviewboard.mozilla.org/r/25727/#review23255

> Maybe also abort if the version file contains a character that's neither a dot not a number.

Docker tags can be alphanumeric so restricting it only to numbers might not be ideal.  Going to leave this for now unless there is a reason we should fix this now.
Comment on attachment 8689791 [details]
MozReview Request: Bug 1226413 - Allow task images to be built upon push r=wcosta

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25727/diff/3-4/
Comment on attachment 8689791 [details]
MozReview Request: Bug 1226413 - Allow task images to be built upon push r=wcosta

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25727/diff/4-5/
Here's a (In reply to Greg Arndt [:garndt] from comment #15)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=636c4ffc3374

This push contains the changes to the image builder image requested in the previous reviews.
Comment on attachment 8689791 [details]
MozReview Request: Bug 1226413 - Allow task images to be built upon push r=wcosta

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25727/diff/5-6/
Attachment #8689791 - Flags: review?(wcosta) → review+
Comment on attachment 8689791 [details]
MozReview Request: Bug 1226413 - Allow task images to be built upon push r=wcosta

https://reviewboard.mozilla.org/r/25727/#review23445

Two nits, but lgtm

::: testing/docker/image_builder/bin/build_image.sh:28
(Diff revision 6)
> +    test -d $CONTEXT_PATH || raise_error "Context Path $CONTEXT_PATH does not exist."

If $CONTEXT_PATH is empty, this will succeed, is this behavior right?

::: testing/docker/image_builder/bin/build_image.sh:31
(Diff revision 6)
> +docker build -t $PROJECT:$HASH $CONTEXT_PATH

What if $HASH is empty?
Comment on attachment 8689791 [details]
MozReview Request: Bug 1226413 - Allow task images to be built upon push r=wcosta

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25727/diff/6-7/
https://hg.mozilla.org/mozilla-central/rev/e55ddcc37c63
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Component: Integration → Services
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: