1459737 - run-task refactoring

Status: RESOLVED FIXED

(Firefox Build System :: Task Configuration, task)

Description

[Gregory Szorc [:gps]]

Filing a bug to hold code reviews for patches I've already written.

Comment 1

[Gregory Szorc [:gps]]

Attachment: Bug 1459737 - Move run-task into taskcluster/scripts;

In preparation for making it usable on Windows, after which point
having it in a directory with "docker" in it doesn't make much sense.

Review commit: https://reviewboard.mozilla.org/r/242194/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/242194/

Comment 2

[Gregory Szorc [:gps]]

Attachment: Bug 1459737 - Make run-task more runnable on non-POSIX platforms;

I want to make run-task work on Windows. The script is currently very
POSIX oriented, as it assumes the existence of the grp and pwd modules,
that user IDs are numeric, and that system calls like setresgid() and
setresuid() are available, etc.

This commit starts to make some of the POSIX-centric code conditional
on running on POSIX.

Code for uid/gid extraction has been moved to its own function. Some
error messages were tweaked slightly as part of the move. Otherwise,
the changes should be pretty straightforward.

There are still other parts of this file that won't work on e.g.
Windows. But this gets us a big step closer.

Review commit: https://reviewboard.mozilla.org/r/242196/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/242196/

Comment 3

[Gregory Szorc [:gps]]

Attachment: Bug 1459737 - Teach run-task where to find Mercurial on Windows;

Because we can't rely on it being in %PATH%.

Review commit: https://reviewboard.mozilla.org/r/242198/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/242198/

Comment 4

[Gregory Szorc [:gps]]

Attachment: Bug 1459737 - Move closure to module-level function;

Review commit: https://reviewboard.mozilla.org/r/242200/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/242200/

Comment 5

[Gregory Szorc [:gps]]

Attachment: Bug 1459737 - Require to run on root on POSIX platforms;

The code for cache and volume normalization makes assumptions that uid
and gid are defined. They are not defined if not running as root and
Python would crash in these code paths.

So, presumably this means that all tasks using run-task are running as
root. Let's codify that requirement.

This requirement is arbitrary. But let's not scope bloat run-task to
support scenarios until we need them.

Review commit: https://reviewboard.mozilla.org/r/242202/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/242202/

Comment 6

[Gregory Szorc [:gps]]

Attachment: Bug 1459737 - Move cache configuration to standalone function;

main() is quite long. And the control flow will become more complicated
as we support Windows.

Let's move the bulk of the cache configuration code into a standalone
function so main() is less cluttered.

Review commit: https://reviewboard.mozilla.org/r/242204/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/242204/

Comment 7

[Gregory Szorc [:gps]]

Attachment: Bug 1459737 - Move volume configuration to standalone function;

Do to volumes what we did to caches.

Review commit: https://reviewboard.mozilla.org/r/242206/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/242206/

Comment 8

[Gregory Szorc [:gps]]

Attachment: Bug 1459737 - Assert that volumes aren't used on Windows;

Volumes are a docker-worker concept. They shouldn't be encountered on
Windows, which uses generic-worker.

Review commit: https://reviewboard.mozilla.org/r/242208/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/242208/

Comment 9

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8973832 [details]
Bug 1459737 - Move run-task into taskcluster/scripts;

https://reviewboard.mozilla.org/r/242194/#review248232

Comment 10

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8973833 [details]
Bug 1459737 - Make run-task more runnable on non-POSIX platforms;

https://reviewboard.mozilla.org/r/242196/#review248236

Comment 11

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8973834 [details]
Bug 1459737 - Teach run-task where to find Mercurial on Windows;

https://reviewboard.mozilla.org/r/242198/#review248238

Comment 12

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8973835 [details]
Bug 1459737 - Move closure to module-level function;

https://reviewboard.mozilla.org/r/242200/#review248242

Comment 13

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8973836 [details]
Bug 1459737 - Require to run on root on POSIX platforms;

