curtin-dev team mailing list archive

[Dan Bungert <mp+443382@xxxxxxxxxxxxxxxxxx>]

Dan Bungert has proposed merging ~dbungert/curtin:compat into curtin:master with ~dbungert/curtin:image-writer as a prerequisite.

Commit message:
add compat module, and use for sfdisk/util-linux quirks


Requested reviews:
  curtin developers (curtin-dev)

For more details, see:
https://code.launchpad.net/~dbungert/curtin/+git/curtin/+merge/443382
-- 
Your team curtin developers is requested to review the proposed merge of ~dbungert/curtin:compat into curtin:master.

diff --git a/curtin/commands/block_meta.py b/curtin/commands/block_meta.py
index 24ceaf4..6ce8440 100644
--- a/curtin/commands/block_meta.py
+++ b/curtin/commands/block_meta.py
@@ -1,7 +1,7 @@
 # This file is part of curtin. See LICENSE file for copyright and license info.
 
 from collections import OrderedDict, namedtuple
-from curtin import (block, config, paths, util)
+from curtin import (block, compat, config, paths, util)
 from curtin.block import schemas
 from curtin.block import (bcache, clear_holders, dasd, iscsi, lvm, mdadm, mkfs,
                           multipath, zfs)
@@ -653,7 +653,6 @@ DEVS = set()
 def image_handler(info, storage_config, context):
     path = info['path']
     size = int(util.human2bytes(info['size']))
-    sector_size = str(int(util.human2bytes(info.get('sector_size', 512))))
     if info.get('preserve', False):
         actual_size = os.stat(path).st_size
         if size != actual_size:
@@ -670,10 +669,14 @@ def image_handler(info, storage_config, context):
             if os.path.exists(path):
                 os.unlink(path)
             raise
+
+    cmd = ['losetup', '--show', '--find', path]
+    sector_size = int(util.human2bytes(info.get('sector_size', 512)))
+    if sector_size != 512:
+        compat.supports_large_sectors(fatal=True)
+        cmd.extend(('--sector-size', str(sector_size)))
     try:
-        dev = util.subp([
-            'losetup', '--show', '--sector-size', sector_size, '--find', path],
-            capture=True)[0].strip()
+        dev = util.subp(cmd, capture=True)[0].strip()
     except BaseException:
         if os.path.exists(path) and not info.get('preserve'):
             os.unlink(path)
