1478835 - Allow configuration of queue for different mirroring configurations

Status: RESOLVED FIXED

(Taskcluster :: Services, enhancement)

Description

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

In particular, a deployment currently does no mirroring, so all artifacts should be served from the CDN except those from the AWS region containing the bucket (or something like that)..

Comment 1

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

John, do you have thoughts on how this would work?  I'd like to get the ability to just upload all artifacts to a single bucket and send "get" requests to that bucket, as that will be simplest and not too expensive for a development or staging cluster.  But if the same configurability can also work for S3, blobs, and the object service, all the better.

I'm happy to do the work here -- just looking for direction on how you see this being designed.

Comment 2

[John Ford [:jhford] CET/CEST Berlin Time]

The design of the object service is that the queue will only store a mapping between the name of the artifact in the queue and the name of the object in the object service.  All configuration of which bucket to use, where to store things, and where to redirect to will be in the object service.  The queue will always redirect to the object service.  The interface to the object service will be the existing blob artifact api.

We should deprecate the S3 artifact type rather than fix anything for it, it's not safe to use and a new deployment shouldn't carry it over.  I think time is better spent driving forward on the new artifact type than supporting mirroring on the old type.  Taskcluster-lib-artifact[-go] makes it pretty easy to use the new api correctly.

We should not store S3 artifacts in the same bucket as blob artifacts.  As long as we keep the S3 artifact type, we will need two buckets.

For a staging/development cluster, I don't think there's any mirroring needed.  If we start having significant load on a new cluster before the object service is deployed, we should investigate using a second cloud-mirror instance, patched to use taskcluster-lib-artifact.

Comment 3

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

I think your last question gets to the issue -- how do we turn off mirroring?  There's no config for that right now.

Comment 4

[John Ford [:jhford] CET/CEST Berlin Time]

(In reply to Dustin J. Mitchell [:dustin] pronoun: he from comment #3)
> I think your last question gets to the issue -- how do we turn off
> mirroring?  

Ahh, ok.  That wasn't clear to me based on the bug, but good that we're on the same page now.

> There's no config for that right now.

We do have config for this, but it's slightly confusing since a patch broke the more clearly named cfg.app.useCloudMirror option.  We can set the cfg.app.cloudMirrorRegions to an empty list and cfg.app.useCloudMirror to true (yes, confusing) right now without doing any code changes to disable all usage of cloud-mirror.  I would agree that this is not very clear.

The commit which broke cfg.app.useCloudMirror is https://github.com/taskcluster/taskcluster-queue/commit/9a3f94a14a6c958486446f671aa42b40bd3affef .  The problem with it is that the false case of the ternary operator in the EC2RegionResolver constructor call is missing the canonical artifact region, which means that when cfg.app.useCloudMirror is false, we cannot resolve any EC2 regions, which I believe means every request goes to CDN.  Basically, as the code is today, cfg.app.useCloudMirror *must* always be true.

I can think of two easy ways to make things a lot more clear.  First option is that we remove the cfg.app.useCloudMirror option and always cfg.app.cloudMirrorRegions + cfg.app.artifactRegion in the region resolver constructor.  Disabling cloud mirror is passing an empty list or falsy value (e.g the default of undefined).  I think the following patch should be enough to do this:

diff --git a/config.yml b/config.yml
index 9257cb97..b4370859 100644
--- a/config.yml
+++ b/config.yml
@@ -158,7 +158,6 @@ production:
       - us-east-2
       - us-west-1
       - eu-central-1
-    useCloudMirror: !env:bool USE_PUBLIC_ARTIFACT_BUCKET_PROXY
   server:
     env:              production
     trustProxy:       true
diff --git a/src/main.js b/src/main.js
index d5885b7a..84e0ab2b 100644
--- a/src/main.js
+++ b/src/main.js
@@ -392,9 +392,11 @@ let load = loader({
   regionResolver: {
     requires: ['cfg'],
     setup: async ({cfg}) => {
-      let regionResolver = new EC2RegionResolver(
-        cfg.app.useCloudMirror ? [...cfg.app.cloudMirrorRegions, cfg.aws.region] : []
-      );
+      let regions = (cfg.app.cloudMirrorRegions || []).slice();
+      if (!regions.includes(cfg.aws.region)) {
+        regions.push(cfg.aws.region);
+      }
+      let regionResolver = new EC2RegionResolver(regions);
       await regionResolver.loadIpRanges();
       return regionResolver;
     },

The second option would be to land a patch which fixes the problems with cfg.app.useCloudMirror and explicitly disables cloud-mirror redirects in the api handler.  I think the following patch should be everything needed to do this.

diff --git a/src/artifacts.js b/src/artifacts.js
index 81ed100b..992c9886 100644
--- a/src/artifacts.js
+++ b/src/artifacts.js
@@ -567,6 +567,10 @@ var replyWithArtifact = async function(taskId, runId, name, req, res) {
         skipCache = true;
       }

+      if (!this.useCloudMirror) {
+        skipCache = true;
+      }
+
       if (skipCache && skipCDN) {
         url = this.publicBucket.createGetUrl(prefix, true);
       } else if (skipCache || !region) {
diff --git a/src/main.js b/src/main.js
index d5885b7a..420d9486 100644
--- a/src/main.js
+++ b/src/main.js
@@ -393,7 +393,7 @@ let load = loader({
     requires: ['cfg'],
     setup: async ({cfg}) => {
       let regionResolver = new EC2RegionResolver(
-        cfg.app.useCloudMirror ? [...cfg.app.cloudMirrorRegions, cfg.aws.region] : []
+        cfg.app.useCloudMirror ? [...cfg.app.cloudMirrorRegions, cfg.aws.region] : [cfg.aws.region]
       );
       await regionResolver.loadIpRanges();
       return regionResolver;
@@ -450,6 +450,7 @@ let load = loader({
         credentials:      ctx.cfg.taskcluster.credentials,
         cloudMirrorHost:  ctx.cfg.app.cloudMirrorHost,
         artifactRegion:   ctx.cfg.aws.region,
+        useCloudMirror:   ctx.cfg.app.useCloudMirror,
         monitor:          ctx.monitor.prefix('api-context'),
         workClaimer:      ctx.workClaimer,
         workerInfo:       ctx.workerInfo,

I haven't tested either of these, and they might not be a full solution, but as I was looking at the code anyway, I figured it was more illustrative than a paragraph explaining the changes shown in the diffs.

Comment 5

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

https://github.com/taskcluster/taskcluster-queue/pull/290

Comment 6

[[github robot]]

Commits pushed to master at https://github.com/taskcluster/taskcluster-queue

https://github.com/taskcluster/taskcluster-queue/commit/08b4faa44417d3f1149d3935c5a81e5925179055
Bug 1478835 - fix support for useCloudMirror

This includes renaming the env var to USE_CLOUD_MIRROR for consistency
with the JS name and removing the unused `usePublicArtifactBucketProxy`
configuration property.

Thanks to @jhford for most of this patch :)

https://github.com/taskcluster/taskcluster-queue/commit/7c76631a1cdfd9811936cf6f58430d9da042f2df
Merge pull request #290 from djmitche/bug1478835

Bug 1478835 - fix support for useCloudMirror

Comment 7

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

So I think in a staging cluster we can use `USE_CLOUD_MIRROR=false` and unset `PUBLIC_ARTIFACT_BUCKET_CDN` to get a situation where we just stuff blobs into S3 and point to that bucket.