Merge lp:~caio1982/ubuntu-system-image/sis-skip404 into lp:ubuntu-system-image/server

[Caio Begotti]

Image server goes crazy if the configured image is not there at the time of importing. What happens is that it fetches an Apache error page (HTML) and try to import it, which surprisingly works. Then when the image is finally there it tries to generate a delta, and that obviously fails because one of the files (the source one) is not an image.

The only way to recover from this state is to manually intervene and remove the first processed files, json, images etc. This is very problematic for us in PES.

Although this MR is kind of hackish it does the job for running image server on Prodstack. I understand a more robust solution would be needed to handle corner cases (perhaps using Requests instead of urllib, to start with).

[Stéphane Graber (stgraber)]

review: Needs Fixing

[lp:~caio1982/ubuntu-system-image/sis-skip404]

262. By Caio Begotti on 2015-03-11

catch all http errors above 400 instead and make the logging message more generic to reflect it

263. By Caio Begotti on 2015-03-12

transform all calls to urlopen and urlretrieve in wrapper calls so we catch all timeouts and >= 400 http errors better

264. By Caio Begotti on 2015-03-12

fix flake errors due to leftovers from previous commit

[Caio Begotti (caio1982)]

Stephane, can you tell me if this is more or less how you imaged it to be like? If it is I'll get to work on the tests as they are failing now because of these changes. If it's not what you asked for then I think I'll just stick to my hack in a private branch as I believe it would be more benefit to have more time to convert it all to use Requests.

[lp:~caio1982/ubuntu-system-image/sis-skip404]

265. By Caio Begotti on 2015-03-12

add more debug logging to the methods we are wrapping around

[Stéphane Graber (stgraber)]

This change causes clear behavioral changes on error, please fix.

review: Needs Fixing

[Stéphane Graber (stgraber)]

> Stephane, can you tell me if this is more or less how you imaged it to be
> like? If it is I'll get to work on the tests as they are failing now because
> of these changes. If it's not what you asked for then I think I'll just stick
> to my hack in a private branch as I believe it would be more benefit to have
> more time to convert it all to use Requests.

This is basically what I imagined except for the part where you no longer return None, which will cause a crash.

Unmerged revisions

265. By Caio Begotti on 2015-03-12

add more debug logging to the methods we are wrapping around

264. By Caio Begotti on 2015-03-12

fix flake errors due to leftovers from previous commit

263. By Caio Begotti on 2015-03-12

transform all calls to urlopen and urlretrieve in wrapper calls so we catch all timeouts and >= 400 http errors better

262. By Caio Begotti on 2015-03-11

catch all http errors above 400 instead and make the logging message more generic to reflect it

261. By Caio Begotti on 2015-03-11

make s-i-s a bit smarted by returning none if the actual file was not found, otherwise it will process a dummy html page with a 404 error message inside it... this is more like a hack because using urllib these days seems already wrong and i dont have the time to fully fix how all files are urlopened or urlretrieved in its code

=== modified file 'lib/systemimage/generators.py'
--- lib/systemimage/generators.py	2015-03-11 20:55:00 +0000
+++ lib/systemimage/generators.py	2015-03-12 14:10:25 +0000
@@ -35,6 +35,56 @@
 CACHE = {}
 
 
