Merge ~mwhudson/curtin:partition-verify-dasd into curtin:master

[Michael Hudson-Doyle]

fix verification of vtoc partitions

Currently curtin verifies the size and flags of a partition by
using sfdisk to inspect it and checking the results. But that
doesn't work on a dasd's vtoc partition table, so this branch
renames parition_verify to parition_verify_sfdisk, adds
parition_verify_fdasd and calls the latter when ptable ==
'vtoc'. The branch does a little more than is strictly necessary,
perhaps, but the other stuff will come in useful soon, I promise
:)

LP: #1899471

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

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

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

review: Needs Fixing (continuous-integration)

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

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

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

review: Approve (continuous-integration)

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

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

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

review: Needs Fixing (continuous-integration)

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

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

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

review: Approve (continuous-integration)

[Dimitri John Ledkov (xnox)]

This is fine refactor.

review: Approve

[Michael Hudson-Doyle (mwhudson)]

Thanks for the review. I actually think there is another branch I need to land before this one, let me get that up for review pronto.

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

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

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

review: Approve (continuous-integration)

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

Commit message lints:
- Line #2 has 360 too many characters. Line starts with: "Currently curtin verifies"...

review: Needs Fixing

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

review: Approve (continuous-integration)

diff --git a/curtin/block/dasd.py b/curtin/block/dasd.py
index daafeb8..7450300 100644
--- a/curtin/block/dasd.py
+++ b/curtin/block/dasd.py
@@ -2,7 +2,6 @@
 
 import collections
 import os
-import re
 import tempfile
 from curtin import util
 from curtin.log import LOG, logged_time
@@ -10,6 +9,86 @@ from curtin.log import LOG, logged_time
 Dasdvalue = collections.namedtuple('Dasdvalue', ['hex', 'dec', 'txt'])
 
 
