Closed Bug 1289249 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox Build System :: Task Configuration, task)

Product:
Firefox Build System
Component:
Task Configuration
Type:
task
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla52

People

(Reporter: gps, Assigned: gps)

References

(Blocks 2 open bugs)

Details

Attachments

(13 files, 10 obsolete files)

Bug 1289249 - Use vendored tooltool in desktop-build image;
58 bytes, text/x-review-board-request
dustin
: review+
Details
Bug 1289249 - Extract tooltool_fetch to its own shell script;
58 bytes, text/x-review-board-request
dustin
: review+
Details
Bug 1289249 - Install Mercurial 3.9.1 in desktop-build;
58 bytes, text/x-review-board-request
dustin
: review+
Details
Bug 1289249 - Add volumes for Mercurial checkouts;
58 bytes, text/x-review-board-request
dustin
: review+
Details
Bug 1289249 - Add run-task to desktop-build image;
58 bytes, text/x-review-board-request
dustin
: review+
Details
Bug 1289249 - Make build-linux.sh executable;
58 bytes, text/x-review-board-request
dustin
: review+
Details
Bug 1289249 - Use run-task in spidermonkey tasks;
58 bytes, text/x-review-board-request
gps
: review+
Details
Bug 1289249 - Use run-task for mozharness jobs on docker-worker;
58 bytes, text/x-review-board-request
dustin
: review+
Details
Bug 1289249 - Set permissions of parent directory during --chown-recursive;
58 bytes, text/x-review-board-request
dustin
: review+
Details
Bug 1289249 - Make version control interaction generic;
58 bytes, text/x-review-board-request
dustin
: review+
Details
Bug 1289249 - Add argument to perform build/tools checkout;
58 bytes, text/x-review-board-request
dustin
: review+
Details
Bug 1289249 - Ensure cwd is /home/worker;
58 bytes, text/x-review-board-request
dustin
: review+
Details
Bug 1289249 - Use vendored tooltool.py in SpiderMonkey tasks;
58 bytes, text/x-review-board-request
dustin
: review+
Details 
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.
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)
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/
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/
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/
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/
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+
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 :/
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.
Blocks: thunder-try
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
Attachment #8774585 - Attachment is obsolete: true
Attachment #8774586 - Attachment is obsolete: true
Attachment #8774587 - Attachment is obsolete: true
Attachment #8774588 - Attachment is obsolete: true
Attachment #8774589 - Attachment is obsolete: true
Attachment #8774590 - Attachment is obsolete: true
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 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 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 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 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 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 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 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 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+
CC sfink as a courtesy since I'm touching spidermonkey automation.
Attachment #8796014 - Attachment is obsolete: true
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)
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)
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 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 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 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 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 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 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 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 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.
Attachment #8796019 - Attachment is obsolete: true
Attachment #8796020 - Attachment is obsolete: true
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
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.
Attachment #8796715 - Attachment is obsolete: true
Attachment #8796715 - Flags: review?(garndt)
Attachment #8796017 - Flags: review?(dustin) → review+
Blocks: 1306768
Depends on: 1307030
Blocks: 1308950
Product: TaskCluster → Firefox Build System
You need to log in before you can comment on or make changes to this bug.