Closed Bug 1304063 Opened 5 years ago Closed 4 years ago

Autophone - investigate and resolve failure to emit TEST-PASS, TEST-UNEXPECTED-FAIL for local S1S2Tests


(Testing :: Autophone, defect)

Not set


(Not tracked)



(Reporter: bc, Assigned: bc)




(5 files, 1 obsolete file)

Ever since we moved to having Treeherder parse logs, it became apparent that there is an issue with emitting to the logs on the production Ubuntu servers that is not seen locally by me on my Fedora workstation.


> 2016-09-19 05:21:14,362|16860|MainThread|root|DEBUG|S1S2TestJob|nexus-6p-6|20160919102718|remote-blank|analyze_logcat: base: 2016-09-19 08:21:09.542000, start: 916.0, throbber start: 1349.0, throbber stop: 3054.0, throbber time: 1705.0 
2016-09-19 05:21:14,575|16860|MainThread|root|INFO|S1S2TestJob|nexus-6p-6|20160919102718|remote-blank| TEST-PASS | http://remote/files/s1s2/blank.html | Ok.


016-09-20 06:09:08,297|6279|MainThread|root|INFO|S1S2TestJob|nexus-4-5|20160920115016|local-twitter|analyze_logcat: base_time: 09-20 13:09:07.399
2016-09-20 06:09:08,297|6279|MainThread|root|INFO|S1S2TestJob|nexus-4-5|20160920115016|local-twitter|analyze_logcat: new start_time: 09-20 13:09:07.850 gecko

I've tried to resolve this in various means. See bug 1300890.

Until this is resolved, the results are still being posted to phonedash but the blank s1s2tests will show orange if more than one measurement is missing. The Failure summary will be blank however.
minor lint type fixes.
Attachment #8834285 - Flags: review?(jmaher)
Attachment #8834286 - Flags: review?(jmaher)
I ran tests earlier with DEBUG logging enabled, but am running another test now with INFO logging. I have an external script which is using ADBAndroid.pkill using signal 11 (SIGSEGV) to kill org.mozilla.fennec on my local devices randomly. This generates crashes with dumps and stresses out the crash detecting and reporting. It is ongoing at the moment but may be finished by the time you see this.
Comment on attachment 8834285 [details] [diff] [review]

Review of attachment 8834285 [details] [diff] [review]:

linting patches are fun to review
Attachment #8834285 - Flags: review?(jmaher) → review+
Comment on attachment 8834286 [details] [diff] [review]

Review of attachment 8834286 [details] [diff] [review]:

this is a lot of changes, but ignoring the status messages shift, it is a simpler patch to review.  Lets go with this for now- I suspect in the longer term future if we run on taskcluster we will be revisiting some of this.
Attachment #8834286 - Flags: review?(jmaher) → review+
Comment on attachment 8834286 [details] [diff] [review]

>     def add_failure(self, testpath, test_status, text, testresult_status):
>+        """Report a test failure.
>+        :param testpath: A string identifying the test.
>+        :param test_status: A string identifying the type of test failure.
>+        :param text: A string decribing the failure.
>+        :param testresult_status: Test status to be reported to Treeherder.
>+        """
>         self.message = text
>         self.update_status(message=text)
>         testpath = _normalize_testpath(testpath)
>-        if testresult_status:
>+        if self.status == TreeherderStatus.SUCCESS:
>+            # Only use the first testresult_status when reporting to Treeherder.
>             self.status = testresult_status

I'm not sure about this restriction on only using the first failure status. If we get a TESTFAILED first, then a BUSTED we won't see the BUSTED. I think it would be better if I removed the

>+        if self.status == TreeherderStatus.SUCCESS:

and just set the self.status = testresult_status unconditionally. Is that ok?
Flags: needinfo?(jmaher)
fyi, beginning with the build at Mon Feb 6, 19:49:03 I included this change in the test. As you can see, we are getting BUSTED for tpn. Note the Android 4.0 / samsung-gs3-3 was charging before and all of its results will be after this change.
I am fine with ignoring the first failure by marking it as SUCCESS
Flags: needinfo?(jmaher)
Attached file status patch (obsolete) —
Actually I meant this.
I had some network issue which caused the build downloads to temporarily fail perhaps related to my setting up a virtual machine in kvm on my workstation at the same time the test was running. This resulted in some builds not being tested and which were not cleaned up. In addition there was an exception due to errors during a pull: should do a better job of reporting download errors and clean up when it can not download/install a build.

I'll come back to this after I spend some more quality time with awsf.
Assignee: nobody → bob
Make it formal.
Attachment #8834407 - Attachment is obsolete: true
Attachment #8834785 - Flags: review?(jmaher)
1. Tweaks PhoneTest teardown_job to not do the debug logcat.get if debugging is not set.

2. Tweak PhoneWorkerSubProcess install_build to keep trying for the full range of retries and to attempt to recover via ping.

