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)
Taskcluster
Services
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla45
People
(Reporter: garndt, Assigned: garndt)
Details
Attachments
(1 file)
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.
Assignee | ||
Comment 1•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=643882a6bed4
Assignee | ||
Comment 2•9 years ago
|
||
Bug 1226413 - Allow task images to be built upon push r=wcosta
Attachment #8689791 -
Flags: review?(wcosta)
Assignee | ||
Comment 3•9 years ago
|
||
(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 4•9 years ago
|
||
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)
Assignee | ||
Comment 5•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9fd779d8c2d1
Assignee | ||
Comment 6•9 years ago
|
||
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.
Assignee | ||
Comment 7•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f691f17384af
Assignee | ||
Comment 8•9 years ago
|
||
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)
Assignee | ||
Comment 9•9 years ago
|
||
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/
Assignee | ||
Comment 10•9 years ago
|
||
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.
Assignee | ||
Comment 11•9 years ago
|
||
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.
Assignee | ||
Comment 12•9 years ago
|
||
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/
Assignee | ||
Comment 13•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d13e068ab816
Assignee | ||
Comment 14•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=821a2902864b
Assignee | ||
Comment 15•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=636c4ffc3374
Assignee | ||
Comment 16•9 years ago
|
||
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/
Assignee | ||
Comment 17•9 years ago
|
||
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.
Assignee | ||
Comment 18•9 years ago
|
||
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/
Updated•9 years ago
|
Attachment #8689791 -
Flags: review?(wcosta) → review+
Comment 19•9 years ago
|
||
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?
Assignee | ||
Comment 20•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8464b96cc2f7
Assignee | ||
Comment 21•9 years ago
|
||
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/
Assignee | ||
Comment 23•9 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/e55ddcc37c63e3683e20f4e60a5eb5f25f89b67b Bug 1226413 - Allow task images to be built upon push r=wcosta
Comment 24•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e55ddcc37c63
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Updated•5 years ago
|
Component: Integration → Services
You need to log in
before you can comment on or make changes to this bug.
Description
•