Closed Bug 1291070 Opened 3 years ago Closed 3 years ago

run-task doesn't preserve exit code

Categories

(Firefox Build System :: Task Configuration, task)

task
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: gps, Assigned: gps)

References

Details

Attachments

(2 files)

https://tools.taskcluster.net/task-inspector/#JwegKrBNR02fojVRKDAhAA/0 demonstrates a task failure that resulted in exit code 0. I believe the bug is that we're not calling poll() on the process object.
Before, we were returning None, which gets converted to 0.
Derp.

Also fix a flake8 failure introduced by 9f5fbb3066c9.

We'll also need to generate a new decision image. But that will
require someone with TC privileges to be around. That can be done
in a separate commit to unblock this from landing and fixing
consumers of run-task that aren't the decision image.

Review commit: https://reviewboard.mozilla.org/r/68440/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68440/
Attachment #8776771 - Flags: review?(mh+mozilla)
https://reviewboard.mozilla.org/r/68440/#review65562

::: testing/docker/recipes/run-task:64
(Diff revision 1)
>          if data == '':
>              break
>  
>          print_line(prefix, data)
>  
> -    return p.returncode
> +    return p.poll()

This seems about as racy as returning p.returncode. You want p.wait().
Comment on attachment 8776771 [details]
Bug 1291070 - Return process exit code properly;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68440/diff/1-2/
Comment on attachment 8776771 [details]
Bug 1291070 - Return process exit code properly;

https://reviewboard.mozilla.org/r/68440/#review65568
Attachment #8776771 - Flags: review?(mh+mozilla) → review+
Pushed by gszorc@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/51f838971d62
Return process exit code properly; r=glandium
Blocks: 1291148
Sorry had to back out your push due to Taskgraph Test failure. e.g., https://treeherder.mozilla.org/logviewer.html#?job_id=1278585&repo=autoland#L287
Flags: needinfo?(gps)
Backout by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ea6e87bbd03e
Backed out changeset 51f838971d62 for Taskgraph test failure
(In reply to Iris Hsiao [:ihsiao] from comment #6)
> Sorry had to back out your push due to Taskgraph Test failure. e.g.,
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=1278585&repo=autoland#L287

This is one of the rare case where backing out was the wrong thing to do, because you just end up hiding a real problem, and possible future problems. The reason being, what this bug fixes is errors being ignored. So what happened is that what you backed out was just revealing an error that was ignored on preceding pushes. For instance, you'll find the same error in the log for the taskgraph test log on the previous push, except the job is not marked as failed because of this bug. 

https://treeherder.mozilla.org/logviewer.html#?job_id=1276396&repo=autoland
The error is:
   AssertionError: 'e61e675ce05e8c11424437db3f1004079374c1a5fe6ad6800346cebe137b0797' != u'f13cde7fef96593671a41f81f9f0871c929a7dcba41d5a2dde1e8e1576b2bca0'

https://hg.mozilla.org/integration/autoland/rev/aac97e9f9addbc974882a05253149ee646494415
changed that test.
Glandium's right on all points.  The hash failure is from bug 1290611.  This bug can be re-landed, and let's roll the decision image version bump (bug 1291148) in here while doing so.  Greg, I'll wait for your go-ahead and a sha256 to push to docker hub.
Duplicate of this bug: 1291148
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/78e29c684909
Return process exit code properly; r=glandium
To pull in run-task exit code fix.

Review commit: https://reviewboard.mozilla.org/r/68576/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68576/
Attachment #8776962 - Flags: review?(dustin)
Comment on attachment 8776962 [details]
Bug 1291070 - Tag and use decision image 0.1.4;

https://reviewboard.mozilla.org/r/68576/#review65660
Attachment #8776962 - Flags: review?(dustin) → review+
Pushed by dmitchell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4509bcf4ba36
Tag and use decision image 0.1.4; r=dustin
Fixes landed. Autoland repo is green. Nothing left to do here.
Flags: needinfo?(gps)
I think this is going to cause flake8 bustage on central when it gets merged in as an infraction slipped in from inbound in the meantime.

TEST-UNEXPECTED-ERROR | taskcluster/taskgraph/task/docker_image.py:77:100 | line too long (100 > 99 characters) (E501)

From:
https://public-artifacts.taskcluster.net/G3KJ131PSJSzmK5Z1rBxVA/0/public/logs/live_backing.log
https://hg.mozilla.org/mozilla-central/rev/78e29c684909
https://hg.mozilla.org/mozilla-central/rev/4509bcf4ba36
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Duplicate of this bug: 1291496
Product: TaskCluster → Firefox Build System
You need to log in before you can comment on or make changes to this bug.