From 53fc7fb2e587f9019e7325a571884990d754a534 Mon Sep 17 00:00:00 2001 From: Ilya Etingof Date: Wed, 21 Nov 2018 11:01:41 +0100 Subject: [PATCH] Add Redfish inspect interface follow up This commit addresses minor issues found in patch [1]. In particular, the `drivers/modules/inspect_utils.py` module has been introduced. 1. Change-Id: Ie3167569db060620fe0b9784bc7d7a854f2ef754 Change-Id: I8499793e51fce73a94f438b8fbab20e9062df56d --- doc/source/admin/drivers/redfish.rst | 13 ++-- ironic/drivers/modules/deploy_utils.py | 28 -------- ironic/drivers/modules/ilo/inspect.py | 4 +- ironic/drivers/modules/inspect_utils.py | 51 +++++++++++++++ ironic/drivers/modules/redfish/inspect.py | 52 ++++++++------- .../unit/drivers/modules/ilo/test_inspect.py | 12 ++-- .../drivers/modules/redfish/test_inspect.py | 12 ++-- .../unit/drivers/modules/test_deploy_utils.py | 33 ---------- .../drivers/modules/test_inspect_utils.py | 65 +++++++++++++++++++ 9 files changed, 164 insertions(+), 106 deletions(-) create mode 100644 ironic/drivers/modules/inspect_utils.py create mode 100644 ironic/tests/unit/drivers/modules/test_inspect_utils.py diff --git a/doc/source/admin/drivers/redfish.rst b/doc/source/admin/drivers/redfish.rst index bcfb683214..7e1eba5dfe 100644 --- a/doc/source/admin/drivers/redfish.rst +++ b/doc/source/admin/drivers/redfish.rst @@ -110,16 +110,15 @@ Out-Of-Band inspection ^^^^^^^^^^^^^^^^^^^^^^ The ``redfish`` hardware type can inspect the bare metal node by querying -Redfish BMC. This process if quick and reliable compared to the way -how the ``inspector`` hardware type works e.g. booting bare metal node into -the introspection ramdisk. +Redfish compatible BMC. This process is quick and reliable compared to the +way the ``inspector`` hardware type works i.e. booting bare metal node +into the introspection ramdisk. .. note:: - The ``redfish`` inspect interface largely relies on the optional parts - of the Redfish specification. Not all Redfish-compliant BMCs might serve - the required information, in which case bare metal node inspection would - fail. + The ``redfish`` inspect interface relies on the optional parts of the + Redfish specification. Not all Redfish-compliant BMCs might serve the + required information, in which case bare metal node inspection will fail. .. _Redfish: http://redfish.dmtf.org/ .. _Sushy: https://git.openstack.org/cgit/openstack/sushy diff --git a/ironic/drivers/modules/deploy_utils.py b/ironic/drivers/modules/deploy_utils.py index 21cb895eb1..ba7990117c 100644 --- a/ironic/drivers/modules/deploy_utils.py +++ b/ironic/drivers/modules/deploy_utils.py @@ -579,34 +579,6 @@ def get_single_nic_with_vif_port_id(task): return port.address -def create_ports_if_not_exist(task, macs): - """Create ironic ports for the mac addresses. - - Creates ironic ports for the mac addresses returned with inspection - or as requested by operator. - - :param task: a TaskManager instance. - :param macs: A dictionary of port numbers to mac addresses - returned by node inspection. - - """ - node = task.node - for port_num, mac in macs.items(): - port_dict = {'address': mac, 'node_id': node.id} - port = objects.Port(task.context, **port_dict) - - try: - port.create() - LOG.info(_("Port %(port_num)s created for MAC address %(address)s " - "for node %(node)s"), - {'address': mac, 'node': node.uuid, 'port_num': port_num}) - except exception.MACAlreadyExists: - LOG.warning(_("Port %(port_num)s already exists for MAC address" - "%(address)s for node %(node)s"), - {'address': mac, 'node': node.uuid, - 'port_num': port_num}) - - def agent_get_clean_steps(task, interface=None, override_priorities=None): """Get the list of cached clean steps from the agent. diff --git a/ironic/drivers/modules/ilo/inspect.py b/ironic/drivers/modules/ilo/inspect.py index 231a0d90b4..ee4e5c7ba5 100644 --- a/ironic/drivers/modules/ilo/inspect.py +++ b/ironic/drivers/modules/ilo/inspect.py @@ -22,8 +22,8 @@ from ironic.common import states from ironic.common import utils from ironic.conductor import utils as conductor_utils from ironic.drivers import base -from ironic.drivers.modules import deploy_utils from ironic.drivers.modules.ilo import common as ilo_common +from ironic.drivers.modules import inspect_utils ilo_error = importutils.try_import('proliantutils.exception') @@ -244,7 +244,7 @@ class IloInspect(base.InspectInterface): task.node.save() # Create ports for the nics detected. - deploy_utils.create_ports_if_not_exist(task, result['macs']) + inspect_utils.create_ports_if_not_exist(task, result['macs']) LOG.debug("Node properties for %(node)s are updated as " "%(properties)s", diff --git a/ironic/drivers/modules/inspect_utils.py b/ironic/drivers/modules/inspect_utils.py new file mode 100644 index 0000000000..8a7f6f5577 --- /dev/null +++ b/ironic/drivers/modules/inspect_utils.py @@ -0,0 +1,51 @@ +# Copyright 2018 Red Hat, Inc. +# All Rights Reserved. +# +# 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. + +from oslo_log import log as logging + +from ironic.common import exception +from ironic import objects + +LOG = logging.getLogger(__name__) + + +def create_ports_if_not_exist(task, macs): + """Create ironic ports for the mac addresses. + + Creates ironic ports for the mac addresses returned with inspection + or as requested by operator. + + :param task: A TaskManager instance. + :param macs: A dictionary of port numbers to mac addresses + returned by node inspection. + + """ + node = task.node + for port_num, mac in macs.items(): + # TODO(etingof): detect --pxe-enabled flag + port_dict = {'address': mac, 'node_id': node.id} + port = objects.Port(task.context, **port_dict) + + try: + port.create() + LOG.info("Port %(port_num)s created for MAC address %(address)s " + "for node %(node)s", {'address': mac, 'node': node.uuid, + 'port_num': port_num}) + except exception.MACAlreadyExists: + LOG.warning("Port %(port_num)s already exists for " + "MAC address %(address)s for node " + "%(node)s", {'address': mac, + 'node': node.uuid, + 'port_num': port_num}) diff --git a/ironic/drivers/modules/redfish/inspect.py b/ironic/drivers/modules/redfish/inspect.py index 8fc92a15e2..fb57a75642 100644 --- a/ironic/drivers/modules/redfish/inspect.py +++ b/ironic/drivers/modules/redfish/inspect.py @@ -21,7 +21,7 @@ from ironic.common import exception from ironic.common.i18n import _ from ironic.common import states from ironic.drivers import base -from ironic.drivers.modules import deploy_utils +from ironic.drivers.modules import inspect_utils from ironic.drivers.modules.redfish import utils as redfish_utils LOG = log.getLogger(__name__) @@ -108,10 +108,9 @@ class RedfishInspect(base.InspectInterface): inspected_properties['cpu_arch'] = CPU_ARCH_MAP[arch] except KeyError: - LOG.warning( - _("Unknown CPU arch %(arch)s discovered " - "for Node %(node)s"), {'node': task.node.uuid, - 'arch': arch}) + LOG.warning("Unknown CPU arch %(arch)s discovered " + "for node %(node)s", {'node': task.node.uuid, + 'arch': arch}) simple_storage_size = 0 @@ -125,10 +124,10 @@ class RedfishInspect(base.InspectInterface): simple_storage_size = simple_storage_size[0] - except sushy.SushyError: - LOG.info( - _("No simple storage information discovered " - "for Node %(node)s"), {'node': task.node.uuid}) + except sushy.SushyError as ex: + LOG.debug("No simple storage information discovered " + "for node %(node)s: %(err)s", {'node': task.node.uuid, + 'err': ex}) storage_size = 0 @@ -141,23 +140,31 @@ class RedfishInspect(base.InspectInterface): storage_size = storage_size[0] - except sushy.SushyError: - LOG.info(_("No storage volume information discovered " - "for Node %(node)s"), {'node': task.node.uuid}) + except sushy.SushyError as ex: + LOG.debug("No storage volume information discovered " + "for node %(node)s: %(err)s", {'node': task.node.uuid, + 'err': ex}) - local_gb = max(simple_storage_size, storage_size) + # NOTE(etingof): pick the smallest disk larger than 4G among available + if simple_storage_size and storage_size: + local_gb = min(simple_storage_size, storage_size) + + else: + local_gb = max(simple_storage_size, storage_size) # Note(deray): Convert the received size to GiB and reduce the # value by 1 GB as consumers like Ironic requires the ``local_gb`` # to be returned 1 less than actual size. local_gb = max(0, int(local_gb / units.Gi - 1)) + # TODO(etingof): should we respect root device hints here? + if local_gb: inspected_properties['local_gb'] = str(local_gb) else: - LOG.warning(_("Could not provide a valid storage size configured " - "for Node %(node)s"), {'node': task.node.uuid}) + LOG.warning("Could not provide a valid storage size configured " + "for node %(node)s", {'node': task.node.uuid}) valid_keys = self.ESSENTIAL_PROPERTIES missing_keys = valid_keys - set(inspected_properties) @@ -171,22 +178,19 @@ class RedfishInspect(base.InspectInterface): task.node.properties = inspected_properties task.node.save() - LOG.debug(_("Node properties for %(node)s are updated as " - "%(properties)s"), - {'properties': inspected_properties, - 'node': task.node.uuid}) + LOG.debug("Node properties for %(node)s are updated as " + "%(properties)s", {'properties': inspected_properties, + 'node': task.node.uuid}) if (system.ethernet_interfaces and system.ethernet_interfaces.eth_summary): macs = system.ethernet_interfaces.eth_summary # Create ports for the nics detected. - deploy_utils.create_ports_if_not_exist(task, macs) + inspect_utils.create_ports_if_not_exist(task, macs) else: - LOG.info(_("No NIC information discovered " - "for Node %(node)s"), {'node': task.node.uuid}) - - LOG.info(_("Node %(node)s inspected."), {'node': task.node.uuid}) + LOG.warning("No NIC information discovered " + "for node %(node)s", {'node': task.node.uuid}) return states.MANAGEABLE diff --git a/ironic/tests/unit/drivers/modules/ilo/test_inspect.py b/ironic/tests/unit/drivers/modules/ilo/test_inspect.py index 2efc104f0f..f32a34bb90 100644 --- a/ironic/tests/unit/drivers/modules/ilo/test_inspect.py +++ b/ironic/tests/unit/drivers/modules/ilo/test_inspect.py @@ -23,10 +23,10 @@ from ironic.common import states from ironic.common import utils from ironic.conductor import task_manager from ironic.conductor import utils as conductor_utils -from ironic.drivers.modules import deploy_utils from ironic.drivers.modules.ilo import common as ilo_common from ironic.drivers.modules.ilo import inspect as ilo_inspect from ironic.drivers.modules.ilo import power as ilo_power +from ironic.drivers.modules import inspect_utils from ironic.tests.unit.drivers.modules.ilo import test_common @@ -51,7 +51,7 @@ class IloInspectTestCase(test_common.BaseIloTest): @mock.patch.object(ilo_inspect, '_get_capabilities', spec_set=True, autospec=True) - @mock.patch.object(deploy_utils, 'create_ports_if_not_exist', + @mock.patch.object(inspect_utils, 'create_ports_if_not_exist', spec_set=True, autospec=True) @mock.patch.object(ilo_inspect, '_get_essential_properties', spec_set=True, autospec=True) @@ -88,7 +88,7 @@ class IloInspectTestCase(test_common.BaseIloTest): spec_set=True, autospec=True) @mock.patch.object(ilo_inspect, '_get_capabilities', spec_set=True, autospec=True) - @mock.patch.object(deploy_utils, 'create_ports_if_not_exist', + @mock.patch.object(inspect_utils, 'create_ports_if_not_exist', spec_set=True, autospec=True) @mock.patch.object(ilo_inspect, '_get_essential_properties', spec_set=True, autospec=True) @@ -131,7 +131,7 @@ class IloInspectTestCase(test_common.BaseIloTest): @mock.patch.object(ilo_inspect, '_get_capabilities', spec_set=True, autospec=True) - @mock.patch.object(deploy_utils, 'create_ports_if_not_exist', + @mock.patch.object(inspect_utils, 'create_ports_if_not_exist', spec_set=True, autospec=True) @mock.patch.object(ilo_inspect, '_get_essential_properties', spec_set=True, autospec=True) @@ -170,7 +170,7 @@ class IloInspectTestCase(test_common.BaseIloTest): @mock.patch.object(ilo_inspect, '_get_capabilities', spec_set=True, autospec=True) - @mock.patch.object(deploy_utils, 'create_ports_if_not_exist', + @mock.patch.object(inspect_utils, 'create_ports_if_not_exist', spec_set=True, autospec=True) @mock.patch.object(ilo_inspect, '_get_essential_properties', spec_set=True, autospec=True) @@ -209,7 +209,7 @@ class IloInspectTestCase(test_common.BaseIloTest): @mock.patch.object(ilo_inspect, '_get_capabilities', spec_set=True, autospec=True) - @mock.patch.object(deploy_utils, 'create_ports_if_not_exist', + @mock.patch.object(inspect_utils, 'create_ports_if_not_exist', spec_set=True, autospec=True) @mock.patch.object(ilo_inspect, '_get_essential_properties', spec_set=True, autospec=True) diff --git a/ironic/tests/unit/drivers/modules/redfish/test_inspect.py b/ironic/tests/unit/drivers/modules/redfish/test_inspect.py index b705856385..283cb13b39 100644 --- a/ironic/tests/unit/drivers/modules/redfish/test_inspect.py +++ b/ironic/tests/unit/drivers/modules/redfish/test_inspect.py @@ -19,7 +19,7 @@ from oslo_utils import units from ironic.common import exception from ironic.conductor import task_manager -from ironic.drivers.modules import deploy_utils +from ironic.drivers.modules import inspect_utils from ironic.drivers.modules.redfish import utils as redfish_utils from ironic.tests.unit.db import base as db_base from ironic.tests.unit.db import utils as db_utils @@ -79,13 +79,13 @@ class RedfishInspectTestCase(db_base.DbTestCase): mock_parse_driver_info.assert_called_once_with(task.node) @mock.patch.object(redfish_utils, 'get_system', autospec=True) - @mock.patch.object(deploy_utils, 'create_ports_if_not_exist', + @mock.patch.object(inspect_utils, 'create_ports_if_not_exist', autospec=True) def test_inspect_hardware_ok(self, mock_create_ports_if_not_exist, mock_get_system): expected_properties = { 'cpu_arch': 'mips', 'cpus': '8', - 'local_gb': '4', 'memory_mb': '2048' + 'local_gb': '3', 'memory_mb': '2048' } system = self.init_system_mock(mock_get_system.return_value) @@ -118,7 +118,7 @@ class RedfishInspectTestCase(db_base.DbTestCase): shared=True) as task: expected_properties = { 'cpu_arch': 'x86_64', 'cpus': '8', - 'local_gb': '4', 'memory_mb': '2048' + 'local_gb': '3', 'memory_mb': '2048' } task.driver.inspect.inspect_hardware(task) self.assertEqual(expected_properties, task.node.properties) @@ -170,13 +170,13 @@ class RedfishInspectTestCase(db_base.DbTestCase): shared=True) as task: expected_properties = { 'cpu_arch': 'mips', 'cpus': '8', - 'local_gb': '4', 'memory_mb': '4096' + 'local_gb': '3', 'memory_mb': '4096' } task.driver.inspect.inspect_hardware(task) self.assertEqual(expected_properties, task.node.properties) @mock.patch.object(redfish_utils, 'get_system', autospec=True) - @mock.patch.object(deploy_utils, 'create_ports_if_not_exist', + @mock.patch.object(inspect_utils, 'create_ports_if_not_exist', autospec=True) def test_inspect_hardware_ignore_missing_nics( self, mock_create_ports_if_not_exist, mock_get_system): diff --git a/ironic/tests/unit/drivers/modules/test_deploy_utils.py b/ironic/tests/unit/drivers/modules/test_deploy_utils.py index 6f93548569..3cecc5ec1f 100644 --- a/ironic/tests/unit/drivers/modules/test_deploy_utils.py +++ b/ironic/tests/unit/drivers/modules/test_deploy_utils.py @@ -44,7 +44,6 @@ from ironic.drivers.modules import image_cache from ironic.drivers.modules import pxe from ironic.drivers.modules.storage import cinder from ironic.drivers import utils as driver_utils -from ironic import objects from ironic.tests import base as tests_base from ironic.tests.unit.db import base as db_base from ironic.tests.unit.db import utils as db_utils @@ -1308,38 +1307,6 @@ class OtherFunctionTestCase(db_base.DbTestCase): self.assertRaises(exception.InvalidParameterValue, utils.get_ironic_api_url) - @mock.patch.object(utils.LOG, 'info', spec_set=True, autospec=True) - @mock.patch.object(objects, 'Port', spec_set=True, autospec=True) - def test_create_ports_if_not_exist(self, port_mock, log_mock): - macs = {'Port 1': 'aa:aa:aa:aa:aa:aa', 'Port 2': 'bb:bb:bb:bb:bb:bb'} - node_id = self.node.id - port_dict1 = {'address': 'aa:aa:aa:aa:aa:aa', 'node_id': node_id} - port_dict2 = {'address': 'bb:bb:bb:bb:bb:bb', 'node_id': node_id} - port_obj1, port_obj2 = mock.MagicMock(), mock.MagicMock() - port_mock.side_effect = [port_obj1, port_obj2] - with task_manager.acquire(self.context, self.node.uuid, - shared=False) as task: - utils.create_ports_if_not_exist(task, macs) - self.assertTrue(log_mock.called) - expected_calls = [mock.call(task.context, **port_dict1), - mock.call(task.context, **port_dict2)] - port_mock.assert_has_calls(expected_calls, any_order=True) - port_obj1.create.assert_called_once_with() - port_obj2.create.assert_called_once_with() - - @mock.patch.object(utils.LOG, 'warning', - spec_set=True, autospec=True) - @mock.patch.object(objects.Port, 'create', spec_set=True, autospec=True) - def test_create_ports_if_not_exist_mac_exception(self, - create_mock, - log_mock): - create_mock.side_effect = exception.MACAlreadyExists('f') - macs = {'Port 1': 'aa:aa:aa:aa:aa:aa', 'Port 2': 'bb:bb:bb:bb:bb:bb'} - with task_manager.acquire(self.context, self.node.uuid, - shared=False) as task: - utils.create_ports_if_not_exist(task, macs) - self.assertEqual(2, log_mock.call_count) - class GetSingleNicTestCase(db_base.DbTestCase): diff --git a/ironic/tests/unit/drivers/modules/test_inspect_utils.py b/ironic/tests/unit/drivers/modules/test_inspect_utils.py new file mode 100644 index 0000000000..9bbe8c3d06 --- /dev/null +++ b/ironic/tests/unit/drivers/modules/test_inspect_utils.py @@ -0,0 +1,65 @@ +# Copyright 2018 Red Hat, Inc. +# All Rights Reserved. +# +# 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 mock + +from ironic.common import exception +from ironic.conductor import task_manager +from ironic.drivers.modules import inspect_utils as utils +from ironic import objects +from ironic.tests.unit.db import base as db_base +from ironic.tests.unit.objects import utils as obj_utils + + +@mock.patch('time.sleep', lambda sec: None) +class InspectFunctionTestCase(db_base.DbTestCase): + + def setUp(self): + super(InspectFunctionTestCase, self).setUp() + self.node = obj_utils.create_test_node(self.context, + boot_interface='pxe') + + @mock.patch.object(utils.LOG, 'info', spec_set=True, autospec=True) + @mock.patch.object(objects, 'Port', spec_set=True, autospec=True) + def test_create_ports_if_not_exist(self, port_mock, log_mock): + macs = {'Port 1': 'aa:aa:aa:aa:aa:aa', 'Port 2': 'bb:bb:bb:bb:bb:bb'} + node_id = self.node.id + port_dict1 = {'address': 'aa:aa:aa:aa:aa:aa', 'node_id': node_id} + port_dict2 = {'address': 'bb:bb:bb:bb:bb:bb', 'node_id': node_id} + port_obj1, port_obj2 = mock.MagicMock(), mock.MagicMock() + port_mock.side_effect = [port_obj1, port_obj2] + with task_manager.acquire(self.context, self.node.uuid, + shared=False) as task: + utils.create_ports_if_not_exist(task, macs) + self.assertTrue(log_mock.called) + expected_calls = [mock.call(task.context, **port_dict1), + mock.call(task.context, **port_dict2)] + port_mock.assert_has_calls(expected_calls, any_order=True) + port_obj1.create.assert_called_once_with() + port_obj2.create.assert_called_once_with() + + @mock.patch.object(utils.LOG, 'warning', + spec_set=True, autospec=True) + @mock.patch.object(objects.Port, 'create', spec_set=True, autospec=True) + def test_create_ports_if_not_exist_mac_exception(self, + create_mock, + log_mock): + create_mock.side_effect = exception.MACAlreadyExists('f') + macs = {'Port 1': 'aa:aa:aa:aa:aa:aa', 'Port 2': 'bb:bb:bb:bb:bb:bb'} + with task_manager.acquire(self.context, self.node.uuid, + shared=False) as task: + utils.create_ports_if_not_exist(task, macs) + self.assertEqual(2, log_mock.call_count)