1391476 - uid/gid mismatch between checkouts can lead to permission failure doing checkout

Status: RESOLVED FIXED

(Firefox Build System :: Task Configuration, task)

Description

[Gregory Szorc [:gps]]

From https://public-artifacts.taskcluster.net/f4JHtpnhQJyhTsYEm4G-LQ/0/public/logs/live_backing.log:

[taskcluster 2017-08-17 21:53:49.252Z] Task ID: f4JHtpnhQJyhTsYEm4G-LQ
[taskcluster 2017-08-17 21:53:49.253Z] Worker ID: i-00638e1e6479b6c06
[taskcluster 2017-08-17 21:53:49.253Z] Worker Group: us-east-1
[taskcluster 2017-08-17 21:53:49.253Z] Worker Node Type: c4.2xlarge
[taskcluster 2017-08-17 21:53:49.253Z] Worker Type: gecko-1-b-android
[taskcluster 2017-08-17 21:53:49.253Z] Public IP: 54.91.181.90
[taskcluster 2017-08-17 21:53:49.253Z] using cache "tooltool-cache" -> /home/worker/tooltool-cache
[taskcluster 2017-08-17 21:53:49.253Z] using cache "level-1-checkouts-v1" -> /home/worker/checkouts

[taskcluster 2017-08-17 21:53:50.218Z] Downloading artifact "public/image.tar.zst" from task ID: C1rHajDgRYi2RE5ANX64aQ.
[taskcluster 2017-08-17 21:54:10.231Z] Download Progress: 10.21%
[taskcluster 2017-08-17 21:54:15.230Z] Download Progress: 33.74%
[taskcluster 2017-08-17 21:54:20.230Z] Download Progress: 57.12%
[taskcluster 2017-08-17 21:54:25.230Z] Download Progress: 80.34%
[taskcluster 2017-08-17 21:54:29.362Z] Downloaded artifact successfully.
[taskcluster 2017-08-17 21:54:29.363Z] Downloaded 449.712 mb
[taskcluster 2017-08-17 21:54:29.363Z] Decompressing downloaded image
[taskcluster 2017-08-17 21:54:31.109Z] Loading docker image from downloaded archive.
[taskcluster 2017-08-17 21:55:02.634Z] Image 'public/image.tar.zst' from task 'C1rHajDgRYi2RE5ANX64aQ' loaded.  Using image ID sha256:c40afb824e2f6a00b688d856378b6c059bb420c1bde04d654ad86bbc1733fb9b.
[taskcluster 2017-08-17 21:55:02.834Z] === Task Starting ===
[setup 2017-08-17T21:55:03.058941Z] run-task started
[chown 2017-08-17T21:55:03.060631Z] recursively changing ownership of /home/worker/workspace to worker:worker
[chown 2017-08-17T21:55:03.060708Z] recursively changing ownership of /home/worker/tooltool-cache to worker:worker
[setup 2017-08-17T21:55:03.065489Z] running as worker:worker
[vcs 2017-08-17T21:55:03.065540Z] executing ['hg', 'robustcheckout', '--sharebase', '/home/worker/checkouts/hg-store', '--purge', '--upstream', 'https://hg.mozilla.org/mozilla-unified', '--revision', '5b394c411b250de133a11512ce97efce97bc045b', 'https://hg.mozilla.org/try', '/home/worker/workspace/build/src']
[vcs 2017-08-17T21:55:03.114611Z] ensuring https://hg.mozilla.org/try@5b394c411b250de133a11512ce97efce97bc045b is available at /home/worker/workspace/build/src
[vcs 2017-08-17T21:55:03.114763Z] (cloning from upstream repo https://hg.mozilla.org/mozilla-unified)
[vcs 2017-08-17T21:55:03.888652Z] (sharing from existing pooled repository 8ba995b74e18334ab3707f27e9eb8f4e37ba3d29)
[vcs 2017-08-17T21:55:03.889747Z] not trusting file /home/worker/checkouts/hg-store/8ba995b74e18334ab3707f27e9eb8f4e37ba3d29/.hg/hgrc from untrusted user 500, group 500
[vcs 2017-08-17T21:55:03.896502Z] abort: could not lock repository /home/worker/workspace/build/src: Permission denied
[taskcluster 2017-08-17 21:55:04.077Z] === Task Finished ===
[taskcluster 2017-08-17 21:55:04.146Z] Artifact "public/build" not found at "/home/worker/artifacts/"
[taskcluster 2017-08-17 21:55:04.209Z] Artifact "private/android-sdk" not found at "/home/worker/private/android-sdk"
[taskcluster 2017-08-17 21:55:04.309Z] Artifact "private/java_home" not found at "/home/worker/private/java_home"
[taskcluster 2017-08-17 21:55:04.646Z] Unsuccessful task run with exit code: 255 completed in 75.395 seconds

I reckon this was caused by /home/worker/checkouts being owned by a different uid/gid than what the task ran as. This can happen if 2 tasks with different uid/gid run on the same worker.

Ideally we normalize our uid/gid to 1000:1000 to avoid ownership mismatch. But it can happen. Especially on Try. We should fix it by doing a recursive chown of the version control share base.

Comment 1

[Gregory Szorc [:gps]]

Attachment: Bug 1391476 - Extract permission setting to own functions;

This will make it easier to reuse the functionality in a new
consumer.

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

Comment 2

[Gregory Szorc [:gps]]

Attachment: Bug 1391476 - Ensure version control repository have proper permissions;

Before, we only set the owner of the root directory of the repo. If
subsequent tasks on the same worker used a different uid/gid, this
could lead to filesystem permissions problems.

We recursively chown all version control files as a mitigation against
permissions failures.

This will add overhead to run-task. But it is necessary to eliminate
permissions problems, especially on Try.

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

Comment 3

[Gregory Szorc [:gps]]

Comment on attachment 8898540 [details]
Bug 1391476 - Ensure version control repository have proper permissions;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169906/diff/1-2/

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 8898540 [details]
Bug 1391476 - Ensure version control repository have proper permissions;

This is still buggy in Try.

Comment 5

[Gregory Szorc [:gps]]

Attachment: Bug 1391476 - Inline list append;

Static analysis in my editor was complaining about the old pattern.
Why not fix it while I'm here.

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

Comment 6

[Gregory Szorc [:gps]]

Attachment: Bug 1391476 - Dedent block;

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

Comment 7

[Gregory Szorc [:gps]]

Attachment: Bug 1391476 - Don't use ~ in paths;

~ in paths is mostly a shell-ism. Let's not use it.

The real reason for this is it interfes with upcoming commits that
audit cache and volume paths.

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

Comment 8

[Gregory Szorc [:gps]]

Attachment: Bug 1391476 - Print cache info;

For forensic purposes.

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

Comment 9

[Gregory Szorc [:gps]]

Attachment: Bug 1391476 - Track whether caches should be used in untrusted environments;

Previously, we conditionally added caches to a task if the current
parameters warranted it.

In order to audit that all caches fulfill basic requirements, we need
to have unconditional knowledge of all caches.

This commit introduces an optional key on each cache entry stating
whether it should be skipped in "untrusted" environments. When we
convert a task definition to a worker payload, we filter out these
caches if necessary.

This change uncovered an inconsistency with filtering caches. In
one location we filtered on the source repo name. In others, we
filtered on the SCM level.

Setting the caches in the spidermonkey kind also changed slightly
to ensure we're not overwriting existing caches. I don't think this
has any behavior changes. But the new method is more correct.

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

Comment 10

[Gregory Szorc [:gps]]

Attachment: Bug 1391476 - Capture Docker volumes in docker-worker config;

Docker volumes are host-mounted filesystems. We typically mount
caches at their location. But not always. The reason we define
VOLUME in Dockerfiles is we're guaranteed to get a fast host
filesystem instead of AUFS when a cache isn't mounted.

In this commit, we teach the docker-worker payload builder about
the existence of Docker volumes. Docker volumes can be declared
inline in the YAML. More conveniently, we automatically parse out
VOLUME lines from corresponding in-tree Dockerfile.

We'll do useful things with this data in subsequent commits.

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

Comment 11

[Gregory Szorc [:gps]]

Attachment: Bug 1391476 - Require that all cache paths be declared as volumes;

See the inline comment for the rationale here.

Depending on the order transforms run in, it is possible to evade this
check. But I think it is comprehensive enough.

Several Dockerfile have been updated to add missing VOLUME so the check
passes.

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

Comment 12

[Gregory Szorc [:gps]]

Attachment: Bug 1391476 - Tell run-task about volumes so it can sanitize them;

We recently introduced support for telling run-task about caches so
it could sanitize them automatically. We also recently taught
docker-worker and docker-engine how to declare volumes.

Building on that work, we now pass a list of paths corresponding
to Docker volumes to run-task.

run-task now verifies volumes behave as expected. Unless the volume
paths correspond to caches, run-task verifies they are empty and chowns
them to an appropriate owner.

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

Comment 13

[Gregory Szorc [:gps]]

Attachment: Bug 1391476 - Automatically set cache/volume permissions in run-task;

run-task's --chown and --chown-recursive are only used on volumes and
caches - the only locations that aren't controlled by the Docker image
itself and thus whose permissions could be "undefined."

Previous commits have taught run-task about the locations of all caches
and volumes. Therefore, we no longer need to manually define paths to
chown. Instead, we can chown as a side-effect of the path being a
cache or a volume.

So, this commit changes run-task to chown caches and volumes
automatically. Since we no longer have a use for --chown and
--chown-recursive, those arguments are removed.

There /could/ be some paths that are caches or volumes but aren't
getting defined as such in Taskgraph. I consider this a bug in
Taskgraph and the recourse is to properly define a path as a cache or
a volume there.

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

Comment 14

[Gregory Szorc [:gps]]

Attachment: Bug 1391476 - Add UID and GID to cache parameters;

The UID and GID that a task executes under is dynamic. As a result,
caches need to be aware of the UID and GID that owns files otherwise
subsequent tasks could run into permission denied errors. This is
why `run-task --chown-recursive` exists. By recursively changing
ownership of persisted files, we ensure the current task is able
to read and write all existing files.

When you take a step back, you realize that chowning of cached
files is an expensive workaround. Yes, this results in cache hits.
But the cost is you potentially have to perform hundreds of thousands
of I/O system calls to mass chown. The ideal situation is that
UID/GID is consistent across tasks on any given cache and
potentially expensive permissions setting can be avoided. So, that's
what this commit does.

We add the task's UID and GID to run-task's requirements. When we
first see a cache, we record a UID and GID with it and chown the
empty cache directory to that UID and GID. Subsequent tasks using
this cache *must* use the same UID and GID or else run-task will
fail.

Since run-task now guarantees that all cache consumers use the same
UID and GID, we can avoid a potentially expensive recursive chown.

But there is an exception. In untrusted environments (namely Try),
we recursively chown existing caches if there is a uid/gid mismatch.
We do this because Try is a sandbox and any random task could
experiment with a non-standard uid/gid. That populated cache would
"poison" the cache for the next caller. Or vice-versa. It would be
annoying if caches were randomly poisoned due to Try pushes that
didn't realize there was a UID/GID mismatch. We could outlaw "bad"
UID and GIDs. But that makes the barrier to testing things on Try
harder. So, we go with the flow and recursively chown caches in
this scenario.

This change will shine light on all tasks using inconsistent UID
and GID values on the same cache. Bustage is anticipated.
Unfortunately, we can't easily know what will break. So it will be
one of those things where we will have to fix problems as they arise.
Fortunately, because caches are now tied to the content of run-task,
we only need to back out this change and tasks should revert to caches
without UID and GID pinning requirements and everything will work
again.

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

Comment 15

[Gregory Szorc [:gps]]

Comment on attachment 8900044 [details]
Bug 1391476 - Require that all cache paths be declared as volumes;

https://reviewboard.mozilla.org/r/171384/#review176556

::: taskcluster/docker/desktop1604-test/Dockerfile:8
(Diff revision 1)
>  
>  RUN useradd -d /home/worker -s /bin/bash -m worker
>  WORKDIR /home/worker
>  
> +VOLUME /home/worker/tooltool-cache
> +VOLUME /home/worker/.cache

This breaks Docker image building because it attempts to `rm -rf /home/worker/.cache` and this fails because you can't remove a mount point. Good times.

Comment 16

[Gregory Szorc [:gps]]

Comment on attachment 8900044 [details]
Bug 1391476 - Require that all cache paths be declared as volumes;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171384/diff/1-2/

Comment 17

[Gregory Szorc [:gps]]

Comment on attachment 8900045 [details]
Bug 1391476 - Tell run-task about volumes so it can sanitize them;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171386/diff/1-2/

Comment 18

[Gregory Szorc [:gps]]

Comment on attachment 8900046 [details]
Bug 1391476 - Automatically set cache/volume permissions in run-task;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171388/diff/1-2/

Comment 19

[Gregory Szorc [:gps]]

Comment on attachment 8900047 [details]
Bug 1391476 - Add UID and GID to cache parameters;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171390/diff/1-2/

Comment 20

[Gregory Szorc [:gps]]

Comment on attachment 8900045 [details]
Bug 1391476 - Tell run-task about volumes so it can sanitize them;

https://reviewboard.mozilla.org/r/171386/#review176576

::: taskcluster/docker/recipes/run-task:352
(Diff revision 2)
> +        if os.listdir(volume):
> +            print('error: volume %s is not empty; non-cache volumes are '
> +                  'supposed to be empty; you likely found a TaskCluster '
> +                  'worker bug' % volume)
> +            return 1

This assertion isn't true.

When Docker builds images, a VOLUME will result in copying any files present in the specified path at the time of the VOLUME directive.

However, I thought this wasn't getting preserved by the way TaskCluster does things. At least one of the Android images breaks this assumption.

So we can:

1) Forbid volumes from having files
2) Allow volumes to have files and chown the root directory
3) Allow volumes to have files and recursively chown

