curtin-dev team mailing list archive

[Dan Bungert <mp+419640@xxxxxxxxxxxxxxxxxx>]

Dan Bungert has proposed merging ~dbungert/curtin:resize-no-format-action into curtin:master.

Commit message:
Fix two forms of resize data corruption

* if resizing downward, a wipe of a partition could be done too early
* if format was unspecified, the resize would not happen, and if the
  partition needed a resize downward it would be corrupt


Requested reviews:
  curtin developers (curtin-dev)

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

diff --git a/curtin/commands/block_meta_v2.py b/curtin/commands/block_meta_v2.py
index c1e3630..fd87acc 100644
--- a/curtin/commands/block_meta_v2.py
+++ b/curtin/commands/block_meta_v2.py
@@ -60,13 +60,13 @@ def resize_ext(path, size):
     util.subp(['resize2fs', path, f'{size_k}k'])
 
 
-def perform_resize(kname, size, direction):
+def perform_resize(kname, resize, direction):
     path = block.kname_to_path(kname)
-    fstype = _get_volume_fstype(path)
-    if fstype:
-        LOG.debug('Resizing %s of type %s %s to %s',
-                  path, fstype, direction, size)
-        resizers[fstype](path, size)
+    fstype = resize['fstype']
+    size = resize['end']
+    LOG.debug('Resizing %s of type %s %s to %s',
+              path, fstype, direction, size)
+    resizers[fstype](path, size)
 
 
 resizers = {
@@ -241,7 +241,10 @@ def _wipe_for_action(action):
 
 
 def _prepare_resize(entry, part_info, table):
+    devpath = os.path.realpath(part_info['node'])
+    fstype = _get_volume_fstype(devpath)
     return {
+        'fstype': fstype,
         'start': table.sectors2bytes(part_info['size']),
         'end': table.sectors2bytes(entry.size),
     }
@@ -253,26 +256,28 @@ def needs_resize(storage_config, part_action, sfdisk_part_info):
     if not part_action.get('resize'):
         return False
 
+    devpath = os.path.realpath(sfdisk_part_info['node'])
+    fstype = _get_volume_fstype(devpath)
+    if fstype == '':
+        return False
+
     volume = part_action['id']
     format_actions = select_configs(storage_config, type='format',
-                                    volume=volume, preserve=True)
-    if len(format_actions) < 1:
-        return False
+                                    volume=volume)
     if len(format_actions) > 1:
         raise Exception(f'too many format actions for volume {volume}')
 
-    if not format_actions[0].get('preserve'):
-        return False
+    if len(format_actions) == 1:
+        if not format_actions[0].get('preserve'):
+            return False
 
-    devpath = os.path.realpath(sfdisk_part_info['node'])
-    fstype = _get_volume_fstype(devpath)
-    target_fstype = format_actions[0]['fstype']
-    msg = (
-        'Verifying %s format, expecting %s, found %s' % (
-         devpath, fstype, target_fstype))
-    LOG.debug(msg)
-    if fstype != target_fstype:
-        raise RuntimeError(msg)
+        target_fstype = format_actions[0]['fstype']
+        msg = (
+            'Verifying %s format, expecting %s, found %s' % (
+             devpath, fstype, target_fstype))
+        LOG.debug(msg)
+        if fstype != target_fstype:
+            raise RuntimeError(msg)
 
     if part_action.get('resize'):
         msg = 'Resize requested for format %s' % (fstype, )
@@ -351,18 +356,7 @@ def disk_handler_v2(info, storage_config, handlers):
             if needs_resize(storage_config, action, part_info):
                 resizes[entry.start] = _prepare_resize(entry, part_info, table)
             preserved_offsets.add(entry.start)
-        wipe = wipes[entry.start] = _wipe_for_action(action)
-        if wipe is not None:
-            # We do a quick wipe of where any new partitions will be,
-            # because if there is bcache or other metadata there, this
-            # can cause the partition to be used by a storage
-            # subsystem and preventing the exclusive open done by the
-            # wipe_volume call below. See
-            # https://bugs.launchpad.net/curtin/+bug/1718699 for all
-            # the gory details.
-            wipe_offset = table.sectors2bytes(entry.start)
-            LOG.debug('Wiping 1M on %s at offset %s', disk, wipe_offset)
-            block.zero_file_at_offsets(disk, [wipe_offset], exclusive=False)
+        wipes[entry.start] = _wipe_for_action(action)
 
     for kname, nr, offset, size in block.sysfs_partition_data(disk):
         offset_sectors = table.bytes2sectors(offset)
