Closed Bug 1382911 Opened 7 years ago Closed 7 years ago

Custom action callback to trigger a bunch of jobs

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:
mozilla56

People

(Reporter: hassan, Assigned: hassan, Mentored)

Details

Attachments

(1 file, 3 obsolete files)

Bug 1382911 - Custom action callback to trigger a bunch of jobs
59 bytes, text/x-review-board-request
dustin
: review+
Details 
      No description provided.
Invoking create_task() complained that I was missing scopes:

[task 2017-07-21T02:18:18.715592Z] In other words you are missing scopes from one of the options:
[task 2017-07-21T02:18:18.715623Z]  * Option 0:
[task 2017-07-21T02:18:18.715678Z]     - "queue:create-task:highest:aws-provisioner-v1/gecko-1-b-win2012", and
[task 2017-07-21T02:18:18.715718Z]     - "queue:scheduler-id:-"
[task 2017-07-21T02:18:18.715754Z]  * Option 1:
[task 2017-07-21T02:18:18.715805Z]     - "queue:create-task:very-high:aws-provisioner-v1/gecko-1-b-win2012", and
[task 2017-07-21T02:18:18.715847Z]     - "queue:scheduler-id:-"
[task 2017-07-21T02:18:18.715876Z]  * Option 2:
[task 2017-07-21T02:18:18.715927Z]     - "queue:create-task:high:aws-provisioner-v1/gecko-1-b-win2012", and
[task 2017-07-21T02:18:18.715964Z]     - "queue:scheduler-id:-"
[task 2017-07-21T02:18:18.715997Z]  * Option 3:
[task 2017-07-21T02:18:18.716047Z]     - "queue:create-task:medium:aws-provisioner-v1/gecko-1-b-win2012", and
[task 2017-07-21T02:18:18.716103Z]     - "queue:scheduler-id:-"
[task 2017-07-21T02:18:18.716154Z]  * Option 4:
[task 2017-07-21T02:18:18.716204Z]     - "queue:create-task:low:aws-provisioner-v1/gecko-1-b-win2012", and
[task 2017-07-21T02:18:18.716241Z]     - "queue:scheduler-id:-"
[task 2017-07-21T02:18:18.716270Z]  * Option 5:
[task 2017-07-21T02:18:18.716302Z]     - "queue:scheduler-id:-"
Comment on attachment 8888602 [details]
Bug 1382911 - Custom action callback to trigger a bunch of jobs

https://reviewboard.mozilla.org/r/159604/#review164976