I'm not sure what the uid/gid of files copied to the volume will be. Presumably what the Docker image set up. So I would assume they would be properly set and we wouldn't need to chown them. But I wouldn't put anything past Docker.

Comment 21

[Gregory Szorc [:gps]]

Comment on attachment 8900045 [details]
Bug 1391476 - Tell run-task about volumes so it can sanitize them;

https://reviewboard.mozilla.org/r/171386/#review176576

> This assertion isn't true.
> 
> When Docker builds images, a VOLUME will result in copying any files present in the specified path at the time of the VOLUME directive.
> 
> However, I thought this wasn't getting preserved by the way TaskCluster does things. At least one of the Android images breaks this assumption.
> 
> So we can:
> 
> 1) Forbid volumes from having files
> 2) Allow volumes to have files and chown the root directory
> 3) Allow volumes to have files and recursively chown
> 
> I'm not sure what the uid/gid of files copied to the volume will be. Presumably what the Docker image set up. So I would assume they would be properly set and we wouldn't need to chown them. But I wouldn't put anything past Docker.

My distrust of Docker is correct: Docker does **NOT** preserve the uid on files in a VOLUME. I tested this by running ``chown <path-in-volume>`` and confirmed from a running container that the files are owned by ``root:root``. And to be extra sure, the ``layer.tar`` file as part of the tarball produced by ``docker save`` confirmed that the uid/gid are ``0:0``.

