Bug 183222 – [webkitpy] Bugzilla class should use NetworkTransaction for network GET requests

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Aakash Jain]

commit-queue failed to land https://webkit-queues.webkit.org/patch/334757/commit-queue

Logs indicate network issue. We should add retry on network error on all the Bugzilla operations now.

2018-02-28 12:52:12,529 - Patch does not pass tests
2018-02-28 12:52:15,976 - Fetching: https://bugs.webkit.org/attachment.cgi?id=334757&action=edit
2018-02-28 12:52:16,255 - Fetching: https://bugs.webkit.org/show_bug.cgi?id=183171&ctype=xml&excludefield=attachmentdata
2018-02-28 12:52:16,522 - Running: webkit-patch --status-host=webkit-queues.webkit.org --bot-id=webkit-cq-01 build-and-test --no-clean --no-update --test --non-interactive --build-style=release --port=mac
2018-02-28 13:03:27,741 - Passed tests
2018-02-28 13:04:04,687 - Bug does not already exist for media/video-poster.html, creating.
2018-02-28 13:04:04,687 - Creating bug with title "Flaky Test: media/video-poster.html"
Traceback (most recent call last):
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/bot/queueengine.py", line 103, in run
    if not self._delegate.process_work_item(work_item):
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/commands/queues.py", line 340, in process_work_item
    if task.run():
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/bot/commitqueuetask.py", line 88, in run
    if not self._did_pass_tests_recently():
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/bot/commitqueuetask.py", line 72, in _did_pass_tests_recently
    return self._test_patch()
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py", line 374, in _test_patch
    return self._retry_layout_tests()
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py", line 305, in _retry_layout_tests
    self._report_flaky_tests(first_results.failing_test_results(), first_results_archive)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py", line 217, in _report_flaky_tests
    self._delegate.report_flaky_tests(self._patch, flaky_test_results, results_archive)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/commands/queues.py", line 403, in report_flaky_tests
    reporter.report_flaky_tests(patch, flaky_test_results, results_archive)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/bot/flakytestreporter.py", line 184, in report_flaky_tests
    flake_bug_id = self._create_bug_for_flaky_test(flaky_test, author_emails, latest_flake_message)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/bot/flakytestreporter.py", line 116, in _create_bug_for_flaky_test
    blocked="50856")
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py", line 711, in create_bug
    self.browser.open(config_urls.bug_server_url + "enter_bug.cgi?product=WebKit")
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_mechanize.py", line 203, in open
    return self._mech_open(url, data, timeout=timeout)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_mechanize.py", line 230, in _mech_open
    response = UserAgentBase.open(self, request, data)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_opener.py", line 193, in open
    response = urlopen(self, req, data)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_urllib2_fork.py", line 344, in _open
    '_open', req)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_urllib2_fork.py", line 332, in _call_chain
    result = func(*args)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_urllib2_fork.py", line 1170, in https_open
    return self.do_open(conn_factory, req)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_urllib2_fork.py", line 1118, in do_open
    raise URLError(err)
URLError: <urlopen error [Errno 60] Operation timed out>
2018-02-28 13:04:12,557 - Exception while preparing queue Sleeping until 2018-02-28 13:06:12 (120 seconds).

[Aakash Jain]

https://bugs.webkit.org/show_bug.cgi?id=183156 added retry on some of the network operations while talking to bugzilla, and that helped.

We should add retry in all the network operations in Bugzilla class.

[Aakash Jain]

One more examples:

https://webkit-queues.webkit.org/results/6711146

File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py", line 493, in fetch_attachment_contents

[Aakash Jain]

Updated title. NetworkTransaction class has the retry logic. Bugzilla class should use NetworkTransaction for the network operations.

[Aakash Jain]

Created attachment 334920 [details]
Proposed patch

Using method open_url() which uses NetworkTransaction for all self.browser.open() 

Also reverting changes done in https://trac.webkit.org/changeset/229064/webkit and using open_url() instead as this is cleaner approach.

[Dean Johnson]

Comment on attachment 334920 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=334920&action=review

I really like the philosophy of the patch (retry instead of failing completely If necessary), just have a small issue with the implementation of NetworkTransaction. Let's chat in-person Monday and see if we can find a better way to do this. If not, the benefit of the patch probably outweighs the new logging in NetworkTransaction.

> Tools/Scripts/webkitpy/common/net/networktransaction.py:58
> +                _log.warn('Received HTTP status {} while loading {}. Retrying in {} seconds...'.format(e.code, url, self._backoff_seconds))

Why make this change? Is there a way for us to bubble up e.<some_var> for the URL instead of requiring it to be passed into NetworkTransaction? I don't like that passing url is now required for useful logging.

> Tools/Scripts/webkitpy/common/net/networktransaction.py:62
> +                _log.warn('Received URLError: "{}" while loading {}. Retrying in {} seconds...'.format(e.reason, url, self._backoff_seconds))

Ditto.

[Alexey Proskuryakov]

> Using method open_url() which uses NetworkTransaction for all self.browser.open() 

Does this only apply retries to GET, never POST?

Given that we can't retry POST, the only real fix is to fix the network.

[Aakash Jain]

Created attachment 334933 [details]
Updated patch without additional logging

Patch without additional logging. Will add logs in a separate patch. Let's fix the urgent issue causing visible EWS problems first.

[Aakash Jain]

> I really like the philosophy of the patch (retry instead of failing
> completely If necessary), just have a small issue with the implementation of
> NetworkTransaction. Let's chat in-person Monday and see if we can find a
> better way to do this. If not, the benefit of the patch probably outweighs
> the new logging in NetworkTransaction.
Sure. For now, I have removed the additional logging. Let's discuss on Monday and add that in a separate patch.

> Does this only apply retries to GET, never POST?
Yes, this apply retries only to POST (only to browser.open(), not to browser.submit())
Reference for open(): http://mechanize.readthedocs.io/en/latest/browser_api.html#mechanize.Browser.open

[Aakash Jain]

> Yes, this apply retries only to POST (only to browser.open(), not to
I meant only to GET. (wish bugzilla has option to edit comments).

[WebKit Commit Bot]

Comment on attachment 334933 [details]
Updated patch without additional logging

Clearing flags on attachment: 334933

Committed r229296: <https://trac.webkit.org/changeset/229296>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/38161814>