Fixes get_holder disks with nvme drives

Change-Id: I195ffdeeb3c13bdec5fc1735b82efa53c8d9d3de

4 files changed, 116 insertions, 14 deletions

diff --git a/ironic_python_agent/hardware.py b/ironic_python_agent/hardware.py
index 37e36f7c..6f9ebe29 100644
--- a/ironic_python_agent/hardware.py
+++ b/ironic_python_agent/hardware.py
@@ -168,9 +168,20 @@ def get_holder_disks(raid_device):
 
     lines = out.splitlines()
     # the first line contains the md device itself
+
+    holder_parts = []
     for line in lines[1:]:
-        device = re.findall(r'/dev/\D+', line)
-        holder_disks += device
+        device = re.findall(r'/dev/\w+', line)
+        holder_parts += device
+
+    for part in holder_parts:
+
+        device = utils.extract_device(part)
+        if not device:
+            msg = ('Could not get holder disks of %s: unexpected pattern '
+                   'for partition %s') % (raid_device, part)
+            raise errors.SoftwareRAIDError(msg)
+        holder_disks.append(device)
 
     return holder_disks
 
diff --git a/ironic_python_agent/tests/unit/test_hardware.py b/ironic_python_agent/tests/unit/test_hardware.py
index a8496934..9f4eb47b 100644
--- a/ironic_python_agent/tests/unit/test_hardware.py
+++ b/ironic_python_agent/tests/unit/test_hardware.py
@@ -639,6 +639,34 @@ Consistency Policy : resync
        1     253       80        1      active sync   /dev/vdf1
 """)
 
+MDADM_DETAIL_OUTPUT_NVME = ("""/dev/md0:
+        Version : 1.2
+  Creation Time : Wed Aug  7 13:47:27 2019
+     Raid Level : raid1
+     Array Size : 439221248 (418.87 GiB 449.76 GB)
+  Used Dev Size : 439221248 (418.87 GiB 449.76 GB)
+   Raid Devices : 2
+  Total Devices : 2
+    Persistence : Superblock is persistent
+
+  Intent Bitmap : Internal
+
+    Update Time : Wed Aug  7 14:37:21 2019
+          State : clean
+ Active Devices : 2
+Working Devices : 2
+ Failed Devices : 0
+  Spare Devices : 0
+
+           Name : rescue:0  (local to host rescue)
+           UUID : abe222bc:98735860:ab324674:e4076313
+         Events : 426
+
+    Number   Major   Minor   RaidDevice State
+       0     259        2        0      active sync   /dev/nvme0n1p1
+       1     259        3        1      active sync   /dev/nvme1n1p1
+""")
+
 
 MDADM_DETAIL_OUTPUT_BROKEN_RAID0 = ("""/dev/md126:
            Version : 1.2
@@ -2872,33 +2900,43 @@ class TestGenericHardwareManager(base.IronicAgentTest):
     @mock.patch.object(utils, 'execute', autospec=True)
     def test__get_component_devices(self, mocked_execute):
         mocked_execute.side_effect = [(MDADM_DETAIL_OUTPUT, '')]
-        raid_device = hardware.BlockDevice('/dev/md0', 'RAID-1',
-                                           1073741824, True)
-        component_devices = hardware._get_component_devices(raid_device.name)
+        component_devices = hardware._get_component_devices('/dev/md0')
         self.assertEqual(['/dev/vde1', '/dev/vdf1'], component_devices)
 
     @mock.patch.object(utils, 'execute', autospec=True)
     def test__get_component_devices_broken_raid0(self, mocked_execute):
         mocked_execute.side_effect = [(MDADM_DETAIL_OUTPUT_BROKEN_RAID0, '')]
-        raid_device = hardware.BlockDevice('/dev/md126', 'RAID-0',
-                                           1073741824, True)
-        component_devices = hardware._get_component_devices(raid_device.name)
+        component_devices = hardware._get_component_devices('/dev/md126')
         self.assertEqual(['/dev/sda2'], component_devices)
 
     @mock.patch.object(utils, 'execute', autospec=True)
     def test_get_holder_disks(self, mocked_execute):
         mocked_execute.side_effect = [(MDADM_DETAIL_OUTPUT, '')]
-        raid_device = hardware.BlockDevice('/dev/md0', 'RAID-1',
-                                           1073741824, True)
-        holder_disks = hardware.get_holder_disks(raid_device.name)
+        holder_disks = hardware.get_holder_disks('/dev/md0')
         self.assertEqual(['/dev/vde', '/dev/vdf'], holder_disks)
 
     @mock.patch.object(utils, 'execute', autospec=True)