@@ -371,19 +365,28 @@ def disk_handler_v2(info, storage_config, handlers):
             block.wipe_volume(block.kname_to_path(kname), 'superblock')
         resize = resizes.get(offset_sectors)
         if resize and size > resize['end']:
-            perform_resize(kname, resize['end'], 'down')
+            perform_resize(kname, resize, 'down')
 
     table.apply(disk)
 
     for kname, number, offset, size in block.sysfs_partition_data(disk):
         offset_sectors = table.bytes2sectors(offset)
-        mode = wipes[offset_sectors]
-        if mode is not None:
+        wipe = wipes[offset_sectors]
+        if wipe is not None:
+            # We do a quick wipe of where any new partitions will be,
+            # because if there is bcache or other metadata there, this
+            # can cause the partition to be used by a storage
+            # subsystem and preventing the exclusive open done by the
+            # wipe_volume call below. See
+            # https://bugs.launchpad.net/curtin/+bug/1718699 for all
+            # the gory details.
+            LOG.debug('Wiping 1M on %s at offset %s', disk, offset)
+            block.zero_file_at_offsets(disk, [offset], exclusive=False)
             # Wipe the new partitions as needed.
-            block.wipe_volume(block.kname_to_path(kname), mode)
+            block.wipe_volume(block.kname_to_path(kname), wipe)
         resize = resizes.get(offset_sectors)
         if resize and resize['start'] < size:
-            perform_resize(kname, resize['end'], 'up')
+            perform_resize(kname, resize, 'up')
 
     # Make the names if needed
     if 'name' in info:
diff --git a/tests/unittests/test_commands_block_meta.py b/tests/unittests/test_commands_block_meta.py
index 3698d32..3ccddab 100644
--- a/tests/unittests/test_commands_block_meta.py
+++ b/tests/unittests/test_commands_block_meta.py
@@ -2702,6 +2702,34 @@ class TestPartitionNeedsResize(CiTestCase):
             self.format['id']: self.format,
         }
 
+    def test_partition_resize_happy_path(self):
+        self.partition['preserve'] = True
+        self.partition['resize'] = True
+        self.format['preserve'] = True
+        self.format['fstype'] = 'ext4'
+        self.m_get_volume_fstype.return_value = 'ext4'
+        self.assertTrue(
+            block_meta_v2.needs_resize(
+                self.storage_config, self.partition, self.sfdisk_part_info))
+
+    def test_partition_resize_no_format_action(self):
+        self.partition['preserve'] = True
+        self.partition['resize'] = True
+        self.storage_config = {self.partition['id']: self.partition}
+        self.m_get_volume_fstype.return_value = 'ext4'
+        self.assertTrue(
+            block_meta_v2.needs_resize(
+                self.storage_config, self.partition, self.sfdisk_part_info))
+
+    def test_partition_resize_unformatted(self):
+        self.partition['preserve'] = True
+        self.partition['resize'] = True
+        self.storage_config = {self.partition['id']: self.partition}
+        self.m_get_volume_fstype.return_value = ''
+        self.assertFalse(
+            block_meta_v2.needs_resize(
+                self.storage_config, self.partition, self.sfdisk_part_info))
+
     def test_partition_resize_change_fs(self):
         self.partition['preserve'] = True
         self.partition['resize'] = True
@@ -2730,8 +2758,9 @@ class TestPartitionNeedsResize(CiTestCase):
         self.format['preserve'] = False
         self.format['fstype'] = 'reiserfs'
         self.m_get_volume_fstype.return_value = 'reiserfs'
-        block_meta_v2.needs_resize(
-            self.storage_config, self.partition, self.sfdisk_part_info)
+        self.assertFalse(
+            block_meta_v2.needs_resize(
+                self.storage_config, self.partition, self.sfdisk_part_info))
 
     def test_partition_resize_partition_preserve_false(self):
         # not a resize - partition is recreated
@@ -2740,8 +2769,9 @@ class TestPartitionNeedsResize(CiTestCase):
         self.format['preserve'] = False
         self.format['fstype'] = 'reiserfs'
         self.m_get_volume_fstype.return_value = 'reiserfs'
-        block_meta_v2.needs_resize(
-            self.storage_config, self.partition, self.sfdisk_part_info)
+        self.assertFalse(
+            block_meta_v2.needs_resize(
+                self.storage_config, self.partition, self.sfdisk_part_info))
 
 
 class TestPartitionVerifyFdasd(CiTestCase):