Add port option support for ipmitool

This adds support for ipmitools port (-p) option allowing operators
to run ipmi on non-default ports.

Change-Id: I9203c2db01cc46a183901c454e98426b4583ce08
closes-bug: #1523577

doc/source/deploy/install-guide.rst

@@ -1460,6 +1460,12 @@ and may be combined if desired.
     driver_info/ipmi_password=$PASS \
     driver_info/ipmi_address=$ADDRESS
 
+.. note::
+   If IPMI is running on a port other than 623 (the default). The port must
+   be added to ``driver_info`` by specifying the ``ipmi_port`` value.
+   Example:
+    ironic node-update $NODE_UUID add driver_info/ipmi_port=$PORT_NUMBER
+
    Note that you may also specify all ``driver_info`` parameters during
    ``node-create`` by passing the **-i** option multiple times.
 

ironic/drivers/modules/ipmitool.py

@@ -76,6 +76,7 @@ REQUIRED_PROPERTIES = {
 }
 OPTIONAL_PROPERTIES = {
     'ipmi_password': _("password. Optional."),
+    'ipmi_port': _("remote IPMI RMCP port. Optional."),
     'ipmi_priv_level': _("privilege level; default is ADMINISTRATOR. One of "
                          "%s. Optional.") % ', '.join(VALID_PRIV_LEVELS),
     'ipmi_username': _("username; default is NULL user. Optional."),
@@ -255,6 +256,7 @@ def _parse_driver_info(node):
     address = info.get('ipmi_address')
     username = info.get('ipmi_username')
     password = six.text_type(info.get('ipmi_password', ''))
+    dest_port = info.get('ipmi_port')
     port = info.get('ipmi_terminal_port')
     priv_level = info.get('ipmi_priv_level', 'ADMINISTRATOR')
     bridging_type = info.get('ipmi_bridging', 'no')
@@ -276,6 +278,9 @@ def _parse_driver_info(node):
     if port is not None:
         port = utils.validate_network_port(port, 'ipmi_terminal_port')
 
+    if dest_port is not None:
+        dest_port = utils.validate_network_port(dest_port, 'ipmi_port')
+
     # check if ipmi_bridging has proper value
     if bridging_type == 'no':
         # if bridging is not selected, then set all bridging params to None
@@ -325,6 +330,7 @@ def _parse_driver_info(node):
 
     return {
         'address': address,
+        'dest_port': dest_port,
         'username': username,
         'password': password,
         'port': port,
@@ -361,6 +367,11 @@ def _exec_ipmitool(driver_info, command):
             driver_info['address'],
             '-L', driver_info['priv_level']
             ]
+
+    if driver_info['dest_port']:
+        args.append('-p')
+        args.append(driver_info['dest_port'])
+
     if driver_info['username']:
         args.append('-U')
         args.append(driver_info['username'])

ironic/tests/unit/conductor/test_manager.py

@@ -3309,7 +3309,7 @@ class ManagerTestProperties(tests_db_base.DbTestCase):
 
     def test_driver_properties_fake_ipmitool(self):
         expected = ['ipmi_address', 'ipmi_terminal_port',
-                    'ipmi_password', 'ipmi_priv_level',
+                    'ipmi_password', 'ipmi_port', 'ipmi_priv_level',
                     'ipmi_username', 'ipmi_bridging',
                     'ipmi_transit_channel', 'ipmi_transit_address',
                     'ipmi_target_channel', 'ipmi_target_address',
@@ -3346,7 +3346,7 @@ class ManagerTestProperties(tests_db_base.DbTestCase):
 
     def test_driver_properties_pxe_ipmitool(self):
         expected = ['ipmi_address', 'ipmi_terminal_port',
-                    'ipmi_password', 'ipmi_priv_level',
+                    'ipmi_password', 'ipmi_port', 'ipmi_priv_level',
                     'ipmi_username', 'ipmi_bridging', 'ipmi_transit_channel',
                     'ipmi_transit_address', 'ipmi_target_channel',
                     'ipmi_target_address', 'ipmi_local_address',

ironic/tests/unit/drivers/modules/test_ipmitool.py

@@ -603,6 +603,20 @@ class IPMIToolPrivateMethodTestCase(db_base.DbTestCase):
         self.assertRaises(exception.InvalidParameterValue,
                           ipmi._parse_driver_info, node)
 
+    def test__parse_driver_info_invalid_ipmi_port(self, mock_sleep):
+        info = dict(INFO_DICT)
+        info['ipmi_port'] = '700000'
+        node = obj_utils.get_test_node(self.context, driver_info=info)
+        self.assertRaises(exception.InvalidParameterValue,
+                          ipmi._parse_driver_info, node)
+
+    def test__parse_driver_info_ipmi_port_valid(self, mock_sleep):
+        info = dict(INFO_DICT)
+        info['ipmi_port'] = '623'
+        node = obj_utils.get_test_node(self.context, driver_info=info)
+        ret = ipmi._parse_driver_info(node)
+        self.assertEqual(623, ret['dest_port'])
+
     @mock.patch.object(ipmi, '_is_option_supported', autospec=True)
     @mock.patch.object(ipmi, '_make_password_file', autospec=True)
     @mock.patch.object(utils, 'execute', autospec=True)
@@ -1069,6 +1083,38 @@ class IPMIToolPrivateMethodTestCase(db_base.DbTestCase):
         self.assertTrue(mock_pwf.called)
         mock_exec.assert_called_once_with(*args)
 
+    @mock.patch.object(ipmi, '_is_option_supported', autospec=True)
+    @mock.patch.object(ipmi, '_make_password_file', autospec=True)
+    @mock.patch.object(utils, 'execute', autospec=True)
+    def test__exec_ipmitool_with_port(self, mock_exec, mock_pwf, mock_support,
+                                      mock_sleep):
+        self.info['dest_port'] = '1623'
+        ipmi.LAST_CMD_TIME = {}
+        pw_file_handle = tempfile.NamedTemporaryFile()
+        pw_file = pw_file_handle.name
+        file_handle = open(pw_file, "w")
+        args = [
+            'ipmitool',
+            '-I', 'lanplus',
+            '-H', self.info['address'],
+            '-L', self.info['priv_level'],
+            '-p', '1623',
+            '-U', self.info['username'],
+            '-f', file_handle,
+            'A', 'B', 'C',
+        ]
+
+        mock_support.return_value = False
+        mock_pwf.return_value = file_handle
+        mock_exec.return_value = (None, None)
+
+        ipmi._exec_ipmitool(self.info, 'A B C')
+
+        mock_support.assert_called_once_with('timing')
+        mock_pwf.assert_called_once_with(self.info['password'])
+        mock_exec.assert_called_once_with(*args)
+        self.assertFalse(mock_sleep.called)
+
     @mock.patch.object(ipmi, '_exec_ipmitool', autospec=True)
     def test__power_status_on(self, mock_exec, mock_sleep):
         mock_exec.return_value = ["Chassis Power is on\n", None]

releasenotes/notes/Add-port-option-support-to-ipmitool-e125d07fe13c53e7.yaml

@@ -0,0 +1,5 @@
+---
+features:
+  - Add support for ipmitool's port (-p) option. This
+    allows ipmitool support for operators that do not
+    use the default port (623) as their IPMI port.