::: taskcluster/actions/add-new-jobs.py:30
(Diff revision 1)
> +        'type': 'object',
> +        'properties': {
> +            'tasks': {
> +                'type': 'array',
> +                'default': [
> +                    'build-win64/opt'    

watch out for trailing whitespace..
Assignee: nobody → helfi92
Hassan, can you link to the action task where you saw those errors?
(In reply to Dustin J. Mitchell [:dustin] from comment #4)
> Hassan, can you link to the action task where you saw those errors?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=61fb5544aac6a119f90891d43390c74ba86f3303&selectedJob=116227379
Ah, I think the issue is that the task you're creating doesn't have schedulerId set, so it's defaulting to "-", which is why that scope is required ("queue:scheduler-id:-").  Hopefully that points you in the right direction?
(In reply to Dustin J. Mitchell [:dustin] from comment #6)
> Ah, I think the issue is that the task you're creating doesn't have
> schedulerId set, so it's defaulting to "-", which is why that scope is
> required ("queue:scheduler-id:-").  Hopefully that points you in the right
> direction?

You were right, I added the schedulerId and then the task retriggered successfully [1]. But there is something strange however. The schedulerId property in task details [2] still shows "-" as the schedulerId.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=72022a3bf620ab7d211645aa9722959f000ae115&selectedJob=116368885
[2] https://tools.taskcluster.net/groups/XOBHDZpvTsmTVbG4JDHqwg/tasks/XOBHDZpvTsmTVbG4JDHqwg/details
[2] is the action task itself.  The task it retriggers is https://tools.taskcluster.net/groups/a0d5r1wbTyuo73h0w3wKvg/tasks/a0d5r1wbTyuo73h0w3wKvg/details which does have the correct schedulerId.  However, we should be setting the correct schedulerId on action tasks, too -- I'll open a separate bug for that.
Attachment #8888762 - Attachment is obsolete: true
Attachment #8888765 - Attachment is obsolete: true
Attachment #8888816 - Attachment is obsolete: true
Comment on attachment 8888602 [details]
Bug 1382911 - Custom action callback to trigger a bunch of jobs

https://reviewboard.mozilla.org/r/159604/#review165246

Looking good! Just some small changes.

::: taskcluster/actions/add-new-jobs.py:18
(Diff revision 3)
> +TASKCLUSTER_INDEX_URL = 'https://index.taskcluster.net/v1/task'
> +
> +logger = logging.getLogger(__name__)
> +
> +
> +@register_callback_action(

Rather than calling this retrigger tasks, let's call it "add new jobs". That should be in name, title, and description.

::: taskcluster/actions/add-new-jobs.py:21
(Diff revision 3)
> +
> +
> +@register_callback_action(
> +    name='Retrigger tasks',
> +    title='Schedule Tasks Retrigger',
> +    symbol='rj-custom',

I'm not sure this is a good symbol. Perhaps `a-n` or `add-new`? Our rules for what is a good symbol are undefined afaict.

::: taskcluster/actions/add-new-jobs.py:30
(Diff revision 3)
> +    schema={
> +        'type': 'object',
> +        'properties': {
> +            'tasks': {
> +                'type': 'array',
> +                'default': [

We should not have a default task to add. We should specify that the elements of the array are strings though. http://json-schema.org/latest/json-schema-validation.html#rfc.section.6.9 is how to specify elements of an array in jsonschema

::: taskcluster/actions/add-new-jobs.py:43
(Diff revision 3)
> +    s = requests.Session()
> +    task_graph = s.get(url='{}/gecko.v2.{}.pushlog-id.{}.decision/artifacts/{}'.format(
> +                       TASKCLUSTER_INDEX_URL,
> +                       parameters['project'],
> +                       parameters['pushlog_id'],
> +                       'public/full-task-graph.json')).json()

This is _probably_ ok, but just a heads up that calling `.json()` directly on the `s.get(...)` creates a confusing error if you get a 404 or something. Check out `status_code` and `raise_for_status` in https://reviewboard.mozilla.org/r/158948/diff/1#index_header

I don't promise that I'm doing it correctly either, but maybe you can figure out a nice way to do it.

::: taskcluster/actions/add-new-jobs.py:46
(Diff revision 3)
> +                       parameters['project'],
> +                       parameters['pushlog_id'],
> +                       'public/full-task-graph.json')).json()
> +
> +    for elem in input['tasks']:
> +        if elem in task_graph:

Let's also have an `else` that logs that the task wasn't in the task graph to begin with.

::: taskcluster/actions/add-new-jobs.py:52
(Diff revision 3)
> +            new_task_definition = copy.copy(task_graph[elem]['task'])
> +
> +            # set new created, deadline, and expiry fields
> +            new_task_definition['created'] = current_json_time()
> +            new_task_definition['deadline'] = json_time_from_now('1d')
> +            new_task_definition['expires'] = json_time_from_now('30d')

Unfortunately, we also need to go through and update the timestamps for `expires` in artifacts. I have some code for it in the PR I linked above as well that you can find. If you basically copy-paste that for now, I can move it out into a nice shared function when I land my changes.
Comment on attachment 8888602 [details]
Bug 1382911 - Custom action callback to trigger a bunch of jobs

https://reviewboard.mozilla.org/r/159604/#review165884

::: taskcluster/actions/add-new-jobs.py:19
(Diff revision 6)
> +
> +logger = logging.getLogger(__name__)
> +
> +
> +@register_callback_action(
> +    name='add new jobs',

name should not have spaces. perhaps use underscore case.
Comment on attachment 8888602 [details]
Bug 1382911 - Custom action callback to trigger a bunch of jobs

https://reviewboard.mozilla.org/r/159604/#review165898

Awesome!
Attachment #8888602 - Flags: review?(bstack) → review+
Keywords: checkin-needed
Autoland can't push this because it doesn't meet the necessary review requirements in MozReview.
http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/autoland.html#landing-commits
Keywords: checkin-needed
Comment on attachment 8888602 [details]
Bug 1382911 - Custom action callback to trigger a bunch of jobs

https://reviewboard.mozilla.org/r/159604/#review165972

::: taskcluster/actions/add-new-jobs.py:32
(Diff revision 7)
> +        'properties': {
> +            'tasks': {
> +                'type': 'array',
> +                'items': {
> +                    'type': 'string'
> +                }

This should have some descriptions and/or titles to indicate what it's an array of -- I *think* it's an array of task labels?

::: taskcluster/actions/add-new-jobs.py:47
(Diff revision 7)
> +              parameters['pushlog_id'],
> +              'public/full-task-graph.json'))
> +
> +    if r.status_code != requests.codes.ok:
> +        if r.status_code == 404:
> +            logger.error('Could not find task graph on push {}. Reason: {}'.format(

`logger.error` doesn't automaticlally raise an exception or terminate the process.  These (both usees of `logger.error` in this file) would probably be better handled with `raise Exception(..)`

::: taskcluster/actions/add-new-jobs.py:61
(Diff revision 7)
> +            new_task_definition = copy.copy(task_graph[elem]['task'])
> +
> +            # set new created, deadline, and expiry fields
> +            new_task_definition['created'] = current_json_time()
> +            new_task_definition['deadline'] = json_time_from_now('1d')
> +            new_task_definition['expires'] = json_time_from_now('30d')

I don't think these, or the artifact expiration updates below, are necessary -- `taskgraph.create.create_task` replaces `{relative-datestamp: ..}` with actual datestamps before creating the task, and the tasks in `full-task-graph.json` contain datestamps in that format.

::: taskcluster/actions/add-new-jobs.py:71
(Diff revision 7)
> +            if isinstance(artifacts, dict):
> +                artifacts = artifacts.values()
> +            for artifact in artifacts:
> +                artifact['expires'] = new_task_definition['expires']
> +
> +            logger.info("New full task definition: %s", new_task_definition)

It's OK for testing in try, but we shouldn't land code that has such big logging output.

::: taskcluster/actions/add-new-jobs.py:76
(Diff revision 7)
> +            logger.info("New full task definition: %s", new_task_definition)
> +
> +            # actually create the new task
> +            new_task_id = slugid()
> +
> +            create_task(s, new_task_id, 'add-new', new_task_definition)

Aside from `relative-datestamp`, the tasks in `full-task-graph.json` also contain `{task-reference: "..<..>.."}` which is a way to refer to the taskId of a dependent task.  I strongly suspect that this action won't work, since it doesn't do anything with those task references.

You can see how I handled that in https://reviewboard.mozilla.org/r/159452/diff/4#index_header
Attachment #8888602 - Flags: review-
Comment on attachment 8888602 [details]
Bug 1382911 - Custom action callback to trigger a bunch of jobs

I rewrote the implementation based on [1]. One thing I noticed is that label-to-taskid.json doesn't have a full mapping of full-task-graph.json labels. For example, android-api-15-gradle-dependencies is shown in the latter but not on the former. Is that normal? Any clarifications would be appreciated.

[1] https://reviewboard.mozilla.org/r/159452/diff/4#index_header
Attachment #8888602 - Flags: review+ → feedback?(dustin)
Yes, that's normal -- label-to-taskid is only for jobs that were actually created by the original decision task, so anything that it optimized out will not be present.  If you find yourself creating a task which has a dependency that is not in label-to-taskid, then you'll also need to create that dependency task (first).  I was hoping that wouldn't happen for this first round of actions, and that once we have some common utility functions we could just implement that create-dependencies-as-needed behavior once.
(In reply to Dustin J. Mitchell [:dustin] from comment #29)
> Yes, that's normal -- label-to-taskid is only for jobs that were actually
> created by the original decision task, so anything that it optimized out
> will not be present.  If you find yourself creating a task which has a
> dependency that is not in label-to-taskid, then you'll also need to create
> that dependency task (first).  I was hoping that wouldn't happen for this
> first round of actions, and that once we have some common utility functions
> we could just implement that create-dependencies-as-needed behavior once.

Makes a lot of sense!
Attachment #8888602 - Flags: feedback?(dustin) → review?(dustin)
Comment on attachment 8888602 [details]
Bug 1382911 - Custom action callback to trigger a bunch of jobs

https://reviewboard.mozilla.org/r/159604/#review166772
Attachment #8888602 - Flags: review?(dustin) → review+
Pushed by dmitchell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/36df21c0cba7
Custom action callback to trigger a bunch of jobs r=bstack,dustin
https://hg.mozilla.org/mozilla-central/rev/36df21c0cba7
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Product: TaskCluster → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: