curtin-dev team mailing list archive

[Michael Hudson-Doyle <mwhudsonlp@xxxxxxxxxxx>]

Michael Hudson-Doyle has proposed merging ~mwhudson/curtin:lp-1878041-4k-multipath into curtin:master.

Commit message:
fix calculating start and offset of partition on multipathed 4k disk

calc_dm_partition_info returns start and offset in 512-byte sectors but
calc_partition_info only adjusted the sectors in the non-multipath case.

I also did some code tidying, including moving the responsibility for
raising an exception if dmsetup produces no output to
calc_dm_partition_info.

LP: #1878041


Requested reviews:
  curtin developers (curtin-dev)
Related bugs:
  Bug #1878041 in curtin: "installation fails creating user on large multipath system - multipathd show command times out in curtin log with 163 paths"
  https://bugs.launchpad.net/curtin/+bug/1878041

For more details, see:
https://code.launchpad.net/~mwhudson/curtin/+git/curtin/+merge/396703
-- 
Your team curtin developers is requested to review the proposed merge of ~mwhudson/curtin:lp-1878041-4k-multipath into curtin:master.

diff --git a/curtin/commands/block_meta.py b/curtin/commands/block_meta.py
index e29c1e4..25910a0 100644
--- a/curtin/commands/block_meta.py
+++ b/curtin/commands/block_meta.py
@@ -663,18 +663,17 @@ def find_extended_partition(part_device, storage_config):
             return item_id
 
 
-def calc_dm_partition_info(partition):
-    # dm- partitions are not in the same dir as disk dm device,
+def calc_dm_partition_info(partition_kname):
+    # finding the start of a dm partition appears to only be possible
+    # via parsing dmsetup output. the size is present in sysfs but
+    # given that it is included in the dmsetup output anyway, we take
+    # it from there too.
+    #
     # dmsetup table <dm_name>
-    # handle linear types only
-    #    mpatha-part1: 0 6291456 linear 253:0, 2048
+    #    mpatha-part1: 0 6291456 linear 253:0 2048
     #    <dm_name>: <log. start sec> <num sec> <type> <dest dev>  <start sec>
-    #
-    # Mapping this:
-    #   previous_size_sectors = <num_sec> | /sys/class/block/dm-1/size
-    #   previous_start_sectors = <start_sec> |  No 'start' sysfs file
-    pp_size_sec = pp_start_sec = None
-    mpath_id = multipath.get_mpath_id_from_device(block.dev_path(partition))
+    mpath_id = multipath.get_mpath_id_from_device(
+        block.dev_path(partition_kname))
     if mpath_id is None:
         raise RuntimeError('Failed to find mpath_id for partition')
     table_cmd = ['dmsetup', 'table', '--target', 'linear', mpath_id]
@@ -682,32 +681,31 @@ def calc_dm_partition_info(partition):
     if out:
         (_logical_start, previous_size_sectors, _table_type,
          _destination, previous_start_sectors) = out.split()
-        pp_size_sec = int(previous_size_sectors)
-        pp_start_sec = int(previous_start_sectors)
-
-    return (pp_start_sec, pp_size_sec)
+        return int(previous_start_sectors), int(previous_size_sectors)
+    else:
+        raise RuntimeError('Failed to find mpath_id for partition')
 
 
-def calc_partition_info(disk, partition, logical_block_size_bytes):
-    if partition.startswith('dm-'):
-        pp = partition
-        pp_start_sec, pp_size_sec = calc_dm_partition_info(partition)
+def calc_partition_info(partition_kname, logical_block_size_bytes):
+    if partition_kname.startswith('dm-'):
+        p_start, p_size = calc_dm_partition_info(partition_kname)
     else:
-        pp = os.path.join(disk, partition)
-        # XXX: sys/block/X/{size,start} is *ALWAYS* in 512b value
-        pp_size = int(
-            util.load_file(os.path.join(pp, "size")))
-        pp_size_sec = int(pp_size * 512 / logical_block_size_bytes)
-        pp_start = int(util.load_file(os.path.join(pp, "start")))
-        pp_start_sec = int(pp_start * 512 / logical_block_size_bytes)
-
-    LOG.debug("previous partition: %s size_sectors=%s start_sectors=%s",
-              pp, pp_size_sec, pp_start_sec)
-    if not all([pp_size_sec, pp_start_sec]):
+        pdir = block.sys_block_path(partition_kname)
+        p_size = int(util.load_file(os.path.join(pdir, "size")))
+        p_start = int(util.load_file(os.path.join(pdir, "start")))
+
+    # NB: sys/block/X/{size,start} and dmsetup output are both always
+    # in 512b sectors
+    p_size_sec = p_size * 512 // logical_block_size_bytes
+    p_start_sec = p_start * 512 // logical_block_size_bytes
+
+    LOG.debug("calc_partition_info: %s size_sectors=%s start_sectors=%s",
+              partition_kname, p_size_sec, p_start_sec)
+    if not all([p_size_sec, p_start_sec]):
         raise RuntimeError(
-            'Failed to determine previous partition %s info', partition)
+            'Failed to determine partition %s info', partition_kname)
 
-    return (pp_start_sec, pp_size_sec)
+    return (p_start_sec, p_size_sec)
 
 
 def verify_exists(devpath):
@@ -808,7 +806,6 @@ def partition_handler(info, storage_config):
     disk = get_path_to_storage_volume(device, storage_config)
     partnumber = determine_partition_number(info.get('id'), storage_config)
     disk_kname = block.path_to_kname(disk)
-    disk_sysfs_path = block.sys_block_path(disk)
 
     # consider the disks logical sector size when calculating sectors
     try:
@@ -843,8 +840,7 @@ def partition_handler(info, storage_config):
         partition_kname = block.partition_kname(disk_kname, pnum)
         LOG.debug('partition_kname=%s', partition_kname)
         (previous_start_sectors, previous_size_sectors) = (
-            calc_partition_info(disk_sysfs_path, partition_kname,
-                                logical_block_size_bytes))
+            calc_partition_info(partition_kname, logical_block_size_bytes))
 
     # Align to 1M at the beginning of the disk and at logical partitions
     alignment_offset = int((1 << 20) / logical_block_size_bytes)
diff --git a/tests/unittests/test_commands_block_meta.py b/tests/unittests/test_commands_block_meta.py
index 499f2d1..75f6613 100644
--- a/tests/unittests/test_commands_block_meta.py
+++ b/tests/unittests/test_commands_block_meta.py
@@ -2308,41 +2308,54 @@ class TestCalcPartitionInfo(CiTestCase):
         super(TestCalcPartitionInfo, self).setUp()
         self.add_patch('curtin.commands.block_meta.util.load_file',
                        'm_load_file')
-
-    def _prepare_load_file_mocks(self, start, size, logsize):
-        partition_size = str(int(size / logsize))
-        partition_start = str(int(start / logsize))
-        self.m_load_file.side_effect = iter([partition_size, partition_start])
+        self.add_patch('curtin.commands.block_meta.block.sys_block_path',
+                       'm_block_path')
+
+    def _prepare_mocks(self, start, size):
+        prefix = self.random_string()
+        self.m_block_path.return_value = prefix
+        partition_size = str(int(size / 512))
+        partition_start = str(int(start / 512))
+        self.m_load_file.side_effect = {
+            prefix + '/size': partition_size,
+            prefix + '/start': partition_start,
+            }.__getitem__
 
     def test_calc_partition_info(self):
-        disk = self.random_string()
         partition = self.random_string()
-        part_path = os.path.join(disk, partition)
         part_size = 10 * 1024 * 1024
         part_start = 1 * 1024 * 1024
         blk_size = 512
-        self._prepare_load_file_mocks(part_start, part_size, blk_size)
+        self._prepare_mocks(part_start, part_size)
 
         (start, size) = block_meta.calc_partition_info(
-            disk, partition, blk_size)
+             partition, blk_size)
+
+        self.assertEqual(part_start / blk_size, start)
+        self.assertEqual(part_size / blk_size, size)
+
+    def test_calc_partition_info_4k(self):
+        partition = self.random_string()
+        part_size = 10 * 1024 * 1024
+        part_start = 1 * 1024 * 1024
+        blk_size = 4096
+        self._prepare_mocks(part_start, part_size)
+
+        (start, size) = block_meta.calc_partition_info(
+             partition, blk_size)
 
         self.assertEqual(part_start / blk_size, start)
         self.assertEqual(part_size / blk_size, size)
-        self.assertEqual(
-            [call(part_path + '/size'), call(part_path + '/start')],
-            self.m_load_file.call_args_list)
 
     @patch('curtin.commands.block_meta.calc_dm_partition_info')
     def test_calc_partition_info_dm_part(self, m_calc_dm):
-        disk = self.random_string()
         partition = 'dm-237'
         part_size = 10 * 1024 * 1024
         part_start = 1 * 1024 * 1024
         blk_size = 512
-        m_calc_dm.return_value = (part_start / blk_size, part_size / blk_size)
+        m_calc_dm.return_value = (part_start / 512, part_size / 512)
 
-        (start, size) = block_meta.calc_partition_info(
-            disk, partition, blk_size)
+        (start, size) = block_meta.calc_partition_info(partition, blk_size)
 
         self.assertEqual(part_start / blk_size, start)
         self.assertEqual(part_size / blk_size, size)
@@ -2350,15 +2363,17 @@ class TestCalcPartitionInfo(CiTestCase):
         self.assertEqual([], self.m_load_file.call_args_list)
 
     @patch('curtin.commands.block_meta.calc_dm_partition_info')
-    def test_calc_partition_info_none_start_sec_raise_exc(self, m_calc_dm):
-        disk = self.random_string()
+    def test_calc_partition_info_dm_part(self, m_calc_dm):
         partition = 'dm-237'
-        blk_size = 512
-        m_calc_dm.return_value = (None, None)
+        part_size = 10 * 1024 * 1024
+        part_start = 1 * 1024 * 1024
+        blk_size = 4096
+        m_calc_dm.return_value = (part_start / 512, part_size / 512)
 
-        with self.assertRaises(RuntimeError):
-            block_meta.calc_partition_info(disk, partition, blk_size)
+        (start, size) = block_meta.calc_partition_info(partition, blk_size)
 
+        self.assertEqual(part_start / blk_size, start)
+        self.assertEqual(part_size / blk_size, size)
         self.assertEqual([call(partition)], m_calc_dm.call_args_list)
         self.assertEqual([], self.m_load_file.call_args_list)
 
@@ -2380,9 +2395,8 @@ class TestCalcDMPartitionInfo(CiTestCase):
 
     def test_calc_dm_partition_info_return_none_with_no_dmsetup_output(self):
         self.m_subp.return_value = ("", "")
-        self.assertEqual(
-            (None, None),
-            block_meta.calc_dm_partition_info(self.random_string()))
+        with self.assertRaises(RuntimeError):
+            block_meta.calc_dm_partition_info(self.random_string())
 
     def test_calc_dm_partition_info_calls_dmsetup_table(self):
         partition = 'dm-245'