openstack/ironic-python-agent
Code Issues Proposed changes

Revert "Add hardware manager interface for hardware initialization"

Browse Source 
I would've voted -1 on the patch in question had I reviewed it, and per
standard OpenStack/Ironic procedure, I'm reverting it for re-review and
discussion.

In this case; I don't think the new method in the HWM interface is
needed, and that evaluate_hardware_support() is intended to handle the
cases handled.

This reverts commit 0962cae1da.

Change-Id: Ic08e44bdf116403444b257ee9f4e5b906f5eac53
This commit is contained in:
Jay Faulkner
2016-05-23 16:12:51 +00:00
parent 0962cae1da
commit 5a1a1ca61c
5 changed files with 8 additions and 125 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
ironic_python_agent/agent.py
View File

@@ -289,8 +289,6 @@ class IronicPythonAgent(base.ExecuteCommandMixin):
# Cached hw managers at runtime, not load time. See bug 1490008.
hardware.load_managers()
# Request the hw manager to do long initializations
hardware.dispatch_to_managers('initialize_hardware')
if not self.standalone:
# Inspection should be started before call to lookup, otherwise

49
ironic_python_agent/hardware.py
View File

@@ -16,7 +16,6 @@ import abc
import functools
import os
import shlex
import time
import netifaces
from oslo_concurrency import processutils
@@ -39,9 +38,6 @@ UNIT_CONVERTER = pint.UnitRegistry(filename=None)
UNIT_CONVERTER.define('MB = []')
UNIT_CONVERTER.define('GB = 1024 MB')
_DISK_WAIT_ATTEMPTS = 5
_DISK_WAIT_DELAY = 3
def _get_device_vendor(dev):
"""Get the vendor name of a given device."""
@@ -389,57 +385,17 @@ class HardwareManager(object):
'version': getattr(self, 'HARDWARE_MANAGER_VERSION', '1.0')
}
def initialize_hardware(self):
"""Initialize hardware on the agent start up.
This method will be called once on start up before any calls
to list_hardware_info are made.
The default implementation does nothing.
"""
class GenericHardwareManager(HardwareManager):
HARDWARE_MANAGER_NAME = 'generic_hardware_manager'
HARDWARE_MANAGER_VERSION = '1.0'
# These modules are rarely loaded automatically
_PRELOADED_MODULES = ['ipmi_msghandler', 'ipmi_devintf', 'ipmi_si']
def __init__(self):
self.sys_path = '/sys'
def evaluate_hardware_support(self):
return HardwareSupport.GENERIC
def initialize_hardware(self):
LOG.debug('Initializing hardware')
self._preload_modules()
_udev_settle()
self._wait_for_disks()
def _preload_modules(self):
# TODO(dtantsur): try to load as many kernel modules for present
# hardware as it's possible.
for mod in self._PRELOADED_MODULES:
utils.try_execute('modprobe', mod)
def _wait_for_disks(self):
# Wait for at least one suitable disk to show up, otherwise neither
# inspection not deployment have any chances to succeed.
for attempt in range(_DISK_WAIT_ATTEMPTS):
try:
self.get_os_install_device()
except errors.DeviceNotFound:
LOG.debug('Still waiting for at least one disk to appear, '
'attempt %d of %d', attempt + 1, _DISK_WAIT_ATTEMPTS)
time.sleep(_DISK_WAIT_DELAY)
else:
break
else:
LOG.warning('No disks detected in %d seconds',
_DISK_WAIT_DELAY * _DISK_WAIT_ATTEMPTS)
def _get_interface_info(self, interface_name):
addr_path = '{0}/class/net/{1}/address'.format(self.sys_path,
interface_name)
@@ -782,6 +738,11 @@ class GenericHardwareManager(HardwareManager):
return True
def get_bmc_address(self):
# These modules are rarely loaded automatically
utils.try_execute('modprobe', 'ipmi_msghandler')
utils.try_execute('modprobe', 'ipmi_devintf')
utils.try_execute('modprobe', 'ipmi_si')
try:
out, _e = utils.execute(
"ipmitool lan print | grep -e 'IP Address [^S]' "

11
ironic_python_agent/tests/unit/test_agent.py
View File

@@ -167,12 +167,9 @@ class TestBaseAgent(test_base.BaseTestCase):
self.assertEqual(pkg_resources.get_distribution('ironic-python-agent')
.version, status.version)
@mock.patch.object(hardware.GenericHardwareManager, 'initialize_hardware',
autospec=True)
@mock.patch('wsgiref.simple_server.make_server', autospec=True)
@mock.patch.object(hardware.HardwareManager, 'list_hardware_info')
def test_run(self, mocked_list_hardware, wsgi_server_cls,
mocked_init_hardware):
def test_run(self, mocked_list_hardware, wsgi_server_cls):
CONF.set_override('inspection_callback_url', '', enforce_type=True)
wsgi_server = wsgi_server_cls.return_value
wsgi_server.start.side_effect = KeyboardInterrupt()
@@ -196,15 +193,12 @@ class TestBaseAgent(test_base.BaseTestCase):
wsgi_server.serve_forever.assert_called_once_with()
self.agent.heartbeater.start.assert_called_once_with()
mocked_init_hardware.assert_called_once_with(mock.ANY)
@mock.patch.object(inspector, 'inspect', autospec=True)
@mock.patch.object(hardware.GenericHardwareManager, 'initialize_hardware',
autospec=True)
@mock.patch('wsgiref.simple_server.make_server', autospec=True)
@mock.patch.object(hardware.HardwareManager, 'list_hardware_info')
def test_run_with_inspection(self, mocked_list_hardware, wsgi_server_cls,
mocked_init_hardware, mocked_inspector):
mocked_inspector):
CONF.set_override('inspection_callback_url', 'http://foo/bar',
enforce_type=True)
@@ -237,7 +231,6 @@ class TestBaseAgent(test_base.BaseTestCase):
self.agent.api_client.lookup_node.call_args[1]['node_uuid'])
self.agent.heartbeater.start.assert_called_once_with()
mocked_init_hardware.assert_called_once_with(mock.ANY)
def test_async_command_success(self):
result = base.AsyncCommandResult('foo_command', {'fail': False},

64
ironic_python_agent/tests/unit/test_hardware.py
View File

@@ -12,11 +12,9 @@
# See the License for the specific language governing permissions and
# limitations under the License.
import os
import time
import mock
import netifaces
import os
from oslo_concurrency import processutils
from oslo_utils import units
from oslotest import base as test_base
@@ -1087,66 +1085,6 @@ class TestGenericHardwareManager(test_base.BaseTestCase):
self.hardware.get_system_vendor_info().manufacturer)
@mock.patch.object(time, 'sleep', autospec=True)
@mock.patch.object(hardware.GenericHardwareManager,
'get_os_install_device', autospec=True)
@mock.patch.object(utils, 'execute', autospec=True)
class TestGenericHardwareManagerInitializeHardware(test_base.BaseTestCase):
def setUp(self):
super(TestGenericHardwareManagerInitializeHardware, self).setUp()
self.hardware = hardware.GenericHardwareManager()
def test_ok(self, mocked_execute, mocked_os_dev, mocked_sleep):
self.hardware.initialize_hardware()
expected_execute_calls = [
mock.call('modprobe', mod)
for mod in self.hardware._PRELOADED_MODULES
]
expected_execute_calls.append(mock.call('udevadm', 'settle'))
self.assertEqual(expected_execute_calls, mocked_execute.call_args_list)
mocked_os_dev.assert_called_once_with(mock.ANY)
self.assertFalse(mocked_sleep.called)
def test_disk_delayed(self, mocked_execute, mocked_os_dev, mocked_sleep):
mocked_os_dev.side_effect = [
errors.DeviceNotFound(''),
errors.DeviceNotFound(''),
None
]
self.hardware.initialize_hardware()
expected_execute_calls = [
mock.call('modprobe', mod)
for mod in self.hardware._PRELOADED_MODULES
]
expected_execute_calls.append(mock.call('udevadm', 'settle'))
self.assertEqual(expected_execute_calls, mocked_execute.call_args_list)
mocked_os_dev.assert_called_with(mock.ANY)
self.assertEqual(3, mocked_os_dev.call_count)
self.assertEqual(2, mocked_sleep.call_count)
@mock.patch.object(hardware.LOG, 'warning', autospec=True)
def test_no_disk(self, mocked_warn, mocked_execute, mocked_os_dev,
mocked_sleep):
mocked_os_dev.side_effect = errors.DeviceNotFound('')
self.hardware.initialize_hardware()
expected_execute_calls = [
mock.call('modprobe', mod)
for mod in self.hardware._PRELOADED_MODULES
]
expected_execute_calls.append(mock.call('udevadm', 'settle'))
self.assertEqual(expected_execute_calls, mocked_execute.call_args_list)
mocked_os_dev.assert_called_with(mock.ANY)
self.assertEqual(hardware._DISK_WAIT_ATTEMPTS,
mocked_os_dev.call_count)
self.assertEqual(hardware._DISK_WAIT_ATTEMPTS, mocked_sleep.call_count)
self.assertTrue(mocked_warn.called)
@mock.patch.object(utils, 'execute', autospec=True)
class TestModuleFunctions(test_base.BaseTestCase):

7
releasenotes/notes/initialize-hardware-db00d62c3669be93.yaml
View File

@@ -1,7 +0,0 @@
---
features:
- Add a new hardware manager method "initialize_hardware" which can be
overriden to provide early oneshot hardware initialization, such as loading
kernel modules or probing hardware. The generic implementation loads the
IPMI modules, calls "udev settle" and waits for at least one suitable disk
to appear in the "lsblk" output.