Merge "Add secondary sorting by name when guessing root disk"

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.