From c491e93a082ede262e617cebfd3022090e3e715b Mon Sep 17 00:00:00 2001 From: Julia Kreger Date: Wed, 27 Jul 2016 14:50:51 -0400 Subject: [PATCH] Add storage_interface to base driver class In order to properly support booting and maintenance of systems that boot from a remote storage device, we need an interface to associate the driver with. This commit adds a basic storage_interface and noop and fake interfaces along with the appropriate handling for configuration in the event that the driver list is blank, or is missing the noop driver. Co-Authored-By: Stephane Miller Change-Id: Ib21eda88f207f18675c8580cd7fd37eab6fd70bf Partial-Bug: #1559691 --- etc/ironic/ironic.conf.sample | 20 ++++++ ironic/common/driver_factory.py | 22 ++++--- ironic/conductor/base_manager.py | 7 ++- ironic/conf/default.py | 5 ++ ironic/drivers/base.py | 40 ++++++++++++ ironic/drivers/fake_hardware.py | 5 ++ ironic/drivers/hardware_type.py | 6 ++ ironic/drivers/modules/fake.py | 19 ++++++ ironic/drivers/modules/storage/__init__.py | 0 ironic/drivers/modules/storage/noop.py | 32 ++++++++++ ironic/objects/node.py | 9 ++- .../tests/unit/common/test_driver_factory.py | 62 ++++++++++++++++--- .../tests/unit/conductor/test_base_manager.py | 5 +- ironic/tests/unit/conductor/test_manager.py | 4 +- ironic/tests/unit/objects/test_objects.py | 2 +- ...dd-storage-interface-d4e64224804207fc.yaml | 6 ++ setup.cfg | 4 ++ 17 files changed, 225 insertions(+), 23 deletions(-) create mode 100644 ironic/drivers/modules/storage/__init__.py create mode 100644 ironic/drivers/modules/storage/noop.py create mode 100644 releasenotes/notes/add-storage-interface-d4e64224804207fc.yaml diff --git a/etc/ironic/ironic.conf.sample b/etc/ironic/ironic.conf.sample index 4a7a2e31ee..ac5c5cf7dc 100644 --- a/etc/ironic/ironic.conf.sample +++ b/etc/ironic/ironic.conf.sample @@ -233,6 +233,26 @@ # entrypoint. (string value) #default_raid_interface = +# Specify the list of storage interfaces to load during +# service initialization. Missing storage interfaces, or +# storage interfaces which fail to initialize, will prevent +# the ironic-conductor service from starting. The default +# value is a recommended set of production-oriented storage +# interfaces. A complete list of storage interfaces present on +# your system may be found by enumerating the +# "ironic.hardware.interfaces.storage" entrypoint. When +# setting this value, please make sure that every enabled +# hardware type will have the same set of enabled storage +# interfaces on every ironic-conductor service. (list value) +#enabled_storage_interfaces = noop + +# Default storage interface to be used for nodes that do not +# have storage_interface field set. A complete list of storage +# interfaces present on your system may be found by +# enumerating the "ironic.hardware.interfaces.storage" +# entrypoint. (list value) +#default_storage_interface = + # WARNING: This configuration option is part of the incomplete # driver composition work, changing it's setting has no # effect. Specify the list of vendor interfaces to load during diff --git a/ironic/common/driver_factory.py b/ironic/common/driver_factory.py index fead4ef548..4ff27b0d3b 100644 --- a/ironic/common/driver_factory.py +++ b/ironic/common/driver_factory.py @@ -67,8 +67,9 @@ def _attach_interfaces_to_driver(bare_driver, node, driver_or_hw_type): """Attach interface implementations to a bare driver object. For classic drivers, copies implementations from the singleton driver - object, then attaches the dynamic interfaces (network_interface for classic - drivers, all interfaces for dynamic drivers made of hardware types). + object, then attaches the dynamic interfaces (network and storage + interfaces for classic drivers, all interfaces for dynamic drivers + made of hardware types). For hardware types, load all interface implementations dynamically. @@ -88,9 +89,9 @@ def _attach_interfaces_to_driver(bare_driver, node, driver_or_hw_type): impl = getattr(driver_or_hw_type, iface, None) setattr(bare_driver, iface, impl) - # NOTE(dtantsur): only network interface is dynamic for classic - # drivers, thus it requires separate treatment. - dynamic_interfaces = ['network'] + # NOTE(TheJulia): This list of interfaces to be applied + # to classic drivers, thus requiring separate treatment. + dynamic_interfaces = ['network', 'storage'] for iface in dynamic_interfaces: impl_name = getattr(node, '%s_interface' % iface) @@ -190,16 +191,18 @@ def check_and_update_node_interfaces(node, driver_or_hw_type=None): is_hardware_type = isinstance(driver_or_hw_type, hardware_type.AbstractHardwareType) - # Legacy network interface defaults + # Explicit interface defaults additional_defaults = { - 'network': 'flat' if CONF.dhcp.dhcp_provider == 'neutron' else 'noop' + 'network': 'flat' if CONF.dhcp.dhcp_provider == 'neutron' else 'noop', + 'storage': 'noop' } if is_hardware_type: factories = _INTERFACE_LOADERS else: - # Only network interface is dynamic for classic drivers - factories = {'network': _INTERFACE_LOADERS['network']} + # Only network and storage interfaces are dynamic for classic drivers + factories = {'network': _INTERFACE_LOADERS['network'], + 'storage': _INTERFACE_LOADERS['storage']} # Result - whether the node object was modified result = False @@ -453,3 +456,4 @@ _INTERFACE_LOADERS = { # TODO(dtantsur): This factory is still used explicitly in many places, # refactor them later to use _INTERFACE_LOADERS. NetworkInterfaceFactory = _INTERFACE_LOADERS['network'] +StorageInterfaceFactory = _INTERFACE_LOADERS['storage'] diff --git a/ironic/conductor/base_manager.py b/ironic/conductor/base_manager.py index 5d02609373..f63738b337 100644 --- a/ironic/conductor/base_manager.py +++ b/ironic/conductor/base_manager.py @@ -80,11 +80,12 @@ class BaseConductorManager(object): """Consistent hash ring which maps drivers to conductors.""" # NOTE(deva): these calls may raise DriverLoadError or DriverNotFound - # NOTE(vdrok): instantiate network interface factory on startup so that - # all the network interfaces are loaded at the very beginning, and - # failures prevent the conductor from starting. + # NOTE(vdrok): Instantiate network and storage interface factory on + # startup so that all the interfaces are loaded at the very + # beginning, and failures prevent the conductor from starting. drivers = driver_factory.drivers() driver_factory.NetworkInterfaceFactory() + driver_factory.StorageInterfaceFactory() if not drivers: msg = _LE("Conductor %s cannot be started because no drivers " "were loaded. This could be because no drivers were " diff --git a/ironic/conf/default.py b/ironic/conf/default.py index e52c5ca38a..54dbe846d8 100644 --- a/ironic/conf/default.py +++ b/ironic/conf/default.py @@ -148,6 +148,11 @@ driver_opts = [ help=_ENABLED_IFACE_HELP_WITH_WARNING.format('raid')), cfg.StrOpt('default_raid_interface', help=_DEFAULT_IFACE_HELP_WITH_WARNING.format('raid')), + cfg.ListOpt('enabled_storage_interfaces', + default=['noop'], + help=_ENABLED_IFACE_HELP.format('storage')), + cfg.ListOpt('default_storage_interface', + help=_DEFAULT_IFACE_HELP.format('storage')), cfg.ListOpt('enabled_vendor_interfaces', default=['no-vendor'], help=_ENABLED_IFACE_HELP_WITH_WARNING.format('vendor')), diff --git a/ironic/drivers/base.py b/ironic/drivers/base.py index 2d49f6fe2e..7ee2a1684b 100644 --- a/ironic/drivers/base.py +++ b/ironic/drivers/base.py @@ -170,6 +170,13 @@ class BareDriver(BaseDriver): """ self.core_interfaces.append('network') + self.storage = None + """`Standard` attribute for (remote) storage interface. + + A reference to an instance of :class:StorageInterface. + """ + self.standard_interfaces.append('storage') + ALL_INTERFACES = set(BareDriver().all_interfaces) """Constant holding all known interfaces. @@ -988,6 +995,39 @@ class NetworkInterface(BaseInterface): """ +@six.add_metaclass(abc.ABCMeta) +class StorageInterface(BaseInterface): + """Base class for storage interfaces.""" + + interface_type = 'storage' + + @abc.abstractmethod + def attach_volumes(self, task): + """Informs the storage subsystem to attach all volumes for the node. + + :param task: a TaskManager instance. + :raises: UnsupportedDriverExtension + """ + + @abc.abstractmethod + def detach_volumes(self, task): + """Informs the storage subsystem to detach all volumes for the node. + + :param task: a TaskManager instance. + :raises: UnsupportedDriverExtension + """ + + @abc.abstractmethod + def should_write_image(self, task): + """Determines if deploy should perform the image write-out. + + :param task: a TaskManager instance. + :returns: Boolean value to indicate if the interface expects + the image to be written by Ironic. + :raises: UnsupportedDriverExtension + """ + + def _validate_argsinfo(argsinfo): """Validate args info. diff --git a/ironic/drivers/fake_hardware.py b/ironic/drivers/fake_hardware.py index 769a6664d8..f6320de0cf 100644 --- a/ironic/drivers/fake_hardware.py +++ b/ironic/drivers/fake_hardware.py @@ -66,6 +66,11 @@ class FakeHardware(hardware_type.AbstractHardwareType): """List of classes of supported raid interfaces.""" return [fake.FakeRAID] + @property + def supported_storage_interfaces(self): + """List of classes of supported storage interfaces.""" + return [fake.FakeStorage] + @property def supported_vendor_interfaces(self): """List of classes of supported rescue interfaces.""" diff --git a/ironic/drivers/hardware_type.py b/ironic/drivers/hardware_type.py index 71287235ab..001dfe8001 100644 --- a/ironic/drivers/hardware_type.py +++ b/ironic/drivers/hardware_type.py @@ -22,6 +22,7 @@ import six from ironic.drivers.modules.network import noop as noop_net from ironic.drivers.modules import noop +from ironic.drivers.modules.storage import noop as noop_storage @six.add_metaclass(abc.ABCMeta) @@ -80,6 +81,11 @@ class AbstractHardwareType(object): """List of supported raid interfaces.""" return [noop.NoRAID] + @property + def supported_storage_interfaces(self): + """List of supported storage interfaces.""" + return [noop_storage.NoopStorage] + @property def supported_vendor_interfaces(self): """List of supported vendor interfaces.""" diff --git a/ironic/drivers/modules/fake.py b/ironic/drivers/modules/fake.py index bb2aab875f..42641e4ca6 100644 --- a/ironic/drivers/modules/fake.py +++ b/ironic/drivers/modules/fake.py @@ -225,3 +225,22 @@ class FakeRAID(base.RAIDInterface): def delete_configuration(self, task): pass + + +class FakeStorage(base.StorageInterface): + """Example implementation of simple storage Interface.""" + + def validate(self, task): + pass + + def get_properties(self): + return {} + + def attach_volumes(self, task): + pass + + def detach_volumes(self, task): + pass + + def should_write_image(self, task): + return True diff --git a/ironic/drivers/modules/storage/__init__.py b/ironic/drivers/modules/storage/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/ironic/drivers/modules/storage/noop.py b/ironic/drivers/modules/storage/noop.py new file mode 100644 index 0000000000..87a4e69342 --- /dev/null +++ b/ironic/drivers/modules/storage/noop.py @@ -0,0 +1,32 @@ +# 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 ironic.drivers import base + + +class NoopStorage(base.StorageInterface): + """No-op Storage Interface.""" + + def validate(self, task): + pass + + def get_properties(self): + return {} + + def attach_volumes(self, task): + pass + + def detach_volumes(self, task): + pass + + def should_write_image(self, task): + return True diff --git a/ironic/objects/node.py b/ironic/objects/node.py index bd63a17707..fb29c3265a 100644 --- a/ironic/objects/node.py +++ b/ironic/objects/node.py @@ -54,7 +54,8 @@ class Node(base.IronicObject, object_base.VersionedObjectDictCompat): # deploy_interface, inspect_interface, management_interface, # power_interface, raid_interface, vendor_interface # Version 1.20: Type of network_interface changed to just nullable string - VERSION = '1.20' + # Version 1.21: Add storage_interface field + VERSION = '1.21' dbapi = db_api.get_instance() @@ -122,6 +123,7 @@ class Node(base.IronicObject, object_base.VersionedObjectDictCompat): 'network_interface': object_fields.StringField(nullable=True), 'power_interface': object_fields.StringField(nullable=True), 'raid_interface': object_fields.StringField(nullable=True), + 'storage_interface': object_fields.StringField(nullable=True), 'vendor_interface': object_fields.StringField(nullable=True), } @@ -436,6 +438,11 @@ class NodePayload(notification.NotificationPayloadBase): 'updated_at': ('node', 'updated_at'), 'uuid': ('node', 'uuid') } + # TODO(TheJulia): At a later point in time, once storage_interfaces + # are able to be leveraged, we need to add the sotrage_interface + # field to payload and increment the object versions for all objects + # that inherit the NodePayload object. + # Version 1.0: Initial version, based off of Node version 1.18. # Version 1.1: Type of network_interface changed to just nullable string # similar to version 1.20 of Node. diff --git a/ironic/tests/unit/common/test_driver_factory.py b/ironic/tests/unit/common/test_driver_factory.py index 5a0c23dd22..54f241c8a1 100644 --- a/ironic/tests/unit/common/test_driver_factory.py +++ b/ironic/tests/unit/common/test_driver_factory.py @@ -139,10 +139,11 @@ class NetworkInterfaceFactoryTestCase(db_base.DbTestCase): factory._entrypoint_name) self.assertEqual(['flat', 'neutron', 'noop'], sorted(factory._enabled_driver_list)) - # NOTE(jroll) 5 checks, one for the driver we're building and - # one for each of the 3 network interfaces, the last - for the fake - # hardware type. - self.assertEqual(5, mock_warn.call_count) + # NOTE(TheJulia) We should only check that the warn check is called, + # as opposed to that the check is called a specific number of times, + # during driver/interface loading in ironic. This is due to the fact + # each activated interface or driver causes the number to increment. + self.assertTrue(mock_warn.called) def test_build_driver_for_task_default_is_none(self): # flat, neutron, and noop network interfaces are enabled in base test @@ -190,6 +191,24 @@ class NetworkInterfaceFactoryTestCase(db_base.DbTestCase): task_manager.acquire, self.context, node.id) +class StorageInterfaceFactoryTestCase(db_base.DbTestCase): + + def setUp(self): + super(StorageInterfaceFactoryTestCase, self).setUp() + driver_factory.DriverFactory._extension_manager = None + driver_factory.StorageInterfaceFactory._extension_manager = None + self.config(enabled_drivers=['fake']) + + def test_build_interface_for_task(self): + """Validate a node has no default storage interface.""" + factory = driver_factory.StorageInterfaceFactory + node = obj_utils.create_test_node(self.context, driver='fake') + with task_manager.acquire(self.context, node.id) as task: + manager = factory._extension_manager + self.assertIn('noop', manager) + self.assertEqual('noop', task.node.storage_interface) + + class NewDriverFactory(driver_factory.BaseDriverFactory): _entrypoint_name = 'woof' @@ -226,9 +245,10 @@ class CheckAndUpdateNodeInterfacesTestCase(db_base.DbTestCase): # "none" dhcp provider corresponds to "noop" network_interface self.assertEqual('noop', node.network_interface) - def test_valid_network_interface(self): + def test_valid_interfaces(self): node = obj_utils.get_test_node(self.context, driver='fake', - network_interface='noop') + network_interface='noop', + storage_interface='noop') self.assertFalse(driver_factory.check_and_update_node_interfaces(node)) def test_invalid_network_interface(self): @@ -296,6 +316,9 @@ class HardwareTypeLoadTestCase(db_base.DbTestCase): if iface == 'network': self.ifaces[iface] = 'noop' enabled = ['noop'] + elif iface == 'storage': + self.ifaces[iface] = 'noop' + enabled = ['noop'] else: self.ifaces[iface] = 'fake' enabled = ['fake'] @@ -366,7 +389,7 @@ class HardwareTypeLoadTestCase(db_base.DbTestCase): def test_build_driver_for_task_no_defaults(self): self.config(dhcp_provider=None, group='dhcp') for iface in drivers_base.ALL_INTERFACES: - if iface != 'network': + if iface not in ['network', 'storage']: self.config(**{'enabled_%s_interfaces' % iface: []}) self.config(**{'default_%s_interface' % iface: None}) node = obj_utils.create_test_node(self.context, driver='fake-hardware') @@ -398,3 +421,28 @@ class HardwareTypeLoadTestCase(db_base.DbTestCase): node = obj_utils.create_test_node(self.context, driver='fake-hardware') self.assertRaises(exception.InterfaceNotFoundInEntrypoint, task_manager.acquire, self.context, node.id) + + def test_no_storage_interface(self): + node = obj_utils.get_test_node(self.context, driver='fake') + self.assertTrue(driver_factory.check_and_update_node_interfaces(node)) + self.assertEqual('noop', node.storage_interface) + + def test_none_storage_interface(self): + node = obj_utils.get_test_node(self.context, driver='fake', + storage_interface=None) + self.assertTrue(driver_factory.check_and_update_node_interfaces(node)) + self.assertEqual('noop', node.storage_interface) + + def test_no_storage_interface_default_from_conf(self): + self.config(enabled_storage_interfaces=['noop', 'fake']) + self.config(default_storage_interface='fake') + node = obj_utils.get_test_node(self.context, driver='fake') + self.assertTrue(driver_factory.check_and_update_node_interfaces(node)) + self.assertEqual('fake', node.storage_interface) + + def test_invalid_storage_interface(self): + node = obj_utils.get_test_node(self.context, driver='fake', + storage_interface='scoop') + self.assertRaises(exception.InterfaceNotFoundInEntrypoint, + driver_factory.check_and_update_node_interfaces, + node) diff --git a/ironic/tests/unit/conductor/test_base_manager.py b/ironic/tests/unit/conductor/test_base_manager.py index 215228ed62..4feca2ab17 100644 --- a/ironic/tests/unit/conductor/test_base_manager.py +++ b/ironic/tests/unit/conductor/test_base_manager.py @@ -77,8 +77,10 @@ class StartStopTestCase(mgr_utils.ServiceSetUpMixin, tests_db_base.DbTestCase): @mock.patch.object(driver_factory.DriverFactory, '__getitem__', lambda *args: mock.MagicMock()) + @mock.patch.object(driver_factory, 'StorageInterfaceFactory') @mock.patch.object(driver_factory, 'NetworkInterfaceFactory') - def test_start_registers_driver_names(self, net_factory): + def test_start_registers_driver_names(self, net_factory, + storage_factory): init_names = ['fake1', 'fake2'] restart_names = ['fake3', 'fake4'] @@ -101,6 +103,7 @@ class StartStopTestCase(mgr_utils.ServiceSetUpMixin, tests_db_base.DbTestCase): self.hostname) self.assertEqual(restart_names, res['drivers']) self.assertEqual(2, net_factory.call_count) + self.assertEqual(2, storage_factory.call_count) @mock.patch.object(driver_factory.DriverFactory, '__getitem__') def test_start_registers_driver_specific_tasks(self, get_mock): diff --git a/ironic/tests/unit/conductor/test_manager.py b/ironic/tests/unit/conductor/test_manager.py index c3e2b75a7e..185339e6c6 100644 --- a/ironic/tests/unit/conductor/test_manager.py +++ b/ironic/tests/unit/conductor/test_manager.py @@ -2590,7 +2590,9 @@ class MiscTestCase(mgr_utils.ServiceSetUpMixin, mgr_utils.CommonMixIn, 'boot': {'result': True}, 'raid': {'result': True}, 'deploy': {'result': True}, - 'network': {'result': True}} + 'network': {'result': True}, + 'storage': {'result': True}} + self.assertEqual(expected, ret) mock_iwdi.assert_called_once_with(self.context, node.instance_info) diff --git a/ironic/tests/unit/objects/test_objects.py b/ironic/tests/unit/objects/test_objects.py index 82a6cfec5d..e458256c7e 100644 --- a/ironic/tests/unit/objects/test_objects.py +++ b/ironic/tests/unit/objects/test_objects.py @@ -404,7 +404,7 @@ class TestObject(_LocalTest, _TestObject): # version bump. It is an MD5 hash of the object fields and remotable methods. # The fingerprint values should only be changed if there is a version bump. expected_object_fingerprints = { - 'Node': '1.20-b6a13eb50f9d64fa6c9d614c61dbec31', + 'Node': '1.21-52674c214141cf3e09f8688bfed54577', 'MyObj': '1.5-4f5efe8f0fcaf182bbe1c7fe3ba858db', 'Chassis': '1.3-d656e039fd8ae9f34efc232ab3980905', 'Port': '1.6-609504503d68982a10f495659990084b', diff --git a/releasenotes/notes/add-storage-interface-d4e64224804207fc.yaml b/releasenotes/notes/add-storage-interface-d4e64224804207fc.yaml new file mode 100644 index 0000000000..a19f21f323 --- /dev/null +++ b/releasenotes/notes/add-storage-interface-d4e64224804207fc.yaml @@ -0,0 +1,6 @@ +--- +features: + - | + Adds the initial substrate to allow for the creation of storage + interfaces. The default storage interface for nodes is ``noop``, + which routes to a no-op driver that is included with the substrate. diff --git a/setup.cfg b/setup.cfg index 3cf4ccda7a..5a0e1cd953 100644 --- a/setup.cfg +++ b/setup.cfg @@ -125,6 +125,10 @@ ironic.hardware.interfaces.raid = ironic.hardware.interfaces.rescue = no-rescue = ironic.drivers.modules.noop:NoRescue +ironic.hardware.interfaces.storage = + fake = ironic.drivers.modules.fake:FakeStorage + noop = ironic.drivers.modules.storage.noop:NoopStorage + ironic.hardware.interfaces.vendor = fake = ironic.drivers.modules.fake:FakeVendorB no-vendor = ironic.drivers.modules.noop:NoVendor