From 86e3a100a3a6fcfcf9d498886e1d7784f07ecd73 Mon Sep 17 00:00:00 2001 From: Richard Pioso Date: Sun, 13 Aug 2017 21:10:06 -0400 Subject: [PATCH] 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 | 4 +- ironic/drivers/fake.py | 3 +- ironic/drivers/modules/drac/deploy.py | 54 --------- .../unit/drivers/modules/drac/test_deploy.py | 101 ---------------- ironic/tests/unit/drivers/test_drac.py | 108 ++++++++++++++++++ ...manage-provide-cycle-6ac8a427068f87fe.yaml | 8 ++ 6 files changed, 119 insertions(+), 159 deletions(-) delete mode 100644 ironic/drivers/modules/drac/deploy.py delete mode 100644 ironic/tests/unit/drivers/modules/drac/test_deploy.py create mode 100644 ironic/tests/unit/drivers/test_drac.py create mode 100644 releasenotes/notes/drac-fix-double-manage-provide-cycle-6ac8a427068f87fe.yaml diff --git a/ironic/drivers/drac.py b/ironic/drivers/drac.py index 811a5da74f..ea0abb0654 100644 --- a/ironic/drivers/drac.py +++ b/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() diff --git a/ironic/drivers/fake.py b/ironic/drivers/fake.py index d43f83def9..28ddf1f7f6 100644 --- a/ironic/drivers/fake.py +++ b/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() diff --git a/ironic/drivers/modules/drac/deploy.py b/ironic/drivers/modules/drac/deploy.py deleted file mode 100644 index 722fa944e4..0000000000 --- a/ironic/drivers/modules/drac/deploy.py +++ /dev/null @@ -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) diff --git a/ironic/tests/unit/drivers/modules/drac/test_deploy.py b/ironic/tests/unit/drivers/modules/drac/test_deploy.py deleted file mode 100644 index 03bec356e8..0000000000 --- a/ironic/tests/unit/drivers/modules/drac/test_deploy.py +++ /dev/null @@ -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) diff --git a/ironic/tests/unit/drivers/test_drac.py b/ironic/tests/unit/drivers/test_drac.py new file mode 100644 index 0000000000..4d37947be4 --- /dev/null +++ b/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) diff --git a/releasenotes/notes/drac-fix-double-manage-provide-cycle-6ac8a427068f87fe.yaml b/releasenotes/notes/drac-fix-double-manage-provide-cycle-6ac8a427068f87fe.yaml new file mode 100644 index 0000000000..20d13f2db4 --- /dev/null +++ b/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 + `_.