+class DasdPartition:
+    def __init__(self, table, device, start, end, length, id, system):
+        self.device = device
+        self.start = int(start)
+        self.end = int(end)
+        self.length = int(length)
+        self.id = id
+        self.system = system
+
+
+class DasdPartitionTable:
+    def __init__(self, devname, blocks_per_track, bytes_per_block):
+        self.devname = devname
+        self.blocks_per_track = blocks_per_track
+        self.bytes_per_block = bytes_per_block
+        self.partitions = []
+
+    @property
+    def bytes_per_track(self):
+        return self.bytes_per_block * self.blocks_per_track
+
+    def tracks_needed(self, size_in_bytes):
+        return ((size_in_bytes - 1) // self.bytes_per_track) + 1
+
+    @classmethod
+    def from_fdasd(cls, devname):
+        """Use fdasd to construct a DasdPartitionTable.
+
+        % fdasd --table /dev/dasdc
+        reading volume label ..: VOL1
+        reading vtoc ..........: ok
+
+
+        Disk /dev/dasdc:
+          cylinders ............: 10017
+          tracks per cylinder ..: 15
+          blocks per track .....: 12
+          bytes per block ......: 4096
+          volume label .........: VOL1
+          volume serial ........: 0X1522
+          max partitions .......: 3
+
+         ------------------------------- tracks -------------------------------
+                       Device      start      end   length   Id  System
+                  /dev/dasdc1          2    43694    43693    1  Linux native
+                  /dev/dasdc2      43695    87387    43693    2  Linux native
+                  /dev/dasdc3      87388   131080    43693    3  Linux native
+                                  131081   150254    19174       unused
+        exiting...
+        """
+        cmd = ['fdasd', '--table', devname]
+        out, _err = util.subp(cmd, capture=True)
+        line_iter = iter(out.splitlines())
+        for line in line_iter:
+            if line.startswith("Disk"):
+                break
+        kw = {'devname': devname}
+        label_to_attr = {
+            'blocks per track': 'blocks_per_track',
+            'bytes per block': 'bytes_per_block'
+            }
+        for line in line_iter:
+            if '--- tracks ---' in line:
+                break
+            if ':' in line:
+                label, value = line.split(':', 1)
+                label = label.strip(' .')
+                value = value.strip()
+                if label in label_to_attr:
+                    kw[label_to_attr[label]] = int(value)
+        table = cls(**kw)
+        for line in line_iter:
+            if line.startswith('exiting'):
+                break
+            vals = line.split(maxsplit=5)
+            if vals[0].startswith('/dev/'):
+                table.partitions.append(DasdPartition(*vals))
+        return table
+
+
 def dasdinfo(device_id):
     ''' Run dasdinfo command and return the exported values.
 
@@ -325,83 +404,6 @@ class DasdDevice(CcwDevice):
     def devname(self):
         return '/dev/%s' % self.kname
 
-    def _bytes_to_tracks(self, geometry, request_size):
-        """ Return the number of tracks needed to hold the request size.
-
-        :param geometry: dictionary from dasdview output which includes
-            info on number of cylinders, tracks and blocksize.
-        :param request_size: size in Bytes
-
-        :raises: ValueError on missing or invalid geometry dict, missing
-            request_size.
-
-        Example geometry:
-        'geometry': {
-            'blocks_per_track': Dasdvalue(hex='0xc', dec=12, txt=None),
-            'blocksize': Dasdvalue(hex='0x1000', dec=4096, txt=None),
-            'number_of_cylinders':
-                 Dasdvalue(hex='0x2721', dec=10017, txt=None),
-            'tracks_per_cylinder': Dasdvalue(hex='0xf', dec=15, txt=None)}
-        }
-        """
-
-        if not geometry or not isinstance(geometry, dict):
-            raise ValueError('Missing or invalid geometry parameter.')
-
-        if not all([key for key in geometry.keys()
-                    if key in ['blocksize', 'blocks_per_track']]):
-            raise ValueError('Geometry dict missing required keys')
-
-        if not request_size or not isinstance(request_size,
-                                              util.numeric_types):
-            raise ValueError('Missing or invalid request_size.')
-
-        # helper to extract the decimal value from Dasdvalue objects
-        def _dval(dval):
-            return dval.dec
-
-        bytes_per_track = (
-            _dval(geometry['blocksize']) * _dval(geometry['blocks_per_track']))
-        tracks_needed = ((request_size - 1) // bytes_per_track) + 1
-        return tracks_needed
-
-    def get_partition_table(self):
-        """ Use fdasd to query the partition table (VTOC).
-
-            Returns a list of tuples, each tuple composed of the first 6
-            fields of matching lines in the output.
-
-        % fdasd --table /dev/dasdc
-        reading volume label ..: VOL1
-        reading vtoc ..........: ok
-
-
-        Disk /dev/dasdc:
-          cylinders ............: 10017
-          tracks per cylinder ..: 15
-          blocks per track .....: 12
-          bytes per block ......: 4096
-          volume label .........: VOL1
-          volume serial ........: 0X1522
-          max partitions .......: 3
-
-         ------------------------------- tracks -------------------------------
-                       Device      start      end   length   Id  System
-                  /dev/dasdc1          2    43694    43693    1  Linux native
-                  /dev/dasdc2      43695    87387    43693    2  Linux native
-                  /dev/dasdc3      87388   131080    43693    3  Linux native
-                                  131081   150254    19174       unused
-        exiting...
-        """
-        cmd = ['fdasd', '--table', self.devname]
-        out, _err = util.subp(cmd, capture=True)
-        lines = re.findall('.*%s.*Linux.*' % self.devname, out)
-        partitions = []
-        for line in lines:
-            partitions.append(tuple(line.split()[0:5]))
-
-        return partitions
-
     def partition(self, partnumber, partsize, strict=True):
         """ Add a partition to this DasdDevice specifying partnumber and size.
 
@@ -422,31 +424,24 @@ class DasdDevice(CcwDevice):
         if strict and not os.path.exists(self.devname):
             raise RuntimeError("devname '%s' does not exist" % self.devname)
 
-        info = dasdview(self.devname)
-        geo = info['geometry']
-
-        existing_partitions = self.get_partition_table()
-        partitions = []
-        for partinfo in existing_partitions[0:partnumber]:
-            # (devpath, start_track, end_track, nr_tracks, partnum)
-            start = partinfo[1]
-            end = partinfo[2]
-            partitions.append((start, end))
+        pt = DasdPartitionTable.from_fdasd(self.devname)
+        new_partitions = []
+        for partinfo in pt.partitions[0:partnumber]:
+            new_partitions.append((partinfo.start, partinfo.end))
 
         # first partition always starts at track 2
         # all others start after the previous partition ends
         if partnumber == 1:
             start = 2
         else:
-            start = int(partitions[-1][1]) + 1
-        # end is size + 1
-        tracks_needed = int(self._bytes_to_tracks(geo, partsize))
-        end = start + tracks_needed + 1
-        partitions.append(("%s" % start, "%s" % end))
+            start = int(pt.partitions[-1].end) + 1
+        # end is inclusive
+        end = start + pt.tracks_needed(partsize) - 1
+        new_partitions.append((start, end))
 
         content = "\n".join(["[%s,%s]" % (part[0], part[1])
-                             for part in partitions])
-        LOG.debug("fdasd: partitions to be created: %s", partitions)
+                             for part in new_partitions])
+        LOG.debug("fdasd: partitions to be created: %s", new_partitions)
         LOG.debug("fdasd: content=\n%s", content)
         wfp = tempfile.NamedTemporaryFile(suffix=".fdasd", delete=False)
         wfp.close()
@@ -479,14 +474,6 @@ class DasdDevice(CcwDevice):
         """
         return self.ccw_device_attr('online') == "1"
 
-    def status(self):
-        """ Read and return device_id's 'status' sysfs attribute value'
-
-        :param device_id: string of device ccw bus_id.
-        :returns: string: the value inside the 'status' sysfs attribute.
-        """
-        return self.ccw_device_attr('status')
-
     def blocksize(self):
         """ Read and return device_id's 'blocksize' value.
 
diff --git a/curtin/commands/block_meta.py b/curtin/commands/block_meta.py
index 874ccbb..66dfdb4 100644
--- a/curtin/commands/block_meta.py
+++ b/curtin/commands/block_meta.py
@@ -767,7 +767,7 @@ def verify_ptable_flag(devpath, expected_flag, sfdisk_info=None):
         raise RuntimeError(msg)
 
 
-def partition_verify(devpath, info):
+def partition_verify_sfdisk(devpath, info):
     verify_exists(devpath)
     sfdisk_info = block.sfdisk_info(devpath)
     if not sfdisk_info:
@@ -779,6 +779,21 @@ def partition_verify(devpath, info):
         verify_ptable_flag(devpath, info['flag'], sfdisk_info=sfdisk_info)
 
 
+def partition_verify_fdasd(disk_path, partnumber, info):
+    verify_exists(disk_path)
+    pt = dasd.DasdPartitionTable.from_fdasd(disk_path)
+    pt_entry = pt.partitions[partnumber-1]
+    expected_tracks = pt.tracks_needed(util.human2bytes(info['size']))
+    msg = (
+        'Verifying %s part %s size, expecting %s tracks, found %s tracks' % (
+         disk_path, partnumber, expected_tracks, pt_entry.length))
+    LOG.debug(msg)
+    if expected_tracks != pt_entry.length:
+        raise RuntimeError(msg)
+    if info.get('flag', 'linux') != 'linux':
+        raise RuntimeError("dasd partitions do not support flags")
+
+
 def partition_handler(info, storage_config):
     device = info.get('device')
     size = info.get('size')
@@ -870,9 +885,12 @@ def partition_handler(info, storage_config):
     # Handle preserve flag
     create_partition = True
     if config.value_as_boolean(info.get('preserve')):
-        part_path = block.dev_path(
-            block.partition_kname(disk_kname, partnumber))
-        partition_verify(part_path, info)
+        if disk_ptable == 'vtoc':
+            partition_verify_fdasd(disk, partnumber, info)
+        else:
+            part_path = block.dev_path(
+                block.partition_kname(disk_kname, partnumber))
+            partition_verify_sfdisk(part_path, info)
         LOG.debug('Partition %s already present, skipping create', part_path)
         create_partition = False
 
diff --git a/tests/unittests/test_block_dasd.py b/tests/unittests/test_block_dasd.py
index 393e288..dfd62d3 100644
--- a/tests/unittests/test_block_dasd.py
+++ b/tests/unittests/test_block_dasd.py
@@ -204,11 +204,6 @@ class TestDasdCcwDeviceAttr(CiTestCase):
         self.m_loadfile.return_value = self.random_string()
         self.assertFalse(self.dasd.is_online())
 
-    def test_status_returns_device_status_attr(self):
-        status_val = self.random_string()
-        self.m_loadfile.return_value = status_val
-        self.assertEqual(status_val, self.dasd.status())
-
     def test_blocksize(self):
         blocksize_val = '%d' % random.choice([512, 1024, 2048, 4096])
         self.m_loadfile.return_value = blocksize_val
diff --git a/tests/unittests/test_commands_block_meta.py b/tests/unittests/test_commands_block_meta.py
index ffec434..a5129a5 100644
--- a/tests/unittests/test_commands_block_meta.py
+++ b/tests/unittests/test_commands_block_meta.py
@@ -5,7 +5,9 @@ from collections import OrderedDict
 import copy
 from mock import patch, call
 import os
+import random
 
+from curtin.block import dasd
 from curtin.commands import block_meta
 from curtin import paths, util
 from .helpers import CiTestCase
@@ -2398,10 +2400,10 @@ class TestCalcDMPartitionInfo(CiTestCase):
             self.m_subp.call_args_list)
 
 
-class TestPartitionVerify(CiTestCase):
+class TestPartitionVerifySfdisk(CiTestCase):
 
     def setUp(self):
-        super(TestPartitionVerify, self).setUp()
+        super(TestPartitionVerifySfdisk, self).setUp()
         base = 'curtin.commands.block_meta.'
         self.add_patch(base + 'verify_exists', 'm_verify_exists')
         self.add_patch(base + 'block.sfdisk_info', 'm_block_sfdisk_info')
@@ -2418,8 +2420,8 @@ class TestPartitionVerify(CiTestCase):
         self.part_size = int(util.human2bytes(self.info['size']))
         self.devpath = self.random_string()
 
-    def test_partition_verify(self):
-        block_meta.partition_verify(self.devpath, self.info)
+    def test_partition_verify_sfdisk(self):
+        block_meta.partition_verify_sfdisk(self.devpath, self.info)
         self.assertEqual(
             [call(self.devpath)],
             self.m_verify_exists.call_args_list)
@@ -2437,7 +2439,7 @@ class TestPartitionVerify(CiTestCase):
 
     def test_partition_verify_skips_ptable_no_flag(self):
         del self.info['flag']
-        block_meta.partition_verify(self.devpath, self.info)
+        block_meta.partition_verify_sfdisk(self.devpath, self.info)
         self.assertEqual(
             [call(self.devpath)],
             self.m_verify_exists.call_args_list)
@@ -2451,6 +2453,47 @@ class TestPartitionVerify(CiTestCase):
         self.assertEqual([], self.m_verify_ptable_flag.call_args_list)
 
 
+class TestPartitionVerifyFdasd(CiTestCase):
+
+    def setUp(self):
+        super(TestPartitionVerifyFdasd, self).setUp()
+        base = 'curtin.commands.block_meta.'
+        self.add_patch(base + 'verify_exists', 'm_verify_exists')
+        self.add_patch(
+            base + 'dasd.DasdPartitionTable.from_fdasd', 'm_from_fdasd',
+            autospec=False, spec=dasd.DasdPartitionTable.from_fdasd)
+        self.add_patch(base + 'verify_size', 'm_verify_size')
+        self.info = {
+            'id': 'disk-sda-part-2',
+            'type': 'partition',
+            'device': 'sda',
+            'number': 2,
+            'size': '5GB',
+            'flag': 'linux',
+        }
+        self.part_size = int(util.human2bytes(self.info['size']))
+        self.devpath = self.random_string()
+
+    def test_partition_verify_fdasd(self):
+        blocks_per_track = random.randint(10, 20)
+        bytes_per_block = random.randint(1, 8)*512
+        fake_pt = dasd.DasdPartitionTable(
+            '/dev/dasdfoo', blocks_per_track, bytes_per_block)
+        tracks_needed = fake_pt.tracks_needed(
+            util.human2bytes(self.info['size']))
+        fake_pt.partitions = [
+            dasd.DasdPartition(fake_pt, '', 0, 0, tracks_needed, '1', 'Linux'),
+            ]
+        self.m_from_fdasd.side_effect = iter([fake_pt])
+        block_meta.partition_verify_fdasd(self.devpath, 1, self.info)
+        self.assertEqual(
+            [call(self.devpath)],
+            self.m_verify_exists.call_args_list)
+        self.assertEqual(
+            [call(self.devpath)],
+            self.m_from_fdasd.call_args_list)
+
+
 class TestVerifyExists(CiTestCase):
 
     def setUp(self):