Remove assumption that a valid IPMI channel cannot follow an invalid one

dtantsur · dtantsur · commit d6ff5116f4a5 · 2017-09-11T18:43:24.000+02:00
It seems to be incorrect at least for some iLO machines.
Also harden the code against invalid output from ipmitool.

Change-Id: I733785e9c7d86eadca963f0776910504bf91bcfe
Closes-Bug: #1714944
diff --git a/ironic_python_agent/hardware.py b/ironic_python_agent/hardware.py
@@ -21,6 +21,7 @@
 
 from ironic_lib import disk_utils
 from ironic_lib import utils as il_utils
+import netaddr
 from oslo_concurrency import processutils
 from oslo_config import cfg
 from oslo_log import log
@@ -944,14 +945,20 @@ def get_bmc_address(self):
                 out, e = utils.execute(
                     "ipmitool lan print {} | awk '/IP Address[[:space:]]*:/"
                     " {{print $4}}'".format(channel), shell=True)
-                # Invalid channel cannot be followed by a valid one, so we can
-                # safely break here
                 if e.startswith("Invalid channel"):
-                    break
-                # In case we get empty IP or 0.0.0.0 on a valid channel,
-                # we need to keep querying
-                if out.strip() not in ('', '0.0.0.0'):
-                    return out.strip()
+                    continue
+                out = out.strip()
+
+                try:
+                    netaddr.IPAddress(out)
+                except netaddr.AddrFormatError:
+                    LOG.warning('Invalid IP address: %s', out)
+                    continue
+
+                # In case we get 0.0.0.0 on a valid channel, we need to keep
+                # querying
+                if out != '0.0.0.0':
+                    return out
 
         except (processutils.ProcessExecutionError, OSError) as e:
             # Not error, because it's normal in virtual environment
diff --git a/ironic_python_agent/tests/unit/test_hardware.py b/ironic_python_agent/tests/unit/test_hardware.py
@@ -1564,7 +1564,11 @@ def test_get_bmc_address_iterate_channels(self, mocked_execute):
         # and for any other we return a correct IP address
         def side_effect(*args, **kwargs):
             if args[0].startswith("ipmitool lan print 1"):
+                return '', 'Invalid channel 1\n'
+            elif args[0].startswith("ipmitool lan print 2"):
                 return '0.0.0.0\n', ''
+            elif args[0].startswith("ipmitool lan print 3"):
+                return 'meow', ''
             else:
                 return '192.1.2.3\n', ''
         mocked_execute.side_effect = side_effect
diff --git a/releasenotes/notes/ipmi-address-channel-b6b8010c41d05c1b.yaml b/releasenotes/notes/ipmi-address-channel-b6b8010c41d05c1b.yaml
@@ -0,0 +1,6 @@
+---
+fixes:
+  - |
+    Fixes incorrect assumption that a valid channel cannot follow an invalid
+    one in IPMI (`bug 1714944
+    <https://bugs.launchpad.net/ironic-python-agent/+bug/1714944>`_).
