Overhaul desktop-build image/task interaction with hg.mozilla.org

RESOLVED FIXED in mozilla52

Status

RESOLVED FIXED
2 years ago
8 months ago

People

(Reporter: gps, Assigned: gps)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla52
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(13 attachments, 10 obsolete attachments)

58 bytes, text/x-review-board-request
dustin
: review+
Details
58 bytes, text/x-review-board-request
dustin
: review+
Details
58 bytes, text/x-review-board-request
dustin
: review+
Details
58 bytes, text/x-review-board-request
dustin
: review+
Details
58 bytes, text/x-review-board-request
dustin
: review+
Details
58 bytes, text/x-review-board-request
dustin
: review+
Details
58 bytes, text/x-review-board-request
gps
: review+
Details
58 bytes, text/x-review-board-request
dustin
: review+
Details
58 bytes, text/x-review-board-request
dustin
: review+
Details
58 bytes, text/x-review-board-request
dustin
: review+
Details
58 bytes, text/x-review-board-request
dustin
: review+
Details
58 bytes, text/x-review-board-request
dustin
: review+
Details
58 bytes, text/x-review-board-request
dustin
: review+
Details
(Assignee)

Description

2 years ago
This is follow-up work from bug 1247168. We want the desktop-build image/task to use robustcheckout, stop using tc-vcs, using caches appropriately, etc.
(Assignee)

Comment 1

2 years ago
Created attachment 8774585 [details]
Bug 1289249 - Remove REGISTRY and VERSION files from android-gradle-build;

AFAICT these files were cargo culted: we don't need these files because
the Docker images is built as part of the task graph.

Review commit: https://reviewboard.mozilla.org/r/67038/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67038/
Attachment #8774585 - Flags: review?(dustin)
Attachment #8774586 - Flags: review?(dustin)
Attachment #8774587 - Flags: review?(dustin)
Attachment #8774588 - Flags: review?(dustin)
Attachment #8774589 - Flags: review?(dustin)
Attachment #8774590 - Flags: review?(dustin)
(Assignee)

Comment 2

2 years ago
Created attachment 8774586 [details]
Bug 1289249 - Unify desktop-build-upd versions;

After this commit, all consumers of the desktop-build-upd image are
using the same version.

Review commit: https://reviewboard.mozilla.org/r/67040/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67040/
(Assignee)

Comment 3

2 years ago
Created attachment 8774587 [details]
Bug 1289249 - Remove centos6-build-upd image;

This image was introduced in bug 1203158. The intent of the image
was to minimize layer sizes so people with slower internet connections
would only have to download a minimal layer update whenever package
versions were bumped.

Fast forward a year and I don't think we're actually using images this
way. We don't really have people downloading images in bulk to run
them locally. Instead, I think people are more likely to obtain a
one-click loaner to obtain an instance running the image. I view the
centos6-build-upd image as avoidable complexity standing in the way
of future refactoring of various build images. So, this commit
eliminates it.

Review commit: https://reviewboard.mozilla.org/r/67042/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67042/
(Assignee)

Comment 4

2 years ago
Created attachment 8774588 [details]
Bug 1289249 - Move environment variable declaration to build.yml;

In preparation for eliminating Docker image inheritance for the
desktop-build image, we move declaring of environment variables
from the base image's Dockerfile to build.yml, which is inherited at
run-time for every task using the desktop-build image.

Review commit: https://reviewboard.mozilla.org/r/67044/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67044/
(Assignee)

Comment 5

2 years ago
Created attachment 8774589 [details]
Bug 1289249 - Inline hgrc into system-setup.sh;

Continuing to minimize centos6-build's Dockerfile so we can eliminate
the image.

Review commit: https://reviewboard.mozilla.org/r/67046/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67046/
(Assignee)

Comment 6

2 years ago
Created attachment 8774590 [details]
Bug 1289249 - Remove centos6-build image;

The system-setup.sh script has been moved to the "desktop-build.sh"
recipe and included in each of the Dockerfiles that referenced it.
The remaining lines from centos6-build's Dockerfile have
essentially been inlined into the Dockerfiles that previously inherited
from centos6-build.

As of this change, we no longer require someone with TaskCluster
privileges to upload a base image to the Docker Registry: it is
possible to make changes to the environment used to build Firefox
in-tree. You can easily experiment with new build environments by
pushing to Try.

Review commit: https://reviewboard.mozilla.org/r/67048/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67048/
Comment on attachment 8774585 [details]
Bug 1289249 - Remove REGISTRY and VERSION files from android-gradle-build;

https://reviewboard.mozilla.org/r/67038/#review64540
Attachment #8774585 - Flags: review?(dustin) → review+
Attachment #8774586 - Flags: review?(dustin) → review+
Comment on attachment 8774587 [details]
Bug 1289249 - Remove centos6-build-upd image;

