Merge lp:~smoser/software-properties/trunk.lp-1779302-retry-recv-keys into lp:software-properties

[Scott Moser]

Retry on failed lookups of gpg keys.

Recent events have made keyserver.ubuntu.com (both hkp and https
interfaces) less reliable than they previously were.

The change here retries on failure to read from key server other than
a 404 error.

[lp:~smoser/software-properties/trunk.lp-1779302-retry-recv-keys]

1043. By Scott Moser on 2018-07-10

remove debug pdb imports and fix flake8 reports

1044. By Scott Moser on 2018-07-10

make sure to set/re-set err_msg and err

[Brian Murray (brian-murray)]

I've a couple of in-line questions.

review: Needs Information

[Seth Arnold (seth-arnold)]

retries seems a funny name for something that actually describes sleep duration; I didn't expect to see the retries=[] assignment. But that's just nit-picking..

Thanks

[Scott Moser (smoser)]

if retries is None:
   retries = []

that is just to make sure that
a.) retries is an iterable
b.) avoid modification of a default in parameter. Since None is a immutable but list() is not.

suggestions on the name?

retry_delays ?

[Brian Murray (brian-murray)]

I've an in-line comment about the HTTPError.

With regards to retries, I think retry_delays is a better name and maybe sleep_waits instead of sleeps but again this is just nitpicking.

[lp:~smoser/software-properties/trunk.lp-1779302-retry-recv-keys]

1045. By Scott Moser on 2018-07-16

update HTTPError comment per bdmurray review

1046. By Scott Moser on 2018-07-16

adjust for bdmurray feedback. Add docstring to get_info_from_https.

sleeps -> sleep_delays
retries -> retry_delays

[Scott Moser (smoser)]

I adjusted per both of Brian's suggestions.

It is hard to use 'add-apt-repository' in a loop and see that it
used retries at all because there is no logging or '-v' output
or anything in that path. So, I have a script that I'm testing with at:
  http://paste.ubuntu.com/p/7QZftQNCPv/

If you wait long enough you'll see a 'Succeeded on try 1' message,
or a failure that will mention trying 3 times.

$ python ./my.py --help

This is test script for merge proposal for bug 1779302 at
https://code.launchpad.net/~smoser/software-properties/trunk.lp-1779302-retry-recv-keys/+merge/349213

Run it like:

  python testme.py https://..../
    or
  python testme.py fingerprint
    or
  python testme.py

  fingerprint:
     use keyserver.ubuntu.com url to read fprint as add-apt-repository does.
  https://
     read the url provided

  if no argument given, default
  fingerprint=03A13844F2209DCECC02378B251157FC154A23ED

python can be python2 or python3.

[Scott Moser (smoser)]

Just for later info, my posted script in python2 eventually caught both a success-on-retry and an error-after-retry:

$ python ./my.py
Looking at url=https://keyserver.ubuntu.com/pks/lookup?op=get&options=mr&exact=on&search=0x03A13844F2209DCECC02378B251157FC154A23ED
. [0.597575]: 3137 retries-used=0
<snip>
. [0.545431]: 3137 retries-used=0
. [0.538139]: 3137 retries-used=0
. [0.549522]: 3137 retries-used=0
. [0.541063]: 3137 retries-used=0
. [0.548106]: 3137 retries-used=0
. [0.541548]: 3137 retries-used=0
. [6.445298]: 3137 retries-used=0
Returning delay 1
Returning delay 2
. [64.300604]: 3137 retries-used=2
Suceeded on try 3
Returning delay 1
Returning delay 2
Errored out, Used 2 retries
Traceback (most recent call last):
  File "./my.py", line 69, in <module>
    retry_delays=myiter)
  File "/home/smoser-public/src/software-properties/trunk.lp-1779302-retry-recv-keys/softwareproperties/ppa.py", line 96, in get_info_from_https
    data = func(lp_url=url, accept_json=accept_json, retry_delays=retry_delays)
  File "/home/smoser-public/src/software-properties/trunk.lp-1779302-retry-recv-keys/softwareproperties/ppa.py", line 199, in _get_https_content_pycurl
    original_error=err)
softwareproperties.ppa.PPAException: 'Error reading https://keyserver.ubuntu.com/pks/lookup?op=get&options=mr&exact=on&search=0x03A13844F2209DCECC02378B251157FC154A23ED (3 tries): response code 503'

=== modified file 'softwareproperties/ppa.py'
--- softwareproperties/ppa.py	2018-03-30 13:59:59 +0000
+++ softwareproperties/ppa.py	2018-07-16 17:34:48 +0000
@@ -28,6 +28,7 @@
 import shutil
 import subprocess
 import tempfile
+import time
 
 from gettext import gettext as _
 from threading import Thread
@@ -79,18 +80,26 @@
 def encode(s):
     return re.sub("[^a-zA-Z0-9_-]", "_", s)
 
-def get_info_from_https(url, accept_json):
-    if NEED_PYCURL:
-        # python2 has no cert verification so we need pycurl
-        data = _get_https_content_pycurl(url, accept_json)
-    else:
-        # python3 has cert verification so we can use the buildin urllib
-        data = _get_https_content_py3(url, accept_json)
+
+def get_info_from_https(url, accept_json, retry_delays=None):
+    """Return the content from url.
+    accept_json indicates that:
+        a.) Send header Accept: 'application/json'
+        b.) Instead of raw content, return json.loads(content)
+    retry_delays is None or an iterator (including list or tuple)
+        If it is None, no retries will be done.
+        If it is an iterator, each value is number of seconds to delay before
+        retrying.  For example, retry_delays=(3,5) means to try up to 3
+        times, with a 3s delay after first failure and 5s delay after second.
+        Retries will not be done on 404."""
+    func = _get_https_content_pycurl if NEED_PYCURL else _get_https_content_py3
+    data = func(lp_url=url, accept_json=accept_json, retry_delays=retry_delays)
     if accept_json:
         return json.loads(data)
     else:
         return data
 
+
 def get_info_from_lp(lp_url):
     return get_info_from_https(lp_url, True)
 
@@ -113,41 +122,81 @@
     return os.path.basename(get_info_from_lp(lp_url)["current_series_link"])
 
 
-def _get_https_content_py3(lp_url, accept_json):
-    try:
-        headers = {"Accept":" application/json"} if accept_json else {}
-        request = urllib.request.Request(str(lp_url), headers=headers)
-        lp_page = urllib.request.urlopen(request, cafile=LAUNCHPAD_PPA_CERT)
-        data = lp_page.read().decode("utf-8", "strict")
-    except (URLError, HTTPException) as e:
-        # HTTPException doesn't have a reason but might have a string
-        # representation
-        reason = hasattr(e, "reason") and e.reason or e
-        raise PPAException("Error reading %s: %s" % (lp_url, reason), e)
-    return data
-
-def _get_https_content_pycurl(lp_url, accept_json):
+def _get_https_content_py3(lp_url, accept_json, retry_delays=None):
+    if retry_delays is None:
+        retry_delays = []
+
+    trynum = 0
+    err = None
+    sleep_waits = iter(retry_delays)
+    headers = {"Accept": "application/json"} if accept_json else {}
+
+    while True:
+        trynum += 1
+        try:
+            request = urllib.request.Request(str(lp_url), headers=headers)
+            lp_page = urllib.request.urlopen(request,
+                                             cafile=LAUNCHPAD_PPA_CERT)
+            return lp_page.read().decode("utf-8", "strict")
+        except (HTTPException, URLError) as e:
+            err = PPAException(
+                "Error reading %s (%d tries): %s" % (lp_url, trynum, e.reason),
+                e)
+            # do not retry on 404. HTTPError is a subclass of URLError.
+            if isinstance(e, HTTPError) and e.code == 404:
+                break
+        try:
+            time.sleep(next(sleep_waits))
+        except StopIteration:
+            break
+
+    raise err
+
+
+def _get_https_content_pycurl(lp_url, accept_json, retry_delays=None):
     # this is the fallback code for python2
-    try:
-        callback = CurlCallback()
-        curl = pycurl.Curl()
-        curl.setopt(pycurl.SSL_VERIFYPEER, 1)
-        curl.setopt(pycurl.SSL_VERIFYHOST, 2)
-        curl.setopt(pycurl.WRITEFUNCTION, callback.body_callback)
-        if LAUNCHPAD_PPA_CERT:
-            curl.setopt(pycurl.CAINFO, LAUNCHPAD_PPA_CERT)
-        curl.setopt(pycurl.URL, str(lp_url))
-        if accept_json:
-            curl.setopt(pycurl.HTTPHEADER, ["Accept: application/json"])
-        curl.perform()
-        response = curl.getinfo(curl.RESPONSE_CODE)
-        curl.close()
-        data = callback.contents
-    except pycurl.error as e:
-        raise PPAException("Error reading %s: %s" % (lp_url, e), e)
-    if response != 200:
-        raise PPAException("Error reading %s: response code %i" % (lp_url, response))
-    return data
+    if retry_delays is None:
+        retry_delays = []
+
+    trynum = 0
+    sleep_waits = iter(retry_delays)
+
+    while True:
+        err_msg = None
+        err = None
+        trynum += 1
+        try:
+            callback = CurlCallback()
+            curl = pycurl.Curl()
+            curl.setopt(pycurl.SSL_VERIFYPEER, 1)
+            curl.setopt(pycurl.SSL_VERIFYHOST, 2)
+            curl.setopt(pycurl.WRITEFUNCTION, callback.body_callback)
+            if LAUNCHPAD_PPA_CERT:
+                curl.setopt(pycurl.CAINFO, LAUNCHPAD_PPA_CERT)
+            curl.setopt(pycurl.URL, str(lp_url))
+            if accept_json:
+                curl.setopt(pycurl.HTTPHEADER, ["Accept: application/json"])
+            curl.perform()
+            response = curl.getinfo(curl.RESPONSE_CODE)
+            curl.close()
+
+            if response != 200:
+                err_msg = "response code %i" % response
+        except pycurl.error as e:
+            err_msg = str(e)
+            err = e
+
+        if err_msg is None:
+            return callback.contents
+
+        try:
+            time.sleep(next(sleep_waits))
+        except StopIteration:
+            break
+
+    raise PPAException(
+        "Error reading %s (%d tries): %s" % (lp_url, trynum, err_msg),
+        original_error=err)
 
 
 def mangle_ppa_shortcut(shortcut):
@@ -199,7 +248,8 @@
             print("Error: signing key fingerprint does not exist")
             return False
 
-        return get_info_from_https(SKS_KEYSERVER % signing_key_fingerprint, accept_json=False)
+        return get_info_from_https(SKS_KEYSERVER % signing_key_fingerprint,
+                                   accept_json=False, retry_delays=(1, 2))
 
     def _minimize_key(self, key):
         p = self.gpg_cmd("import-minimal,import-export")