+def _urlopen(url):
+    res = None
+    old_timeout = socket.getdefaulttimeout()
+    socket.setdefaulttimeout(5)
+
+    try:
+        res = urlopen(url)
+    except socket.timeout:
+        return None
+    except IOError:
+        return None
+
+    socket.setdefaulttimeout(old_timeout)
+
+    # Make sure it's there before retrieval
+    if res.getcode() >= 400:
+        logger.debug("Server side error, skipping %s" % url)
+        return None
+
+    logger.debug("Returning URL opening with %s" % res)
+    return res
+
+
+def _urlretrieve(url, save):
+    res = None
+    old_timeout = socket.getdefaulttimeout()
+    socket.setdefaulttimeout(5)
+
+    try:
+        res = urlretrieve(url, save)
+    except socket.timeout:
+        if os.path.isfile(save):
+            os.remove(save)
+        elif os.path.isdir(save):
+            shutil.rmtree(save)
+        return None
+    except IOError:
+        if os.path.isfile(save):
+            os.remove(save)
+        elif os.path.isdir(save):
+            shutil.rmtree(save)
+        return None
+
+    socket.setdefaulttimeout(old_timeout)
+
+    logger.debug("Save path requested was %s" % save)
+    logger.debug("Returning retrieval with %s" % res)
+    return res
+
+
 def list_versions(cdimage_path):
     return sorted([version for version in os.listdir(cdimage_path)
                    if version not in ("pending", "current")],
@@ -832,15 +882,7 @@
     if "monitor" in options or version:
         if not version:
             # Grab the current version number
-            old_timeout = socket.getdefaulttimeout()
-            socket.setdefaulttimeout(5)
-            try:
-                version = urlopen(options['monitor']).read().strip()
-            except socket.timeout:
-                return None
-            except IOError:
-                return None
-            socket.setdefaulttimeout(old_timeout)
+            version = _urlopen(options['monitor']).read().strip()
 
             # Validate the version number
             if not version or len(version.split("\n")) > 1:
@@ -892,17 +934,7 @@
 
     # Grab the real thing
     tempdir = tempfile.mkdtemp()
-    old_timeout = socket.getdefaulttimeout()
-    socket.setdefaulttimeout(5)
-    try:
-        urlretrieve(url, os.path.join(tempdir, "download"))
-    except socket.timeout:
-        shutil.rmtree(tempdir)
-        return None
-    except IOError:
-        shutil.rmtree(tempdir)
-        return None
-    socket.setdefaulttimeout(old_timeout)
+    _urlretrieve(url, os.path.join(tempdir, "download"))
 
     # Hash it if we don't have a version number
     if not version:
@@ -1067,16 +1099,8 @@
         device_name = options['device']
 
     # Fetch and validate the remote channels.json
-    old_timeout = socket.getdefaulttimeout()
-    socket.setdefaulttimeout(5)
-    try:
-        channel_json = json.loads(urlopen("%s/channels.json" %
-                                          base_url).read().decode().strip())
-    except socket.timeout:
-        return None
-    except IOError:
-        return None
-    socket.setdefaulttimeout(old_timeout)
+    channel_json = json.loads(_urlopen("%s/channels.json" %
+                                       base_url).read().decode().strip())
 
     if channel_name not in channel_json:
         return None
@@ -1095,15 +1119,7 @@
                            [device_name]['index'])
 
     # Fetch and validate the remote index.json
-    old_timeout = socket.getdefaulttimeout()
-    socket.setdefaulttimeout(5)
-    try:
-        index_json = json.loads(urlopen(index_url).read().decode())
-    except socket.timeout:
-        return None
-    except IOError:
-        return None
-    socket.setdefaulttimeout(old_timeout)
+    index_json = json.loads(_urlopen(index_url).read().decode())
 
     # Grab the list of full images
     full_images = sorted([image for image in index_json['images']
@@ -1130,18 +1146,7 @@
 
             # Grab the file
             file_url = "%s/%s" % (base_url, file_entry['path'])
-            socket.setdefaulttimeout(5)
-            try:
-                urlretrieve(file_url, path)
-            except socket.timeout:
-                if os.path.exists(path):
-                    os.remove(path)
-                return None
-            except IOError:
-                if os.path.exists(path):
-                    os.remove(path)
-                return None
-            socket.setdefaulttimeout(old_timeout)
+            _urlretrieve(file_url, path)
 
             if "keyring" in options:
                 if not tools.repack_recovery_keyring(conf, path,
@@ -1153,18 +1158,9 @@
             gpg.sign_file(conf, "image-signing", path)
 
             # Attempt to grab an associated json
-            socket.setdefaulttimeout(5)
             json_path = path.replace(".tar.xz", ".json")
             json_url = file_url.replace(".tar.xz", ".json")
-            try:
-                urlretrieve(json_url, json_path),
-            except socket.timeout:
-                if os.path.exists(json_path):
-                    os.remove(json_path)
-            except IOError:
-                if os.path.exists(json_path):
-                    os.remove(json_path)
-            socket.setdefaulttimeout(old_timeout)
+            _urlretrieve(json_url, json_path)
 
             if os.path.exists(json_path):
                 gpg.sign_file(conf, "image-signing", json_path)