Merge lp:~sil2100/ubuntu-system-image/server-per_device_redirect into lp:ubuntu-system-image/server

[Łukasz Zemczak]

Allow adding per-device channel redirects, allowing redirecting single devices out of a channel to a different one - similar to the normal channel redirects. This required some additional care-handling in multiple places.

Allow adding per-device channel redirects, allowing redirecting single devices out of a channel to a different one - similar to the normal channel redirects. This required some additional care-handling in multiple places.

[Barry Warsaw (barry)]

LGTM, though I'd really like to do some real-world testing with a device to make sure that the redirect is doing what it's supposed to do on the phone.

review: Approve

[Łukasz Zemczak (sil2100)]

Yeah, agreed on that! Will not merge this just yet, will test it locally on a device here first, then I'll deploy in production and do some more testing on a completely detached set of channels.

[Łukasz Zemczak (sil2100)]

Ok, so I did as many tests as possible locally. Sadly I couldn't do an upgrade-test since I could not force ubuntu-download-manager to accept my local self-signed key (even when using the -self-signed-certs <DIR> parameter). But at least the channels.json modification parts are working and a fresh flash of a channel with the redirected device results in pulling in the right files and channel info (from the target channel).

Merging this in and then proceeding with testing on completely new test-related channels.

=== modified file 'lib/systemimage/tests/test_tree.py'
--- lib/systemimage/tests/test_tree.py	2015-05-14 22:33:12 +0000
+++ lib/systemimage/tests/test_tree.py	2016-06-20 12:44:28 +0000
@@ -389,6 +389,103 @@
         self.assertEqual(device.list_images(), target.list_images())
 
     @unittest.skipUnless(HAS_TEST_KEYS, MISSING_KEYS_WARNING)
+    def test_per_device_redirect(self):
+        """ """
+        test_tree = tree.Tree(self.config)
+
+        # Create some channels
+        test_tree.create_channel("parent")
+        test_tree.create_channel("redirect")
+
+        # Create the required devices
+        test_tree.create_device("parent", "device")
+        test_tree.create_device("parent", "other")
+        test_tree.create_device("redirect", "other")
+
+        # Test standard failure cases
+        self.assertRaises(KeyError, 
+                          test_tree.create_per_device_channel_redirect,
+                          "device", "redirect1", "parent")
+        self.assertRaises(KeyError, 
+                          test_tree.create_per_device_channel_redirect,
+                          "other", "redirect", "parent")
+        self.assertRaises(KeyError,
+                          test_tree.create_per_device_channel_redirect,
+                          "device", "redirect", "parent1")
+        self.assertRaises(KeyError, 
+                          test_tree.create_per_device_channel_redirect,
+                          "device2", "redirect", "parent")
+
+        # Create the device channel redirect
+        test_tree.create_per_device_channel_redirect(
+            "device", "redirect", "parent")
+
+        # Publish an image
+
+        # # First file
+        first = os.path.join(self.config.publish_path, "parent/device/full")
+        open(first, "w+").close()
+        gpg.sign_file(self.config, "image-signing", first)
+
+        # # Second file
+        second = os.path.join(self.config.publish_path,
+                              "parent/device/version-1234.tar.xz")
+
+        tools.generate_version_tarball(self.config, "parent", "test", "1234",
+                                       second.replace(".xz", ""))
+        tools.xz_compress(second.replace(".xz", ""))
+        os.remove(second.replace(".xz", ""))
+        gpg.sign_file(self.config, "image-signing", second)
+
+        with open(second.replace(".tar.xz", ".json"), "w+") as fd:
+            metadata = {}
+            metadata['channel.ini'] = {}
+            metadata['channel.ini']['version_detail'] = "test"
+            fd.write(json.dumps(metadata))
+        gpg.sign_file(self.config, "image-signing",
+                      second.replace(".tar.xz", ".json"))
+
+        # # Adding the entry
+        device = test_tree.get_device("parent", "device")
+        device.create_image("full", 1234, "abc",
+                            ["parent/device/full",
+                             "parent/device/version-1234.tar.xz"])
+        device.set_phased_percentage(1234, 50)
+
+        # # Get the target
+        target = test_tree.get_device("redirect", "device")
+
+        # Confirm the fs layout
+        self.assertTrue(os.path.exists(os.path.join(
+            self.config.publish_path, "redirect")))
+        self.assertFalse(os.path.exists(os.path.join(
+            self.config.publish_path, "redirect", "device")))
+        self.assertTrue(os.path.exists(os.path.join(
+            self.config.publish_path, "parent", "device")))
+        self.assertEqual(device.list_images(), target.list_images())
+
+        # Check the channels index
+        channels = test_tree.list_channels()
+        self.assertIn("other", channels['redirect']['devices'])
+        self.assertEqual(
+            channels['redirect']['devices']['other']['index'],
+            "/redirect/other/index.json")
+        self.assertIn("device", channels['redirect']['devices'])
+        self.assertEqual(
+            channels['redirect']['devices']['device']['index'],
+            "/parent/device/index.json")
+        self.assertIn(
+            "redirect",
+            channels['redirect']['devices']['device'])
+
+        # Try removing a per-channel redirect
+        self.assertTrue(test_tree.remove_device("redirect", "device"))
+
+        # Confirm files are not removed
+        self.assertTrue(os.path.exists(os.path.join(
+            self.config.publish_path, "parent", "device", "index.json")))
+
+    @unittest.skipUnless(HAS_TEST_KEYS, MISSING_KEYS_WARNING)
     def test_redirect_alias(self):
         # LP: #1455119 - a channel which is both an alias and a redirect is
         # culled from sync_aliases().
