Clean up deprecated items in the inspection code

* Remove support for setting IPMI credentials (removed from inspector in Pike)
* Stop sending the ipmi_address field (bmc_address is used instead since Pike)

Change-Id: I1696041db62ba27e5d31e8481cb225a43d7e2a46
Closes-Bug: #1654318

ironic_python_agent/inspector.py

@@ -98,9 +98,6 @@ def inspect():
         LOG.info('stopping inspection, as inspector returned an error')
         return
 
-    # Optionally update IPMI credentials
-    setup_ipmi_credentials(resp)
-
     LOG.info('inspection finished successfully')
     return resp.get('uuid')
 
@@ -127,36 +124,6 @@ def call_inspector(data, failures):
     return resp.json()
 
 
-def setup_ipmi_credentials(resp):
-    """Setup IPMI credentials, if requested.
-
-    :param resp: JSON response from inspector.
-    """
-    if not resp.get('ipmi_setup_credentials'):
-        LOG.info('setting IPMI credentials was not requested')
-        return
-
-    user, password = resp['ipmi_username'], resp['ipmi_password']
-    LOG.debug('setting IPMI credentials: user %s', user)
-
-    commands = [
-        ('user', 'set', 'name', '2', user),
-        ('user', 'set', 'password', '2', password),
-        ('user', 'enable', '2'),
-        ('channel', 'setaccess', '1', '2',
-         'link=on', 'ipmi=on', 'callin=on', 'privilege=4'),
-    ]
-
-    for cmd in commands:
-        try:
-            utils.execute('ipmitool', *cmd)
-        except processutils.ProcessExecutionError:
-            LOG.exception('failed to update IPMI credentials')
-            raise errors.InspectionError('failed to update IPMI credentials')
-
-    LOG.info('successfully set IPMI credentials: user %s', user)
-
-
 def _normalize_mac(mac):
     """Convert MAC to a well-known format aa:bb:cc:dd:ee:ff."""
     if '-' in mac:
@@ -237,12 +204,11 @@ def collect_default(data, failures):
     else:
         data['root_disk'] = root_disk
         LOG.debug('default root device is %s', root_disk.name)
-    # Both boot interface and IPMI address might not be present,
+    # The boot interface might not be present, we don't count it as failure.
-    # we don't count it as failure
+    # TODO(dtantsur): stop using the boot_interface field.
     data['boot_interface'] = inventory['boot'].pxe_interface
     LOG.debug('boot devices was %s', data['boot_interface'])
-    data['ipmi_address'] = inventory.get('bmc_address')
+    LOG.debug('BMC IP address: %s', inventory.get('bmc_address'))
-    LOG.debug('BMC IP address: %s', data['ipmi_address'])
 
 
 def collect_logs(data, failures):

ironic_python_agent/tests/unit/test_inspector.py

@@ -59,7 +59,6 @@ class TestMisc(base.IronicAgentTest):
                                ['foobar'])
 
 
-@mock.patch.object(inspector, 'setup_ipmi_credentials', autospec=True)
 @mock.patch.object(inspector, 'call_inspector', new_callable=AcceptingFailure)
 @mock.patch.object(stevedore, 'NamedExtensionManager', autospec=True)
 class TestInspect(base.IronicAgentTest):
@@ -71,7 +70,7 @@ class TestInspect(base.IronicAgentTest):
         self.mock_ext = mock.Mock(spec=['plugin', 'name'],
                                   plugin=self.mock_collect)
 
-    def test_ok(self, mock_ext_mgr, mock_call, mock_setup_ipmi):
+    def test_ok(self, mock_ext_mgr, mock_call):
         mock_ext_mgr.return_value = [self.mock_ext]
         mock_call.return_value = {'uuid': 'uuid1'}
 
@@ -80,9 +79,8 @@ class TestInspect(base.IronicAgentTest):
         self.mock_collect.assert_called_with_failure()
         mock_call.assert_called_with_failure()
         self.assertEqual('uuid1', result)
-        mock_setup_ipmi.assert_called_once_with(mock_call.return_value)
 
