From d84e95af0ac8be72e6514194e845ce297c4f6dda Mon Sep 17 00:00:00 2001
From: Ankit Kumar <ankit.dhn31@gmail.com>
Date: Wed, 24 Aug 2016 09:03:09 +0000
Subject: [PATCH] Fix iLO drivers to not clear local_gb if its not detected

In certain cases, due to hardware limitation disk data cannot be
retrieved out-of-band. Currently iLO drivers overwrite the
'local_gb' property of node with '0' during inspection even when
it fails to detect it.
This patch fixes this behavior to ensure that the operator provided
value for 'local_gb' are not overwritten if inspection fails to
detect disk size.

Co-Authored-By: Shivanand Tendulker <stendulker@gmail.com>
Closes-bug: 1602269
Change-Id: Idad3e3fa547022da6213b048cd16c91faae3ff14
---
 ironic/drivers/modules/ilo/inspect.py         | 15 +++++++
 .../unit/drivers/modules/ilo/test_inspect.py  | 43 +++++++++++++++++++
 .../ilo-fix-inspection-b169ad0a22aea2ff.yaml  |  5 +++
 3 files changed, 63 insertions(+)
 create mode 100644 releasenotes/notes/ilo-fix-inspection-b169ad0a22aea2ff.yaml

diff --git a/ironic/drivers/modules/ilo/inspect.py b/ironic/drivers/modules/ilo/inspect.py
index 9f37816efb..81c11a1daf 100644
--- a/ironic/drivers/modules/ilo/inspect.py
+++ b/ironic/drivers/modules/ilo/inspect.py
@@ -217,7 +217,22 @@ class IloInspect(base.InspectInterface):
 
         inspected_properties = {}
         result = _get_essential_properties(task.node, ilo_object)
+
+        # A temporary hook for OOB inspection to not to update 'local_gb'
+        # for hardware if the storage is a "Direct Attached Storage" or
+        # "Dynamic Smart Array Controllers" and the operator has manually
+        # updated the local_gb in node properties prior to node inspection.
+        # This will be removed once we have inband inspection support for
+        # ilo drivers.
+        current_local_gb = task.node.properties.get('local_gb')
         properties = result['properties']
+        if current_local_gb:
+            if properties['local_gb'] == 0 and current_local_gb > 0:
+                properties['local_gb'] = current_local_gb
+                LOG.warning(_LW('Could not discover size of disk on the node '
+                                '%s. Value of `properties/local_gb` of the '
+                                'node is not overwritten.'), task.node.uuid)
+
         for known_property in self.ESSENTIAL_PROPERTIES:
             inspected_properties[known_property] = properties[known_property]
         node_properties = task.node.properties
diff --git a/ironic/tests/unit/drivers/modules/ilo/test_inspect.py b/ironic/tests/unit/drivers/modules/ilo/test_inspect.py
index 52dde9f6a8..53cdd58496 100644
--- a/ironic/tests/unit/drivers/modules/ilo/test_inspect.py
+++ b/ironic/tests/unit/drivers/modules/ilo/test_inspect.py
@@ -94,6 +94,49 @@ class IloInspectTestCase(db_base.DbTestCase):
                                                           ilo_object_mock)
             create_port_mock.assert_called_once_with(task, macs)
 
+    @mock.patch.object(ilo_inspect.LOG, 'warning',
+                       spec_set=True, autospec=True)
+    @mock.patch.object(ilo_inspect, '_get_capabilities', spec_set=True,
+                       autospec=True)
+    @mock.patch.object(ilo_inspect, '_create_ports_if_not_exist',
+                       spec_set=True, autospec=True)
+    @mock.patch.object(ilo_inspect, '_get_essential_properties', spec_set=True,
+                       autospec=True)
+    @mock.patch.object(ilo_power.IloPower, 'get_power_state', spec_set=True,
+                       autospec=True)
+    @mock.patch.object(ilo_common, 'get_ilo_object', spec_set=True,
+                       autospec=True)
+    def test_inspect_essential_ok_local_gb_zero(self, get_ilo_object_mock,
+                                                power_mock,
+                                                get_essential_mock,
+                                                create_port_mock,
+                                                get_capabilities_mock,
+                                                log_mock):
+        ilo_object_mock = get_ilo_object_mock.return_value
+        properties = {'memory_mb': '512', 'local_gb': 0,
+                      'cpus': '1', 'cpu_arch': 'x86_64'}
+        macs = {'Port 1': 'aa:aa:aa:aa:aa:aa', 'Port 2': 'bb:bb:bb:bb:bb:bb'}
+        capabilities = ''
+        result = {'properties': properties, 'macs': macs}
+        get_essential_mock.return_value = result
+        get_capabilities_mock.return_value = capabilities
+        power_mock.return_value = states.POWER_ON
+        with task_manager.acquire(self.context, self.node.uuid,
+                                  shared=False) as task:
+            task.node.properties['local_gb'] = 10
+            task.node.save()
+            expected_properties = {'memory_mb': '512', 'local_gb': 10,
+                                   'cpus': '1', 'cpu_arch': 'x86_64'}
+            task.driver.inspect.inspect_hardware(task)
+            self.assertEqual(expected_properties, task.node.properties)
+            power_mock.assert_called_once_with(mock.ANY, task)
+            get_essential_mock.assert_called_once_with(task.node,
+                                                       ilo_object_mock)
+            self.assertTrue(log_mock.called)
+            get_capabilities_mock.assert_called_once_with(task.node,
+                                                          ilo_object_mock)
+            create_port_mock.assert_called_once_with(task, macs)
+
     @mock.patch.object(ilo_inspect, '_get_capabilities', spec_set=True,
                        autospec=True)
     @mock.patch.object(ilo_inspect, '_create_ports_if_not_exist',
diff --git a/releasenotes/notes/ilo-fix-inspection-b169ad0a22aea2ff.yaml b/releasenotes/notes/ilo-fix-inspection-b169ad0a22aea2ff.yaml
new file mode 100644
index 0000000000..9990757af1
--- /dev/null
+++ b/releasenotes/notes/ilo-fix-inspection-b169ad0a22aea2ff.yaml
@@ -0,0 +1,5 @@
+---
+fixes:
+  - Fixes a bug in the iLO drivers' inspection where
+    an existing ``local_gb`` node property was
+    overwritten with "0" if not detected.