Fix DRAC classic driver double manage/provide

This change fixes an issue that caused a node using a Dell EMC
integrated Dell Remote Access Controller (iDRAC) classic driver,
'pxe_drac' or 'pxe_drac_inspector', to be placed in the 'clean failed'
state after a double manage/provide cycle, instead of the 'available'
state.

The deploy interface implementation used by iDRAC classic drivers has
been class ironic.drivers.modules.drac.deploy.DracDeploy, which is
derived from class ironic.drivers.modules.iscsi_deploy.ISCSIDeploy. The
only difference between them is that DracDeploy overrides the
prepare_cleaning() method to prevent the booting of the Ironic Python
Agent (IPA) ramdisk when only out-of-band RAID clean steps are
requested. However, it caused the issue and did not have its intended
effect, because Ironic Conductor boots the ramdisk regardless. The
Ironic Conductor should be modified to preclude the booting of the IPA
ramdisk fix, rather than leaving it to individual drivers.

The iDRAC classic drivers' deploy interface implementation has been
changed to ISCSIDeploy. Since class DracDeploy is no longer needed, its
source code and automated tests have been removed.

Change-Id: Ib2c9b7f9f780aaf5f6345825b1f6c9ddb4f9c41f
Closes-Bug: #1676387
Related-Bug: #1572529
Related-Bug: #1705741

ironic/drivers/drac.py

@@ -20,13 +20,13 @@ from oslo_utils import importutils
 from ironic.common import exception
 from ironic.common.i18n import _
 from ironic.drivers import base
-from ironic.drivers.modules.drac import deploy
 from ironic.drivers.modules.drac import inspect as drac_inspect
 from ironic.drivers.modules.drac import management
 from ironic.drivers.modules.drac import power
 from ironic.drivers.modules.drac import raid
 from ironic.drivers.modules.drac import vendor_passthru
 from ironic.drivers.modules import inspector
+from ironic.drivers.modules import iscsi_deploy
 from ironic.drivers.modules import pxe
 
 
@@ -41,7 +41,7 @@ class PXEDracDriver(base.BaseDriver):
 
         self.power = power.DracPower()
         self.boot = pxe.PXEBoot()
-        self.deploy = deploy.DracDeploy()
+        self.deploy = iscsi_deploy.ISCSIDeploy()
         self.management = management.DracManagement()
         self.raid = raid.DracRAID()
         self.vendor = vendor_passthru.DracVendorPassthru()

ironic/drivers/fake.py

@@ -25,7 +25,6 @@ from ironic.drivers import base
 from ironic.drivers.modules import agent
 from ironic.drivers.modules.cimc import management as cimc_mgmt
 from ironic.drivers.modules.cimc import power as cimc_power
-from ironic.drivers.modules.drac import deploy as drac_deploy
 from ironic.drivers.modules.drac import inspect as drac_inspect
 from ironic.drivers.modules.drac import management as drac_mgmt
 from ironic.drivers.modules.drac import power as drac_power
@@ -145,7 +144,7 @@ class FakeDracDriver(base.BaseDriver):
                 reason=_('Unable to import python-dracclient library'))
 
         self.power = drac_power.DracPower()
-        self.deploy = drac_deploy.DracDeploy()
+        self.deploy = iscsi_deploy.ISCSIDeploy()
         self.management = drac_mgmt.DracManagement()
         self.raid = drac_raid.DracRAID()
         self.vendor = drac_vendor.DracVendorPassthru()

ironic/drivers/modules/drac/deploy.py

@@ -1,54 +0,0 @@
-#
-# Licensed under the Apache License, Version 2.0 (the "License"); you may
-# not use this file except in compliance with the License. You may obtain
-# a copy of the License at
-#
-#      http://www.apache.org/licenses/LICENSE-2.0
-#
-# Unless required by applicable law or agreed to in writing, software
-# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
-# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
-# License for the specific language governing permissions and limitations
-# under the License.
-
-"""
-DRAC deploy interface
-"""
-
-from ironic_lib import metrics_utils
-
-from ironic.drivers.modules import deploy_utils
-from ironic.drivers.modules import iscsi_deploy
-
-_OOB_CLEAN_STEPS = [
-    {'interface': 'raid', 'step': 'create_configuration'},
-    {'interface': 'raid', 'step': 'delete_configuration'}
-]
-
-METRICS = metrics_utils.get_metrics_logger(__name__)
-
-
-class DracDeploy(iscsi_deploy.ISCSIDeploy):
-
-    @METRICS.timer('DracDeploy.prepare_cleaning')
-    def prepare_cleaning(self, task):
-        """Prepare environment for cleaning
-
-        Boot into the agent to prepare for cleaning if in-band cleaning step
-        is requested.
-
-        :param task: a TaskManager instance containing the node to act on.
-        :returns: states.CLEANWAIT if there is any in-band clean step to
-                  signify an asynchronous prepare.
-        """
-        node = task.node
-
-        inband_steps = [step for step
-                        in node.driver_internal_info.get('clean_steps', [])
-                        if {'interface': step['interface'],
-                            'step': step['step']} not in _OOB_CLEAN_STEPS]
-
-        if ('agent_cached_clean_steps' not in node.driver_internal_info or
-            inband_steps):
-                return deploy_utils.prepare_inband_cleaning(task,
-                                                            manage_boot=True)