3. Record failure to get the build after the maximum retries as a failure rather than just setting the test's status to BUSTED. Also call teardown_job for each test when we can't get the build so that Treeherder is updated.

Test Run:

first build was allowed to complete normally.

second build was triggered with added to the /etc/hosts to simulate a network failure.

subsequent builds were run while the fennec killer script was running to randomly kill fennec processes on the devices.
Attachment #8834788 - Flags: review?(jmaher)
Attachment #8834785 - Flags: review?(jmaher) → review+
Comment on attachment 8834788 [details] [diff] [review]

Review of attachment 8834788 [details] [diff] [review]:

this looks like a good set of changes
Attachment #8834788 - Flags: review?(jmaher) → review+
Well, we've had two instances where the failure messages were not emitted in the log in production.

So, whatever improvements we did make and there were several, we are still not getting the output on the Ubuntu 16.04 machines though we do on my Fedora 25 workstation.
Resolution: FIXED → ---
Blocks: 1347839
I've almost gotten the logging rewritten to only use Filehandlers on the main process and to send logging requests from the worker subprocesses to the main process using a SocketServer.StreamRequestHandler. There is an outstanding issue regarding carrying over the logs from one job to the next, but I expect to have that fixed soon.

Some example jobs for comparison:

Current logging using loglevel DEBUG:

Current logging using loglevel INFO:

New logging using loglevel DEBUG:

New logging using loglevel INFO:

Note that one change is that the Autophone log, the logcat log and the Unit test log if there is one are now combined into a single Autophone.log. This will make it much easier to see all errors in one place in the logviewer and will mean all errors are parsed by the log parser and are available sheriffing.

The outstanding issue for the new logging is that some of the logs are not being uploaded and are being combined with logs from other tests. I have an idea for the cause and will test the fix soonest.
Status update: We almost have it. Example runs:

All of the logs are successfully uploaded now but it appears the extra long failure lines with the autophone specific leading data especially with DEBUG logging enabled is causing Treeherder to miss failures due to assertion counts in the Unit tests. One last tweak to emit the "Treeherder"/"Tinderbox" failure message on a separate line and another test of a durable queue with  large number of messages and I think we'll have it.
The tweak I made to put the failure on its own line only affected autophone-only failure messages and not the unit tests. If the issue is the prefix causing Treeherder to miss the assertion failure then, that tweak was insufficient so I've reverted and restarted the server.

Note that with the patch from Bug 1353509, we successfully handled over 16,000 pending build finished messages from the queue though it took close to 50 minutes over my T1 connection. We may want to take the parts of the message handling that has to communicate with hg and taskcluster to determine build properties off of the same thread that consumes the messages but that is for another day.

For the curious you can see the current test run on for autoland, mozilla-inbound, mozilla-central, mozilla-beta and mozilla-release. Each device has 72+ builds queued up including opt and debug. I'll test usercancels then disable pulse and let the tests run. I'll update with exact links after I've disabled pulse and the revision ranges are stable.
This patch changes the logging in Autophone to:

1. Use a logging.handlers.SocketHandler in the worker sub process to communicate with a SocketServer.ThreadingTCPServer to send all logging requests from the non-main processes to the main process where all logging file i/o is performed.

2. Eliminates the use log global logger variables with the exception of the main process.

3. Uses the message queue and the log records to communicate to the main process synchronize and control log filehandler flushing, closing and initialization.

4. Improves handling of third partly library loggers which are no longer disabled.

5. Changed some logging messages from debug to info to get them into the normal production logs. We probably can do more but this is a start.

6. The logs are now consolidated into a single log. This allows Treeherder to parse all failure messages and will hopefully improve the user experience and help sheriffing.

7. The worker logs now propagate into the main process's log so that a full accounting for the main process and all worker processes is available on the host even when truncated worker logs are submitted to Treeherder.

8. Logs now contain steps which are parsed and linked by the log viewer.

I did attempt to use a separate Queue for the log control messages to the main process logging server, but had some difficulties and fell back to this approach which I had already demonstrated.

Current runs with loglevel INFO:

If this doesn't fix the error message handling in production, I will switch to Fedora during mozsfo.
Attachment #8865020 - Flags: review?(jmaher)
Comment on attachment 8865020 [details] [diff] [review]

Review of attachment 8865020 [details] [diff] [review]:

we are always fiddling with the logger and levels of logging, a full pass on this didn't raise any red flags
Attachment #8865020 - Flags: review?(jmaher) → review+

deployed 2017-05-06 ~11:25.
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Depends on: 1362899
This *did not* fix the issue with the perf tests. I hate ubuntu.
Depends on: 1366731
Depends on: 1367584
Reopening. We will resolve this by changing to Fedora in production.
Resolution: FIXED → ---
Blocks: 1375621
Closed: 4 years ago4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1381511
You need to log in before you can comment on or make changes to this bug.