Merge "Move loading of IPMI module loading to a single point"

ironic_python_agent/hardware.py

@@ -127,6 +127,17 @@ def _udev_settle():
         return
 
 
+def _load_ipmi_modules():
+    """Load kernel modules required for IPMI interaction.
+
+    This is required to be called at least once before attempting to use
+    ipmitool or related tools.
+    """
+    il_utils.try_execute('modprobe', 'ipmi_msghandler')
+    il_utils.try_execute('modprobe', 'ipmi_devintf')
+    il_utils.try_execute('modprobe', 'ipmi_si')
+
+
 def _check_for_iscsi():
     """Connect iSCSI shared connected via iBFT or OF.
 
@@ -984,6 +995,7 @@ class GenericHardwareManager(HardwareManager):
         # Do some initialization before we declare ourself ready
         _check_for_iscsi()
         _md_scan_and_assemble()
+        _load_ipmi_modules()
         self.wait_for_disks()
         return HardwareSupport.GENERIC
 
@@ -1765,11 +1777,6 @@ class GenericHardwareManager(HardwareManager):
         :return: IP address of lan channel or 0.0.0.0 in case none of them is
                  configured properly
         """
-        # These modules are rarely loaded automatically
-        il_utils.try_execute('modprobe', 'ipmi_msghandler')
-        il_utils.try_execute('modprobe', 'ipmi_devintf')
-        il_utils.try_execute('modprobe', 'ipmi_si')
-
         try:
             # From all the channels 0-15, only 1-11 can be assigned to
             # different types of communication media and protocols and
@@ -1807,11 +1814,6 @@ class GenericHardwareManager(HardwareManager):
         :return: MAC address of the first LAN channel or 00:00:00:00:00:00 in
                  case none of them has one or is configured properly
         """
-        # These modules are rarely loaded automatically
-        il_utils.try_execute('modprobe', 'ipmi_msghandler')
-        il_utils.try_execute('modprobe', 'ipmi_devintf')
-        il_utils.try_execute('modprobe', 'ipmi_si')
-
         try:
             # From all the channels 0-15, only 1-11 can be assigned to
             # different types of communication media and protocols and
@@ -1859,11 +1861,6 @@ class GenericHardwareManager(HardwareManager):
                  configured properly. May return None value if it cannot
                  interract with system tools or critical error occurs.
         """
-        # These modules are rarely loaded automatically
-        il_utils.try_execute('modprobe', 'ipmi_msghandler')
-        il_utils.try_execute('modprobe', 'ipmi_devintf')
-        il_utils.try_execute('modprobe', 'ipmi_si')
-
         null_address_re = re.compile(r'^::(/\d{1,3})*$')
 
         def get_addr(channel, dynamic=False):

ironic_python_agent/tests/unit/test_agent.py

@@ -159,6 +159,7 @@ class TestHeartbeater(ironic_agent_base.IronicAgentTest):
 
 @mock.patch.object(hardware, '_md_scan_and_assemble', lambda: None)
 @mock.patch.object(hardware, '_check_for_iscsi', lambda: None)
+@mock.patch.object(hardware, '_load_ipmi_modules', lambda: None)
 @mock.patch.object(hardware.GenericHardwareManager, 'wait_for_disks',
                    lambda self: None)
 class TestBaseAgent(ironic_agent_base.IronicAgentTest):
@@ -959,6 +960,7 @@ class TestAgentStandalone(ironic_agent_base.IronicAgentTest):
 
 @mock.patch.object(hardware, '_md_scan_and_assemble', lambda: None)
 @mock.patch.object(hardware, '_check_for_iscsi', lambda: None)
+@mock.patch.object(hardware, '_load_ipmi_modules', lambda: None)
 @mock.patch.object(hardware.GenericHardwareManager, 'wait_for_disks',
                    lambda self: None)
 @mock.patch.object(socket, 'gethostbyname', autospec=True)

ironic_python_agent/tests/unit/test_hardware.py

@@ -2246,41 +2246,35 @@ class TestGenericHardwareManager(base.IronicAgentTest):
             mock_dev_file.side_effect = reads
             self.assertTrue(self.hardware._is_read_only_device(device))
 
-    @mock.patch.object(il_utils, 'try_execute', autospec=True)
     @mock.patch.object(utils, 'execute', autospec=True)
