Merge ~raharper/curtin:fix/dasd-disk-vtoc-preserve into curtin:master

[Ryan Harper]

block-meta: handle preserve with vtoc ptable

DASD devices have a 'vtoc' partition table type. When we're handling
a disk config with 'preserve': True we raised a ValueError expecting
either 'gpt' or 'msdos'. Fix this by refactoring the code to check
the expected ptable value and adding a check_vtoc_signature method.

LP: #1871158

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:2dc55f0e337ba1034bb54cf869a763e366577518
https://jenkins.ubuntu.com/server/job/curtin-ci/15/
Executed test runs:
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/15/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/15/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/15/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/15/

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/15//rebuild

review: Needs Fixing (continuous-integration)

[Chad Smith (chad.smith)]

review: Needs Information

[Ryan Harper (raharper)]

Thanks, I'll update.

8da06ac... by Ryan Harper on 2020-04-07

Consolidate ptable values into variables; drop unneeded ValueError

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:8da06ac2b07bf801bd82d01304e53fac335f51d0
https://jenkins.ubuntu.com/server/job/curtin-ci/21/
Executed test runs:
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/21/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/21/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/21/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/21/

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/21//rebuild

review: Needs Fixing (continuous-integration)

[Ryan Harper (raharper)]

ValueError: ('Invalid partition table type: %s in %s', None, {'id': 'sparedisk_id', 'type': 'disk', 'serial': 'disk-b', 'name': 'sparedisk', 'wipe': 'superblock'})
[ 63.818482] cloud-init[1063]: ('Invalid partition table type: %s in %s', None, {'id': 'sparedisk_id', 'type': 'disk', 'serial': 'disk-b', 'name': 'sparedisk', 'wipe': 'superblock'})

c11a97e... by Ryan Harper on 2020-04-07

Handle no ptable value in disk config

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:c11a97ecd4d185389f81f93c82845f304846df2f
https://jenkins.ubuntu.com/server/job/curtin-ci/22/
Executed test runs:
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/22/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/22/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/22/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/22/

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/22//rebuild

review: Needs Fixing (continuous-integration)

c623f08... by Ryan Harper on 2020-04-07

Fix assertEqual on call_count

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:c623f08bfa742f196d901082b57442f2faec5819
https://jenkins.ubuntu.com/server/job/curtin-ci/23/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/23/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/23/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/23/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/23/

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/23//rebuild

review: Approve (continuous-integration)

[Joshua Powers (powersj)]

From IRC:

<blackboxsw> rharper: in merge #381785 per your change +PTABLES_SUPPORTED = schemas._ptables the 'rub' was that the variable name is schemas._ptables instead of schemas.PTABLES_SUPPORTED and schemas._ptables_valid instead of schemas.PTABLES_VALID (no need to declare the 2 variables inside curtin/commands/block_meta.py)

[Chad Smith (chad.smith)]

+1

review: Approve

diff --git a/curtin/block/__init__.py b/curtin/block/__init__.py
index 6e5354e..aa5c6f7 100644
--- a/curtin/block/__init__.py
+++ b/curtin/block/__init__.py
@@ -922,7 +922,8 @@ def get_part_table_type(device):
     # signature, because a gpt formatted disk usually has a valid mbr to
     # protect the disk from being modified by older partitioning tools
     return ('gpt' if check_efi_signature(device) else
-            'dos' if check_dos_signature(device) else None)
+            'dos' if check_dos_signature(device) else
+            'vtoc' if check_vtoc_signature(device) else None)
 
 
 def check_dos_signature(device):
@@ -959,6 +960,17 @@ def check_efi_signature(device):
                             offset=sector_size) == b'EFI PART'))
 
 
