Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2868703002:
Retry browser startup in desktop_browser_finder. (Closed)
|
Created:
3 years, 7 months ago by Ken Russell (switch to Gerrit) Modified:
3 years, 7 months ago Reviewers:
nednguyen CC:
catapult-reviews_chromium.org, telemetry-reviews_chromium.org, eyaich1, eakuefner, ashleymarie1 Target Ref:
refs/heads/master Project:
catapult Visibility:
Public. |
DescriptionRetry browser startup in desktop_browser_finder.
Intermittently, the browser appears to hang upon startup on multiple
platforms, including while running performance tests. Make
desktop_browser_finder retry starting the browser up to three times.
Tested by injecting random "crashes" and hangs to the browser's main
loop, and ensuring that the browser was started reliably. Also tested
having the browser fail to start 3 times in a row, and ensured that
the fall-through failure case works as expected.
BUG=chromium:718300
Review-Url: https://codereview.chromium.org/2868703002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/030963fb8869548243807cdb0a2dafe5c76495fa
Patch Set 1 #
Total comments: 6
Patch Set 2 : Addressed review feedback from nednguyen. #
Total comments: 1
Patch Set 3 : Stop fetching tab. Clarify retry messages. #Messages
Total messages: 17 (8 generated)
kbr@chromium.org changed reviewers: + nednguyen@google.com
Note: this was mainly ported straight from the code eyaich@ added some time ago here: https://cs.chromium.org/chromium/src/content/test/gpu/gpu_tests/gpu_integrati... which has been working well. This was tested by injecting failures in Chrome and commenting out the retry loop in gpu_integration_test.py. This retry logic seems to be working at this level. However, I probably didn't inject failures at exactly the same points where they're seen in the real world (like after Telemetry establishes its WebSocket connection to the browser). I couldn't figure out an easy way to write a unit test for this. It would probably require faking out more of the browser's internals so that PossibleDesktopBrowser could bring up a fake browser. PTAL. Thanks.
lgtm with some nits to reduce the log. https://codereview.chromium.org/2868703002/diff/1/telemetry/telemetry/interna... File telemetry/telemetry/internal/backends/chrome/desktop_browser_finder.py (right): https://codereview.chromium.org/2868703002/diff/1/telemetry/telemetry/interna... telemetry/telemetry/internal/backends/chrome/desktop_browser_finder.py:68: restart = 'Starting browser, attempt %d of %d' % ((x + 1), num_retries) Can you remove this the line below. We shouldn't add logging in case there is no retry. https://codereview.chromium.org/2868703002/diff/1/telemetry/telemetry/interna... telemetry/telemetry/internal/backends/chrome/desktop_browser_finder.py:83: logging.warning('Started browser successfully.') I think this log is not needed. https://codereview.chromium.org/2868703002/diff/1/telemetry/telemetry/interna... telemetry/telemetry/internal/backends/chrome/desktop_browser_finder.py:86: logging.warning('Browser creation failed, retrying') logging.warning('Browser creation failed, retrying (attempt %d of %d)', x + 1, num_retries)
On 2017/05/07 23:47:44, nednguyen wrote: > lgtm with some nits to reduce the log. > > https://codereview.chromium.org/2868703002/diff/1/telemetry/telemetry/interna... > File telemetry/telemetry/internal/backends/chrome/desktop_browser_finder.py > (right): > > https://codereview.chromium.org/2868703002/diff/1/telemetry/telemetry/interna... > telemetry/telemetry/internal/backends/chrome/desktop_browser_finder.py:68: > restart = 'Starting browser, attempt %d of %d' % ((x + 1), num_retries) > Can you remove this the line below. We shouldn't add logging in case there is no > retry. > > https://codereview.chromium.org/2868703002/diff/1/telemetry/telemetry/interna... > telemetry/telemetry/internal/backends/chrome/desktop_browser_finder.py:83: > logging.warning('Started browser successfully.') > I think this log is not needed. > > https://codereview.chromium.org/2868703002/diff/1/telemetry/telemetry/interna... > telemetry/telemetry/internal/backends/chrome/desktop_browser_finder.py:86: > logging.warning('Browser creation failed, retrying') > logging.warning('Browser creation failed, retrying (attempt %d of %d)', x + 1, > num_retries) Also after this, I think we can clean up https://cs.chromium.org/chromium/src/content/test/gpu/gpu_tests/gpu_integrati... ?
> Also after this, I think we can clean up > https://cs.chromium.org/chromium/src/content/test/gpu/gpu_tests/gpu_integrati... > ? Yes, I'll remove that retry loop as soon as this rolls in. https://codereview.chromium.org/2868703002/diff/1/telemetry/telemetry/interna... File telemetry/telemetry/internal/backends/chrome/desktop_browser_finder.py (right): https://codereview.chromium.org/2868703002/diff/1/telemetry/telemetry/interna... telemetry/telemetry/internal/backends/chrome/desktop_browser_finder.py:68: restart = 'Starting browser, attempt %d of %d' % ((x + 1), num_retries) On 2017/05/07 23:47:43, nednguyen wrote: > Can you remove this the line below. We shouldn't add logging in case there is no > retry. Yes, done. https://codereview.chromium.org/2868703002/diff/1/telemetry/telemetry/interna... telemetry/telemetry/internal/backends/chrome/desktop_browser_finder.py:83: logging.warning('Started browser successfully.') On 2017/05/07 23:47:43, nednguyen wrote: > I think this log is not needed. OK, removed. https://codereview.chromium.org/2868703002/diff/1/telemetry/telemetry/interna... telemetry/telemetry/internal/backends/chrome/desktop_browser_finder.py:86: logging.warning('Browser creation failed, retrying') On 2017/05/07 23:47:43, nednguyen wrote: > logging.warning('Browser creation failed, retrying (attempt %d of %d)', x + 1, > num_retries) Done, slightly rephrased.
The CQ bit was checked by kbr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com Link to the patchset: https://codereview.chromium.org/2868703002/#ps20001 (title: "Addressed review feedback from nednguyen.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Catapult Mac Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Ma...)
https://codereview.chromium.org/2868703002/diff/20001/telemetry/telemetry/int... File telemetry/telemetry/internal/backends/chrome/desktop_browser_finder.py (right): https://codereview.chromium.org/2868703002/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/backends/chrome/desktop_browser_finder.py:79: _ = returned_browser.tabs[0] The fetching of the zeroth tab is causing DevToolsClientBackendTest.testGetUpdatedInspectableContexts to fail. Even if this tab is fetched and then immediately closed, this test still fails. I'm removing this part of the CL. It looks like it still detects whether the browser came up successfully.
The CQ bit was checked by kbr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com Link to the patchset: https://codereview.chromium.org/2868703002/#ps40001 (title: "Stop fetching tab. Clarify retry messages.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494278798670870,
"parent_rev": "3a50234ab81f55c15fe6913a3b0ecdbf3a655529", "commit_rev":
"030963fb8869548243807cdb0a2dafe5c76495fa"}
Message was sent while issue was closed.
Description was changed from ========== Retry browser startup in desktop_browser_finder. Intermittently, the browser appears to hang upon startup on multiple platforms, including while running performance tests. Make desktop_browser_finder retry starting the browser up to three times. Tested by injecting random "crashes" and hangs to the browser's main loop, and ensuring that the browser was started reliably. Also tested having the browser fail to start 3 times in a row, and ensured that the fall-through failure case works as expected. BUG=chromium:718300 ========== to ========== Retry browser startup in desktop_browser_finder. Intermittently, the browser appears to hang upon startup on multiple platforms, including while running performance tests. Make desktop_browser_finder retry starting the browser up to three times. Tested by injecting random "crashes" and hangs to the browser's main loop, and ensuring that the browser was started reliably. Also tested having the browser fail to start 3 times in a row, and ensured that the fall-through failure case works as expected. BUG=chromium:718300 Review-Url: https://codereview.chromium.org/2868703002 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