-    def test_get_bmc_address(self, mocked_execute, mte):
+    def test_get_bmc_address(self, mocked_execute):
         mocked_execute.return_value = '192.1.2.3\n', ''
         self.assertEqual('192.1.2.3', self.hardware.get_bmc_address())
 
-    @mock.patch.object(il_utils, 'try_execute', autospec=True)
     @mock.patch.object(utils, 'execute', autospec=True)
-    def test_get_bmc_address_virt(self, mocked_execute, mte):
+    def test_get_bmc_address_virt(self, mocked_execute):
         mocked_execute.side_effect = processutils.ProcessExecutionError()
         self.assertIsNone(self.hardware.get_bmc_address())
 
-    @mock.patch.object(il_utils, 'try_execute', autospec=True)
     @mock.patch.object(utils, 'execute', autospec=True)
-    def test_get_bmc_address_zeroed(self, mocked_execute, mte):
+    def test_get_bmc_address_zeroed(self, mocked_execute):
         mocked_execute.return_value = '0.0.0.0\n', ''
         self.assertEqual('0.0.0.0', self.hardware.get_bmc_address())
 
-    @mock.patch.object(il_utils, 'try_execute', autospec=True)
     @mock.patch.object(utils, 'execute', autospec=True)
-    def test_get_bmc_address_invalid(self, mocked_execute, mte):
+    def test_get_bmc_address_invalid(self, mocked_execute):
         # In case of invalid lan channel, stdout is empty and the error
         # on stderr is "Invalid channel"
         mocked_execute.return_value = '\n', 'Invalid channel: 55'
         self.assertEqual('0.0.0.0', self.hardware.get_bmc_address())
 
-    @mock.patch.object(il_utils, 'try_execute', autospec=True)
     @mock.patch.object(utils, 'execute', autospec=True)
-    def test_get_bmc_address_random_error(self, mocked_execute, mte):
+    def test_get_bmc_address_random_error(self, mocked_execute):
         mocked_execute.return_value = '192.1.2.3\n', 'Random error message'
         self.assertEqual('192.1.2.3', self.hardware.get_bmc_address())
 
-    @mock.patch.object(il_utils, 'try_execute', autospec=True)
     @mock.patch.object(utils, 'execute', autospec=True)
-    def test_get_bmc_address_iterate_channels(self, mocked_execute, mte):
+    def test_get_bmc_address_iterate_channels(self, mocked_execute):
         # For channel 1 we simulate unconfigured IP
         # and for any other we return a correct IP address
         def side_effect(*args, **kwargs):
@@ -2295,57 +2289,49 @@ class TestGenericHardwareManager(base.IronicAgentTest):
         mocked_execute.side_effect = side_effect
         self.assertEqual('192.1.2.3', self.hardware.get_bmc_address())
 
-    @mock.patch.object(il_utils, 'try_execute', autospec=True)
     @mock.patch.object(utils, 'execute', autospec=True)
-    def test_get_bmc_address_not_available(self, mocked_execute, mte):
+    def test_get_bmc_address_not_available(self, mocked_execute):
         mocked_execute.return_value = '', ''
         self.assertEqual('0.0.0.0', self.hardware.get_bmc_address())
 
-    @mock.patch.object(il_utils, 'try_execute', autospec=True)
     @mock.patch.object(utils, 'execute', autospec=True)
-    def test_get_bmc_mac_not_available(self, mocked_execute, mte):
+    def test_get_bmc_mac_not_available(self, mocked_execute):
         mocked_execute.return_value = '', ''
         self.assertRaises(errors.IncompatibleHardwareMethodError,
                           self.hardware.get_bmc_mac)
 
-    @mock.patch.object(il_utils, 'try_execute', autospec=True)
     @mock.patch.object(utils, 'execute', autospec=True)
-    def test_get_bmc_mac(self, mocked_execute, mte):
+    def test_get_bmc_mac(self, mocked_execute):
         mocked_execute.return_value = '192.1.2.3\n01:02:03:04:05:06', ''
         self.assertEqual('01:02:03:04:05:06', self.hardware.get_bmc_mac())
 
-    @mock.patch.object(il_utils, 'try_execute', autospec=True)
     @mock.patch.object(utils, 'execute', autospec=True)