+def check_vtoc_signature(device):
+    """ check if the specified device has a vtoc partition table. """
+    devname = dev_path(path_to_kname(device))
+    try:
+        util.subp(['fdasd', '--table', devname])
+    except util.ProcessExecutionError:
+        return False
+
+    return True
+
+
 def is_extended_partition(device):
     """
     check if the specified device path is a dos extended partition
diff --git a/curtin/block/schemas.py b/curtin/block/schemas.py
index edca9e6..4de1daa 100644
--- a/curtin/block/schemas.py
+++ b/curtin/block/schemas.py
@@ -7,8 +7,9 @@ _path_nondev = r'(^/$|^(/[^/]+)+$)'
 _fstypes = ['btrfs', 'ext2', 'ext3', 'ext4', 'fat', 'fat12', 'fat16', 'fat32',
             'iso9660', 'vfat', 'jfs', 'ntfs', 'reiserfs', 'swap', 'xfs',
             'zfsroot']
-_ptables = ['dos', 'gpt', 'msdos', 'vtoc']
 _ptable_unsupported = 'unsupported'
+_ptables = ['dos', 'gpt', 'msdos', 'vtoc']
+_ptables_valid = _ptables + [_ptable_unsupported]
 
 definitions = {
     'id': {'type': 'string'},
@@ -16,7 +17,7 @@ definitions = {
     'devices': {'type': 'array', 'items': {'$ref': '#/definitions/ref_id'}},
     'name': {'type': 'string'},
     'preserve': {'type': 'boolean'},
-    'ptable': {'type': 'string', 'enum': _ptables + [_ptable_unsupported]},
+    'ptable': {'type': 'string', 'enum': _ptables_valid},
     'size': {'type': ['string', 'number'],
              'minimum': 1,
              'pattern': r'^([1-9]\d*(.\d+)?|\d+.\d+)(K|M|G|T)?B?'},
diff --git a/curtin/commands/block_meta.py b/curtin/commands/block_meta.py
index 6c6bfc7..6c145e3 100644
--- a/curtin/commands/block_meta.py
+++ b/curtin/commands/block_meta.py
@@ -34,6 +34,8 @@ SIMPLE = 'simple'
 SIMPLE_BOOT = 'simple-boot'
 CUSTOM = 'custom'
 PTABLE_UNSUPPORTED = schemas._ptable_unsupported
+PTABLES_SUPPORTED = schemas._ptables
+PTABLES_VALID = schemas._ptables_valid
 
 SGDISK_FLAGS = {
     "boot": 'ef00',
@@ -551,14 +553,17 @@ def dasd_handler(info, storage_config):
 def disk_handler(info, storage_config):
     _dos_names = ['dos', 'msdos']
     ptable = info.get('ptable')
-    disk = get_path_to_storage_volume(info.get('id'), storage_config)
+    if ptable and ptable not in PTABLES_VALID:
+        raise ValueError(
+            'Invalid partition table type: %s in %s' % (ptable, info))
 
+    disk = get_path_to_storage_volume(info.get('id'), storage_config)
     if config.value_as_boolean(info.get('preserve')):
         # Handle preserve flag, verifying if ptable specified in config
-        if config.value_as_boolean(ptable) and ptable != PTABLE_UNSUPPORTED:
+        if ptable != PTABLE_UNSUPPORTED:
             current_ptable = block.get_part_table_type(disk)
-            if not ((ptable in _dos_names and current_ptable in _dos_names) or
-                    (ptable == 'gpt' and current_ptable == 'gpt')):
+            LOG.debug('disk: current ptable type: %s', current_ptable)
+            if current_ptable not in PTABLES_SUPPORTED:
                 raise ValueError(
                     "disk '%s' does not have correct partition table or "
                     "cannot be read, but preserve is set to true. "
@@ -583,8 +588,6 @@ def disk_handler(info, storage_config):
             elif ptable == "vtoc":
                 # ignore dasd partition tables
                 pass
-            else:
-                raise ValueError('invalid partition table type: %s', ptable)
         holders = clear_holders.get_holders(disk)
         if len(holders) > 0:
             LOG.info('Detected block holders on disk %s: %s', disk, holders)
diff --git a/tests/unittests/test_block.py b/tests/unittests/test_block.py
index e70503d..8ba8333 100644
--- a/tests/unittests/test_block.py
+++ b/tests/unittests/test_block.py
@@ -567,6 +567,17 @@ class TestPartTableSignature(CiTestCase):
                 (self.assertTrue if expected else self.assertFalse)(
                     block.check_efi_signature(self.blockdev))
 
+    @mock.patch('curtin.block.util.subp')
+    def test_check_vtoc_signature_finds_vtoc_returns_true(self, mock_subp):
+        mock_subp.return_value = ("vtoc.....ok", "")
+        self.assertTrue(block.check_vtoc_signature(self.blockdev))
+
+    @mock.patch('curtin.block.util.subp')
+    def test_check_vtoc_signature_returns_false_with_no_sig(self, mock_subp):
+        mock_subp.side_effect = [
+            util.ProcessExecutionError(stdout="", stderr="", exit_code=1)]
+        self.assertFalse(block.check_vtoc_signature(self.blockdev))
+
 
 class TestNonAscii(CiTestCase):
     @mock.patch('curtin.block.util.subp')
diff --git a/tests/unittests/test_commands_block_meta.py b/tests/unittests/test_commands_block_meta.py
index 231b2d1..7b4f99c 100644
--- a/tests/unittests/test_commands_block_meta.py
+++ b/tests/unittests/test_commands_block_meta.py
@@ -1212,6 +1212,79 @@ class TestDasdHandler(CiTestCase):
         self.assertEqual(0, m_dasd_format.call_count)
 
 
+class TestDiskHandler(CiTestCase):
+
+    with_logs = True
+
+    @patch('curtin.commands.block_meta.block')
+    @patch('curtin.commands.block_meta.util')
+    @patch('curtin.commands.block_meta.get_path_to_storage_volume')
+    def test_disk_handler_preserves_known_ptable(self, m_getpath, m_util,
+                                                 m_block):
+        storage_config = OrderedDict()
+        info = {'ptable': 'vtoc', 'serial': 'LX260B',
+                'preserve': True, 'name': '', 'grub_device': False,
+                'device_id': '0.0.260b', 'type': 'disk', 'id': 'disk-dasda'}
+
+        disk_path = "/wark/dasda"
+        m_getpath.return_value = disk_path
+        m_block.get_part_table_type.return_value = 'vtoc'
+        m_getpath.return_value = disk_path
+        block_meta.disk_handler(info, storage_config)
+        m_getpath.assert_called_with(info['id'], storage_config)
+        m_block.get_part_table_type.assert_called_with(disk_path)
+
+    @patch('curtin.commands.block_meta.block')
+    @patch('curtin.commands.block_meta.util')
+    @patch('curtin.commands.block_meta.get_path_to_storage_volume')
+    def test_disk_handler_allows_unsupported(self, m_getpath, m_util, m_block):
+        storage_config = OrderedDict()
+        info = {'ptable': 'unsupported', 'type': 'disk', 'id': 'disk-foobar',
+                'preserve': True, 'name': '', 'grub_device': False}
+
+        disk_path = "/wark/foobar"
+        m_getpath.return_value = disk_path
+        m_block.get_part_table_type.return_value = self.random_string()
+        m_getpath.return_value = disk_path
+        block_meta.disk_handler(info, storage_config)
+        m_getpath.assert_called_with(info['id'], storage_config)
+        self.assertEqual(0, m_block.get_part_table_type.call_count)
+
+    @patch('curtin.commands.block_meta.block')
+    @patch('curtin.commands.block_meta.util')
+    @patch('curtin.commands.block_meta.get_path_to_storage_volume')
+    def test_disk_handler_allows_no_ptable(self, m_getpath, m_util, m_block):
+        storage_config = OrderedDict()
+        info = {'type': 'disk', 'id': 'disk-foobar',
+                'preserve': True, 'name': '', 'grub_device': False}
+        self.assertNotIn('ptable', info)
+        disk_path = "/wark/foobar"
+        m_getpath.return_value = disk_path
+        m_block.get_part_table_type.return_value = 'gpt'
+        m_getpath.return_value = disk_path
+        block_meta.disk_handler(info, storage_config)
+        m_getpath.assert_called_with(info['id'], storage_config)
+        self.assertEqual(1, m_block.get_part_table_type.call_count)
+
+    @patch('curtin.commands.block_meta.block')
+    @patch('curtin.commands.block_meta.util')
+    @patch('curtin.commands.block_meta.get_path_to_storage_volume')
+    def test_disk_handler_errors_when_reading_current_ptable(self, m_getpath,
+                                                             m_util, m_block):
+        storage_config = OrderedDict()
+        info = {'ptable': 'gpt', 'type': 'disk', 'id': 'disk-foobar',
+                'preserve': True, 'name': '', 'grub_device': False}
+
+        disk_path = "/wark/foobar"
+        m_getpath.return_value = disk_path
+        m_block.get_part_table_type.return_value = None
+        m_getpath.return_value = disk_path
+        with self.assertRaises(ValueError):
+            block_meta.disk_handler(info, storage_config)
+        m_getpath.assert_called_with(info['id'], storage_config)
+        m_block.get_part_table_type.assert_called_with(disk_path)
+
+
 class TestLvmVolgroupHandler(CiTestCase):
 
     def setUp(self):