Merge lp:~caio1982/ubuntu-system-image/remote-sis-generator-skips-metadata into lp:ubuntu-system-image/server

[Caio Begotti]

Feel free to replace this code with whatever you guys think is best but please do fix this bug :-)

I think the commit message is pretty descriptive so let me know if you need more info. We found a situation with Capomastro's instance of image server where only the first device in a channel list would have its Ubuntu rootfs's versioning right, so this address that problem as... well... we definitely need the rootfs versions to be right. People were chasing us for that last week :-(

Before: https://pastebin.canonical.com/132002/

After: https://pastebin.canonical.com/132001/

[lp:~caio1982/ubuntu-system-image/remote-sis-generator-skips-metadata]

273. By Caio Begotti on 2015-05-26

add missing debug logging to the existing (!!!) fixes for the problem we originally saw... so apparently only the remote s-i-s generator was behind this :-)

[Barry Warsaw (barry)]

Thanks for taking on this bug. I've added some comments, but none that you really need to address in this branch.

What I *do* think needs fixing before this can land is... tests! Can you please add a test for the bug? As Steve pointed out in my own branch, the best way to ensure this fixes the problem is to apply the test-adding diff, see the test fail, then add the fix and see it succeed.

The change looks okay on the face of it, but please add a test.

review: Needs Fixing

[Caio Begotti (caio1982)]

Thanks, Barry! Just filed https://bugs.launchpad.net/ubuntu-system-image/+bug/1461263 for the encoding bit. It might take me a while to write tests for this MR though as I honestly don't know the code much and I see all the other generators are missing tests for metadata (at least for version_detail) and they should have some.

[Barry Warsaw (barry)]

On Jun 02, 2015, at 09:05 PM, Caio Begotti wrote:

>Thanks, Barry! Just filed
>https://bugs.launchpad.net/ubuntu-system-image/+bug/1461263 for the encoding
>bit.

Thanks!

>It might take me a while to write tests for this MR though as I honestly
>don't know the code much and I see all the other generators are missing tests
>for metadata (at least for version_detail) and they should have some.

Yep. You can't get to 100% in one fell swoop, but every new test is an
investment in the future quality of the project.

Unmerged revisions

273. By Caio Begotti on 2015-05-26

add missing debug logging to the existing (!!!) fixes for the problem we originally saw... so apparently only the remote s-i-s generator was behind this :-)

272. By Caio Begotti on 2015-05-26

make the remote system-image generator respect the metadata of existing images

this is a really big problem when you have multiple devices in the same channel and both pull the same ubuntu rootfs, exactly like we have right now for arale... e.g. 'arale' and 'generic' devices both pulling a stable ubuntu rootfs from system-image.ubuntu.com

device 'generic' first pulls it just fine and loads its json with the metadata, including ubuntu='' versioning, but then when the 'arale' device imports its stuff, the ubuntu rootfs has been imported already so the path matches and the generator returns it without loading its json file