-    def test_get_bmc_mac_virt(self, mocked_execute, mte):
+    def test_get_bmc_mac_virt(self, mocked_execute):
         mocked_execute.side_effect = processutils.ProcessExecutionError()
         self.assertIsNone(self.hardware.get_bmc_mac())
 
-    @mock.patch.object(il_utils, 'try_execute', autospec=True)
     @mock.patch.object(utils, 'execute', autospec=True)
-    def test_get_bmc_mac_zeroed(self, mocked_execute, mte):
+    def test_get_bmc_mac_zeroed(self, mocked_execute):
         mocked_execute.return_value = '0.0.0.0\n00:00:00:00:00:00', ''
         self.assertRaises(errors.IncompatibleHardwareMethodError,
                           self.hardware.get_bmc_mac)
 
-    @mock.patch.object(il_utils, 'try_execute', autospec=True)
     @mock.patch.object(utils, 'execute', autospec=True)
-    def test_get_bmc_mac_invalid(self, mocked_execute, mte):
+    def test_get_bmc_mac_invalid(self, mocked_execute):
         # In case of invalid lan channel, stdout is empty and the error
         # on stderr is "Invalid channel"
         mocked_execute.return_value = '\n', 'Invalid channel: 55'
         self.assertRaises(errors.IncompatibleHardwareMethodError,
                           self.hardware.get_bmc_mac)
 
-    @mock.patch.object(il_utils, 'try_execute', autospec=True)
     @mock.patch.object(utils, 'execute', autospec=True)
-    def test_get_bmc_mac_random_error(self, mocked_execute, mte):
+    def test_get_bmc_mac_random_error(self, mocked_execute):
         mocked_execute.return_value = ('192.1.2.3\n00:00:00:00:00:02',
                                        'Random error message')
         self.assertEqual('00:00:00:00:00:02', self.hardware.get_bmc_mac())
 
-    @mock.patch.object(il_utils, 'try_execute', autospec=True)
     @mock.patch.object(utils, 'execute', autospec=True)
-    def test_get_bmc_mac_iterate_channels(self, mocked_execute, mte):
+    def test_get_bmc_mac_iterate_channels(self, mocked_execute):
         # For channel 1 we simulate unconfigured IP
         # and for any other we return a correct IP address
         def side_effect(*args, **kwargs):
@@ -2363,15 +2349,13 @@ class TestGenericHardwareManager(base.IronicAgentTest):
         mocked_execute.side_effect = side_effect
         self.assertEqual('01:02:03:04:05:06', self.hardware.get_bmc_mac())
 
-    @mock.patch.object(il_utils, 'try_execute', autospec=True)
     @mock.patch.object(utils, 'execute', autospec=True)
-    def test_get_bmc_v6address_not_enabled(self, mocked_execute, mte):
+    def test_get_bmc_v6address_not_enabled(self, mocked_execute):
         mocked_execute.side_effect = [('ipv4\n', '')] * 11
         self.assertEqual('::/0', self.hardware.get_bmc_v6address())
 
-    @mock.patch.object(il_utils, 'try_execute', autospec=True)
     @mock.patch.object(utils, 'execute', autospec=True)