+    def test_get_holder_disks_with_nvme(self, mocked_execute):
+        mocked_execute.side_effect = [(MDADM_DETAIL_OUTPUT_NVME, '')]
+        holder_disks = hardware.get_holder_disks('/dev/md0')
+        self.assertEqual(['/dev/nvme0n1', '/dev/nvme1n1'], holder_disks)
+
+    @mock.patch.object(utils, 'execute', autospec=True)
+    def test_get_holder_disks_unexpected_devices(self, mocked_execute):
+        side_effect = MDADM_DETAIL_OUTPUT_NVME.replace('nvme1n1p1',
+                                                       'notmatching1a')
+        mocked_execute.side_effect = [(side_effect, '')]
+        self.assertRaisesRegex(
+            errors.SoftwareRAIDError,
+            r'^Software RAID caused unknown error: Could not get holder disks '
+            r'of /dev/md0: unexpected pattern for partition '
+            r'/dev/notmatching1a$',
+            hardware.get_holder_disks, '/dev/md0')
+
+    @mock.patch.object(utils, 'execute', autospec=True)
     def test_get_holder_disks_broken_raid0(self, mocked_execute):
         mocked_execute.side_effect = [(MDADM_DETAIL_OUTPUT_BROKEN_RAID0, '')]
-        raid_device = hardware.BlockDevice('/dev/md126', 'RAID-0',
-                                           1073741824, True)
-        holder_disks = hardware.get_holder_disks(raid_device.name)
+        holder_disks = hardware.get_holder_disks('/dev/md126')
         self.assertEqual(['/dev/sda'], holder_disks)
 
     @mock.patch.object(hardware, 'get_holder_disks', autospec=True)
diff --git a/ironic_python_agent/tests/unit/test_utils.py b/ironic_python_agent/tests/unit/test_utils.py
index 24b5994f..4ee0ebd3 100644
--- a/ironic_python_agent/tests/unit/test_utils.py
+++ b/ironic_python_agent/tests/unit/test_utils.py
@@ -556,3 +556,39 @@ class TestUtils(testtools.TestCase):
                          keyfile='spam', certfile='ham')
         self.assertEqual((True, ('ham', 'spam')),
                          utils.get_ssl_client_options(conf))
+
+    def test_device_extractor(self):
+        self.assertEqual(
+            'md0',
+            utils.extract_device('md0p1')
+        )
+        self.assertEqual(
+            '/dev/md0',
+            utils.extract_device('/dev/md0p1')
+        )
+        self.assertEqual(
+            'sda',
+            utils.extract_device('sda12')
+        )
+        self.assertEqual(
+            '/dev/sda',
+            utils.extract_device('/dev/sda12')
+        )
+        self.assertEqual(
+            'nvme0n1',
+            utils.extract_device('nvme0n1p12')
+        )
+        self.assertEqual(
+            '/dev/nvme0n1',
+            utils.extract_device('/dev/nvme0n1p12')
+        )
+        self.assertEqual(
+            '/dev/hello',
+            utils.extract_device('/dev/hello42')
+        )
+        self.assertIsNone(
+            utils.extract_device('/dev/sda')
+        )
+        self.assertIsNone(
+            utils.extract_device('whatevernotmatchin12a')
+        )
diff --git a/ironic_python_agent/utils.py b/ironic_python_agent/utils.py
index 2c71827a..0257cfb1 100644
--- a/ironic_python_agent/utils.py
+++ b/ironic_python_agent/utils.py
@@ -17,6 +17,7 @@ import errno
 import glob
 import io
 import os
+import re
 import shutil
 import subprocess
 import tarfile
@@ -60,6 +61,9 @@ COLLECT_LOGS_COMMANDS = {
 }
 
 
+DEVICE_EXTRACTOR = re.compile(r'^(?:(.*\d)p|(.*\D))(?:\d+)$')
+
+
 def execute(*cmd, **kwargs):
     """Convenience wrapper around ironic_lib's execute() method.
 
@@ -436,3 +440,16 @@ def get_ssl_client_options(conf):
     else:
         cert = None
     return verify, cert
+
+
+def extract_device(part):
+    """Extract the device from a partition name or path.
+
+    :param part: the partition
+    :return: a device if success, None otherwise
+    """
+
+    m = DEVICE_EXTRACTOR.match(part)
+    if not m:
+        return None
+    return (m.group(1) or m.group(2))