hv_xen: Factorize and test disk configuration
authorMichael Hanselmann <hansmi@google.com>
Tue, 22 Jan 2013 12:31:25 +0000 (13:31 +0100)
committerMichael Hanselmann <hansmi@google.com>
Thu, 24 Jan 2013 12:27:11 +0000 (13:27 +0100)
The “_GetConfigFileDiskData” function is moved to module level and
cleaned up (module-level constants for letters and file I/O drivers).

Until now only 24 disks would be supported (e.g. “sda” to “sdx”), when
the Latin alphabet actually has 26 characters. Now all 26 letters would
be available for use (“constants.MAX_DISKS” is still 16).

Newly added unit tests provide complete coverage for
“_GetConfigFileDiskData”.

Signed-off-by: Michael Hanselmann <hansmi@google.com>
Reviewed-by: Bernardo Dal Seno <bdalseno@google.com>

lib/hypervisor/hv_xen.py
test/py/ganeti.hypervisor.hv_xen_unittest.py

index 001a205..4641a7b 100644 (file)
@@ -24,6 +24,7 @@
 """
 
 import logging
+import string # pylint: disable=W0402
 from cStringIO import StringIO
 
 from ganeti import constants
@@ -41,6 +42,12 @@ XL_CONFIG_FILE = utils.PathJoin(pathutils.XEN_CONFIG_DIR, "xen/xl.conf")
 VIF_BRIDGE_SCRIPT = utils.PathJoin(pathutils.XEN_CONFIG_DIR,
                                    "scripts/vif-bridge")
 _DOM0_NAME = "Domain-0"
+_DISK_LETTERS = string.ascii_lowercase
+
+_FILE_DRIVER_MAP = {
+  constants.FD_LOOP: "file",
+  constants.FD_BLKTAP: "tap:aio",
+  }
 
 
 def _CreateConfigCpus(cpu_mask):
@@ -254,6 +261,45 @@ def _GetNodeInfo(info, fn):
   return _MergeInstanceInfo(_ParseNodeInfo(info), fn)
 
 
+def _GetConfigFileDiskData(block_devices, blockdev_prefix,
+                           _letters=_DISK_LETTERS):
+  """Get disk directives for Xen config file.
+
+  This method builds the xen config disk directive according to the
+  given disk_template and block_devices.
+
+  @param block_devices: list of tuples (cfdev, rldev):
+      - cfdev: dict containing ganeti config disk part
+      - rldev: ganeti.bdev.BlockDev object
+  @param blockdev_prefix: a string containing blockdevice prefix,
+                          e.g. "sd" for /dev/sda
+
+  @return: string containing disk directive for xen instance config file
+
+  """
+  if len(block_devices) > len(_letters):
+    raise errors.HypervisorError("Too many disks")
+
+  disk_data = []
+
+  for sd_suffix, (cfdev, dev_path) in zip(_letters, block_devices):
+    sd_name = blockdev_prefix + sd_suffix
+
+    if cfdev.mode == constants.DISK_RDWR:
+      mode = "w"
+    else:
+      mode = "r"
+
+    if cfdev.dev_type == constants.LD_FILE:
+      driver = _FILE_DRIVER_MAP[cfdev.physical_id[0]]
+    else:
+      driver = "phy"
+
+    disk_data.append("'%s:%s,%s,%s'" % (driver, dev_path, sd_name, mode))
+
+  return disk_data
+
+
 class XenHypervisor(hv_base.BaseHypervisor):
   """Xen generic hypervisor interface
 
@@ -506,45 +552,6 @@ class XenHypervisor(hv_base.BaseHypervisor):
 
     return None
 