I'm leaning towards banning files in volumes because it is simpler and more closely approximates how caches work.

Comment 22

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

> My distrust of Docker is correct

TRUTH

Comment 23

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

Comment on attachment 8900038 [details]
Bug 1391476 - Inline list append;

https://reviewboard.mozilla.org/r/171372/#review176838

Comment 24

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

Comment on attachment 8900039 [details]
Bug 1391476 - Dedent block;

https://reviewboard.mozilla.org/r/171374/#review176840

Comment 25

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

Comment on attachment 8900040 [details]
Bug 1391476 - Don't use ~ in paths;

https://reviewboard.mozilla.org/r/171376/#review176844

Comment 26

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

Comment on attachment 8900041 [details]
Bug 1391476 - Print cache info;

https://reviewboard.mozilla.org/r/171378/#review176846

Comment 27

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

Comment on attachment 8900042 [details]
Bug 1391476 - Track whether caches should be used in untrusted environments;

https://reviewboard.mozilla.org/r/171380/#review176848

Are there any caches without this flag?

Comment 28

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

Comment on attachment 8900043 [details]
Bug 1391476 - Capture Docker volumes in docker-worker config;

https://reviewboard.mozilla.org/r/171382/#review176852

::: taskcluster/taskgraph/transforms/task.py:197
(Diff revision 1)
>          Required('allow-ptrace', default=False): bool,
>          Required('loopback-video', default=False): bool,
>          Required('loopback-audio', default=False): bool,
>          Required('docker-in-docker', default=False): bool,  # (aka 'dind')
>  
> +        # Docker volumes. Caches common share the same path. But not always.