https://reviewboard.mozilla.org/r/67042/#review64544
Attachment #8774587 - Flags: review?(dustin) → review+
Attachment #8774588 - Flags: review?(dustin) → review-
Comment on attachment 8774588 [details]
Bug 1289249 - Move environment variable declaration to build.yml;

https://reviewboard.mozilla.org/r/67044/#review64538

::: taskcluster/ci/legacy/tasks/build.yml:42
(Diff revision 1)
> +      # process. These are taken from the GNU su manual.
> +      LOGNAME: 'worker'
> +      HOME: '/home/worker'
> +      HOSTNAME: 'taskcluster-worker'
> +      SHELL: '/bin/bash'
> +      USER: 'worker'

It feels very wrong to have this in the task definition.  The effect is correct, but this is essentially machine configuration.

I would feel more comfortable duplicating the removed lines in all of the Dockerfiles, even at he cost of not being very DRY.

Could we set these variables in the shell scripts run within these scripts?  Possibly by sourcing some #%include'd script?
Comment on attachment 8774589 [details]
Bug 1289249 - Inline hgrc into system-setup.sh;

https://reviewboard.mozilla.org/r/67046/#review64548
Attachment #8774589 - Flags: review?(dustin) → review+
(Assignee)

Comment 13

2 years ago
https://reviewboard.mozilla.org/r/67048/#review64666

Thanks for the reviews. Unfortunately, I'm going to hold off landing this series until we have a better understanding of image determinism. As it stands, pushing this series will likely break Valgrind due to newer system packages being pulled in :/
(Assignee)

Comment 14

2 years ago
https://reviewboard.mozilla.org/r/67044/#review64538

> It feels very wrong to have this in the task definition.  The effect is correct, but this is essentially machine configuration.
> 
> I would feel more comfortable duplicating the removed lines in all of the Dockerfiles, even at he cost of not being very DRY.
> 
> Could we set these variables in the shell scripts run within these scripts?  Possibly by sourcing some #%include'd script?

Yes, this patch made me feel a bit dirty as well.

I think sourcing from a #%include'd script is a good solution.

Updated

2 years ago
Blocks: 1278680
(Assignee)

Comment 15

2 years ago
I can do this without needing the base image to be updated. It will result in some code duplication. But that's far easier than consolidating images.
No longer depends on: 1289812
(Assignee)

Updated

2 years ago
Attachment #8774585 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8774586 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8774587 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8774588 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8774589 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8774590 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 27

2 years ago
I'm still ironing out the conversion of the tasks to run-task on Try. I should have a complete series ready for review by mid-day Thursday.

I may have to make some additional changes to accommodate uid normalization. So if you want to hold off on review, I don't blame you.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 37

2 years ago
mozreview-review
Comment on attachment 8796010 [details]
Bug 1289249 - Use vendored tooltool in desktop-build image;

https://reviewboard.mozilla.org/r/81996/#review80716
Attachment #8796010 - Flags: review?(dustin) → review+

Comment 38

2 years ago
mozreview-review
Comment on attachment 8796011 [details]
Bug 1289249 - Extract tooltool_fetch to its own shell script;

https://reviewboard.mozilla.org/r/81998/#review80718
Attachment #8796011 - Flags: review?(dustin) → review+

Comment 39

2 years ago
mozreview-review
Comment on attachment 8796012 [details]
Bug 1289249 - Install Mercurial 3.9.1 in desktop-build;

https://reviewboard.mozilla.org/r/82000/#review80720
Attachment #8796012 - Flags: review?(dustin) → review+

Comment 40

2 years ago
mozreview-review
Comment on attachment 8796013 [details]
Bug 1289249 - Add volumes for Mercurial checkouts;

https://reviewboard.mozilla.org/r/82002/#review80722
Attachment #8796013 - Flags: review?(dustin) → review+

Comment 41

2 years ago
mozreview-review
Comment on attachment 8796014 [details]
Bug 1289249 - Hardcode worker uid and gid;

https://reviewboard.mozilla.org/r/82004/#review80726

This is going to introduce a period of instability where we have some new running as uid 1000 and old builds running as uid 500.  If those are sharing caches, then for as long as we have the old jobs hanging around, we're going to have conflicts.

I wonder if we couldn't reduce this by changing to uid 500 instead?  I don't know why there are Ubuntu tasks running on the same workerTypes as CentOS tasks, but there certainly should be *fewer* such Ubuntu builds.

Also, what think ye about having run-tasks optionally chown things, but only if the top-level directory has the wrong uid?  In other words, assume that if /home/worker/hg-shared has the right owner, then there's no need to examine its contents.
Attachment #8796014 - Flags: review?(dustin)

Comment 42

