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

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')

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())

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,
+    If no hints are passed, order the devices by size (primary key) and
-    assume it is the OS disk
+    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
+    # NOTE(arne_wiebalck): Order devices by size and name. Secondary
-    # all cases, is there a more reasonable default behavior?
+    # ordering by name is done to increase chances of successful
-    block_devices.sort(key=lambda device: device.size)
+    # 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 "

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.