From d0a53149f82a3587515a4371f0f4cad8570dc715 Mon Sep 17 00:00:00 2001 From: Ilya Etingof Date: Mon, 28 Aug 2017 16:59:39 +0200 Subject: [PATCH] Report /dev/disk/by-path on inspection When inspecting block devices on a node, discover and report the /dev/disk/by-path/XXX name along with the /dev/XXX block device name. The second name does not change between Linux system reboots and has greater chances to be the same across similarly configured nodes. Note: this patch depends on https://review.openstack.org/#/c/500524/ library patch, but this dependency can't be expressed with Depends-On clause. Therefore once this patch requires a followup patch to enable one currently disabled test in this patch. Change-Id: I09874f19890500d352521f89573e2aaf50a29022 Closes-Bug: #1679726 --- ironic_python_agent/hardware.py | 33 ++++++- .../tests/unit/test_hardware.py | 85 +++++++++++++++---- ...-report-disk-by-path-e3fd4c331d200903.yaml | 4 + 3 files changed, 101 insertions(+), 21 deletions(-) create mode 100644 releasenotes/notes/inspection-to-report-disk-by-path-e3fd4c331d200903.yaml diff --git a/ironic_python_agent/hardware.py b/ironic_python_agent/hardware.py index 568d5bf49..9c47293d5 100644 --- a/ironic_python_agent/hardware.py +++ b/ironic_python_agent/hardware.py @@ -112,6 +112,27 @@ def list_all_block_devices(block_type='disk'): """ _udev_settle() + # map device names to /dev/disk/by-path symbolic links that points to it + + by_path_mapping = {} + + disk_by_path_dir = '/dev/disk/by-path' + + try: + paths = os.listdir(disk_by_path_dir) + + for path in paths: + path = os.path.join(disk_by_path_dir, path) + # Turn possibly relative symbolic link into absolute + devname = os.path.join(disk_by_path_dir, os.readlink(path)) + devname = os.path.abspath(devname) + by_path_mapping[devname] = path + + except OSError as e: + LOG.warning("Path %(path)s is inaccessible, skipping by-path " + "block devices reporting " + "Error: %(error)s", {'path': disk_by_path_dir, 'error': e}) + columns = ['KNAME', 'MODEL', 'SIZE', 'ROTA', 'TYPE'] report = utils.execute('lsblk', '-Pbdi', '-o{}'.format(','.join(columns)), check_exit_code=[0])[0] @@ -138,7 +159,8 @@ def list_all_block_devices(block_type='disk'): raise errors.BlockDeviceError( '%s must be returned by lsblk.' % ', '.join(sorted(missing))) - name = '/dev/' + device['KNAME'] + name = os.path.join('/dev', device['KNAME']) + try: udev = pyudev.Device.from_device_file(context, name) # pyudev started raising another error in 0.18 @@ -166,12 +188,16 @@ def list_all_block_devices(block_type='disk'): LOG.warning('Could not find the SCSI address (HCTL) for ' 'device %s. Skipping', name) + # Not all /dev entries are pointed to from /dev/disk/by-path + by_path_name = by_path_mapping.get(name) + devices.append(BlockDevice(name=name, model=device['MODEL'], size=int(device['SIZE']), rotational=bool(int(device['ROTA'])), vendor=_get_device_info(device['KNAME'], 'block', 'vendor'), + by_path=by_path_name, **extra)) return devices @@ -200,11 +226,11 @@ class HardwareType(object): class BlockDevice(encoding.SerializableComparable): serializable_fields = ('name', 'model', 'size', 'rotational', 'wwn', 'serial', 'vendor', 'wwn_with_extension', - 'wwn_vendor_extension', 'hctl') + 'wwn_vendor_extension', 'hctl', 'by_path') def __init__(self, name, model, size, rotational, wwn=None, serial=None, vendor=None, wwn_with_extension=None, - wwn_vendor_extension=None, hctl=None): + wwn_vendor_extension=None, hctl=None, by_path=None): self.name = name self.model = model self.size = size @@ -215,6 +241,7 @@ class BlockDevice(encoding.SerializableComparable): self.wwn_with_extension = wwn_with_extension self.wwn_vendor_extension = wwn_vendor_extension self.hctl = hctl + self.by_path = by_path class NetworkInterface(encoding.SerializableComparable): diff --git a/ironic_python_agent/tests/unit/test_hardware.py b/ironic_python_agent/tests/unit/test_hardware.py index 216f9776e..b0a7d780f 100644 --- a/ironic_python_agent/tests/unit/test_hardware.py +++ b/ironic_python_agent/tests/unit/test_hardware.py @@ -640,9 +640,14 @@ class TestGenericHardwareManager(base.IronicAgentTest): self.assertEqual('0x1014', interfaces[0].product) self.assertEqual('em0', interfaces[0].biosdevname) + @mock.patch.object(os, 'readlink', autospec=True) + @mock.patch.object(os, 'listdir', autospec=True) @mock.patch.object(hardware, 'get_cached_node', autospec=True) @mock.patch.object(utils, 'execute', autospec=True) - def test_get_os_install_device(self, mocked_execute, mock_cached_node): + def test_get_os_install_device(self, mocked_execute, mock_cached_node, + mocked_listdir, mocked_readlink): + mocked_readlink.return_value = '/dev/sda' + mocked_listdir.return_value = ['1:0:0:0'] mock_cached_node.return_value = None mocked_execute.return_value = (BLK_DEVICE_TEMPLATE, '') self.assertEqual('/dev/sdb', self.hardware.get_os_install_device()) @@ -651,11 +656,16 @@ class TestGenericHardwareManager(base.IronicAgentTest): check_exit_code=[0]) mock_cached_node.assert_called_once_with() + @mock.patch.object(os, 'readlink', autospec=True) + @mock.patch.object(os, 'listdir', autospec=True) @mock.patch.object(hardware, 'get_cached_node', autospec=True) @mock.patch.object(utils, 'execute', autospec=True) def test_get_os_install_device_fails(self, mocked_execute, - mock_cached_node): + mock_cached_node, + mocked_listdir, mocked_readlink): """Fail to find device >=4GB w/o root device hints""" + mocked_readlink.return_value = '/dev/sda' + mocked_listdir.return_value = ['1:0:0:0'] mock_cached_node.return_value = None mocked_execute.return_value = (BLK_DEVICE_TEMPLATE_SMALL, '') ex = self.assertRaises(errors.DeviceNotFound, @@ -690,7 +700,8 @@ class TestGenericHardwareManager(base.IronicAgentTest): wwn='fake-wwn', wwn_with_extension='fake-wwnven0', wwn_vendor_extension='ven0', - serial='fake-serial'), + serial='fake-serial', + by_path='/dev/disk/by-path/1:0:0:0'), ] self.assertEqual(expected_device, @@ -736,6 +747,12 @@ class TestGenericHardwareManager(base.IronicAgentTest): self._get_os_install_device_root_device_hints( {'rotational': value}, '/dev/sdb') + # TODO(etingof): enable this test once the patch below is merged + # https://review.openstack.org/#/c/500524/ + def skip_test_get_os_install_device_root_device_hints_by_path(self): + self._get_os_install_device_root_device_hints( + {'by_path': '/dev/disk/by-path/1:0:0:0'}, '/dev/sdb') + @mock.patch.object(hardware, 'list_all_block_devices', autospec=True) @mock.patch.object(hardware, 'get_cached_node', autospec=True) def test_get_os_install_device_root_device_hints_no_device_found( @@ -911,13 +928,23 @@ class TestGenericHardwareManager(base.IronicAgentTest): list_mock.assert_called_once_with() + @mock.patch.object(os, 'readlink', autospec=True) @mock.patch.object(os, 'listdir', autospec=True) @mock.patch.object(hardware, '_get_device_info', autospec=True) @mock.patch.object(pyudev.Device, 'from_device_file', autospec=False) @mock.patch.object(utils, 'execute', autospec=True) def test_list_all_block_device(self, mocked_execute, mocked_udev, - mocked_dev_vendor, mock_listdir): - mock_listdir.return_value = ['1:0:0:0'] + mocked_dev_vendor, mock_listdir, + mock_readlink): + by_path_map = { + '/dev/disk/by-path/1:0:0:0': '/dev/sda', + '/dev/disk/by-path/1:0:0:1': '/dev/sdb', + '/dev/disk/by-path/1:0:0:2': '/dev/sdc', + '/dev/disk/by-path/1:0:0:3': '/dev/sdd', + } + mock_readlink.side_effect = lambda x, m=by_path_map: m[x] + mock_listdir.return_value = [os.path.basename(x) + for x in sorted(by_path_map)] mocked_execute.return_value = (BLK_DEVICE_TEMPLATE, '') mocked_udev.side_effect = pyudev.DeviceNotFoundError() mocked_dev_vendor.return_value = 'Super Vendor' @@ -928,25 +955,29 @@ class TestGenericHardwareManager(base.IronicAgentTest): size=3116853504, rotational=False, vendor='Super Vendor', - hctl='1:0:0:0'), + hctl='1:0:0:0', + by_path='/dev/disk/by-path/1:0:0:0'), hardware.BlockDevice(name='/dev/sdb', model='Fastable SD131 7', size=10737418240, rotational=False, vendor='Super Vendor', - hctl='1:0:0:0'), + hctl='1:0:0:0', + by_path='/dev/disk/by-path/1:0:0:1'), hardware.BlockDevice(name='/dev/sdc', model='NWD-BLP4-1600', size=1765517033472, rotational=False, vendor='Super Vendor', - hctl='1:0:0:0'), + hctl='1:0:0:0', + by_path='/dev/disk/by-path/1:0:0:2'), hardware.BlockDevice(name='/dev/sdd', model='NWD-BLP4-1600', size=1765517033472, rotational=False, vendor='Super Vendor', - hctl='1:0:0:0'), + hctl='1:0:0:0', + by_path='/dev/disk/by-path/1:0:0:3'), ] self.assertEqual(4, len(devices)) @@ -960,12 +991,21 @@ class TestGenericHardwareManager(base.IronicAgentTest): for dev in ('sda', 'sdb', 'sdc', 'sdd')] mock_listdir.assert_has_calls(expected_calls) + expected_calls = [mock.call('/dev/disk/by-path/1:0:0:%d' % dev) + for dev in range(4)] + mock_readlink.assert_has_calls(expected_calls) + + @mock.patch.object(os, 'readlink', autospec=True) + @mock.patch.object(os, 'listdir', autospec=True) @mock.patch.object(hardware, '_get_device_info', autospec=True) @mock.patch.object(pyudev.Device, 'from_device_file', autospec=False) @mock.patch.object(utils, 'execute', autospec=True) def test_list_all_block_device_udev_17(self, mocked_execute, mocked_udev, - mocked_dev_vendor): + mocked_dev_vendor, mocked_listdir, + mocked_readlink): # test compatibility with pyudev < 0.18 + mocked_readlink.return_value = '/dev/sda' + mocked_listdir.return_value = ['1:0:0:0'] mocked_execute.return_value = (BLK_DEVICE_TEMPLATE, '') mocked_udev.side_effect = OSError() mocked_dev_vendor.return_value = 'Super Vendor' @@ -979,22 +1019,28 @@ class TestGenericHardwareManager(base.IronicAgentTest): def test_list_all_block_device_hctl_fail(self, mocked_execute, mocked_udev, mocked_dev_vendor, mocked_listdir): - mocked_listdir.side_effect = (OSError, IndexError) + mocked_listdir.side_effect = (OSError, OSError, IndexError) mocked_execute.return_value = (BLK_DEVICE_TEMPLATE_SMALL, '') mocked_dev_vendor.return_value = 'Super Vendor' devices = hardware.list_all_block_devices() self.assertEqual(2, len(devices)) - expected_calls = [mock.call('/sys/block/%s/device/scsi_device' % dev) - for dev in ('sda', 'sdb')] - mocked_listdir.assert_has_calls(expected_calls) + expected_calls = [ + mock.call('/dev/disk/by-path'), + mock.call('/sys/block/sda/device/scsi_device'), + mock.call('/sys/block/sdb/device/scsi_device') + ] + self.assertEqual(expected_calls, mocked_listdir.call_args_list) + @mock.patch.object(os, 'readlink', autospec=True) @mock.patch.object(os, 'listdir', autospec=True) @mock.patch.object(hardware, '_get_device_info', autospec=True) @mock.patch.object(pyudev.Device, 'from_device_file', autospec=False) @mock.patch.object(utils, 'execute', autospec=True) def test_list_all_block_device_with_udev(self, mocked_execute, mocked_udev, - mocked_dev_vendor, mock_listdir): - mock_listdir.return_value = ['1:0:0:0'] + mocked_dev_vendor, mocked_listdir, + mocked_readlink): + mocked_readlink.return_value = '/dev/sda' + mocked_listdir.return_value = ['1:0:0:0'] mocked_execute.return_value = (BLK_DEVICE_TEMPLATE, '') mocked_udev.side_effect = iter([ {'ID_WWN': 'wwn%d' % i, 'ID_SERIAL_SHORT': 'serial%d' % i, @@ -1057,7 +1103,7 @@ class TestGenericHardwareManager(base.IronicAgentTest): getattr(device, attr)) expected_calls = [mock.call('/sys/block/%s/device/scsi_device' % dev) for dev in ('sda', 'sdb', 'sdc', 'sdd')] - mock_listdir.assert_has_calls(expected_calls) + mocked_listdir.assert_has_calls(expected_calls) @mock.patch.object(hardware, 'dispatch_to_managers', autospec=True) def test_erase_devices(self, mocked_dispatch): @@ -1758,13 +1804,16 @@ class TestGenericHardwareManager(base.IronicAgentTest): @mock.patch.object(utils, 'execute', autospec=True) class TestModuleFunctions(base.IronicAgentTest): + @mock.patch.object(os, 'readlink', autospec=True) @mock.patch.object(hardware, '_get_device_info', lambda x, y, z: 'FooTastic') @mock.patch.object(hardware, '_udev_settle', autospec=True) @mock.patch.object(hardware.pyudev.Device, "from_device_file", autospec=False) def test_list_all_block_devices_success(self, mocked_fromdevfile, - mocked_udev, mocked_execute): + mocked_udev, mocked_readlink, + mocked_execute): + mocked_readlink.return_value = '/dev/sda' mocked_fromdevfile.return_value = {} mocked_execute.return_value = (BLK_DEVICE_TEMPLATE_SMALL, '') result = hardware.list_all_block_devices() diff --git a/releasenotes/notes/inspection-to-report-disk-by-path-e3fd4c331d200903.yaml b/releasenotes/notes/inspection-to-report-disk-by-path-e3fd4c331d200903.yaml new file mode 100644 index 000000000..5296e3b05 --- /dev/null +++ b/releasenotes/notes/inspection-to-report-disk-by-path-e3fd4c331d200903.yaml @@ -0,0 +1,4 @@ +--- +features: + - The /dev/disk/by-path/XXX device name is added to the storage + hardware inspection report.