curtin-dev team mailing list archive

[Ryan Harper <ryan.harper@xxxxxxxxxxxxx>]

Ryan Harper has proposed merging ~raharper/curtin:fix/vmtest-preserve-partition-wipe-vg-disk-config into curtin:master.

Commit message:
vmtests: fix PreservePartitionWipeVg storage config

This failure

  An error occured handling 'disk-sda': ValueError - disk 'disk-sda'
  does not have correct partition table or cannot be read, but preserve
  is set to true. cannot continue installation.

happens because the storage config for the main disk has both wipe and
preserve settings enabled resulting in the primary disk being wiped.

Curtin can either wipe a disk or preserve it but we cannot do both (unlike
partitions where we can keep the partition location, flag, size, etc,
but wipe the data).  This branch

 - Fixes the storage config in the vmtest
 - Adds a RuntimeException indicating this config is invalid
 - Adds unittests for get_device_paths_from_storage_config


Requested reviews:
  curtin developers (curtin-dev)

For more details, see:
https://code.launchpad.net/~raharper/curtin/+git/curtin/+merge/384992
-- 
Your team curtin developers is requested to review the proposed merge of ~raharper/curtin:fix/vmtest-preserve-partition-wipe-vg-disk-config into curtin:master.

diff --git a/curtin/commands/block_meta.py b/curtin/commands/block_meta.py
index 4bc7da2..dee73b1 100644
--- a/curtin/commands/block_meta.py
+++ b/curtin/commands/block_meta.py
@@ -1744,7 +1744,12 @@ def get_device_paths_from_storage_config(storage_config):
     dpaths = []
     for (k, v) in storage_config.items():
         if v.get('type') in ['disk', 'partition']:
-            if config.value_as_boolean(v.get('wipe')):
+            wipe = config.value_as_boolean(v.get('wipe'))
+            preserve = config.value_as_boolean(v.get('preserve'))
+            if v.get('type') == 'disk' and all([wipe, preserve]):
+                msg = 'type:disk id=%s has both wipe and preserve' % v['id']
+                raise RuntimeError(msg)
+            if wipe:
                 try:
                     # skip paths that do not exit, nothing to wipe
                     dpath = get_path_to_storage_volume(k, storage_config)
diff --git a/examples/tests/preserve-partition-wipe-vg.yaml b/examples/tests/preserve-partition-wipe-vg.yaml
index 97686e1..27a4235 100644
--- a/examples/tests/preserve-partition-wipe-vg.yaml
+++ b/examples/tests/preserve-partition-wipe-vg.yaml
@@ -38,7 +38,6 @@ storage:
     grub_device: true
     type: disk
     id: disk-sda
-    wipe: superblock
   - serial: disk-b
     name: disk-b
     grub_device: false
diff --git a/tests/unittests/test_commands_block_meta.py b/tests/unittests/test_commands_block_meta.py
index 74a14a7..d954296 100644
--- a/tests/unittests/test_commands_block_meta.py
+++ b/tests/unittests/test_commands_block_meta.py
@@ -2586,4 +2586,115 @@ class TestVerifyPtableFlag(CiTestCase):
                                       sfdisk_info=self.sfdisk_info_dos)
 
 
