1398277 - Retriggering a decision task can cause duplicate jobs to be scheduled

Status: RESOLVED FIXED

(Taskcluster :: Services, defect, P5)

Description

[Greg Arndt [:garndt]]

Currently when a task is retriggered, mozilla-taskcluster will respond to the action and duplicate that node along with all descendants.  

Tasks scheduled by a gecko decision task are made to depend on the decision task.

In the situation of retriggering, this means that the decision task along with all the dependent tasks are recreated.  Once the retriggered decision task runs, it will then schedule another duplicate set of jobs.

Comment 1

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

I think the fix to this is to make sure that the "regular" retrigger action doesn't apply to the decision task, and define another retrigger action specifically for the decision task that just duplicates it, without duplicating child tasks.

Or I guess since that's all implemented in Python, it could just detect action/decision tasks and not duplicate their children.

Comment 2

[Yashvardhan Didwania [:ydidwania]]

Hi,  I would like to work on this bug. Can you guide me to the relevant code?

Comment 3

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

The retrigger action is implemented here:
  https://dxr.mozilla.org/mozilla-central/source/taskcluster/taskgraph/actions/retrigger.py

I think the best solution would be to modify that code as suggested in the second paragraph in comment 1.

Comment 4

[yuktikirtani]

Hey,
This is my first attempt at contributing to an open source project.
It would be highly appreciable if you could guide me as to how to get started.
Thanks.

Comment 5

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

Hi! I suspect ydidwania has started working on this.  Let's see if they are still working before you jump in.

Comment 6

[yuktikirtani]

I think ydidwania is working on some other issue(1349261).
Please confirm
Thanks.

Comment 7

[Yashvardhan Didwania [:ydidwania]]

(In reply to yuktikirtani from comment #6)
> I think ydidwania is working on some other issue(1349261).
> Please confirm
> Thanks.

Hi yuktikirtani. Thanks for your interest in fixing this bug. Dustin is right, I am still working on this. You are however,free to try out other bugs you find interesting.

Comment 8

[yuktikirtani]

(In reply to ydidwania from comment #7)
> (In reply to yuktikirtani from comment #6)
> > I think ydidwania is working on some other issue(1349261).
> > Please confirm
> > Thanks.
> 
> Hi yuktikirtani. Thanks for your interest in fixing this bug. Dustin is
> right, I am still working on this. You are however,free to try out other
> bugs you find interesting.

Hey, Okay no problem

Comment 9

[Yashvardhan Didwania [:ydidwania]]

I think modifying the code as below should work.

if task_id != decision_task_id:
    if input.get('downstream'):
        to_run = full_task_graph.graph.transitive_closure(set(to_run), reverse=True).nodes
        to_run = to_run & set(label_to_taskid.keys())
        with_downstream = ' (with downstream) ' 

Basically adding downstream tasks to `to_run` only when the task is not a decision task.
Are action tasks different from decision tasks? I didn't find any mention to action task while going through any of the docs.

Can you tell me why `to_run` is a set of labels rather than task ids.There can be many tasks with the same label.

Comment 10

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

That sounds like a good plan!

Action tasks are sort of like decision tasks, in that they call queue.createTask -- but they serve to modify an already-existing push, rather than start a new one.

It is a set of labels because that represents what we want to retrigger.  If you think of a taskId as a specific instance of a label, it's "do this <label> again" rather than repeating the specific instance (which has already failed and can't be further modified).

Comment 11

[Yashvardhan Didwania [:ydidwania]]

Are action tasks covered in the condition
 
task_id != decision_task_id

Comment 12

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

I'm not worried about that right now -- let's just worry about decision tasks :)

Comment 13

[Yashvardhan Didwania [:ydidwania]]

Attachment: Bug 1398277 - Fixed typo

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

Comment 14

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

Comment on attachment 8916790 [details]
Bug 1398277 - Fixed typo

https://reviewboard.mozilla.org/r/187846/#review193172

Looks good!  I triggered a try push, and will try re-triggering that push's decision task.

Comment 15

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

Comment on attachment 8916790 [details]
Bug 1398277 - Fixed typo

https://reviewboard.mozilla.org/r/187846/#review193184

Hm, didn't work:
  https://public-artifacts.taskcluster.net/NG1i-Na7S6yJTPpznFC93A/0/public/logs/live_backing.log

Comment 16

[Yashvardhan Didwania [:ydidwania]]

I found this in taskcluster/taskgraph/graph.py

def transitive_closure(self, nodes, reverse=False):
        """
        Return the transitive closure of <nodes>: the graph containing all
        specified nodes as well as any nodes reachable from them, and any
        intervening edges.

        If `reverse` is true, the "reachability" will be reversed and this
        will return the set of nodes that can reach the specified nodes.

        Example
        -------

        a ------> b ------> c
                  |
                  `-------> d

        transitive_closure([b]).nodes == set([a, b])
        transitive_closure([c]).nodes == set([c, b, a])
        transitive_closure([c], reverse=True).nodes == set([c])
        transitive_closure([b], reverse=True).nodes == set([b, c, d])
        """

1) I went through the implementation. I think  it is reversed according to the explanation provided. The examples are correct.