2 years ago
mozreview-review
Comment on attachment 8796015 [details]
Bug 1289249 - Add run-task to desktop-build image;

https://reviewboard.mozilla.org/r/82006/#review80728
Attachment #8796015 - Flags: review?(dustin) → review+

Comment 43

2 years ago
mozreview-review
Comment on attachment 8796016 [details]
Bug 1289249 - Make build-linux.sh executable;

https://reviewboard.mozilla.org/r/82008/#review80730

r+ assuming this is just a chmod.  it's making mozreview barf :(
Attachment #8796016 - Flags: review?(dustin) → review+

Comment 44

2 years ago
mozreview-review
Comment on attachment 8796017 [details]
Bug 1289249 - Use run-task in spidermonkey tasks;

https://reviewboard.mozilla.org/r/82010/#review80734
Attachment #8796017 - Flags: review?(dustin) → review+
(Assignee)

Comment 45

2 years ago
CC sfink as a courtesy since I'm touching spidermonkey automation.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8796014 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 85

2 years ago
Comment on attachment 8796017 [details]
Bug 1289249 - Use run-task in spidermonkey tasks;

Need a new review since this changed a bit.
Attachment #8796017 - Flags: review+ → review?(dustin)
(Assignee)

Comment 86

2 years ago
Comment on attachment 8796012 [details]
Bug 1289249 - Install Mercurial 3.9.1 in desktop-build;

Need a new review since this changed.
Attachment #8796012 - Flags: review+ → review?(dustin)
(Assignee)

Comment 87

2 years ago
I'm pretty confident in this series up to the last 2 commits. A try push is at https://treeherder.mozilla.org/#/jobs?repo=try&revision=d58fb76ff1b832b8fa6e5db236d5702c5085dca6.

If I get r+ on everything, I'll just discard the top 2 commits for a follow-up bug. There are some other random build-related tasks not using run-task, so a follow-up is needed anyway. I would also like to refactor where checkouts are stored on disk, since right now we need to perform a full checkout on every task. Again, it should be deferred to a follow-up.

Comment 88

2 years ago
mozreview-review
Comment on attachment 8796012 [details]
Bug 1289249 - Install Mercurial 3.9.1 in desktop-build;

https://reviewboard.mozilla.org/r/82000/#review80970
Attachment #8796012 - Flags: review?(dustin) → review+

Comment 89

2 years ago
mozreview-review
Comment on attachment 8796372 [details]
Bug 1289249 - Set permissions of parent directory during --chown-recursive;

https://reviewboard.mozilla.org/r/82252/#review80972
Attachment #8796372 - Flags: review?(dustin) → review+

Comment 90

2 years ago
mozreview-review
Comment on attachment 8796373 [details]
Bug 1289249 - Make version control interaction generic;

https://reviewboard.mozilla.org/r/82254/#review80976

::: testing/docker/recipes/run-task:226
(Diff revision 1)
>                  set_dir_permissions(os.path.join(root, d), uid, gid)
>  
>              for f in files:
>                  os.chown(os.path.join(root, f), uid, gid)
>  
> -    checkout = args.vcs_checkout
> +    def prepare_checkout_dir(checkout):