@@ -412,6 +509,57 @@
         self.assertFalse(mock.called)
 
     @unittest.skipUnless(HAS_TEST_KEYS, MISSING_KEYS_WARNING)
+    def test_cleanup_device_redirects(self):
+        test_tree = tree.Tree(self.config)
+
+        # Create some channels and devices
+        test_tree.create_channel("parent")
+        test_tree.create_channel("redirect1")
+        test_tree.create_channel("redirect2")
+        test_tree.create_channel("ignore")
+
+        test_tree.create_device("parent", "device")
+        test_tree.create_device("parent", "other")
+        test_tree.create_device("parent", "ignore")
+
+        test_tree.create_device("redirect1", "other")
+        
+        # Create per-device redirects
+        test_tree.create_per_device_channel_redirect(
+            "device", "redirect1", "parent")
+        test_tree.create_per_device_channel_redirect(
+            "other", "redirect2", "parent")
+
+        # Check if removal of an unrelated channel does nothing
+        channels = test_tree.list_channels()
+        devices_before1 = dict(channels['redirect1']['devices'])
+        devices_before2 = dict(channels['redirect2']['devices'])
+        test_tree.remove_channel("ignore")
+        channels = test_tree.list_channels()
+        self.assertEqual(devices_before1, channels['redirect1']['devices'])
+        self.assertEqual(devices_before2, channels['redirect2']['devices'])
+
+        # Check if removal of an unrelated device does nothing
+        channels = test_tree.list_channels()
+        devices_before1 = dict(channels['redirect1']['devices'])
+        devices_before2 = dict(channels['redirect2']['devices'])
+        test_tree.remove_device("parent", "ignore")
+        channels = test_tree.list_channels()
+        self.assertEqual(devices_before1, channels['redirect1']['devices'])
+        self.assertEqual(devices_before2, channels['redirect2']['devices'])
+
+        # Check cleanup after single device removal
+        test_tree.remove_device("parent", "device")
+        channels = test_tree.list_channels()
+        self.assertNotIn("device", channels['redirect1']['devices'])
+        self.assertIn("other", channels['redirect1']['devices'])
+
+        # Check cleanup after whole channel removal
+        test_tree.remove_channel("parent")
+        channels = test_tree.list_channels()
+        self.assertNotIn("other", channels['redirect2']['devices'])
+
+    @unittest.skipUnless(HAS_TEST_KEYS, MISSING_KEYS_WARNING)
     def test_rename(self):
         test_tree = tree.Tree(self.config)
 
@@ -472,6 +620,33 @@
             {'devices': {'device': {'index': '/test/new/device/index.json'}}})
 
     @unittest.skipUnless(HAS_TEST_KEYS, MISSING_KEYS_WARNING)