ironic/tests/unit/drivers/modules/drac/test_deploy.py

@@ -1,101 +0,0 @@
-#
-# Licensed under the Apache License, Version 2.0 (the "License"); you may
-# not use this file except in compliance with the License. You may obtain
-# a copy of the License at
-#
-#      http://www.apache.org/licenses/LICENSE-2.0
-#
-# Unless required by applicable law or agreed to in writing, software
-# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
-# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
-# License for the specific language governing permissions and limitations
-# under the License.
-
-"""
-Test class for DRAC deploy interface
-"""
-
-import mock
-
-from ironic.common import states
-from ironic.conductor import task_manager
-from ironic.drivers.modules import deploy_utils
-from ironic.tests.unit.conductor import mgr_utils
-from ironic.tests.unit.db import base as db_base
-from ironic.tests.unit.db import utils as db_utils
-from ironic.tests.unit.objects import utils as obj_utils
-
-INFO_DICT = db_utils.get_test_drac_info()
-
-
-class DracDeployTestCase(db_base.DbTestCase):
-
-    def setUp(self):
-        super(DracDeployTestCase, self).setUp()
-        mgr_utils.mock_the_extension_manager(driver='fake_drac')
-        self.node = obj_utils.create_test_node(self.context,
-                                               driver='fake_drac',
-                                               driver_info=INFO_DICT)
-
-    @mock.patch.object(deploy_utils, 'prepare_inband_cleaning', spec_set=True,
-                       autospec=True)
-    def test_prepare_cleaning_with_no_clean_step(
-            self, mock_prepare_inband_cleaning):
-        mock_prepare_inband_cleaning.return_value = states.CLEANWAIT
-
-        with task_manager.acquire(self.context, self.node.uuid,
-                                  shared=False) as task:
-            res = task.driver.deploy.prepare_cleaning(task)
-            self.assertEqual(states.CLEANWAIT, res)
-
-        mock_prepare_inband_cleaning.assert_called_once_with(
-            task, manage_boot=True)
-
-    @mock.patch.object(deploy_utils, 'prepare_inband_cleaning', spec_set=True,
-                       autospec=True)
-    def test_prepare_cleaning_with_inband_clean_step(
-            self, mock_prepare_inband_cleaning):
-        self.node.driver_internal_info['clean_steps'] = [
-            {'step': 'erase_disks', 'priority': 20, 'interface': 'deploy'}]
-        mock_prepare_inband_cleaning.return_value = states.CLEANWAIT
-
-        with task_manager.acquire(self.context, self.node.uuid,
-                                  shared=False) as task:
-            res = task.driver.deploy.prepare_cleaning(task)
-            self.assertEqual(states.CLEANWAIT, res)
-
-        mock_prepare_inband_cleaning.assert_called_once_with(
-            task, manage_boot=True)
-
-    @mock.patch.object(deploy_utils, 'prepare_inband_cleaning', spec_set=True,
-                       autospec=True)
-    def test_prepare_cleaning_with_oob_clean_step_with_no_agent_cached_steps(
-            self, mock_prepare_inband_cleaning):
-        self.node.driver_internal_info['clean_steps'] = [
-            {'interface': 'raid', 'step': 'create_configuration'}]
-        mock_prepare_inband_cleaning.return_value = states.CLEANWAIT
-
-        with task_manager.acquire(self.context, self.node.uuid,
-                                  shared=False) as task:
-            res = task.driver.deploy.prepare_cleaning(task)
-            self.assertEqual(states.CLEANWAIT, res)
-
-        mock_prepare_inband_cleaning.assert_called_once_with(
-            task, manage_boot=True)
-
-    @mock.patch.object(deploy_utils, 'prepare_inband_cleaning', spec_set=True,
-                       autospec=True)
-    def test_prepare_cleaning_with_oob_clean_step_with_agent_cached_steps(
-            self, mock_prepare_inband_cleaning):
-        self.node.driver_internal_info['agent_cached_clean_steps'] = []
-        self.node.driver_internal_info['clean_steps'] = [
-            {'interface': 'raid', 'step': 'create_configuration'}]
-        mock_prepare_inband_cleaning.return_value = states.CLEANWAIT
-
-        with task_manager.acquire(self.context, self.node.uuid,
-                                  shared=False) as task:
-            res = task.driver.deploy.prepare_cleaning(task)
-            self.assertEqual(states.CLEANWAIT, res)
-
-        mock_prepare_inband_cleaning.assert_called_once_with(
-            task, manage_boot=True)