-    def test_get_bmc_v6address_dynamic_address(self, mocked_execute, mte):
+    def test_get_bmc_v6address_dynamic_address(self, mocked_execute):
         mocked_execute.side_effect = [
             ('ipv6\n', ''),
             (hws.IPMITOOL_LAN6_PRINT_DYNAMIC_ADDR, '')
@@ -2379,9 +2363,8 @@ class TestGenericHardwareManager(base.IronicAgentTest):
         self.assertEqual('2001:1234:1234:1234:1234:1234:1234:1234',
                          self.hardware.get_bmc_v6address())
 
-    @mock.patch.object(il_utils, 'try_execute', autospec=True)
     @mock.patch.object(utils, 'execute', autospec=True)
-    def test_get_bmc_v6address_static_address_both(self, mocked_execute, mte):
+    def test_get_bmc_v6address_static_address_both(self, mocked_execute):
         dynamic_disabled = \
             hws.IPMITOOL_LAN6_PRINT_DYNAMIC_ADDR.replace('active', 'disabled')
         mocked_execute.side_effect = [
@@ -2392,15 +2375,13 @@ class TestGenericHardwareManager(base.IronicAgentTest):
         self.assertEqual('2001:5678:5678:5678:5678:5678:5678:5678',
                          self.hardware.get_bmc_v6address())
 
-    @mock.patch.object(il_utils, 'try_execute', autospec=True)
     @mock.patch.object(utils, 'execute', autospec=True)
-    def test_get_bmc_v6address_virt(self, mocked_execute, mte):
+    def test_get_bmc_v6address_virt(self, mocked_execute):
         mocked_execute.side_effect = processutils.ProcessExecutionError()
         self.assertIsNone(self.hardware.get_bmc_v6address())
 
-    @mock.patch.object(il_utils, 'try_execute', autospec=True)
     @mock.patch.object(utils, 'execute', autospec=True)
-    def test_get_bmc_v6address_invalid_enables(self, mocked_execute, mte):
+    def test_get_bmc_v6address_invalid_enables(self, mocked_execute):
         def side_effect(*args, **kwargs):
             if args[0].startswith('ipmitool lan6 print'):
                 return '', 'Failed to get IPv6/IPv4 Addressing Enables'
@@ -2408,9 +2389,8 @@ class TestGenericHardwareManager(base.IronicAgentTest):
         mocked_execute.side_effect = side_effect
         self.assertEqual('::/0', self.hardware.get_bmc_v6address())
 
-    @mock.patch.object(il_utils, 'try_execute', autospec=True)
     @mock.patch.object(utils, 'execute', autospec=True)
-    def test_get_bmc_v6address_invalid_get_address(self, mocked_execute, mte):
+    def test_get_bmc_v6address_invalid_get_address(self, mocked_execute):
         def side_effect(*args, **kwargs):
             if args[0].startswith('ipmitool lan6 print'):
                 if args[0].endswith('dynamic_addr') \
@@ -2422,10 +2402,9 @@ class TestGenericHardwareManager(base.IronicAgentTest):
         self.assertEqual('::/0', self.hardware.get_bmc_v6address())
 
     @mock.patch.object(hardware, 'LOG', autospec=True)
-    @mock.patch.object(il_utils, 'try_execute', autospec=True)
     @mock.patch.object(utils, 'execute', autospec=True)
     def test_get_bmc_v6address_ipmitool_invalid_stdout_format(
-            self, mocked_execute, mte, mocked_log):
+            self, mocked_execute, mocked_log):
         def side_effect(*args, **kwargs):
             if args[0].startswith('ipmitool lan6 print'):
                 if args[0].endswith('dynamic_addr') \
@@ -2439,9 +2418,8 @@ class TestGenericHardwareManager(base.IronicAgentTest):
                              'command: %(e)s', mock.ANY)
         mocked_log.warning.assert_has_calls([one_call] * 14)
 
-    @mock.patch.object(il_utils, 'try_execute', autospec=True)
     @mock.patch.object(utils, 'execute', autospec=True)
-    def test_get_bmc_v6address_channel_7(self, mocked_execute, mte):
+    def test_get_bmc_v6address_channel_7(self, mocked_execute):
         def side_effect(*args, **kwargs):
             if not args[0].startswith('ipmitool lan6 print 7'):
                 # ipv6 is not enabled for channels 1-6
@@ -4072,6 +4050,7 @@ class TestGenericHardwareManager(base.IronicAgentTest):
                           self.hardware._nvme_erase, block_device)
 
 
+@mock.patch.object(hardware, '_load_ipmi_modules', autospec=True)
 @mock.patch.object(hardware.GenericHardwareManager,
                    'get_os_install_device', autospec=True)
 @mock.patch.object(hardware, '_md_scan_and_assemble', autospec=True)
@@ -4084,7 +4063,8 @@ class TestEvaluateHardwareSupport(base.IronicAgentTest):
 
     def test_evaluate_hw_waits_for_disks(
             self, mocked_sleep, mocked_check_for_iscsi,
-            mocked_md_assemble, mocked_get_inst_dev):
+            mocked_md_assemble, mocked_get_inst_dev,
+            mocked_load_ipmi_modules):
         mocked_get_inst_dev.side_effect = [
             errors.DeviceNotFound('boom'),
             None
@@ -4092,6 +4072,7 @@ class TestEvaluateHardwareSupport(base.IronicAgentTest):
 
         result = self.hardware.evaluate_hardware_support()
 
+        self.assertTrue(mocked_load_ipmi_modules.called)
         self.assertTrue(mocked_check_for_iscsi.called)
         self.assertTrue(mocked_md_assemble.called)
         self.assertEqual(hardware.HardwareSupport.GENERIC, result)
@@ -4102,7 +4083,8 @@ class TestEvaluateHardwareSupport(base.IronicAgentTest):
     @mock.patch.object(hardware, 'LOG', autospec=True)
     def test_evaluate_hw_no_wait_for_disks(
             self, mocked_log, mocked_sleep, mocked_check_for_iscsi,
-            mocked_md_assemble, mocked_get_inst_dev):
+            mocked_md_assemble, mocked_get_inst_dev,
+            mocked_load_ipmi_modules):
         CONF.set_override('disk_wait_attempts', '0')
 
         result = self.hardware.evaluate_hardware_support()
