From 83782018f73266d52e5c3fab6e82f3d3257fb94d Mon Sep 17 00:00:00 2001 From: Josh Gachnang Date: Fri, 18 Jul 2014 08:13:12 -0700 Subject: [PATCH] Improve Disk Detection The previous implementation of list_block_devices used blockdev, which would list partitions, software RAID and other devices as block devices. By switching to lsblk, the agent can filter down to only physical block devices, which is all the agent cares about for any of its operations. This change adds two new fields to the BlockDevice class: model, a string of the block devices reported model, and rotational, a boolean representing a spinning disk (True) or a solid state disk (False). This data can be useful for vendor hardware managers. Change-Id: I385c3bb378c2c49385bca14a1d7efa074933becf Closes-Bug: 1344351 --- ironic_python_agent/errors.py | 9 +++ ironic_python_agent/hardware.py | 49 +++++++++--- ironic_python_agent/tests/hardware.py | 78 ++++++++++++++----- .../tests/ironic_api_client.py | 14 ++-- 4 files changed, 114 insertions(+), 36 deletions(-) diff --git a/ironic_python_agent/errors.py b/ironic_python_agent/errors.py index be61bdb64..f461aaa1a 100644 --- a/ironic_python_agent/errors.py +++ b/ironic_python_agent/errors.py @@ -214,5 +214,14 @@ class BlockDeviceEraseError(RESTError): self.details = details +class BlockDeviceError(RESTError): + """Error raised when a block devices causes an unknown error""" + message = 'Block device caused unknown error' + + def __init__(self, details): + super(BlockDeviceError, self).__init__(details) + self.details = details + + class ExtensionError(Exception): pass diff --git a/ironic_python_agent/hardware.py b/ironic_python_agent/hardware.py index ef5a0400e..5aeb97ca9 100644 --- a/ironic_python_agent/hardware.py +++ b/ironic_python_agent/hardware.py @@ -15,6 +15,7 @@ import abc import functools import os +import shlex import netifaces import psutil @@ -47,11 +48,13 @@ class HardwareType(object): class BlockDevice(encoding.Serializable): - serializable_fields = ('name', 'size') + serializable_fields = ('name', 'model', 'size', 'rotational') - def __init__(self, name, size): + def __init__(self, name, model, size, rotational): self.name = name + self.model = model self.size = size + self.rotational = rotational class NetworkInterface(encoding.Serializable): @@ -210,18 +213,40 @@ class GenericHardwareManager(HardwareManager): return Memory(int(psutil.phymem_usage().total)) def list_block_devices(self): - report = utils.execute('blockdev', '--report', + """List all physical block devices + + The switches we use for lsblk: P for KEY="value" output, + b for size output in bytes, d to exclude dependant devices + (like md or dm devices), i to ensure ascii characters only, + and o to specify the fields we need + + :return: A list of BlockDevices + """ + report = utils.execute('lsblk', '-PbdioKNAME,MODEL,SIZE,ROTA,TYPE', check_exit_code=[0])[0] lines = report.split('\n') - lines = [line.split() for line in lines if line != ''] - startsec_idx = lines[0].index('StartSec') - device_idx = lines[0].index('Device') - size_idx = lines[0].index('Size') - # If a device doesn't start at sector 0, assume it is a partition - return [BlockDevice(line[device_idx], - int(line[size_idx])) - for line - in lines[1:] if int(line[startsec_idx]) == 0] + + devices = [] + for line in lines: + device = {} + # Split into KEY=VAL pairs + vals = shlex.split(line) + for key, val in (v.split('=', 1) for v in vals): + device[key] = val.strip() + # Ignore non disk + if device.get('TYPE') != 'disk': + continue + + # Ensure all required keys are at least present, even if blank + diff = set(['KNAME', 'MODEL', 'SIZE', 'ROTA']) - set(device.keys()) + if diff: + raise errors.BlockDeviceError( + '%s must be returned by lsblk.' % diff) + devices.append(BlockDevice(name='/dev/' + device['KNAME'], + model=device['MODEL'], + size=int(device['SIZE']), + rotational=bool(int(device['ROTA'])))) + return devices def get_os_install_device(self): # Find the first device larger than 4GB, assume it is the OS disk diff --git a/ironic_python_agent/tests/hardware.py b/ironic_python_agent/tests/hardware.py index 62ef56309..cb76290a5 100644 --- a/ironic_python_agent/tests/hardware.py +++ b/ironic_python_agent/tests/hardware.py @@ -113,6 +113,18 @@ HDPARM_INFO_TEMPLATE = ( 'Checksum: correct\n' ) +BLK_DEVICE_TEMPLATE = ( + 'KNAME="sda" MODEL="TinyUSB Drive" SIZE="3116853504" ' + 'ROTA="0" TYPE="disk"\n' + 'KNAME="sdb" MODEL="Fastable SD131 7" SIZE="31016853504" ' + 'ROTA="0" TYPE="disk"\n' + 'KNAME="sdc" MODEL="NWD-BLP4-1600 " SIZE="1765517033472" ' + ' ROTA="0" TYPE="disk"\n' + 'KNAME="sdd" MODEL="NWD-BLP4-1600 " SIZE="1765517033472" ' + ' ROTA="0" TYPE="disk"\n' + 'KNAME="loop0" MODEL="" SIZE="109109248" ROTA="1" TYPE="loop"' +) + class FakeHardwareManager(hardware.GenericHardwareManager): def __init__(self, hardware_support): @@ -183,18 +195,10 @@ class TestGenericHardwareManager(test_base.BaseTestCase): @mock.patch.object(utils, 'execute') def test_get_os_install_device(self, mocked_execute): - mocked_execute.return_value = ( - 'RO RA SSZ BSZ StartSec Size Device\n' - 'rw 256 512 4096 0 249578283616 /dev/sda\n' - 'rw 256 512 4096 2048 8587837440 /dev/sda1\n' - 'rw 256 512 4096 124967424 15728640 /dev/sda2\n' - 'rw 256 512 4096 0 31016853504 /dev/sdb\n' - 'rw 256 512 4096 0 249578283616 /dev/sdc\n', '') - + mocked_execute.return_value = (BLK_DEVICE_TEMPLATE, '') self.assertEqual(self.hardware.get_os_install_device(), '/dev/sdb') - mocked_execute.assert_called_once_with('blockdev', - '--report', - check_exit_code=[0]) + mocked_execute.assert_called_once_with( + 'lsblk', '-PbdioKNAME,MODEL,SIZE,ROTA,TYPE', check_exit_code=[0]) @mock.patch('psutil.cpu_count') @mock.patch(OPEN_FUNCTION_NAME) @@ -282,8 +286,8 @@ class TestGenericHardwareManager(test_base.BaseTestCase): self.hardware.list_block_devices = mock.Mock() self.hardware.list_block_devices.return_value = [ - hardware.BlockDevice('/dev/sdj', 1073741824), - hardware.BlockDevice('/dev/hdaa', 65535), + hardware.BlockDevice('/dev/sdj', 'big', 1073741824, True), + hardware.BlockDevice('/dev/hdaa', 'small', 65535, False), ] hardware_info = self.hardware.list_hardware_info() @@ -294,6 +298,36 @@ class TestGenericHardwareManager(test_base.BaseTestCase): self.assertEqual(hardware_info['interfaces'], self.hardware.list_network_interfaces()) + @mock.patch.object(utils, 'execute') + def test_list_block_device(self, mocked_execute): + mocked_execute.return_value = (BLK_DEVICE_TEMPLATE, '') + devices = self.hardware.list_block_devices() + expected_devices = [ + hardware.BlockDevice(name='/dev/sda', + model='TinyUSB Drive', + size=3116853504, + rotational=False), + hardware.BlockDevice(name='/dev/sdb', + model='Fastable SD131 7', + size=31016853504, + rotational=False), + hardware.BlockDevice(name='/dev/sdc', + model='NWD-BLP4-1600', + size=1765517033472, + rotational=False), + hardware.BlockDevice(name='/dev/sdd', + model='NWD-BLP4-1600', + size=1765517033472, + rotational=False) + ] + + self.assertEqual(4, len(expected_devices)) + for expected, device in zip(expected_devices, devices): + # Compare all attrs of the objects + for attr in ['name', 'model', 'size', 'rotational']: + self.assertEqual(getattr(expected, attr), + getattr(device, attr)) + @mock.patch.object(utils, 'execute') def test_erase_block_device_ata_success(self, mocked_execute): hdparm_info_fields = { @@ -308,7 +342,8 @@ class TestGenericHardwareManager(test_base.BaseTestCase): (HDPARM_INFO_TEMPLATE % hdparm_info_fields, ''), ] - block_device = hardware.BlockDevice('/dev/sda', 1073741824) + block_device = hardware.BlockDevice('/dev/sda', 'big', 1073741824, + True) self.hardware.erase_block_device(block_device) mocked_execute.assert_has_calls([ mock.call('hdparm', '-I', '/dev/sda'), @@ -327,7 +362,8 @@ class TestGenericHardwareManager(test_base.BaseTestCase): (hdparm_output, '') ] - block_device = hardware.BlockDevice('/dev/sda', 1073741824) + block_device = hardware.BlockDevice('/dev/sda', 'big', 1073741824, + True) self.assertRaises(errors.BlockDeviceEraseError, self.hardware.erase_block_device, block_device) @@ -344,7 +380,8 @@ class TestGenericHardwareManager(test_base.BaseTestCase): (hdparm_output, '') ] - block_device = hardware.BlockDevice('/dev/sda', 1073741824) + block_device = hardware.BlockDevice('/dev/sda', 'big', 1073741824, + True) self.assertRaises(errors.BlockDeviceEraseError, self.hardware.erase_block_device, block_device) @@ -361,7 +398,8 @@ class TestGenericHardwareManager(test_base.BaseTestCase): (hdparm_output, '') ] - block_device = hardware.BlockDevice('/dev/sda', 1073741824) + block_device = hardware.BlockDevice('/dev/sda', 'big', 1073741824, + True) self.assertRaises(errors.BlockDeviceEraseError, self.hardware.erase_block_device, block_device) @@ -378,7 +416,8 @@ class TestGenericHardwareManager(test_base.BaseTestCase): (hdparm_output, '') ] - block_device = hardware.BlockDevice('/dev/sda', 1073741824) + block_device = hardware.BlockDevice('/dev/sda', 'big', 1073741824, + True) self.assertRaises(errors.BlockDeviceEraseError, self.hardware.erase_block_device, block_device) @@ -406,7 +445,8 @@ class TestGenericHardwareManager(test_base.BaseTestCase): (hdparm_output_after, ''), ] - block_device = hardware.BlockDevice('/dev/sda', 1073741824) + block_device = hardware.BlockDevice('/dev/sda', 'big', 1073741824, + True) self.assertRaises(errors.BlockDeviceEraseError, self.hardware.erase_block_device, block_device) diff --git a/ironic_python_agent/tests/ironic_api_client.py b/ironic_python_agent/tests/ironic_api_client.py index 6799a4f19..56b705e85 100644 --- a/ironic_python_agent/tests/ironic_api_client.py +++ b/ironic_python_agent/tests/ironic_api_client.py @@ -46,8 +46,8 @@ class TestBaseIronicPythonAgent(test_base.BaseTestCase): ], 'cpu': hardware.CPU('Awesome Jay CPU x10 9001', '9001', '10'), 'disks': [ - hardware.BlockDevice('/dev/sdj', '9001'), - hardware.BlockDevice('/dev/hdj', '9002'), + hardware.BlockDevice('/dev/sdj', 'small', '9001', False), + hardware.BlockDevice('/dev/hdj', 'big', '9002', False), ], 'memory': hardware.Memory('8675309'), } @@ -162,13 +162,17 @@ class TestBaseIronicPythonAgent(test_base.BaseTestCase): }, u'disks': [ { + u'model': u'small', u'name': u'/dev/sdj', - u'size': u'9001', + u'rotational': False, + u'size': u'9001' }, { + u'model': u'big', u'name': u'/dev/hdj', - u'size': u'9002', - }, + u'rotational': False, + u'size': u'9002' + } ], u'memory': { u'total': u'8675309',