why is this a nested function (not that I'm opposed .. just seems superfluous here)
Attachment #8796373 - Flags: review?(dustin) → review+

Comment 91

2 years ago
mozreview-review
Comment on attachment 8796374 [details]
Bug 1289249 - Add argument to perform build/tools checkout;

https://reviewboard.mozilla.org/r/82256/#review80980

::: testing/docker/recipes/run-task:244
(Diff revision 2)
>          # And that it is owned by the appropriate user/group.
>          os.chown('/home/worker/hg-shared', uid, gid)
>          os.chown(os.path.dirname(checkout), uid, gid)
>  
>      prepare_checkout_dir(args.vcs_checkout)
> +    prepare_checkout_dir(args.tools_checkout)

Ah, that's why!

::: testing/docker/recipes/run-task:284
(Diff revision 2)
>  
> +    if args.tools_checkout:
> +        vcs_checkout(b'https://hg.mozilla.org/build/tools',
> +                     args.tools_checkout,
> +                     # Always check out the latest commit on default branch.
> +                     # This is non-deterministic!

So much fun!
Attachment #8796374 - Flags: review?(dustin) → review+

Comment 92

2 years ago
mozreview-review
Comment on attachment 8796375 [details]
Bug 1289249 - Ensure cwd is /home/worker;

https://reviewboard.mozilla.org/r/82258/#review80982
Attachment #8796375 - Flags: review?(dustin) → review+

Comment 93

2 years ago
mozreview-review
Comment on attachment 8796376 [details]
Bug 1289249 - Use vendored tooltool.py in SpiderMonkey tasks;

https://reviewboard.mozilla.org/r/82260/#review80984
Attachment #8796376 - Flags: review?(dustin) → review+

Comment 94

2 years ago
mozreview-review
Comment on attachment 8796018 [details]
Bug 1289249 - Use run-task for mozharness jobs on docker-worker;

https://reviewboard.mozilla.org/r/82012/#review80988

::: taskcluster/taskgraph/transforms/job/mozharness.py:84
(Diff revision 6)
>      run = job['run']
>  
>      worker = taskdesc['worker']
>      worker['implementation'] = job['worker']['implementation']
>  
>      # running via mozharness assumes desktop-build (which contains build.sh)

You're correct about this assumption :)
Attachment #8796018 - Flags: review?(dustin) → review+

Comment 95

2 years ago
mozreview-review
Comment on attachment 8796019 [details]
Bug 1289249 - Use run-task in marionette-harness task

https://reviewboard.mozilla.org/r/82014/#review80990

I'd really prefer this not use the desktop-build image, since it's a test.  This is a fine fix too, though.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8796019 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8796020 - Attachment is obsolete: true

Comment 109

2 years ago
Pushed by gszorc@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6426086c4c0a
Set permissions of parent directory during --chown-recursive; r=dustin
https://hg.mozilla.org/integration/autoland/rev/db1b85faa8b0
Make version control interaction generic; r=dustin
https://hg.mozilla.org/integration/autoland/rev/ab876459cc14
Add argument to perform build/tools checkout; r=dustin
https://hg.mozilla.org/integration/autoland/rev/a0aaa5f8ec30
Ensure cwd is /home/worker; r=dustin
https://hg.mozilla.org/integration/autoland/rev/e774ca6c841a
Use vendored tooltool in desktop-build image; r=dustin
https://hg.mozilla.org/integration/autoland/rev/2a1ef50facaa
Extract tooltool_fetch to its own shell script; r=dustin
https://hg.mozilla.org/integration/autoland/rev/fb9bc2020790
Install Mercurial 3.9.1 in desktop-build; r=dustin
https://hg.mozilla.org/integration/autoland/rev/3a51bf317438
Use vendored tooltool.py in SpiderMonkey tasks; r=dustin
https://hg.mozilla.org/integration/autoland/rev/c6bd1f367375
Add volumes for Mercurial checkouts; r=dustin
https://hg.mozilla.org/integration/autoland/rev/c650a2f6f8fc
Add run-task to desktop-build image; r=dustin
https://hg.mozilla.org/integration/autoland/rev/149da08ff1c9
Make build-linux.sh executable; r=dustin
https://hg.mozilla.org/integration/autoland/rev/f1160fac6760
Use run-task in spidermonkey tasks; r=dustin
https://hg.mozilla.org/integration/autoland/rev/5df2521786e1
Use run-task for mozharness jobs on docker-worker; r=dustin
(Assignee)

Comment 110

2 years ago
This caused "random" bustage on OS X and Android tasks in TC.

On OS X, we'd see things like:

[chown 2016-09-30T20:23:32.327781Z] recursively changing ownership of /home/worker/workspace to worker:worker
Traceback (most recent call last):
  File "/home/worker/bin/run-task", line 295, in <module>
    sys.exit(main(sys.argv[1:]))
  File "/home/worker/bin/run-task", line 226, in main
    os.chown(os.path.join(root, f), uid, gid)
OSError: [Errno 2] No such file or directory: '/home/worker/workspace/build/src/MacOSX10.7.sdk/Library/Frameworks'

On Android build tasks we'd see something similar:

[setup 2016-09-30T20:23:20.831072Z] run-task started
[chown 2016-09-30T20:23:20.835950Z] recursively changing ownership of /home/worker/workspace to worker:worker
Traceback (most recent call last):
  File "/home/worker/bin/run-task", line 295, in <module>
    sys.exit(main(sys.argv[1:]))
  File "/home/worker/bin/run-task", line 226, in main
    os.chown(os.path.join(root, f), uid, gid)
OSError: [Errno 2] No such file or directory: '/home/worker/workspace/build/src/java_home/jre/lib/audio/default.sf2'

The introduction of run-task introduced a recursive chown of various directories, including the workspace. It certainly looks like something else on the worker is removing the workspace while the task is iterating over it and chowning files. I suspect we uncovered a TaskCluster platform bug.
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8796715 - Attachment is obsolete: true
Attachment #8796715 - Flags: review?(garndt)
(Assignee)

Updated

2 years ago
Attachment #8796017 - Flags: review?(dustin) → review+
(Assignee)

Updated

2 years ago
Blocks: 1306768
Depends on: 1307030

Updated

2 years ago
Blocks: 1308950

Updated

8 months ago
Product: TaskCluster → Firefox Build System
You need to log in before you can comment on or make changes to this bug.