diff --git a/curtin/commands/block_meta_v2.py b/curtin/commands/block_meta_v2.py
index 7f5ebce..570da83 100644
--- a/curtin/commands/block_meta_v2.py
+++ b/curtin/commands/block_meta_v2.py
@@ -8,7 +8,7 @@ from typing import (
 
 import attr
 
-from curtin import (block, util)
+from curtin import (block, compat, util)
 from curtin.commands.block_meta import (
     _get_volume_fstype,
     disk_handler as disk_handler_v1,
@@ -171,9 +171,10 @@ class SFDiskPartTable:
     def apply(self, device):
         sfdisk_script = self.render()
         LOG.debug("sfdisk input:\n---\n%s\n---\n", sfdisk_script)
-        util.subp(
-            ['sfdisk', '--no-tell-kernel', '--no-reread', device],
-            data=sfdisk_script.encode('ascii'))
+        cmd = ['sfdisk', '--no-reread', device]
+        if compat.supports_sfdisk_no_tell_kernel():
+            cmd.append('--no-tell-kernel')
+        util.subp(cmd, data=sfdisk_script.encode('ascii'))
         util.subp(['partprobe', device])
         # sfdisk and partprobe (as invoked here) use ioctls to inform the
         # kernel that the partition table has changed so it can add and remove
diff --git a/curtin/compat.py b/curtin/compat.py
new file mode 100644
index 0000000..c20b650
--- /dev/null
+++ b/curtin/compat.py
@@ -0,0 +1,38 @@
+# This file is part of curtin. See LICENSE file for copyright and license info.
+
+import re
+
+from curtin import util
+
+
+def _get_util_linux_ver():
+    line = util.subp(['losetup', '--version'], capture=True)[0].strip()
+    m = re.fullmatch(r'losetup from util-linux ([\d.]+)', line)
+    if m is None:
+        return None
+    return m.group(1)
+
+
+def _check_util_linux_ver(ver, label='', fatal=False):
+    ul_ver = _get_util_linux_ver()
+    result = ul_ver is not None and ul_ver >= ver
+    if not result and fatal:
+        raise RuntimeError(
+            'this system lacks the required {} support'.format(label))
+    return result
+
+
+def supports_large_sectors(fatal=False):
+    # Known requirements:
+    # * Kernel 4.14+
+    #   * Minimum supported things have a higher kernel, so skip that check
+    # * util-linux 2.30+
+    #   * xenial has 2.27.1, bionic has 2.31.1
+    # However, see also this, which suggests using 2.37.1:
+    # https://lore.kernel.org/lkml/20210615084259.yj5pmyjonfqcg7lg@xxxxxxxxxxx/
+    return _check_util_linux_ver('2.37.1', 'large sector', fatal)
+
+
+def supports_sfdisk_no_tell_kernel(fatal=False):
+    # Needs util-linux 2.29+
+    return _check_util_linux_ver('2.29', 'sfdisk', fatal)
diff --git a/tests/integration/test_block_meta.py b/tests/integration/test_block_meta.py
index e640dcd..ccce3e5 100644
--- a/tests/integration/test_block_meta.py
+++ b/tests/integration/test_block_meta.py
@@ -13,8 +13,7 @@ from typing import Optional
 from unittest import skipIf
 import yaml
 
-from curtin import block, distro, log, udev, util
-
+from curtin import block, compat, distro, log, udev, util
 from curtin.commands.block_meta import _get_volume_fstype
 from curtin.commands.block_meta_v2 import ONE_MIB_BYTES
 
@@ -28,11 +27,10 @@ class IntegrationTestCase(CiTestCase):
 
 @contextlib.contextmanager
 def loop_dev(image, sector_size=512):
-    dev = util.subp([
-        'losetup',
-        '--show', '--find', '--sector-size', str(sector_size),
-        image,
-        ], capture=True, decode='ignore')[0].strip()
+    cmd = ['losetup', '--show', '--find', image]
+    if sector_size != 512:
+        cmd.extend(('--sector-size', str(sector_size)))
+    dev = util.subp(cmd, capture=True, decode='ignore')[0].strip()
     util.subp(['partprobe', dev])
     try:
         udev.udevadm_trigger([dev])
@@ -119,7 +117,7 @@ def summarize_partitions(dev):
     parts = []
     ptable_json = util.subp(['sfdisk', '-J', dev], capture=True)[0]
     ptable = json.loads(ptable_json)['partitiontable']
-    sectorsize = ptable['sectorsize']
+    sectorsize = ptable.get('sectorsize', 512)
     assert dev == ptable['device']
     sysfs_data = block.sysfs_partition_data(dev)
     for part in ptable['partitions']:
@@ -330,15 +328,19 @@ class TestBlockMeta(IntegrationTestCase):
     def test_default_offsets_msdos_v2(self):
         self._test_default_offsets('msdos', 2)
 
+    @skipIf(not compat.supports_large_sectors(), 'test is for large sectors')
     def test_default_offsets_gpt_v1_4k(self):
         self._test_default_offsets('gpt', 1, 4096)
 
+    @skipIf(not compat.supports_large_sectors(), 'test is for large sectors')
     def test_default_offsets_msdos_v1_4k(self):
         self._test_default_offsets('msdos', 1, 4096)
 
+    @skipIf(not compat.supports_large_sectors(), 'test is for large sectors')
     def test_default_offsets_gpt_v2_4k(self):
         self._test_default_offsets('gpt', 2, 4096)
 
+    @skipIf(not compat.supports_large_sectors(), 'test is for large sectors')
     def test_default_offsets_msdos_v2_4k(self):
         self._test_default_offsets('msdos', 2, 4096)
 
diff --git a/tests/unittests/test_compat.py b/tests/unittests/test_compat.py
new file mode 100644
index 0000000..0ba5002
--- /dev/null
+++ b/tests/unittests/test_compat.py
@@ -0,0 +1,34 @@
+# This file is part of curtin. See LICENSE file for copyright and license info.
+
+from unittest import mock
+
+from curtin import compat
+from .helpers import CiTestCase
+
+
+class TestUtilLinuxVer(CiTestCase):
+    @mock.patch('curtin.util.subp')
+    def test_ul_ver(self, m_subp):
+        m_subp.return_value = ('losetup from util-linux 2.31.1', '')
+        self.assertEqual('2.31.1', compat._get_util_linux_ver())
+
+    @mock.patch('curtin.util.subp')
+    def test_ul_malformed(self, m_subp):
+        m_subp.return_value = ('losetup from util-linux asdf', '')
+        self.assertEqual(None, compat._get_util_linux_ver())
+
+    @mock.patch('curtin.compat._get_util_linux_ver')
+    def test_verpass(self, m_gulv):
+        m_gulv.return_value = '1.23.4'
+        self.assertTrue(compat._check_util_linux_ver('1.20'))
+
+    @mock.patch('curtin.compat._get_util_linux_ver')
+    def test_verfail(self, m_gulv):
+        m_gulv.return_value = '1.23.4'
+        self.assertFalse(compat._check_util_linux_ver('1.24'))
+
+    @mock.patch('curtin.compat._get_util_linux_ver')
+    def test_verfatal(self, m_gulv):
+        m_gulv.return_value = '1.23.4'
+        with self.assertRaisesRegex(RuntimeError, '.*my feature.*'):
+            compat._check_util_linux_ver('1.24', 'my feature', fatal=True)