run-task doesn't preserve exit code

RESOLVED FIXED

Status

Firefox Build System
Task Configuration
RESOLVED FIXED
2 years ago
3 months ago

People

(Reporter: gps, Assigned: gps)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

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

Attachments

(2 attachments)

(Assignee)

Description

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

Comment 1

2 years ago
Created attachment 8776771 [details]
Bug 1291070 - Return process exit code properly;

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().
(Assignee)

Comment 3

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

Comment 5

2 years ago
Pushed by gszorc@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/51f838971d62
Return process exit code properly; r=glandium
(Assignee)

Updated

2 years ago
Blocks: 1291148

Comment 6

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

Comment 7

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

Comment 13

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/78e29c684909
Return process exit code properly; r=glandium
(Assignee)

Comment 15

2 years ago
Created attachment 8776962 [details]
Bug 1291070 - Tag and use decision image 0.1.4;

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+

Comment 17

2 years ago
Pushed by dmitchell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4509bcf4ba36
Tag and use decision image 0.1.4; r=dustin
(Assignee)

Comment 18

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

Comment 20

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/78e29c684909
https://hg.mozilla.org/mozilla-central/rev/4509bcf4ba36
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1291496

Updated

3 months ago
Product: TaskCluster → Firefox Build System
You need to log in before you can comment on or make changes to this bug.