-    def test_collectors_option(self, mock_ext_mgr, mock_call, mock_setup_ipmi):
+    def test_collectors_option(self, mock_ext_mgr, mock_call):
         CONF.set_override('inspection_collectors', 'foo,bar')
         mock_ext_mgr.return_value = [
             mock.Mock(spec=['name', 'plugin'], plugin=AcceptingFailure()),
@@ -95,7 +93,7 @@ class TestInspect(base.IronicAgentTest):
             fake_ext.plugin.assert_called_with_failure()
         mock_call.assert_called_with_failure()
 
-    def test_collector_failed(self, mock_ext_mgr, mock_call, mock_setup_ipmi):
+    def test_collector_failed(self, mock_ext_mgr, mock_call):
         mock_ext_mgr.return_value = [self.mock_ext]
         self.mock_collect.side_effect = RuntimeError('boom')
 
@@ -104,18 +102,16 @@ class TestInspect(base.IronicAgentTest):
 
         self.mock_collect.assert_called_with_failure()
         mock_call.assert_called_with_failure(expect_error=True)
-        self.assertFalse(mock_setup_ipmi.called)
 
-    def test_extensions_failed(self, mock_ext_mgr, mock_call, mock_setup_ipmi):
+    def test_extensions_failed(self, mock_ext_mgr, mock_call):
         CONF.set_override('inspection_collectors', 'foo,bar')
         mock_ext_mgr.side_effect = RuntimeError('boom')
 
         self.assertRaisesRegex(RuntimeError, 'boom', inspector.inspect)
 
         mock_call.assert_called_with_failure(expect_error=True)
-        self.assertFalse(mock_setup_ipmi.called)
 
-    def test_inspector_error(self, mock_ext_mgr, mock_call, mock_setup_ipmi):
+    def test_inspector_error(self, mock_ext_mgr, mock_call):
         mock_call.return_value = None
         mock_ext_mgr.return_value = [self.mock_ext]
 
@@ -124,7 +120,6 @@ class TestInspect(base.IronicAgentTest):
         self.mock_collect.assert_called_with_failure()
         mock_call.assert_called_with_failure()
         self.assertIsNone(result)
-        self.assertFalse(mock_setup_ipmi.called)
 
 
 @mock.patch.object(requests, 'post', autospec=True)
@@ -171,58 +166,6 @@ class TestCallInspector(base.IronicAgentTest):
         self.assertIsNone(res)
 
 
-@mock.patch.object(utils, 'execute', autospec=True)
-class TestSetupIpmiCredentials(base.IronicAgentTest):
-    def setUp(self):
-        super(TestSetupIpmiCredentials, self).setUp()
-        self.resp = {'ipmi_username': 'user',
-                     'ipmi_password': 'pwd',
-                     'ipmi_setup_credentials': True}
-
-    def test_disabled(self, mock_call):
-        del self.resp['ipmi_setup_credentials']
-
-        inspector.setup_ipmi_credentials(self.resp)
-
-        self.assertFalse(mock_call.called)
-
-    def test_ok(self, mock_call):
-        inspector.setup_ipmi_credentials(self.resp)
-
-        expected = [
-            mock.call('ipmitool', 'user', 'set', 'name', '2', 'user'),
-            mock.call('ipmitool', 'user', 'set', 'password', '2', 'pwd'),
-            mock.call('ipmitool', 'user', 'enable', '2'),
-            mock.call('ipmitool', 'channel', 'setaccess', '1', '2',
-                      'link=on', 'ipmi=on', 'callin=on', 'privilege=4'),
-        ]
-        self.assertEqual(expected, mock_call.call_args_list)
-
-    def test_user_failed(self, mock_call):
-        mock_call.side_effect = processutils.ProcessExecutionError()
-
-        self.assertRaises(errors.InspectionError,
-                          inspector.setup_ipmi_credentials,
-                          self.resp)
-
-        mock_call.assert_called_once_with('ipmitool', 'user', 'set', 'name',
-                                          '2', 'user')
-
-    def test_password_failed(self, mock_call):
-        mock_call.side_effect = iter((None,
-                                      processutils.ProcessExecutionError))
-
-        self.assertRaises(errors.InspectionError,
-                          inspector.setup_ipmi_credentials,
-                          self.resp)
-
-        expected = [
-            mock.call('ipmitool', 'user', 'set', 'name', '2', 'user'),
-            mock.call('ipmitool', 'user', 'set', 'password', '2', 'pwd')
-        ]
-        self.assertEqual(expected, mock_call.call_args_list)
-
-
 class BaseDiscoverTest(base.IronicAgentTest):
     def setUp(self):
         super(BaseDiscoverTest, self).setUp()
@@ -269,10 +212,9 @@ class TestCollectDefault(BaseDiscoverTest):
 
         inspector.collect_default(self.data, self.failures)
 
-        for key in ('memory', 'interfaces', 'cpu', 'disks'):
+        for key in ('memory', 'interfaces', 'cpu', 'disks', 'bmc_address'):
             self.assertTrue(self.data['inventory'][key])
 
-        self.assertEqual('1.2.3.4', self.data['ipmi_address'])
         self.assertEqual('boot:if', self.data['boot_interface'])
         self.assertEqual(self.inventory['disks'][0].name,
                          self.data['root_disk'].name)
@@ -286,10 +228,9 @@ class TestCollectDefault(BaseDiscoverTest):
 
         inspector.collect_default(self.data, self.failures)
 
-        for key in ('memory', 'interfaces', 'cpu'):
+        for key in ('memory', 'interfaces', 'cpu', 'bmc_address'):
             self.assertTrue(self.data['inventory'][key])
 
-        self.assertEqual('1.2.3.4', self.data['ipmi_address'])
         self.assertEqual('boot:if', self.data['boot_interface'])
         self.assertNotIn('root_disk', self.data)
 

releasenotes/notes/ipmi-cleanup-a4454f6851d81c4d.yaml

@@ -0,0 +1,8 @@
+---
+upgrade:
+  - |
+    Support for setting IPMI credentials during inspection was removed. This
+    feature is not supported by ironic-inspector since the Pike release.
+  - |
+    The ``ipmi_address`` field is no longer sent as part of the inspection
+    process. The ``inventory[bmc_address]`` field should be used instead.