1237300 - Coalescing should just take a URL, rather than looking at routes

Status: RESOLVED FIXED

(Taskcluster :: Workers, defect)

Description

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

This would simplify the worker implementation -- it just hits a URL which returns a sorted list of taskIds.  This has a few implications:

 * docker-worker really only has to know about "superseding" - the word "coalesce" need not appear
 * only the coalescer is concerned about routes and pulse, although it will require that tasks have matching superseding URLs and routes

The changes required:

 * replace payload.coalescer with payload.supersedingUrl
   * while we're here, let's add the taskId as a query argument
     (I suspect we'll want this later)
   * remove the docker-worker code that matches against routes
 * update the response format to use a fixed property name, rather than the coalescing key
 * update documentation

It's not clear when we should do this -- maybe when implementing the taskcluster worker?

Comment 1

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

That taskId will come in handy for bug 1237307 (at least allowing the coalescer to detect problems)

Jake, when do you think we should hack on this?

Comment 2

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

I'll start working on the docker-worker side of this soon.

Comment 3

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

Let's use a fixed name of `supersedes`:
{
  "supersedes": [...]
}

Comment 4

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

Things to do on docker-worker here:
 * rename /coalesc(er|ed|ing|ible)/ appropriately
 * change the payload format to use only `supersedingUrl`
 * append `?taskId=<taskId>` to the URL
 * expect an object with property `supersedes`
 * require that the resulting list contain the querying taskId (ignoring the list otherwise) (bug 1237307)
 * only claim until it finds the querying taskId in the list (bug 1237307)

Comment 5

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

And reflect all of that in tc-docs.

Comment 6

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

I changed my mind - superserderUrl sounds nicer

Comment 7

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

> * only claim until it finds the querying taskId in the list (bug 1237307)
this is wrong; given

  rev1  rev2  rev3  rev4

and an initial claim of rev3, we still want to try to claim all four, and if successful, perform rev4.

Comment 8

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

Attachment: https://github.com/taskcluster/docker-worker/pull/209

Comment 9

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

https://github.com/taskcluster/taskcluster-docs/pull/69 for the docs update.

Comment 10

[Greg Arndt [:garndt]]

Comment on attachment 8713332 [details] [review]
https://github.com/taskcluster/docker-worker/pull/209

Looks good to me after the changes.  Relevant tests pass too.

Comment 11

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

--> hvm-builder:
us-east-1: ami-69fbd603
us-west-1: ami-72fd8a12
us-west-2: ami-381ef958
--> pv-builder:
us-east-1: ami-09f8d563
us-west-1: ami-73fd8a13
us-west-2: ami-de11f6be

Comment 12

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

rebuilt with better DEBUG:

--> hvm-builder:
us-east-1: ami-eff4d985
us-west-1: ami-15f48375
us-west-2: ami-8f10f7ef

--> pv-builder:
us-east-1: ami-e6f6db8c
us-west-1: ami-c7f285a7
us-west-2: ami-6c1afd0c

Comment 13

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

 Jan 29 10:49:55 docker-worker.aws-provisioner.us-west-2a.ami-381ef958.r3-xlarge.i-c9ff250e docker-worker: {"type":"not superseding","source":"top","provisionerId":"aws-provisioner-v1","workerId":"i-c9ff250e","workerGroup":"us-west-2a","workerType":"ami-test","workerNodeType":"r3.xlarge","taskId":"UAZ0kbOLTs2YJAXNvNdZXg","supersederUrl":"https://coalesce.mozilla-releng.net/v1/list/ab","message":"no tasks supersede this one"} 

for a test task.  So I think we can ship these AMIs out to at least the desktop test/build workerTypes.

Maybe Monday :)

Comment 14

[[github robot]]

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

https://github.com/taskcluster/taskcluster-docs/commit/8a9b110244d992bf2a57e7d6397955dc95fb1437
Bug 1237300: update docs to match new simpler supserseding process

https://github.com/taskcluster/taskcluster-docs/commit/8e8b91660adb86fc32097eb96b0a61e27adc58fc
Merge djmitche/taskcluster-docs:bug1237300 (PR #69)

Comment 15

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

OK, I'm out of time trying to land these AMI changes, after struggling with a bunch of other stuff.  I'll try again later this week, unless the AMI fairies beat me to it :)

Comment 16

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

deployed to {opt,dbg}-{linux{32,64},macosx64}, desktop-test{,-xlarge}, android-api-{11,15}

Comment 17

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

There's going to be a lag before there's any real fallout from this, but even if there is, it's tier-2, so..

Comment 18

[[github robot]]

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

https://github.com/taskcluster/docker-worker/commit/e105da56181f3bc0aeaf26e7b2718e816582b95b
Bug 1237300: simplify coalescing to superseding

* rename /coalesc(er|ed|ing|ible)/ appropriately
* change the payload format to use only `supersedingUrl`
* append `?taskId=<taskId>` to the superseding URL
* expect an object with property `supersedes`
* require that the resulting list contain the initial taskId

https://github.com/taskcluster/docker-worker/commit/c056f41d65b9a2bece60b87ab5e18d465ca0ecd6
Merge djmitche/docker-worker:bug1237300 (PR #209)