There's this line in retrigger.py:

            to_run = full_task_graph.graph.transitive_closure(set(to_run), reverse=True).nodes

which is expected to fetch the set of all downstream tasks from a given task.

2) Regarding the error, I think we need to add the keys as is done when we were duplicating the child tasks as well. Something like
    
            to_run = to_run & set(label_to_taskid.keys())

Comment 17

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

The error is coming from a *different* call to transitive_closure, from what I can tell.

Comment 18

[Yashvardhan Didwania [:ydidwania]]

Yeah its because of this call - 

[task 2017-10-10T15:11:49.623251Z] File "/builds/worker/checkouts/gecko/taskcluster/taskgraph/actions/util.py", line 44, in create_tasks
[task 2017-10-10T15:11:49.623313Z] target_graph = full_task_graph.graph.transitive_closure(to_run)

I got confused when I saw the examples in transitive_closure. I think they contradict the explanation of transitive closure of a graph written above.

Comment 19

[Yashvardhan Didwania [:ydidwania]]

There is some error in `to_run`,  the set of labels that we are passing to `create_task() .

Is there a way to test the retrigger action locally?

Comment 20

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

Yes -- if you look at the task details in
  https://tools.taskcluster.net/groups/TuAbomxORdW52JbuFvKRlQ/tasks/NG1i-Na7S6yJTPpznFC93A/details
you will see a bunch of environment variables and a command line.  You can set those variables on your command line, and then run the last bit of that command ("./mach --log-no-times taskgraph action-callback").

Comment 21

[Yashvardhan Didwania [:ydidwania]]

The Gecko Decision Task does not get added to the task graph, hence the statement
   assert nodes <= self.nodes 
in https://dxr.mozilla.org/mozilla-central/source/taskcluster/taskgraph/graph.py fails. 

This failure happens even with the current version of retrigger.py that is in use( I tried it locally), but from the call in line 59 (https://dxr.mozilla.org/mozilla-central/source/taskcluster/taskgraph/actions/retrigger.py#59) . 

So my question is why isn't it getting added to the full_task_graph.json or does the problem exist only locally?

Comment 22

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

That's because the decision task isn't in the task graph -- it *creates* the task graph.  So `create_tasks` is the wrong tool for retriggering the decision task.  It looks like you want `create_task_from_def`:
  https://dxr.mozilla.org/mozilla-central/source/taskcluster/taskgraph/actions/util.py#55
you will need to replace the `created`, `deadline`, and `expires` attributes in the task definition with new timestamps using `{"relative-datestamp": ".."}`.

Comment 23

[Yashvardhan Didwania [:ydidwania]]

How do I obtain the task definition of the decision task since it is not present in the task graph?
Will 'get_decision_parameters' in :
 https://dxr.mozilla.org/mozilla-central/source/taskcluster/taskgraph/decision.py#131
 
be useful here?

Comment 24

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

You can call https://docs.taskcluster.net/reference/platform/taskcluster-queue/references/api#task from the action task to get the task definition, passing it the decision task's taskid.

Comment 25

[Yashvardhan Didwania [:ydidwania]]

This is where I am at currently when task_id == decision_task_id

        new_task_definition = requests.get('https://queue.taskcluster.net/v1/task/%s'%(task_id)).json()
        new_task_definition['created'] = dict({"relative-datestamp": "0 seconds"})
        new_task_definition['deadline'] = dict({"relative-datestamp": "4 days"})
        new_task_definition['expired'] = dict({"relative-datestamp": "14 days"})
        new_task_id=slugid()
        create_task_from_def(new_task_id, new_task_definition, parameters['level'])t
        logger.info('Scheduled {}{}(time {}/{})'.format(label, with_downstream, i+1, times))

I am unable to test it because I  don't have the rights( thats what scopes mean, right?) to create a task using the api.
A decision task shouldn't be retriggered  multiple times hence I did not.
Did I miss anything?

Comment 26

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

I think that looks good.  Do you want to push the patch and we can try it out in try?

Comment 27

[Yashvardhan Didwania [:ydidwania]]

Attachment: Bug 1398277-Retrigger decision task from task definition;

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

Comment 28

[Yashvardhan Didwania [:ydidwania]]

Pushed. Should I add comments in the code?

Comment 29

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

Comment on attachment 8923583 [details]
Bug 1398277-Retrigger decision task from task definition;

https://reviewboard.mozilla.org/r/194714/#review200702

Sorry for the delay -- if you can fix this up I can push the buttons in try and see how it works.

::: taskcluster/taskgraph/actions/retrigger.py:80
(Diff revision 1)
>  
> -        logger.info('Scheduled {}{}(time {}/{})'.format(label, with_downstream, i+1, times))
> +            logger.info('Scheduled {}{}(time {}/{})'.format(label, with_downstream, i+1, times))
> +    else:
> +        print ("hello")
> +        task_id = 'QAs5KRluQqe6PKtx_DpQLQ'
> +        print (task_id)

Can you remove this?  Otherwise it won't work when I push the buttons on the try push :)

Comment 30

[Yashvardhan Didwania [:ydidwania]]

Comment on attachment 8916790 [details]
Bug 1398277 - Fixed typo

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

Comment 31

[Yashvardhan Didwania [:ydidwania]]

I made a commit with the lines removed and pushed only that extra commit for review. I thought it will get mapped to the existing patch (now totalling 3 commits) but instead that patch became obsolete. I have a feeling I messed up. If required I will push the entire revset again for review :)

Comment 32

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

That looks like a mozreview bug to me.  I've triggered the try job, and will try re-triggering the decision task on it.

Comment 33

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=55e59753469fb2a1466335bd2444bc81936764bb

Comment 34

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

[task 2017-11-03T16:08:48.528026Z] Creating task with taskId L7ATmV-WT--bbaDkqvwZdg for Gecko Decision Task
[task 2017-11-03T16:08:48.529131Z] Starting new HTTP connection (1): taskcluster
[task 2017-11-03T16:08:49.377623Z] "PUT /queue/v1/task/L7ATmV-WT--bbaDkqvwZdg HTTP/1.1" 400 3848
[task 2017-11-03T16:08:49.378432Z] 
[task 2017-11-03T16:08:49.378470Z] Schema Validation Failed!
[task 2017-11-03T16:08:49.378504Z] Rejecting Schema: http://schemas.taskcluster.net/queue/v1/create-task-request.json#
[task 2017-11-03T16:08:49.378525Z] Errors:
[task 2017-11-03T16:08:49.378562Z]   * data should NOT have additional properties
[task 2017-11-03T16:08:49.378574Z] ----
[task 2017-11-03T16:08:49.378586Z] method:     createTask
[task 2017-11-03T16:08:49.378599Z] errorCode:  InputValidationError
[task 2017-11-03T16:08:49.378609Z] statusCode: 400
[task 2017-11-03T16:08:49.378622Z] time:       2017-11-03T16:08:49.191Z

That error means that the payload to createTask had an extra top-level attribute that it should not.  It doesn't say *what* that attribute was, unfortunately.  But!  I think I see it: it should be `expires` and not `expired`.

Comment 35

[Yashvardhan Didwania [:ydidwania]]

Comment on attachment 8916790 [details]
Bug 1398277 - Fixed typo

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

Comment 36

[Yashvardhan Didwania [:ydidwania]]

Fixed the typo. Sorry for letting that error pass by. Hope it works fine now  :)

Comment 37

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

Hm, that didn't work either:

[task 2017-11-05T13:39:55.292866Z] In other words you are missing scopes from one of the options:
[task 2017-11-05T13:39:55.292879Z]  * Option 0:
[task 2017-11-05T13:39:55.292911Z]     - "queue:route:[e-mail].*", and
[task 2017-11-05T13:39:55.292935Z]     - "queue:route:[e-mail]-failed", and
[task 2017-11-05T13:39:55.292953Z]     - "queue:route:[e-mail]-exception"

If it's OK with you, I will take this bug and figure out how to finish it up.  I suspect we may have started out in the wrong direction here and I need to rethink it.  Which is not to diminish your work -- I appreciate it!  Hopefully some of the projects we discussed in irc will be more tractable.

Comment 38

[Yashvardhan Didwania [:ydidwania]]

Yeah, its fine with me. Thanks for all your help on this. Learnt a lot. 
I hope you fix this bug soon:)
I'll start on the bug you filed just yesterday.

Comment 39

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3de1de0f5cbbdcf6cc04a8088181e5c5e8103e8

Comment 40

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

I'm deeply suspicious that the fix in that try job will not have the scopes to run a decision task.  At least right now, and possibly once we start assigning more fine-grained scopes.  We'll see..

Comment 41

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=13fa4a7c01c52bb316e283942557a54925f2d074

Comment 42

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

Am I cynical or just accurate?  You decide..

[task 2018-07-06T18:16:27.986Z] You do not have sufficient scopes. You are missing the following scopes:
[task 2018-07-06T18:16:27.986Z] 
[task 2018-07-06T18:16:27.986Z] ```
[task 2018-07-06T18:16:27.986Z] {
[task 2018-07-06T18:16:27.986Z]   "AllOf": [
[task 2018-07-06T18:16:27.986Z]     "assume:repo:hg.mozilla.org/try:branch:default",
[task 2018-07-06T18:16:27.986Z]     "in-tree:hook-action:project-gecko/in-tree-action-1-*"
[task 2018-07-06T18:16:27.986Z]   ]
[task 2018-07-06T18:16:27.986Z] }
[task 2018-07-06T18:16:27.986Z] ```

So I think that retriggering a decision task requires an actionPerm, which requires bug 1472792.  It also means bug 
1470622 can't go ahead yet, as sheriffs need to be able to retrigger decision tasks.

Comment 43

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

This still needs to wait until bug 1472792 is deployed in treeherder.

Comment 44

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

Attachment: Bug 1398277: add a special permission for retriggering decision tasks

Comment 45

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

Attachment: bug1398277.patch

Comment 46

[Brian Stack [:bstack]]

Comment on attachment 8994319 [details] [diff] [review]
bug1398277.patch

I wish all of these datestamps would have been  relative from the get-go. Assuming you ran flake8 on this, r+!

Comment 47

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

Haha, you know me so well.  Yes, flakes fixed.  This can't land until bug 1472792 lands in treeherder -- that's been waiting for two weeks now, and will wait another three until I'm back from PTO.

Comment 48

[Brian Stack [:bstack]]

Comment on attachment 8994315 [details]
Bug 1398277: add a special permission for retriggering decision tasks

Brian Stack [:bstack] has approved the revision.

Comment 49

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab58645e9230620ca45de3ec03ee9e61eb4a7cbf
Bug 1398277: special-case retriggering of tasks not in the taskgraph; r=bstack

Comment 50

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/ab58645e9230

Comment 51

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-release/rev/de2d8106904f

Comment 52

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/1aabfd210ab7