1247085 - Generate an empty task graph on pushes to try without a try syntax

Status: RESOLVED FIXED

(Taskcluster :: Services, defect)

Description

[Andrew Halberstadt [:ahal]]

See:
https://dxr.mozilla.org/mozilla-central/source/testing/taskcluster/mach_commands.py#309

Currently it will error out, but pushing to try without a try syntax is a workflow we want to start supporting. If this happens, we don't want any jobs being scheduled. See bug 1247072 comment 0 for an explanation why.

Comment 1

[Pete Moore [:pmoore][:pete]]

I think in this case, the decision task should just create an empty task graph, and docker worker should only extend the task graph if the extension is non-empty.

If you agree, this looks like it would be an in-tree change for gecko to the ./mach taskcluster-graph command, plus potentially a change to docker-worker (for special handling of empty task graph extensions). It might be that docker worker already handles this though.

Moving this to "Integration" for the gecko tree change (mach) change - if docker worker needs changing, we should create another bug for that.

@garndt: can you confirm if docker-worker handles empty task graph extensions ok (i.e. doesn't extend task graph in this case)?

Comment 2

[Greg Arndt [:garndt]]

Pete, why produce a file at all?  Either the mach target produces a file and docker-worker picks it up to extend the graph, or the file doesn't exist and the task logs will have a message that the file doesn't exist. This is the current behavior.

As far as docker-worker is concerned it will handle an empty (valid) json file (it needs to be "{}" I think), but will parse it and then call the scheduler endpoint to extend the graph.  Will the scheduler handle that or throw an error?

Also, I believe it has been discussed to move this out of the workers and the task would be responsible for using the proxy for scheduling the tasks rather than magic within the worker.

Comment 3

[Greg Arndt [:garndt]]

Sorry, realized that the decision task just pipes the output of the taskcluster-graph target to a file, so if it's valid empty json then extending should be a no-op I would hope.

Comment 4

[Greg Arndt [:garndt]]

Attachment: patch.diff

Something like this might work out.  It was just a quick hack and not completely tested, but didn't seem to break the world.

Comment 5

[Andrew Halberstadt [:ahal]]

Comment on attachment 8717878 [details] [diff] [review]
patch.diff

Review of attachment 8717878 [details] [diff] [review]:
-----------------------------------------------------------------

This looks reasonable to me.

Just to clarify, with this patch the decision task will remain green (due to sys.exit(0)), but it will still be possible to extend the task graph at a later date, correct?

::: testing/taskcluster/mach_commands.py
@@ +316,5 @@
> +        if not job_graph:
> +            sys.stderr.write(
> +                "No jobs were found for the given repository and commit message.\n" \
> +                "If pushing to Try, ensure that the commit message contains valid " \
> +                "try flags or Add Jobs from within Treeherder"

Technically I don't think Add Jobs works with taskcluster yet.. though it's something I want to push as a high priority.

Comment 6

[Andrew Halberstadt [:ahal]]

Is this patch ready to be reviewed? I don't mind taking over if there's more work to be done, though I'd need some pointers on how to test it.

Comment 7

[Greg Arndt [:garndt]]

This patch probably has bitrotted now that some things have changed in our mach target.  I can pick this up and work on it, but realistically it probably won't be until Monday.  Just let me know if I can help out.

Comment 8

[Greg Arndt [:garndt]]

Attachment: MozReview Request: Bug 1247085 - Generate empty graph when no try flags given r=wcosta

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

Comment 9

[Greg Arndt [:garndt]]

Comment on attachment 8724785 [details]
MozReview Request: Bug 1247085 - Generate empty graph when no try flags given r=wcosta

This should allow pushes to be made to try without try flags.  It will generate an empty set of tasks like this one: https://public-artifacts.taskcluster.net/BAHglzqPS6KILPJqwgHanw/0/public/graph.json

Comment 10

[Andrew Halberstadt [:ahal]]

Comment on attachment 8724785 [details]
MozReview Request: Bug 1247085 - Generate empty graph when no try flags given r=wcosta

Thanks, this looks perfect.

If you like I can land my patch in bug 1247072 that disables the hghook so I can test with a real live try push first. Or if you're confident this won't blow up, feel free to land now. Either way wfm.

Comment 11

[Greg Arndt [:garndt]]

Comment on attachment 8724785 [details]
MozReview Request: Bug 1247085 - Generate empty graph when no try flags given r=wcosta

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

Comment 12

[Greg Arndt [:garndt]]

I'm pretty sure this won't blow up, but I always do like testing against real things.  So if it's not a huge hassle to land the other changes first, that would be great.

Comment 13

[Wander Lairson Costa]

Comment on attachment 8724785 [details]
MozReview Request: Bug 1247085 - Generate empty graph when no try flags given r=wcosta

https://reviewboard.mozilla.org/r/37153/#review33757

Comment 14

[Andrew Halberstadt [:ahal]]

Sounds good, I'm a bit wrapped up in something, but I'll try and get to this by mid-week or so. I can take care of landing it after I do the try push.

Comment 15

[Andrew Halberstadt [:ahal]]

Here's the try run, I think it's working:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0c3314e01fe

Just to clarify, an empty task graph means no decision task is expected to run, right?

Comment 16

[Greg Arndt [:garndt]]

Sorry, it was my original intention that a decision task would still run, just would not schedule any tasks.

It looks like our integration component for scheduling these things has a check to see if try flags were given if it was a push to try:
https://github.com/taskcluster/mozilla-taskcluster/blob/master/src/jobs/treeherder_resultset.js#L85-L91

Which leads me to believe we could remove this line from the configuration but keep the logic there in case a branch ever wants to have that filtering:
https://github.com/taskcluster/mozilla-taskcluster/blob/master/src/config/default.yml#L74


Any ideas you can think of that might cause an issue? I don't immediately think of anything.

Comment 17

[Andrew Halberstadt [:ahal]]

That looks like it should work to me. Fwiw, I don't think we'll ever have a branch that requires try syntax again, so maybe that whole if block should just be deleted. I'll leave it up to you to decide.

Comment 18

[Greg Arndt [:garndt]]

I removed this config option from mozilla-taskcluster, would you be able to try again?

Comment 19

[Andrew Halberstadt [:ahal]]

Looks good!
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f41760fb3216

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/99420f88d57e

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/99420f88d57e