@@ -4116,7 +4098,8 @@ class TestEvaluateHardwareSupport(base.IronicAgentTest):
     @mock.patch.object(hardware, 'LOG', autospec=True)
     def test_evaluate_hw_waits_for_disks_nonconfigured(
             self, mocked_log, mocked_sleep, mocked_check_for_iscsi,
-            mocked_md_assemble, mocked_get_inst_dev):
+            mocked_md_assemble, mocked_get_inst_dev,
+            mocked_load_ipmi_modules):
         mocked_get_inst_dev.side_effect = [
             errors.DeviceNotFound('boom'),
             errors.DeviceNotFound('boom'),
@@ -4147,7 +4130,8 @@ class TestEvaluateHardwareSupport(base.IronicAgentTest):
                                                     mocked_sleep,
                                                     mocked_check_for_iscsi,
                                                     mocked_md_assemble,
-                                                    mocked_get_inst_dev):
+                                                    mocked_get_inst_dev,
+                                                    mocked_load_ipmi_modules):
         CONF.set_override('disk_wait_attempts', '1')
 
         mocked_get_inst_dev.side_effect = [
@@ -4167,7 +4151,8 @@ class TestEvaluateHardwareSupport(base.IronicAgentTest):
     def test_evaluate_hw_disks_timeout_unconfigured(self, mocked_sleep,
                                                     mocked_check_for_iscsi,
                                                     mocked_md_assemble,
-                                                    mocked_get_inst_dev):
+                                                    mocked_get_inst_dev,
+                                                    mocked_load_ipmi_modules):
         mocked_get_inst_dev.side_effect = errors.DeviceNotFound('boom')
         self.hardware.evaluate_hardware_support()
         mocked_sleep.assert_called_with(3)
@@ -4175,7 +4160,8 @@ class TestEvaluateHardwareSupport(base.IronicAgentTest):
     def test_evaluate_hw_disks_timeout_configured(self, mocked_sleep,
                                                   mocked_check_for_iscsi,
                                                   mocked_md_assemble,
-                                                  mocked_root_dev):
+                                                  mocked_root_dev,
+                                                  mocked_load_ipmi_modules):
         CONF.set_override('disk_wait_delay', '5')
         mocked_root_dev.side_effect = errors.DeviceNotFound('boom')
 
@@ -4184,7 +4170,8 @@ class TestEvaluateHardwareSupport(base.IronicAgentTest):
 
     def test_evaluate_hw_disks_timeout(
             self, mocked_sleep, mocked_check_for_iscsi,
-            mocked_md_assemble, mocked_get_inst_dev):
+            mocked_md_assemble, mocked_get_inst_dev,
+            mocked_load_ipmi_modules):
         mocked_get_inst_dev.side_effect = errors.DeviceNotFound('boom')
         result = self.hardware.evaluate_hardware_support()
         self.assertEqual(hardware.HardwareSupport.GENERIC, result)
@@ -4274,6 +4261,14 @@ class TestModuleFunctions(base.IronicAgentTest):
         mocked_execute.assert_has_calls([
             mock.call('iscsistart', '-f')])
 
+    @mock.patch.object(il_utils, 'try_execute', autospec=True)
+    def test__load_ipmi_modules(self, mocked_try_execute, me):
+        hardware._load_ipmi_modules()
+        mocked_try_execute.assert_has_calls([
+            mock.call('modprobe', 'ipmi_msghandler'),
+            mock.call('modprobe', 'ipmi_devintf'),
+            mock.call('modprobe', 'ipmi_si')])
+
 
 def create_hdparm_info(supported=False, enabled=False, locked=False,
                        frozen=False, enhanced_erase=False):