+    def test_rename_with_redirects(self):
+        test_tree = tree.Tree(self.config)
+
+        # Create some channels
+        test_tree.create_channel("old")
+        test_tree.create_channel("redirect")
+
+        # Create basic devices
+        test_tree.create_device("old", "device")
+        test_tree.create_device("redirect", "other")
+
+        # Create a redirect
+        test_tree.create_per_device_channel_redirect(
+            "device", "redirect", "old")
+
+        # Rename
+        self.assertTrue(test_tree.rename_channel("old", "new"))
+
+        channels = test_tree.list_channels()
+        self.assertIn("device", channels['redirect']['devices'])
+        self.assertIn("other", channels['redirect']['devices'])
+        device = channels['redirect']['devices']['device']
+        self.assertIn("redirect", device)
+        self.assertEqual(device['redirect'], "new")
+        self.assertEqual(device['index'], "/new/device/index.json")
+
+    @unittest.skipUnless(HAS_TEST_KEYS, MISSING_KEYS_WARNING)
     def test_index(self):
         device = tree.Device(self.config, self.temp_directory)
 
 
=== modified file 'lib/systemimage/tree.py'
--- lib/systemimage/tree.py	2015-10-27 15:25:38 +0000
+++ lib/systemimage/tree.py	2016-06-20 12:44:28 +0000
@@ -306,6 +306,37 @@
 
         return True
 
+    def create_per_device_channel_redirect(self, device_name, channel_name, 
+                                           target_name):
+        """
+            Creates a device-specific channel redirect, redirecting that device
+            to point to a different channel.
+        """
+
+        with channels_json(self.config, self.indexpath, True) as channels:
+            if channel_name not in channels:
+                raise KeyError("Couldn't find channel: %s" % channel_name)
+
+            if device_name in channels[channel_name]['devices']:
+                raise KeyError("Device already exists: %s" % device_name)
+
+            if target_name not in channels:
+                raise KeyError("Couldn't find target channel: %s" %
+                               target_name)
+
+            if device_name not in channels[target_name]['devices']:
+                raise KeyError("Couldn't find device on target channel: "
+                               "%s, %s" %
+                               (target_name, device_name))
+
+            channels[channel_name]['devices'][device_name] = \
+                dict(channels[target_name]['devices'][device_name])
+
+            channels[channel_name]['devices'][device_name]['redirect'] = \
+                target_name
+
+        return True
+
     def create_device(self, channel_name, device_name, keyring_path=None):
         """
             Creates a new device entry in the tree.
@@ -514,6 +545,9 @@
                 shutil.rmtree(channel_path)
             channels.pop(channel_name)
 
+        # Remove all redirect device channels pointing at this channel
+        self.cleanup_device_redirects(channel_name)
+
         return True
 
     def remove_device(self, channel_name, device_name):
@@ -528,14 +562,21 @@
             if device_name not in channels[channel_name]['devices']:
                 raise KeyError("Couldn't find device: %s" % device_name)
 
-            device_path = os.path.join(self.path, channel_name, device_name)
-            if os.path.exists(device_path):
-                shutil.rmtree(device_path)
+            # Do not remove the device files for per-device redirects
+            device = channels[channel_name]['devices'][device_name]
+            if "redirect" not in device:
+                device_path = os.path.join(
+                    self.path, channel_name, device_name)
+                if os.path.exists(device_path):
+                    shutil.rmtree(device_path)
             channels[channel_name]['devices'].pop(device_name)
 
         self.sync_aliases(channel_name)
         self.sync_redirects(channel_name)
 
+        # Remove all redirect channels pointing at this device
+        self.cleanup_device_redirects(channel_name, device_name)
+
         return True
 
     def rename_channel(self, old_name, new_name):
@@ -581,6 +622,15 @@
                                     .replace("/%s/" % old_name,
                                              "/%s/" % new_name)
 
+            # Handle any device-specific channel redirects
+            for channel_name, channel in channels.items():
+                for device_name, device in channel['devices'].items():
+                    if "redirect" in device and device['redirect'] == old_name:
+                        index_path = "/%s/%s/index.json" % (new_name,
+                                                            device_name)
+                        device['redirect'] = new_name
+                        device['index'] = index_path
+
             channels.pop(old_name)
 
         return True
@@ -804,6 +854,24 @@
 
         return True
 
+    def cleanup_device_redirects(self, channel_name,
+                                 redirect_device_name=None):
+        """
+            Cleanup any dangling device-specific channel redirects.
+        """
+
+        with channels_json(self.config, self.indexpath, True) as channels:
+            for target_name, channel in channels.items():
+                devices = dict(channel['devices'])
+                for device_name, device in devices.items():
+                    if ("redirect" in device and 
+                         device['redirect'] == channel_name and
+                         (not redirect_device_name or 
+                          redirect_device_name == device_name)):
+                        channels[target_name]['devices'].pop(device_name)
+
+        return True
+
 
 class Device:
     def __init__(self, config, path):