https://reviewboard.mozilla.org/r/242202/#review248246

Comment 14

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8973837 [details]
Bug 1459737 - Move cache configuration to standalone function;

https://reviewboard.mozilla.org/r/242204/#review248268

Comment 15

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8973838 [details]
Bug 1459737 - Move volume configuration to standalone function;

https://reviewboard.mozilla.org/r/242206/#review248270

::: taskcluster/scripts/run-task:350
(Diff revision 1)
> +        sys.exit(1)
> +
> +    # The volume is almost certainly owned by root:root. Chown it so it
> +    # is writable.
> +
> +    if running_as_root:

aren't we always running as root now?

Comment 16

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8973839 [details]
Bug 1459737 - Assert that volumes aren't used on Windows;

https://reviewboard.mozilla.org/r/242208/#review248272

Comment 17

[Gregory Szorc [:gps]]

Comment on attachment 8973838 [details]
Bug 1459737 - Move volume configuration to standalone function;

https://reviewboard.mozilla.org/r/242206/#review248270

> aren't we always running as root now?

Yes. Old habits die hard, I suppose. I'll submit a minor fixup.

Comment 18

[Gregory Szorc [:gps]]

Comment on attachment 8973838 [details]
Bug 1459737 - Move volume configuration to standalone function;

https://reviewboard.mozilla.org/r/242206/#review248270

> Yes. Old habits die hard, I suppose. I'll submit a minor fixup.

Actually, since this patch was about moving code, I'd rather not do the refactor here. It can be done in a follow-up.

Also, I have a hunch that we'll need to restore !root for run-task on macOS. So I am hold off changing things until we have run-task running there...

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cf8fdc7a19db
Move run-task into taskcluster/scripts; r=dustin
https://hg.mozilla.org/integration/autoland/rev/acfbf6d8eb02
Make run-task more runnable on non-POSIX platforms; r=dustin
https://hg.mozilla.org/integration/autoland/rev/d1e1c418a26e
Teach run-task where to find Mercurial on Windows; r=dustin
https://hg.mozilla.org/integration/autoland/rev/5be219428708
Move closure to module-level function; r=dustin
https://hg.mozilla.org/integration/autoland/rev/327defec799f
Require to run on root on POSIX platforms; r=dustin
https://hg.mozilla.org/integration/autoland/rev/ea2e3306c10f
Move cache configuration to standalone function; r=dustin
https://hg.mozilla.org/integration/autoland/rev/4dd455fe0899
Move volume configuration to standalone function; r=dustin
https://hg.mozilla.org/integration/autoland/rev/3cd1fcae55de
Assert that volumes aren't used on Windows; r=dustin

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/97fd777d726a
Add missing package dependencies to google-play-strings Dockerfile; r=nalexander CLOSED TREE

Comment 21

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/cf8fdc7a19db
https://hg.mozilla.org/mozilla-central/rev/acfbf6d8eb02
https://hg.mozilla.org/mozilla-central/rev/d1e1c418a26e
https://hg.mozilla.org/mozilla-central/rev/5be219428708
https://hg.mozilla.org/mozilla-central/rev/327defec799f
https://hg.mozilla.org/mozilla-central/rev/ea2e3306c10f
https://hg.mozilla.org/mozilla-central/rev/4dd455fe0899
https://hg.mozilla.org/mozilla-central/rev/3cd1fcae55de
https://hg.mozilla.org/mozilla-central/rev/97fd777d726a

Comment 22

[Johan Lorenzo [:jlorenzo] - PTO - Back October 30th]

(In reply to Pulsebot from comment #20)
> Pushed by [e-mail]:
> https://hg.mozilla.org/integration/autoland/rev/97fd777d726a
> Add missing package dependencies to google-play-strings Dockerfile;
> r=nalexander CLOSED TREE

Landed on beta in order to uplift bug 1459980 at https://hg.mozilla.org/releases/mozilla-beta/rev/52f364abcc85336cf3e4fd8f928d570cc4f409a3