Not sure what this sentence means -- is it OK for them to share the same path?  What does it mean?  What if they don't share the same path?

Comment 29

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

I'd like a little feedback from Wander or Greg as Docker experts, too.

Comment 30

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

Comment on attachment 8900044 [details]
Bug 1391476 - Require that all cache paths be declared as volumes;

https://reviewboard.mozilla.org/r/171384/#review176862

::: commit-message-6a1c5:5
(Diff revision 2)
> +Bug 1391476 - Require that all cache paths be declared as volumes; r?dustin
> +
> +See the inline comment for the rationale here.
> +
> +Depending on the order transforms run in, it is possible to evade this

I don't understand what this comment means. The order's not a mystery.. obviously any check written in the decision task could be evaded by just deleting it.

::: taskcluster/docker/android-gradle-build/Dockerfile:7
(Diff revision 2)
>  FROM          taskcluster/centos6-build-upd:0.1.6.20160329195300
>  MAINTAINER    Nick Alexander <[e-mail]>
>  
>  # BEGIN ../desktop-build/Dockerfile
>  
>  # TODO remove when base image is updated

hm, what's supposed to be removed here?

Comment 31

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

Comment on attachment 8900045 [details]
Bug 1391476 - Tell run-task about volumes so it can sanitize them;

https://reviewboard.mozilla.org/r/171386/#review176868