ironic/tests/unit/drivers/test_drac.py

@@ -0,0 +1,108 @@
+# Copyright (c) 2017 Dell Inc. or its subsidiaries.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import inspect
+
+import mock
+
+from oslo_utils import importutils
+
+from ironic.common import exception
+from ironic.drivers import drac as drac_drivers
+from ironic.drivers.modules import drac
+from ironic.drivers.modules import inspector
+from ironic.drivers.modules import iscsi_deploy
+from ironic.drivers.modules.network import flat as flat_net
+from ironic.drivers.modules import pxe
+from ironic.drivers.modules.storage import noop as noop_storage
+from ironic.tests.unit.db import base as db_base
+
+
+class BaseIDRACTestCase(db_base.DbTestCase):
+
+    def setUp(self):
+        super(BaseIDRACTestCase, self).setUp()
+
+    def _validate_interfaces(self, driver, **kwargs):
+        self.assertIsInstance(
+            driver.boot,
+            kwargs.get('boot', pxe.PXEBoot))
+        self.assertIsInstance(
+            driver.deploy,
+            kwargs.get('deploy', iscsi_deploy.ISCSIDeploy))
+        self.assertIsInstance(
+            driver.management,
+            kwargs.get('management', drac.management.DracManagement))
+        self.assertIsInstance(
+            driver.power,
+            kwargs.get('power', drac.power.DracPower))
+
+        # Console interface of iDRAC classic drivers is None.
+        console_interface = kwargs.get('console', None)
+
+        # None is not a class or type.
+        if inspect.isclass(console_interface):
+            self.assertIsInstance(driver.console, console_interface)
+        else:
+            self.assertIs(driver.console, console_interface)
+
+        self.assertIsInstance(
+            driver.inspect,
+            kwargs.get('inspect', drac.inspect.DracInspect))
+
+        # iDRAC classic drivers do not have a network interface.
+        if 'network' in driver.all_interfaces:
+            self.assertIsInstance(
+                driver.network,
+                kwargs.get('network', flat_net.FlatNetwork))
+
+        self.assertIsInstance(
+            driver.raid,
+            kwargs.get('raid', drac.raid.DracRAID))
+
+        # iDRAC classic drivers do not have a storage interface.
+        if 'storage' in driver.all_interfaces:
+            self.assertIsInstance(
+                driver.storage,
+                kwargs.get('storage', noop_storage.NoopStorage))
+
+        self.assertIsInstance(
+            driver.vendor,
+            kwargs.get('vendor', drac.vendor_passthru.DracVendorPassthru))
+
+
+@mock.patch.object(importutils, 'try_import', spec_set=True, autospec=True)
+class DracClassicDriversTestCase(BaseIDRACTestCase):
+
+    def setUp(self):
+        super(DracClassicDriversTestCase, self).setUp()
+
+    def test_pxe_drac_driver(self, mock_try_import):
+        mock_try_import.return_value = True
+
+        driver = drac_drivers.PXEDracDriver()
+        self._validate_interfaces(driver)
+
+    def test___init___try_import_dracclient_failure(self, mock_try_import):
+        mock_try_import.return_value = False
+
+        self.assertRaises(exception.DriverLoadError,
+                          drac_drivers.PXEDracDriver)
+
+    def test_pxe_drac_inspector_driver(self, mock_try_import):
+        self.config(enabled=True, group='inspector')
+        mock_try_import.return_value = True
+
+        driver = drac_drivers.PXEDracInspectorDriver()
+        self._validate_interfaces(driver, inspect=inspector.Inspector)

releasenotes/notes/drac-fix-double-manage-provide-cycle-6ac8a427068f87fe.yaml

@@ -0,0 +1,8 @@
+---
+fixes:
+  - Fixes an issue that caused a node using a Dell EMC integrated Dell Remote
+    Access Controller (iDRAC) *classic driver*, ``pxe_drac`` or
+    ``pxe_drac_inspector``, to be placed in the ``clean failed`` state after a
+    double ``manage``/``provide`` cycle, instead of the ``available`` state.
+    For more information, see `bug 1676387
+    <https://bugs.launchpad.net/ironic/+bug/1676387>`_.