From fb74b556067536b65f2e244f46c6ee9c5c556e1d Mon Sep 17 00:00:00 2001 From: Arne Wiebalck Date: Wed, 6 Feb 2019 19:12:57 +0100 Subject: [PATCH] Add secondary sorting by name when guessing root disk As some BIOSes try to boot only from the "first" disk, Ironic should order potential disks not only by size, but also by name. This patch proposes to add secondary sorting by device name when identifying the root disk. Change-Id: I4017c839eeb9d00d2b4ad5b90e4e9b65b74296c7 Story: #2004976 Task: #29434 --- .../tests/unit/test_inspector.py | 2 +- ironic_python_agent/tests/unit/test_utils.py | 62 +++++++++++++++++++ ironic_python_agent/utils.py | 13 ++-- ...-name-for-root-disks-4de2c0358b9a1e2c.yaml | 10 +++ 4 files changed, 81 insertions(+), 6 deletions(-) create mode 100644 releasenotes/notes/add-secondary-sorting-by-name-for-root-disks-4de2c0358b9a1e2c.yaml diff --git a/ironic_python_agent/tests/unit/test_inspector.py b/ironic_python_agent/tests/unit/test_inspector.py index edd204511..2944bbc6f 100644 --- a/ironic_python_agent/tests/unit/test_inspector.py +++ b/ironic_python_agent/tests/unit/test_inspector.py @@ -216,7 +216,7 @@ class TestCollectDefault(BaseDiscoverTest): self.assertTrue(self.data['inventory'][key]) self.assertEqual('boot:if', self.data['boot_interface']) - self.assertEqual(self.inventory['disks'][0].name, + self.assertEqual(self.inventory['disks'][2].name, self.data['root_disk'].name) mock_dispatch.assert_called_once_with('list_hardware_info') diff --git a/ironic_python_agent/tests/unit/test_utils.py b/ironic_python_agent/tests/unit/test_utils.py index b335d8e51..d99ebfaa7 100644 --- a/ironic_python_agent/tests/unit/test_utils.py +++ b/ironic_python_agent/tests/unit/test_utils.py @@ -30,6 +30,7 @@ import six import testtools from ironic_python_agent import errors +from ironic_python_agent import hardware from ironic_python_agent.tests.unit import base as ironic_agent_base from ironic_python_agent import utils @@ -411,6 +412,67 @@ class TestUtils(testtools.TestCase): 'foo', binary=True, log_stdout=False) self.assertEqual(contents, data.read()) + @mock.patch.object(subprocess, 'check_call', autospec=True) + def test_guess_root_disk_primary_sort(self, mock_call): + block_devices = [ + hardware.BlockDevice(name='/dev/sdc', + model='too small', + size=4294967295, + rotational=True), + hardware.BlockDevice(name='/dev/sda', + model='bigger than sdb', + size=21474836480, + rotational=True), + hardware.BlockDevice(name='/dev/sdb', + model='', + size=10737418240, + rotational=True), + hardware.BlockDevice(name='/dev/sdd', + model='bigger than sdb', + size=21474836480, + rotational=True), + ] + device = utils.guess_root_disk(block_devices) + self.assertEqual(device.name, '/dev/sdb') + + @mock.patch.object(subprocess, 'check_call', autospec=True) + def test_guess_root_disk_secondary_sort(self, mock_call): + block_devices = [ + hardware.BlockDevice(name='/dev/sdc', + model='_', + size=10737418240, + rotational=True), + hardware.BlockDevice(name='/dev/sdb', + model='_', + size=10737418240, + rotational=True), + hardware.BlockDevice(name='/dev/sda', + model='_', + size=10737418240, + rotational=True), + hardware.BlockDevice(name='/dev/sdd', + model='_', + size=10737418240, + rotational=True), + ] + device = utils.guess_root_disk(block_devices) + self.assertEqual(device.name, '/dev/sda') + + @mock.patch.object(subprocess, 'check_call', autospec=True) + def test_guess_root_disk_disks_too_small(self, mock_call): + block_devices = [ + hardware.BlockDevice(name='/dev/sda', + model='too small', + size=4294967295, + rotational=True), + hardware.BlockDevice(name='/dev/sdb', + model='way too small', + size=1, + rotational=True), + ] + self.assertRaises(errors.DeviceNotFound, + utils.guess_root_disk, block_devices) + @mock.patch.object(subprocess, 'check_call', autospec=True) def test_is_journalctl_present(self, mock_call): self.assertTrue(utils.is_journalctl_present()) diff --git a/ironic_python_agent/utils.py b/ironic_python_agent/utils.py index 2bc2bfa11..4621ff8d7 100644 --- a/ironic_python_agent/utils.py +++ b/ironic_python_agent/utils.py @@ -290,12 +290,15 @@ class AccumulatedFailures(object): def guess_root_disk(block_devices, min_size_required=4 * units.Gi): """Find suitable disk provided that root device hints are not given. - If no hints are passed find the first device larger than min_size_required, - assume it is the OS disk + If no hints are passed, order the devices by size (primary key) and + name (secondary key), and return the first device larger than + min_size_required as the root disk. """ - # TODO(russellhaering): This isn't a valid assumption in - # all cases, is there a more reasonable default behavior? - block_devices.sort(key=lambda device: device.size) + # NOTE(arne_wiebalck): Order devices by size and name. Secondary + # ordering by name is done to increase chances of successful + # booting for BIOSes which try only one (the "first") disk. + block_devices.sort(key=lambda device: (device.size, device.name)) + if not block_devices or block_devices[-1].size < min_size_required: raise errors.DeviceNotFound( "No suitable device was found " diff --git a/releasenotes/notes/add-secondary-sorting-by-name-for-root-disks-4de2c0358b9a1e2c.yaml b/releasenotes/notes/add-secondary-sorting-by-name-for-root-disks-4de2c0358b9a1e2c.yaml new file mode 100644 index 000000000..5df483e13 --- /dev/null +++ b/releasenotes/notes/add-secondary-sorting-by-name-for-root-disks-4de2c0358b9a1e2c.yaml @@ -0,0 +1,10 @@ +--- +features: + - | + Adds secondary sorting by device name when guessing the root disk. This + makes the selection process more predicatble and increases the chances + that systems which try only one device for booting will actually + successfully boot after deployment. As the primary sorting is still done + by size, the root device hints still take priority, and the current + behaviour is basically not specifying the order beyond size, this change + does not break backwards compatibility.