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 <stephane@alum.mit.edu>
Change-Id: Ib21eda88f207f18675c8580cd7fd37eab6fd70bf
Partial-Bug: #1559691

etc/ironic/ironic.conf.sample

@@ -233,6 +233,26 @@
 # entrypoint. (string value)
 #default_raid_interface = <None>
 
+# 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 = <None>
+
 # 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

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
+    object, then attaches the dynamic interfaces (network and storage
-    drivers, all interfaces for dynamic drivers made of hardware types).
+    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
+        # NOTE(TheJulia): This list of interfaces to be applied
-        # drivers, thus it requires separate treatment.
+        # to classic drivers, thus requiring separate treatment.
-        dynamic_interfaces = ['network']
+        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
+        # Only network and storage interfaces are dynamic for classic drivers
-        factories = {'network': _INTERFACE_LOADERS['network']}
+        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']

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
+        # NOTE(vdrok): Instantiate network and storage interface factory on
-        # all the network interfaces are loaded at the very beginning, and
+        # startup so that all the interfaces are loaded at the very
-        # failures prevent the conductor from starting.
+        # 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 "

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')),

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.
 

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."""

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."""

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

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

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.

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
+        # NOTE(TheJulia) We should only check that the warn check is called,
-        # one for each of the 3 network interfaces, the last - for the fake
+        # as opposed to that the check is called a specific number of times,
-        # hardware type.
+        # during driver/interface loading in ironic. This is due to the fact
-        self.assertEqual(5, mock_warn.call_count)
+        # 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)

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):

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)
 

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',

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.

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