Comment 32

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

Comment on attachment 8900046 [details]
Bug 1391476 - Automatically set cache/volume permissions in run-task;

https://reviewboard.mozilla.org/r/171388/#review176870

I really like this model

Comment 33

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

Comment on attachment 8900047 [details]
Bug 1391476 - Add UID and GID to cache parameters;

https://reviewboard.mozilla.org/r/171390/#review176876

Comment 34

[Gregory Szorc [:gps]]

Comment on attachment 8900042 [details]
Bug 1391476 - Track whether caches should be used in untrusted environments;

https://reviewboard.mozilla.org/r/171380/#review176848

There are a handful, actually.

The version control cache is the most notable. We trust robustcheckout to do sane things so we don't need to worry about cache poisoning as much.

There are definitely a few caches whose configuration I question. It would be worthwhile to audit things. I wouldn't be opposed to changing the default so caches are disabled in "untrusted" environments by default. This was slightly beyond my tolerance for scope bloat in this series.

Comment 35

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

Fair enough.  Everything is isolated to different workers by level now, so I'm not too worried about *malicious* poisoning.

Comment 36

[Gregory Szorc [:gps]]

Comment on attachment 8900044 [details]
Bug 1391476 - Require that all cache paths be declared as volumes;

https://reviewboard.mozilla.org/r/171384/#review176862

> hm, what's supposed to be removed here?

These TODO comments are no longer valid given our new VOLUME requirements. I'll mass remove them.

Comment 37

[Gregory Szorc [:gps]]

Attachment: Bug 1391476 - Don't install nexus.xml in a Docker volume;

We're about to ban files in Docker volumes so they behave almost
identically to caches (which start empty).

We move the install of nexus.xml from Docker image time to
task time. This also means that changes to nexus.xml don't result
in having to rebuild the Docker image.

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

Comment 38

[Gregory Szorc [:gps]]

Comment on attachment 8900038 [details]
Bug 1391476 - Inline list append;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171372/diff/1-2/

Comment 39

[Gregory Szorc [:gps]]

Comment on attachment 8900039 [details]
Bug 1391476 - Dedent block;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171374/diff/1-2/

Comment 40

[Gregory Szorc [:gps]]

Comment on attachment 8900040 [details]
Bug 1391476 - Don't use ~ in paths;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171376/diff/1-2/

Comment 41

[Gregory Szorc [:gps]]

