Bug 183222 - [webkitpy] Bugzilla class should use NetworkTransaction for network GET requests
Summary: [webkitpy] Bugzilla class should use NetworkTransaction for network GET requests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Aakash Jain
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-02-28 13:26 PST by Aakash Jain
Modified: 2018-03-08 11:14 PST (History)
10 users (show)

See Also:


Attachments
Proposed patch (11.23 KB, patch)
2018-03-02 13:23 PST, Aakash Jain
no flags Details | Formatted Diff | Diff
Updated patch without additional logging (10.29 KB, patch)
2018-03-02 14:54 PST, Aakash Jain
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aakash Jain 2018-02-28 13:26:51 PST
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).
Comment 1 Aakash Jain 2018-02-28 13:27:58 PST
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.
Comment 2 Aakash Jain 2018-02-28 15:45:05 PST
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
Comment 3 Aakash Jain 2018-03-02 12:49:12 PST
Updated title. NetworkTransaction class has the retry logic. Bugzilla class should use NetworkTransaction for the network operations.
Comment 4 Aakash Jain 2018-03-02 13:23:07 PST
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.
Comment 5 Dean Johnson 2018-03-02 13:38:15 PST
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.
Comment 6 Alexey Proskuryakov 2018-03-02 14:08:02 PST
> 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.
Comment 7 Aakash Jain 2018-03-02 14:54:39 PST
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.
Comment 8 Aakash Jain 2018-03-02 14:58:30 PST
> 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
Comment 9 Aakash Jain 2018-03-02 15:01:14 PST
> 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).
Comment 10 WebKit Commit Bot 2018-03-05 17:31:00 PST
Comment on attachment 334933 [details]
Updated patch without additional logging

Clearing flags on attachment: 334933

Committed r229296: <https://trac.webkit.org/changeset/229296>
Comment 11 WebKit Commit Bot 2018-03-05 17:31:02 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Radar WebKit Bug Importer 2018-03-05 17:33:09 PST
<rdar://problem/38161814>