If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED

Status

Taskcluster
Integration
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ahal, Unassigned)

Tracking

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
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.
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)?
Component: Task Configuration → Integration
Flags: needinfo?(garndt)

Comment 2

2 years ago
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.
Flags: needinfo?(garndt)

Comment 3

2 years ago
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

2 years ago
Created attachment 8717878 [details] [diff] [review]
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.
(Reporter)

Comment 5

2 years ago
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.
Attachment #8717878 - Flags: feedback+
(Reporter)

Comment 6

2 years ago
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.
Flags: needinfo?(garndt)

Comment 7

2 years ago
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.
Flags: needinfo?(garndt)

Comment 8

2 years ago
Created attachment 8724785 [details]
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/
Attachment #8724785 - Flags: review?(wcosta)

Comment 9

2 years ago
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
Attachment #8724785 - Flags: feedback?(ahalberstadt)
(Reporter)

Comment 10

2 years ago
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.
Attachment #8724785 - Flags: feedback?(ahalberstadt) → feedback+
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/
Attachment #8724785 - Attachment description: MozReview Request: Bug 1247085 - Generate empty graph when no try flags given r=wcosta f=ahal → MozReview Request: Bug 1247085 - Generate empty graph when no try flags given r=wcosta
Attachment #8724785 - Flags: feedback+

Updated

2 years ago
Attachment #8724785 - Flags: feedback?(ahalberstadt)

Updated

2 years ago
Attachment #8724785 - Flags: feedback?(ahalberstadt)
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 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
Attachment #8724785 - Flags: review?(wcosta) → review+
(Reporter)

Comment 14

2 years ago
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.
(Reporter)

Comment 15

2 years ago
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?
Flags: needinfo?(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.
(Reporter)

Comment 17

2 years ago
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.
I removed this config option from mozilla-taskcluster, would you be able to try again?
Flags: needinfo?(garndt)
(Reporter)

Comment 19

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

Comment 20

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

Comment 21

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/99420f88d57e
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.