From 99b9953fa0649d28b86b30567f3415371a1b69b1 Mon Sep 17 00:00:00 2001 From: Bob Fournier Date: Sun, 7 Mar 2021 15:18:40 -0500 Subject: [PATCH] Check the base device if the read-only file cannot be read For some drives, the partition e.g. `/dev/sda1` will not have the 'ro' file which can result in a metadata erasure failure but the base device (`/dev/sda`) will have this file. Add an additional check for the base device. Change-Id: Ia01bdbf82cee6ce15fabdc42f9c23036df55b4c5 Story: 2008696 Task: 42004 (cherry picked from commit 4afe4f6069bae2215617b676ae9d4860ac84609c) --- ironic_python_agent/hardware.py | 20 ++++++++++++++----- .../tests/unit/test_hardware.py | 20 +++++++++++++++++++ ...ead_only_base_device-5bc15ac2f034aca9.yaml | 7 +++++++ 3 files changed, 42 insertions(+), 5 deletions(-) create mode 100644 releasenotes/notes/partition_check_read_only_base_device-5bc15ac2f034aca9.yaml diff --git a/ironic_python_agent/hardware.py b/ironic_python_agent/hardware.py index ac9bb1ae8..6bd4d71bb 100644 --- a/ironic_python_agent/hardware.py +++ b/ironic_python_agent/hardware.py @@ -22,6 +22,7 @@ from multiprocessing.pool import ThreadPool import os import re import shlex +import string import time from ironic_lib import disk_utils @@ -1379,26 +1380,35 @@ class GenericHardwareManager(HardwareManager): return 'linux_raid_member' in out - def _is_read_only_device(self, block_device): + def _is_read_only_device(self, block_device, partition=False): """Check if a block device is read-only. Checks the device read-only flag in order to identify virtual and firmware driven devices that block write device access. :param block_device: a BlockDevice object + :param partition: if True, this device is a partition :returns: True if the device is read-only. """ try: - dev_name = str(block_device.name)[5:] + dev_name = os.path.basename(block_device.name) + if partition: + # Check the base device + dev_name = dev_name.rstrip(string.digits) with open('/sys/block/%s/ro' % dev_name, 'r') as f: flag = f.read().strip() if flag == '1': return True except IOError as e: - LOG.warning("Could not determine if %s is a read-only device. " - "Error: %s", - block_device.name, e) + # Check underlying device as the file may exist there + if (not partition and dev_name[-1].isdigit() + and 'nvme' not in dev_name): + return self._is_read_only_device(block_device, partition=True) + + LOG.warning("Could not determine if %(name)s is a" + "read-only device. Error: %(err)s", + {'name': block_device.name, 'err': e}) return False def _get_ata_security_lines(self, block_device): diff --git a/ironic_python_agent/tests/unit/test_hardware.py b/ironic_python_agent/tests/unit/test_hardware.py index c3ed4c596..01b9e48c5 100644 --- a/ironic_python_agent/tests/unit/test_hardware.py +++ b/ironic_python_agent/tests/unit/test_hardware.py @@ -2938,6 +2938,26 @@ class TestGenericHardwareManager(base.IronicAgentTest): mock_open.assert_called_once_with( '/sys/block/sdfake/ro', 'r') + def test__is_read_only_device_partition_error(self): + device = hardware.BlockDevice('/dev/sdfake1', 'fake', 1024, False) + with mock.patch( + 'builtins.open', side_effect=IOError, + autospec=True) as mock_open: + self.assertFalse(self.hardware._is_read_only_device(device)) + mock_open.assert_has_calls([ + mock.call('/sys/block/sdfake1/ro', 'r'), + mock.call('/sys/block/sdfake/ro', 'r')]) + + def test__is_read_only_device_partition_ok(self): + fileobj = mock.mock_open(read_data='1\n') + device = hardware.BlockDevice('/dev/sdfake1', 'fake', 1024, False) + reads = [IOError, '1'] + with mock.patch( + 'builtins.open', fileobj, create=True) as mock_open: + mock_dev_file = mock_open.return_value.read + mock_dev_file.side_effect = reads + self.assertTrue(self.hardware._is_read_only_device(device)) + @mock.patch.object(utils, 'execute', autospec=True) def test_get_bmc_address(self, mocked_execute): mocked_execute.return_value = '192.1.2.3\n', '' diff --git a/releasenotes/notes/partition_check_read_only_base_device-5bc15ac2f034aca9.yaml b/releasenotes/notes/partition_check_read_only_base_device-5bc15ac2f034aca9.yaml new file mode 100644 index 000000000..b1451eb1c --- /dev/null +++ b/releasenotes/notes/partition_check_read_only_base_device-5bc15ac2f034aca9.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + Fixes an issue where metadata erasure cleaning fails for partitions + because the read-only file isn't found, while it is available at the + base device. Adds a check for the base device file on failure. See + `story 2008696 `_.