IPMI command should depend on console type

This change implements _get_impi_cmd method for
IPMI consoles. Depends on type of console,
ipmi command should contain different arguments.

Change-Id: I55e712a1a91aed3d533f636e519f0bae1f9be2d4
Closes-bug: #1611285
(cherry picked from commit 0acbf378bb543a3428856d80036d4de9c5d133a7)

3 files changed, 93 insertions, 10 deletions

diff --git a/ironic/drivers/modules/ipmitool.py b/ironic/drivers/modules/ipmitool.py
index 13d8e57b5..14e22f7ec 100644
--- a/ironic/drivers/modules/ipmitool.py
+++ b/ironic/drivers/modules/ipmitool.py
@@ -1159,6 +1159,21 @@ class IPMIConsole(base.ConsoleInterface):
                 "Check the 'ipmi_protocol_version' parameter in "
                 "node's driver_info"))
 
+    def _get_ipmi_cmd(self, driver_info, pw_file):
+        """Get ipmi command for ipmitool usage.
+
+        :param driver_info: driver info with the ipmitool parameters
+        :param pw_file: password file to be used in ipmitool command
+        :returns: returns a command string for ipmitool
+        """
+        user = driver_info.get('username')
+        user = ' -U {}'.format(user) if user else ''
+        return ("ipmitool -H %(address)s -I lanplus"
+                "%(user)s -f %(pwfile)s"
+                % {'address': driver_info['address'],
+                   'user': user,
+                   'pwfile': pw_file})
+
     def _start_console(self, driver_info, start_method):
         """Start a remote console for the node.
 
@@ -1174,14 +1189,7 @@ class IPMIConsole(base.ConsoleInterface):
         path = _console_pwfile_path(driver_info['uuid'])
         pw_file = console_utils.make_persistent_password_file(
             path, driver_info['password'] or '\0')
-
-        ipmi_cmd = ("/:%(uid)s:%(gid)s:HOME:ipmitool -H %(address)s"
-                    " -I lanplus -U %(user)s -f %(pwfile)s"
-                    % {'uid': os.getuid(),
-                       'gid': os.getgid(),
-                       'address': driver_info['address'],
-                       'user': driver_info['username'],
-                       'pwfile': pw_file})
+        ipmi_cmd = self._get_ipmi_cmd(driver_info, pw_file)
 
         for name, option in BRIDGING_OPTIONS:
             if driver_info[name] is not None:
@@ -1201,6 +1209,20 @@ class IPMIConsole(base.ConsoleInterface):
 class IPMIShellinaboxConsole(IPMIConsole):
     """A ConsoleInterface that uses ipmitool and shellinabox."""
 
+    def _get_ipmi_cmd(self, driver_info, pw_file):
+        """Get ipmi command for ipmitool usage.
+
+        :param driver_info: driver info with the ipmitool parameters
+        :param pw_file: password file to be used in ipmitool command
+        :returns: returns a command string for ipmitool
+        """
+        command = super(IPMIShellinaboxConsole, self)._get_ipmi_cmd(
+            driver_info, pw_file)
+        return ("/:%(uid)s:%(gid)s:HOME:%(basic_command)s"
+                % {'uid': os.getuid(),
+                   'gid': os.getgid(),
+                   'basic_command': command})
+
     @METRICS.timer('IPMIShellinaboxConsole.start_console')
     def start_console(self, task):
         """Start a remote console for the node.
diff --git a/ironic/tests/unit/drivers/modules/test_ipmitool.py b/ironic/tests/unit/drivers/modules/test_ipmitool.py
index 5b0b1d0bf..eecdcf402 100644
--- a/ironic/tests/unit/drivers/modules/test_ipmitool.py
+++ b/ironic/tests/unit/drivers/modules/test_ipmitool.py
@@ -1618,6 +1618,34 @@ class IPMIToolDriverTestCase(db_base.DbTestCase):
             self.assertRaises(exception.InvalidParameterValue,
                               task.driver.console.validate, task)
 
+    def test__get_ipmi_cmd(self):
+        with task_manager.acquire(self.context,
+                                  self.node.uuid) as task:
+            driver_info = ipmi._parse_driver_info(task.node)
+            ipmi_cmd = self.driver.console._get_ipmi_cmd(driver_info,
+                                                         'pw_file')
+            expected_ipmi_cmd = ("/:%(uid)s:%(gid)s:HOME:ipmitool "
+                                 "-H %(address)s -I lanplus -U %(user)s "
+                                 "-f pw_file" %
+                                 {'uid': os.getuid(), 'gid': os.getgid(),
+                                  'address': driver_info['address'],
+                                  'user': driver_info['username']})
+        self.assertEqual(expected_ipmi_cmd, ipmi_cmd)
+
+    def test__get_ipmi_cmd_without_user(self):
+        with task_manager.acquire(self.context,
+                                  self.node.uuid) as task:
+            driver_info = ipmi._parse_driver_info(task.node)
+            driver_info['username'] = None
+            ipmi_cmd = self.driver.console._get_ipmi_cmd(driver_info,
+                                                         'pw_file')
+            expected_ipmi_cmd = ("/:%(uid)s:%(gid)s:HOME:ipmitool "
+                                 "-H %(address)s -I lanplus "
+                                 "-f pw_file" %
+                                 {'uid': os.getuid(), 'gid': os.getgid(),
+                                  'address': driver_info['address']})
+        self.assertEqual(expected_ipmi_cmd, ipmi_cmd)
+
     @mock.patch.object(ipmi.IPMIConsole, '_start_console', autospec=True)
     def test_start_console(self, mock_start):
         mock_start.return_value = None
