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.