+class TestGetDevicePathsFromStorageConfig(CiTestCase):
+
+    def setUp(self):
+        super(TestGetDevicePathsFromStorageConfig, self).setUp()
+        base = 'curtin.commands.block_meta.'
+        self.add_patch(base + 'get_path_to_storage_volume', 'mock_getpath')
+        self.add_patch(base + 'os.path.exists', 'm_exists')
+        self.m_exists.return_value = True
+        self.mock_getpath.side_effect = self._getpath
+        self.prefix = '/test/dev/'
+        self.config = {
+            'storage': {
+                'version': 1,
+                'config': [
+                    {'id': 'sda',
+                     'type': 'disk',
+                     'name': 'main_disk',
+                     'ptable': 'gpt',
+                     'serial': 'disk-a'},
+                    {'id': 'disk-sda-part-1',
+                     'type': 'partition',
+                     'device': 'sda',
+                     'name': 'bios_boot',
+                     'number': 1,
+                     'size': '1M',
+                     'flag': 'bios_grub'},
+                    {'id': 'disk-sda-part-2',
+                     'type': 'partition',
+                     'device': 'sda',
+                     'number': 2,
+                     'size': '5GB'},
+                ],
+            }
+        }
+        self.disk1 = self.config['storage']['config'][0]
+        self.part1 = self.config['storage']['config'][1]
+        self.part2 = self.config['storage']['config'][2]
+        self.sconfig = self._sconfig(self.config)
+
+    def _sconfig(self, config):
+        return block_meta.extract_storage_ordered_dict(config)
+
+    def _getpath(self, item_id, _sconfig):
+        return self.prefix + item_id
+
+    def test_devpath_selects_disks_partitions_with_wipe_setting(self):
+        self.disk1['wipe'] = 'superblock'
+        self.part1['wipe'] = 'superblock'
+        self.sconfig = self._sconfig(self.config)
+
+        expected_devpaths = [
+            self.prefix + self.disk1['id'], self.prefix + self.part1['id']]
+        result = block_meta.get_device_paths_from_storage_config(self.sconfig)
+        self.assertEqual(sorted(expected_devpaths), sorted(result))
+        self.assertEqual([
+            call(self.disk1['id'], self.sconfig),
+            call(self.part1['id'], self.sconfig)],
+            self.mock_getpath.call_args_list)
+        self.assertEqual(
+            sorted([call(devpath) for devpath in expected_devpaths]),
+            sorted(self.m_exists.call_args_list))
+
+    def test_devpath_raises_exception_if_wipe_and_preserve_set(self):
+        self.disk1['wipe'] = 'superblock'
+        self.disk1['preserve'] = True
+        self.sconfig = self._sconfig(self.config)
+
+        with self.assertRaises(RuntimeError):
+            block_meta.get_device_paths_from_storage_config(self.sconfig)
+        self.assertEqual([], self.mock_getpath.call_args_list)
+        self.assertEqual([], self.m_exists.call_args_list)
+
+    def test_devpath_check_boolean_value_if_wipe_and_preserve_set(self):
+        self.disk1['wipe'] = 'superblock'
+        self.disk1['preserve'] = False
+        self.sconfig = self._sconfig(self.config)
+
+        expected_devpaths = [self.prefix + self.disk1['id']]
+        result = block_meta.get_device_paths_from_storage_config(self.sconfig)
+        self.assertEqual(expected_devpaths, result)
+        self.assertEqual(
+            [call(self.disk1['id'], self.sconfig)],
+            self.mock_getpath.call_args_list)
+        self.assertEqual(
+            sorted([call(devpath) for devpath in expected_devpaths]),
+            sorted(self.m_exists.call_args_list))
+
+    def test_devpath_check_preserved_devices_skipped(self):
+        self.disk1['preserve'] = True
+        self.sconfig = self._sconfig(self.config)
+
+        result = block_meta.get_device_paths_from_storage_config(self.sconfig)
+        self.assertEqual([], result)
+        self.assertEqual([], self.mock_getpath.call_args_list)
+        self.assertEqual([], self.m_exists.call_args_list)
+
+    def test_devpath_check_missing_path_devices_skipped(self):
+        self.disk1['wipe'] = 'superblock'
+        self.sconfig = self._sconfig(self.config)
+
+        self.m_exists.return_value = False
+        result = block_meta.get_device_paths_from_storage_config(self.sconfig)
+        self.assertEqual([], result)
+        self.assertEqual(
+            [call(self.disk1['id'], self.sconfig)],
+            self.mock_getpath.call_args_list)
+        self.assertEqual(
+            [call(self.prefix + self.disk1['id'])],
+            self.m_exists.call_args_list)
+
+
 # vi: ts=4 expandtab syntax=python