@@ -1630,9 +1658,10 @@ class IPMIToolDriverTestCase(db_base.DbTestCase):
             self.driver.console, driver_info,
             console_utils.start_shellinabox_console)
 
+    @mock.patch.object(ipmi.IPMIConsole, '_get_ipmi_cmd', autospec=True)
     @mock.patch.object(console_utils, 'start_shellinabox_console',
                        autospec=True)
-    def test__start_console(self, mock_start):
+    def test__start_console(self, mock_start, mock_ipmi_cmd):
         mock_start.return_value = None
 
         with task_manager.acquire(self.context,
@@ -1644,6 +1673,8 @@ class IPMIToolDriverTestCase(db_base.DbTestCase):
         mock_start.assert_called_once_with(self.info['uuid'],
                                            self.info['port'],
                                            mock.ANY)
+        mock_ipmi_cmd.assert_called_once_with(self.driver.console,
+                                              driver_info, mock.ANY)
 
     @mock.patch.object(console_utils, 'start_shellinabox_console',
                        autospec=True)
@@ -2167,6 +2198,30 @@ class IPMIToolSocatDriverTestCase(IPMIToolDriverTestCase):
     def setUp(self):
         super(IPMIToolSocatDriverTestCase, self).setUp(terminal="socat")
 
+    def test__get_ipmi_cmd(self):
+        with task_manager.acquire(self.context,
+                                  self.node.uuid) as task:
+            driver_info = ipmi._parse_driver_info(task.node)
+            ipmi_cmd = self.driver.console._get_ipmi_cmd(driver_info,
+                                                         'pw_file')
+            expected_ipmi_cmd = ("ipmitool -H %(address)s -I lanplus "
+                                 "-U %(user)s -f pw_file" %
+                                 {'address': driver_info['address'],
+                                  'user': driver_info['username']})
+        self.assertEqual(expected_ipmi_cmd, ipmi_cmd)
+
+    def test__get_ipmi_cmd_without_user(self):
+        with task_manager.acquire(self.context,
+                                  self.node.uuid) as task:
+            driver_info = ipmi._parse_driver_info(task.node)
+            driver_info['username'] = None
+            ipmi_cmd = self.driver.console._get_ipmi_cmd(driver_info,
+                                                         'pw_file')
+            expected_ipmi_cmd = ("ipmitool -H %(address)s -I lanplus "
+                                 "-f pw_file" %
+                                 {'address': driver_info['address']})
+        self.assertEqual(expected_ipmi_cmd, ipmi_cmd)
+
     @mock.patch.object(ipmi.IPMIConsole, '_start_console', autospec=True)
     @mock.patch.object(ipmi.IPMISocatConsole, '_exec_stop_console',
                        autospec=True)
@@ -2183,9 +2238,10 @@ class IPMIToolSocatDriverTestCase(IPMIToolDriverTestCase):
             self.driver.console, driver_info,
             console_utils.start_socat_console)
 
+    @mock.patch.object(ipmi.IPMISocatConsole, '_get_ipmi_cmd', autospec=True)
     @mock.patch.object(console_utils, 'start_socat_console',
                        autospec=True)
-    def test__start_console(self, mock_start):
+    def test__start_console(self, mock_start, mock_ipmi_cmd):
         mock_start.return_value = None
 
         with task_manager.acquire(self.context,
@@ -2197,6 +2253,8 @@ class IPMIToolSocatDriverTestCase(IPMIToolDriverTestCase):
         mock_start.assert_called_once_with(self.info['uuid'],
                                            self.info['port'],
                                            mock.ANY)
+        mock_ipmi_cmd.assert_called_once_with(self.driver.console,
+                                              driver_info, mock.ANY)
 
     @mock.patch.object(console_utils, 'start_socat_console',
                        autospec=True)
diff --git a/releasenotes/notes/ipmi-cmd-for-impmi-consoles-2e1104f22df3efcd.yaml b/releasenotes/notes/ipmi-cmd-for-impmi-consoles-2e1104f22df3efcd.yaml
new file mode 100644
index 000000000..adbb85361
--- /dev/null
+++ b/releasenotes/notes/ipmi-cmd-for-impmi-consoles-2e1104f22df3efcd.yaml
@@ -0,0 +1,3 @@
+fixes:
+  - Fixes a bug with incorrect base socat command,
+    which prevented the usage of console.