no ubuntu='' version string for any other devices in the channel :-(

fun fact: a 'generic' device is always processed first, even if it was created later because of a sorting done in the device list (i haven't really checked it so i assum it happens), so with the current code ONLY the 'generic' devices get their ubuntu='' version right for all those rootfs tarballs

271. By Caio Begotti on 2015-05-26

remove unnecessary (and too verbose) logging, as over time the server will be so crowded with images this might be totally useless and poluting

=== modified file 'lib/systemimage/generators.py'
--- lib/systemimage/generators.py	2015-04-10 17:35:24 +0000
+++ lib/systemimage/generators.py	2015-05-26 21:23:25 +0000
@@ -283,10 +283,13 @@
 
         # Return pre-existing entries
         if os.path.exists(path):
+            json_path = path.replace(".tar.xz", ".json")
+            logger.debug("Image exists, loading its {}".format(json_path))
             # Get the real version number (in case it got copied)
-            if os.path.exists(path.replace(".tar.xz", ".json")):
-                with open(path.replace(".tar.xz", ".json"), "r") as fd:
+            if os.path.exists(json_path):
+                with open(json_path, "r") as fd:
                     metadata = json.loads(fd.read())
+                    logger.debug("Metadata found: {}".format(metadata))
 
                 if "version_detail" in metadata:
                     version_detail = metadata['version_detail']
@@ -441,10 +444,13 @@
 
         # Return pre-existing entries
         if os.path.exists(path):
+            json_path = path.replace(".tar.xz", ".json")
+            logger.debug("Image exists, loading its {}".format(json_path))
             # Get the real version number (in case it got copied)
-            if os.path.exists(path.replace(".tar.xz", ".json")):
-                with open(path.replace(".tar.xz", ".json"), "r") as fd:
+            if os.path.exists(json_path):
+                with open(json_path, "r") as fd:
                     metadata = json.loads(fd.read())
+                    logger.debug("Metadata found: {}".format(metadata))
 
                 if "version_detail" in metadata:
                     version_detail = metadata['version_detail']
@@ -669,10 +675,13 @@
 
         # Return pre-existing entries
         if os.path.exists(path):
+            json_path = path.replace(".tar.xz", ".json")
+            logger.debug("Image exists, loading its {}".format(json_path))
             # Get the real version number (in case it got copied)
-            if os.path.exists(path.replace(".tar.xz", ".json")):
-                with open(path.replace(".tar.xz", ".json"), "r") as fd:
+            if os.path.exists(json_path):
+                with open(json_path, "r") as fd:
                     metadata = json.loads(fd.read())
+                    logger.debug("Metadata found: {}".format(metadata))
 
                 if "version_detail" in metadata:
                     version_detail = metadata['version_detail']
@@ -792,10 +801,13 @@
 
         # Return pre-existing entries
         if os.path.exists(path):
+            json_path = path.replace(".tar.xz", ".json")
+            logger.debug("Image exists, loading its {}".format(json_path))
             # Get the real version number (in case it got copied)
-            if os.path.exists(path.replace(".tar.xz", ".json")):
-                with open(path.replace(".tar.xz", ".json"), "r") as fd:
+            if os.path.exists(json_path):
+                with open(json_path, "r") as fd:
                     metadata = json.loads(fd.read())
+                    logger.debug("Metadata found: {}".format(metadata))
 
                 if "version_detail" in metadata:
                     version_detail = metadata['version_detail']
@@ -901,6 +913,7 @@
             if os.path.exists(old_path.replace(".tar.xz", ".json")):
                 with open(old_path.replace(".tar.xz", ".json"), "r") as fd:
                     metadata = json.loads(fd.read())
+                    logger.debug("Metadata found: {}".format(metadata))
 
                 if "version_detail" in metadata:
                     version_detail = metadata['version_detail']
@@ -919,10 +932,13 @@
 
         # Return pre-existing entries
         if os.path.exists(path):
+            json_path = path.replace(".tar.xz", ".json")
+            logger.debug("Image exists, loading its {}".format(json_path))
             # Get the real version number (in case it got copied)
-            if os.path.exists(path.replace(".tar.xz", ".json")):
-                with open(path.replace(".tar.xz", ".json"), "r") as fd:
+            if os.path.exists(json_path):
+                with open(json_path, "r") as fd:
                     metadata = json.loads(fd.read())
+                    logger.debug("Metadata found: {}".format(metadata))
 
                 if "version_detail" in metadata:
                     version_detail = metadata['version_detail']
@@ -965,10 +981,13 @@
 
         # Return pre-existing entries
         if os.path.exists(path):
+            json_path = path.replace(".tar.xz", ".json")
+            logger.debug("Image exists, loading its {}".format(json_path))
             # Get the real version number (in case it got copied)
-            if os.path.exists(path.replace(".tar.xz", ".json")):
-                with open(path.replace(".tar.xz", ".json"), "r") as fd:
+            if os.path.exists(json_path):
+                with open(json_path, "r") as fd:
                     metadata = json.loads(fd.read())
+                    logger.debug("Metadata found: {}".format(metadata))
 
                 if "version_detail" in metadata:
                     version_detail = metadata['version_detail']
@@ -1160,7 +1179,6 @@
     full_images = sorted([image for image in index_json['images']
                           if image['type'] == "full"],
                          key=lambda image: image['version'])
-    logger.debug("List of full images founds %s" % full_images)
 
     # No images
     if not full_images:
@@ -1176,6 +1194,17 @@
             logger.debug("Path generated: %s" % path)
 
             if os.path.exists(path):
+                json_path = path.replace(".tar.xz", ".json")
+                logger.debug("Image exists, loading its {}".format(json_path))
+                if os.path.exists(json_path):
+                    with open(json_path, "r") as fd:
+                        metadata = json.loads(fd.read())
+                        logger.debug("Metadata found: {}".format(metadata))
+
+                    if "version_detail" in metadata:
+                        environment['version_detail'].append(
+                            metadata['version_detail'])
+
                 return path
 
             # Create the target if needed
@@ -1224,6 +1253,7 @@
                 gpg.sign_file(conf, "image-signing", json_path)
                 with open(json_path, "r") as fd:
                     metadata = json.loads(fd.read())
+                    logger.debug("Metadata found: {}".format(metadata))
 
                 if "version_detail" in metadata:
                     environment['version_detail'].append(
@@ -1265,7 +1295,6 @@
     full_images = sorted([image for image in device.list_images()
                           if image['type'] == "full"],
                          key=lambda image: image['version'])
-    logger.debug("List of full images founds %s" % full_images)
 
     # No images
     if not full_images:
@@ -1283,6 +1312,7 @@
             if os.path.exists(path.replace(".tar.xz", ".json")):
                 with open(path.replace(".tar.xz", ".json"), "r") as fd:
                     metadata = json.loads(fd.read())
+                    logger.debug("Metadata found: {}".format(metadata))
 
                 if "version_detail" in metadata:
                     environment['version_detail'].append(