From 5f09bd03e17d3973c2a5d68f6c407104d3d3b91e Mon Sep 17 00:00:00 2001
From: Lucas Alvares Gomes <lucasagomes@gmail.com>
Date: Thu, 7 Jul 2016 09:38:17 +0100
Subject: [PATCH] Small refactor in the root device loop matching logic

When matching the root device hints passed to Ironic and the devices
present in the disk, the logic to the "size" attribute was different was
outside the main loop. This patch refactors that uses the same loop for
matching the size attribute, improving a little the readability.

This patch also makes sure that we convert the value of "size" to
integer before attempting to match it.

Change-Id: I619153232b9b58696369185ad5be90ccfd38a4ed
---
 ironic_python_agent/hardware.py               | 26 ++++++++++++-------
 .../tests/unit/test_hardware.py               | 12 +++++++++
 2 files changed, 28 insertions(+), 10 deletions(-)

diff --git a/ironic_python_agent/hardware.py b/ironic_python_agent/hardware.py
index 758ea484b..0c1590728 100644
--- a/ironic_python_agent/hardware.py
+++ b/ironic_python_agent/hardware.py
@@ -610,6 +610,16 @@ class GenericHardwareManager(HardwareManager):
                 if hint == 'rotational':
                     hint_value = strutils.bool_from_string(hint_value)
 
+                elif hint == 'size':
+                    try:
+                        hint_value = int(hint_value)
+                    except (ValueError, TypeError):
+                        LOG.warning(
+                            'Root device hint "size" is not an integer. '
+                            'Current value: "%(value)s"; and type: "%(type)s"',
+                            {'value': hint_value, 'type': type(hint_value)})
+                        return False
+
                 if hint_value != current_value:
                     LOG.debug("Root device hint %(hint)s=%(value)s does not "
                               "match the device %(device)s value of "
@@ -623,7 +633,7 @@ class GenericHardwareManager(HardwareManager):
             def check_device_attrs(device):
                 for key in ('model', 'wwn', 'serial', 'vendor',
                             'wwn_with_extension', 'wwn_vendor_extension',
-                            'name', 'rotational'):
+                            'name', 'rotational', 'size'):
                     if key not in root_device_hints:
                         continue
 
@@ -634,21 +644,17 @@ class GenericHardwareManager(HardwareManager):
                     if isinstance(value, six.string_types):
                         value = utils.normalize(value)
 
+                    if key == 'size':
+                        # Since we don't support units yet we expect the size
+                        # in GiB for now
+                        value = value / units.Gi
+
                     if not match(key, value, device.name):
                         return False
 
                 return True
 
             for dev in block_devices:
-                # TODO(lucasagomes): Add support for operators <, >, =, etc...
-                # to better deal with sizes.
-                if 'size' in root_device_hints:
-                    # Since we don't support units yet we expect the size
-                    # in GiB for now
-                    size = dev.size / units.Gi
-                    if not match('size', size, dev.name):
-                        continue
-
                 if check_device_attrs(dev):
                     return dev.name
 
diff --git a/ironic_python_agent/tests/unit/test_hardware.py b/ironic_python_agent/tests/unit/test_hardware.py
index b84e2086f..dac4bb667 100644
--- a/ironic_python_agent/tests/unit/test_hardware.py
+++ b/ironic_python_agent/tests/unit/test_hardware.py
@@ -477,6 +477,18 @@ class TestGenericHardwareManager(test_base.BaseTestCase):
         self._get_os_install_device_root_device_hints(
             {'size': 10}, '/dev/sdb')
 
+    def test_get_os_install_device_root_device_hints_size_str(self):
+        self._get_os_install_device_root_device_hints(
+            {'size': '10'}, '/dev/sdb')
+
+    @mock.patch.object(hardware.LOG, 'warning')
+    def test_get_os_install_device_root_device_hints_size_not_int(
+            self, mock_log):
+        self.assertRaises(errors.DeviceNotFound,
+                          self._get_os_install_device_root_device_hints,
+                          {'size': 'not-int'}, '/dev/sdb')
+        self.assertTrue(mock_log.called)
+
     def test_get_os_install_device_root_device_hints_vendor(self):
         self._get_os_install_device_root_device_hints(
             {'vendor': 'fake-vendor'}, '/dev/sdb')