-  @staticmethod
-  def _GetConfigFileDiskData(block_devices, blockdev_prefix):
-    """Get disk directive for xen config file.
-
-    This method builds the xen config disk directive according to the
-    given disk_template and block_devices.
-
-    @param block_devices: list of tuples (cfdev, rldev):
-        - cfdev: dict containing ganeti config disk part
-        - rldev: ganeti.bdev.BlockDev object
-    @param blockdev_prefix: a string containing blockdevice prefix,
-                            e.g. "sd" for /dev/sda
-
-    @return: string containing disk directive for xen instance config file
-
-    """
-    FILE_DRIVER_MAP = {
-      constants.FD_LOOP: "file",
-      constants.FD_BLKTAP: "tap:aio",
-      }
-    disk_data = []
-    if len(block_devices) > 24:
-      # 'z' - 'a' = 24
-      raise errors.HypervisorError("Too many disks")
-    namespace = [blockdev_prefix + chr(i + ord("a")) for i in range(24)]
-    for sd_name, (cfdev, dev_path) in zip(namespace, block_devices):
-      if cfdev.mode == constants.DISK_RDWR:
-        mode = "w"
-      else:
-        mode = "r"
-      if cfdev.dev_type == constants.LD_FILE:
-        line = "'%s:%s,%s,%s'" % (FILE_DRIVER_MAP[cfdev.physical_id[0]],
-                                  dev_path, sd_name, mode)
-      else:
-        line = "'phy:%s,%s,%s'" % (dev_path, sd_name, mode)
-      disk_data.append(line)
-
-    return disk_data
-
   def MigrationInfo(self, instance):
     """Get instance information to perform a migration.
 
@@ -765,8 +772,8 @@ class XenPvmHypervisor(XenHypervisor):
         nic_str += ", bridge=%s" % nic.nicparams[constants.NIC_LINK]
       vif_data.append("'%s'" % nic_str)
 
-    disk_data = cls._GetConfigFileDiskData(block_devices,
-                                           hvp[constants.HV_BLOCKDEV_PREFIX])
+    disk_data = \
+      _GetConfigFileDiskData(block_devices, hvp[constants.HV_BLOCKDEV_PREFIX])
 
     config.write("vif = [%s]\n" % ",".join(vif_data))
     config.write("disk = [%s]\n" % ",".join(disk_data))
@@ -918,8 +925,8 @@ class XenHvmHypervisor(XenHypervisor):
 
     config.write("vif = [%s]\n" % ",".join(vif_data))
 
-    disk_data = cls._GetConfigFileDiskData(block_devices,
-                                           hvp[constants.HV_BLOCKDEV_PREFIX])
+    disk_data = \
+      _GetConfigFileDiskData(block_devices, hvp[constants.HV_BLOCKDEV_PREFIX])
 
     iso_path = hvp[constants.HV_CDROM_IMAGE_PATH]
     if iso_path:
index e057187..e4a21d1 100755 (executable)
@@ -21,6 +21,7 @@
 
 """Script for testing ganeti.hypervisor.hv_lxc"""
 
+import string # pylint: disable=W0402
 import unittest
 
 from ganeti import constants
@@ -213,5 +214,79 @@ class TestMergeInstanceInfo(testutils.GanetiTestCase):
       })
 
 
+class TestGetConfigFileDiskData(unittest.TestCase):
+  def testLetterCount(self):
+    self.assertEqual(len(hv_xen._DISK_LETTERS), 26)
+
+  def testNoDisks(self):
+    self.assertEqual(hv_xen._GetConfigFileDiskData([], "hd"), [])
+
+  def testManyDisks(self):
+    for offset in [0, 1, 10]:
+      disks = [(objects.Disk(dev_type=constants.LD_LV), "/tmp/disk/%s" % idx)
+               for idx in range(len(hv_xen._DISK_LETTERS) + offset)]
+
+      if offset == 0:
+        result = hv_xen._GetConfigFileDiskData(disks, "hd")
+        self.assertEqual(result, [
+          "'phy:/tmp/disk/%s,hd%s,r'" % (idx, string.ascii_lowercase[idx])
+          for idx in range(len(hv_xen._DISK_LETTERS) + offset)
+          ])
+      else:
+        try:
+          hv_xen._GetConfigFileDiskData(disks, "hd")
+        except errors.HypervisorError, err:
+          self.assertEqual(str(err), "Too many disks")
+        else:
+          self.fail("Exception was not raised")
+
+  def testTwoLvDisksWithMode(self):
+    disks = [
+      (objects.Disk(dev_type=constants.LD_LV, mode=constants.DISK_RDWR),
+       "/tmp/diskFirst"),
+      (objects.Disk(dev_type=constants.LD_LV, mode=constants.DISK_RDONLY),
+       "/tmp/diskLast"),
+      ]
+
+    result = hv_xen._GetConfigFileDiskData(disks, "hd")
+    self.assertEqual(result, [
+      "'phy:/tmp/diskFirst,hda,w'",
+      "'phy:/tmp/diskLast,hdb,r'",
+      ])
+
+  def testFileDisks(self):
+    disks = [
+      (objects.Disk(dev_type=constants.LD_FILE, mode=constants.DISK_RDWR,
+                    physical_id=[constants.FD_LOOP]),
+       "/tmp/diskFirst"),
+      (objects.Disk(dev_type=constants.LD_FILE, mode=constants.DISK_RDONLY,
+                    physical_id=[constants.FD_BLKTAP]),
+       "/tmp/diskTwo"),
+      (objects.Disk(dev_type=constants.LD_FILE, mode=constants.DISK_RDWR,
+                    physical_id=[constants.FD_LOOP]),
+       "/tmp/diskThree"),
+      (objects.Disk(dev_type=constants.LD_FILE, mode=constants.DISK_RDWR,
+                    physical_id=[constants.FD_BLKTAP]),
+       "/tmp/diskLast"),
+      ]
+
+    result = hv_xen._GetConfigFileDiskData(disks, "sd")
+    self.assertEqual(result, [
+      "'file:/tmp/diskFirst,sda,w'",
+      "'tap:aio:/tmp/diskTwo,sdb,r'",
+      "'file:/tmp/diskThree,sdc,w'",
+      "'tap:aio:/tmp/diskLast,sdd,w'",
+      ])
+
+  def testInvalidFileDisk(self):
+    disks = [
+      (objects.Disk(dev_type=constants.LD_FILE, mode=constants.DISK_RDWR,
+                    physical_id=["#unknown#"]),
+       "/tmp/diskinvalid"),
+      ]
+
+    self.assertRaises(KeyError, hv_xen._GetConfigFileDiskData, disks, "sd")
+
+
 if __name__ == "__main__":
   testutils.GanetiTestProgram()