Comment on attachment 8900041 [details]
Bug 1391476 - Print cache info;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171378/diff/1-2/

Comment 42

[Gregory Szorc [:gps]]

Comment on attachment 8900042 [details]
Bug 1391476 - Track whether caches should be used in untrusted environments;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171380/diff/1-2/

Comment 43

[Gregory Szorc [:gps]]

Comment on attachment 8900043 [details]
Bug 1391476 - Capture Docker volumes in docker-worker config;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171382/diff/1-2/

Comment 44

[Gregory Szorc [:gps]]

Comment on attachment 8900044 [details]
Bug 1391476 - Require that all cache paths be declared as volumes;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171384/diff/2-3/

Comment 45

[Gregory Szorc [:gps]]

Comment on attachment 8900045 [details]
Bug 1391476 - Tell run-task about volumes so it can sanitize them;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171386/diff/2-3/

Comment 46

[Gregory Szorc [:gps]]

Comment on attachment 8900046 [details]
Bug 1391476 - Automatically set cache/volume permissions in run-task;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171388/diff/2-3/

Comment 47

[Gregory Szorc [:gps]]

Comment on attachment 8900047 [details]
Bug 1391476 - Add UID and GID to cache parameters;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171390/diff/2-3/

Comment 48

[Gregory Szorc [:gps]]

Comment on attachment 8900042 [details]
Bug 1391476 - Track whether caches should be used in untrusted environments;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171380/diff/1-2/

Comment 49

[Gregory Szorc [:gps]]

Comment on attachment 8900043 [details]
Bug 1391476 - Capture Docker volumes in docker-worker config;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171382/diff/1-2/

Comment 50

[Gregory Szorc [:gps]]

Comment on attachment 8900044 [details]
Bug 1391476 - Require that all cache paths be declared as volumes;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171384/diff/2-3/

Comment 51

[Gregory Szorc [:gps]]

Comment on attachment 8900383 [details]
Bug 1391476 - Don't install nexus.xml in a Docker volume;

We're about to ban files in Docker volumes so they behave almost
identically to caches (which start empty).

We move the install of nexus.xml from Docker image time to
task time. This also means that changes to nexus.xml don't result
in having to rebuild the Docker image.

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

Comment 52

[Gregory Szorc [:gps]]

Comment on attachment 8900045 [details]
Bug 1391476 - Tell run-task about volumes so it can sanitize them;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171386/diff/2-3/

Comment 53

[Gregory Szorc [:gps]]

Comment on attachment 8900046 [details]
Bug 1391476 - Automatically set cache/volume permissions in run-task;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171388/diff/2-3/

Comment 54

[Gregory Szorc [:gps]]

Comment on attachment 8900047 [details]
Bug 1391476 - Add UID and GID to cache parameters;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171390/diff/2-3/

Comment 55

[Greg Arndt [:garndt]]

If I understand the volume bits correctly, we're basically declaring some paths that are typically used for caches on non-try as volumes in the image's dockerfile so they have the same performance (outside of AUFS) as the caches do while not being persistent like caches are.  Makes sense.  I think it highlights that this could be made more explicit and cleaner if we moved the concept of caches to being declared as volumes in the task payload, and the type of volume is persistent (for caches) or temporary for things like we're doing here.

These volumes should be cleared up by docker worker when the containers are removed:
https://github.com/taskcluster/docker-worker/blob/master/src/lib/gc.js#L113-L118

Comment 56

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

yeah, we could definitely support this better in taskcluster-worker, but this implementation is really nice

Comment 57

[Greg Arndt [:garndt]]

At one point I added persistent volumes to taskcluster-worker, but since then, I think it has been removed.  Perhaps the code created then was not suitable to where taskcluster-worker has been taken now.

Comment 58

[Greg Arndt [:garndt]]

(In reply to Dustin J. Mitchell [:dustin] from comment #56)
> yeah, we could definitely support this better in taskcluster-worker, but
> this implementation is really nice

Totally agree that this implementation is really nice, wasn't suggesting otherwise :)

Comment 59

[Gregory Szorc [:gps]]

Comment on attachment 8900046 [details]
Bug 1391476 - Automatically set cache/volume permissions in run-task;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171388/diff/3-4/

Comment 60

[Gregory Szorc [:gps]]

Comment on attachment 8900047 [details]
Bug 1391476 - Add UID and GID to cache parameters;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171390/diff/3-4/

Comment 61

[Michael Shal [:mshal]]

Comment on attachment 8900383 [details]
Bug 1391476 - Don't install nexus.xml in a Docker volume;

https://reviewboard.mozilla.org/r/171734/#review177114

LGTM, assuming try is happy.

Comment 62

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f6f4c5e1b725
Inline list append; r=dustin
https://hg.mozilla.org/integration/autoland/rev/78aac0824b7e
Dedent block; r=dustin
https://hg.mozilla.org/integration/autoland/rev/88cd6294bef4
Don't use ~ in paths; r=dustin
https://hg.mozilla.org/integration/autoland/rev/548978f63439
Print cache info; r=dustin
https://hg.mozilla.org/integration/autoland/rev/985a6df057cb
Track whether caches should be used in untrusted environments; r=dustin
https://hg.mozilla.org/integration/autoland/rev/52a96399c1dc
Capture Docker volumes in docker-worker config; r=dustin
https://hg.mozilla.org/integration/autoland/rev/84fd52d2832a
Require that all cache paths be declared as volumes; r=dustin
https://hg.mozilla.org/integration/autoland/rev/047cc1e53f50
Don't install nexus.xml in a Docker volume; r=mshal
https://hg.mozilla.org/integration/autoland/rev/1768e5267bcf
Tell run-task about volumes so it can sanitize them; r=dustin
https://hg.mozilla.org/integration/autoland/rev/06d162a52dcd
Automatically set cache/volume permissions in run-task; r=dustin
https://hg.mozilla.org/integration/autoland/rev/8e3a645ad94b
Add UID and GID to cache parameters; r=dustin

Comment 63

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/f6f4c5e1b725
https://hg.mozilla.org/mozilla-central/rev/78aac0824b7e
https://hg.mozilla.org/mozilla-central/rev/88cd6294bef4
https://hg.mozilla.org/mozilla-central/rev/548978f63439
https://hg.mozilla.org/mozilla-central/rev/985a6df057cb
https://hg.mozilla.org/mozilla-central/rev/52a96399c1dc
https://hg.mozilla.org/mozilla-central/rev/84fd52d2832a
https://hg.mozilla.org/mozilla-central/rev/047cc1e53f50
https://hg.mozilla.org/mozilla-central/rev/1768e5267bcf
https://hg.mozilla.org/mozilla-central/rev/06d162a52dcd
https://hg.mozilla.org/mozilla-central/rev/8e3a645ad94b

Comment 64

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8900383 [details]
Bug 1391476 - Don't install nexus.xml in a Docker volume;

https://reviewboard.mozilla.org/r/171734/#review177526

Thanks for doing this.  If `android-dependencies` is green on try, it's good for me.

Comment 65

[Gregory Szorc [:gps]]

Comment on attachment 8900383 [details]
Bug 1391476 - Don't install nexus.xml in a Docker volume;

https://reviewboard.mozilla.org/r/171734/#review177526

I did get a green job on Try.

Comment 66

[Mike Hommey [:glandium]]

Comment on attachment 8900042 [details]
Bug 1391476 - Track whether caches should be used in untrusted environments;

https://reviewboard.mozilla.org/r/171380/#review196082

::: taskcluster/taskgraph/transforms/job/run_task.py
(Diff revision 2)
>  def docker_worker_run_task(config, job, taskdesc):
>      run = job['run']
>      worker = taskdesc['worker'] = job['worker']
>      common_setup(config, job, taskdesc)
>  
> -    if run.get('cache-dotcache') and int(config.params['level']) > 1:

I don't think the cache-dotcache condition should have been removed. For one, it broke using run-task on the desktop-build docker image (because the desktop-build docker image doesn't have that path as a VOLUME)

Comment 67

[Intermittent Failures Robot]

1 failures in 701 pushes (0.001 failures/push) were associated with this bug in the last 7 days.    

Repository breakdown:
* autoland: 1

Platform breakdown:
* toolchains: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1391476&startday=2018-01-22&endday=2018-01-28&tree=all