Merge "Fix reported storage_protocol"

cinder/api/views/capabilities.py

@@ -27,13 +27,20 @@ class ViewBuilder(common.ViewBuilder):
 
     def summary(self, request, capabilities, id):
         """Summary view of a backend capabilities."""
+        # Internally storage_protocol can be a list with all the variants (eg.
+        # FC, fibre_channel), but we return a single value to users.  The first
+        # value is the preferred variant.
+        storage_protocol = capabilities.get('storage_protocol')
+        if isinstance(storage_protocol, list):
+            storage_protocol = storage_protocol[0]
+
         return {
             'namespace': 'OS::Storage::Capabilities::%s' % id,
             'vendor_name': capabilities.get('vendor_name'),
             'volume_backend_name': capabilities.get('volume_backend_name'),
             'pool_name': capabilities.get('pool_name'),
             'driver_version': capabilities.get('driver_version'),
-            'storage_protocol': capabilities.get('storage_protocol'),
+            'storage_protocol': storage_protocol,
             'display_name': capabilities.get('display_name'),
             'description': capabilities.get('description'),
             'visibility': capabilities.get('visibility'),

cinder/api/views/scheduler_stats.py

@@ -35,10 +35,20 @@ class ViewBuilder(common.ViewBuilder):
 
     def detail(self, request, pool):
         """Detailed view of a single pool."""
+        # Internally storage_protocol can be a list with all the variants (eg.
+        # FC, fibre_channel), but we return a single value to users.  The first
+        # value is the preferred variant.
+        capabilities = pool.get('capabilities')
+        if capabilities:
+            protocol = capabilities.get('storage_protocol')
+            if isinstance(protocol, list):
+                capabilities = capabilities.copy()
+                capabilities['storage_protocol'] = protocol[0]
+
         return {
             'pool': {
                 'name': pool.get('name'),
-                'capabilities': pool.get('capabilities'),
+                'capabilities': capabilities,
             }
         }
 

cinder/interface/volume_driver.py

@@ -78,7 +78,17 @@ class VolumeDriverCore(base.CinderInterface):
             it tends to follow typical major.minor.revision ideas.
         * storage_protocol
             The protocol used to connect to the storage, this should be a short
-            string such as: "iSCSI", "FC", "nfs", "ceph", etc.
+            string such as: "iSCSI", "FC", "NFS", "ceph", etc.
+            Available protocols are present in cinder.common.constants and they
+            must be used instead of string literals.
+            Variant values only exist for older drivers that were already
+            reporting those values.  New drivers must use non variant versions.
+            In some cases this may be the same value as the driver_volume_type
+            returned by the initialize_connection method, but they are not the
+            same thing, since this one is meant to be used by the scheduler,
+            while the latter is the os-brick connector identifier used in the
+            factory method.
+
         * total_capacity_gb
             The total capacity in gigabytes (GiB) of the storage backend being
             used to store Cinder volumes. Use keyword 'unknown' if the backend

cinder/scheduler/filters/driver_filter.py

@@ -34,11 +34,14 @@ class DriverFilter(filters.BaseBackendFilter):
         stats = self._generate_stats(backend_state, filter_properties)
 
         LOG.debug("Checking backend '%s'",
-                  stats['backend_stats']['backend_id'])
+                  stats[0]['backend_stats']['backend_id'])
-        result = self._check_filter_function(stats)
+        # Run the filter function for all possible storage_protocol values
+        # (e.g. FC, fibre_channel) and if any of them passes the filter, then
+        # the backend passes.
+        result = any(self._check_filter_function(stat) for stat in stats)
         LOG.debug("Result: %s", result)
         LOG.debug("Done checking backend '%s'",
-                  stats['backend_stats']['backend_id'])
+                  stats[0]['backend_stats']['backend_id'])
 
         return result
 
@@ -95,7 +98,12 @@ class DriverFilter(filters.BaseBackendFilter):
         return result
 
     def _generate_stats(self, backend_state, filter_properties):
-        """Generates statistics from backend and volume data."""
+        """Generates statistics from backend and volume data.
+
+        Returns a list where each entry corresponds to a different
+        storage_protocol value for those backends that use a storage protocol
+        that has variants, but only if the function actually uses the protocol.
+        """
 
         backend_stats = {
             'host': backend_state.host,
@@ -116,10 +124,12 @@ class DriverFilter(filters.BaseBackendFilter):
         backend_caps = backend_state.capabilities
 
         filter_function = None
+        uses_protocol = False
 
         if ('filter_function' in backend_caps and
                 backend_caps['filter_function'] is not None):
             filter_function = str(backend_caps['filter_function'])
+            uses_protocol = 'storage_protocol' in filter_function
 
         qos_specs = filter_properties.get('qos_specs', {})
 
@@ -139,4 +149,18 @@ class DriverFilter(filters.BaseBackendFilter):
             'filter_function': filter_function,
         }
 
-        return stats
+        # Only create individual entries for the different protocols variants
+        # if the function uses the protocol and there are variants.
+        if uses_protocol and isinstance(backend_state.storage_protocol, list):
+            result = []
+            for protocol in backend_state.storage_protocol:
+                new_stats = stats.copy()
+                new_stats['backend_stats'] = dict(new_stats['backend_stats'])
+                new_stats['backend_stats']['storage_protocol'] = protocol
+                new_stats['backend_caps'] = dict(new_stats['backend_caps'])
+                new_stats['backend_caps']['storage_protocol'] = protocol
+                result.append(new_stats)
+
+        else:
+            result = [stats]
+        return result

cinder/scheduler/host_manager.py

@@ -294,8 +294,13 @@ class BackendState(object):
         if not pool_cap.get('volume_backend_name', None):
             pool_cap['volume_backend_name'] = self.volume_backend_name
 
-        if not pool_cap.get('storage_protocol', None):
+        protocol = pool_cap.get('storage_protocol', None)
-            pool_cap['storage_protocol'] = self.storage_protocol
+        if protocol:
+            # Protocols that have variants are replaced with ALL the variants
+            storage_protocol = self.get_storage_protocol_variants(protocol)
+        else:  # Backend protocol has already been transformed with variants
+            storage_protocol = self.storage_protocol
+        pool_cap['storage_protocol'] = storage_protocol
 
         if not pool_cap.get('vendor_name', None):
             pool_cap['vendor_name'] = self.vendor_name
@@ -320,7 +325,11 @@ class BackendState(object):
         self.volume_backend_name = capability.get('volume_backend_name', None)
         self.vendor_name = capability.get('vendor_name', None)
         self.driver_version = capability.get('driver_version', None)
-        self.storage_protocol = capability.get('storage_protocol', None)
+        self.driver_version = capability.get('driver_version', None)
+
+        # Protocols that have variants are replaced with ALL the variants
+        protocol = capability.get('storage_protocol', None)
+        self.storage_protocol = self.get_storage_protocol_variants(protocol)
         self.updated = capability['timestamp']
 
     def consume_from_volume(self,
@@ -370,6 +379,18 @@ class BackendState(object):
                  'thick': self.thick_provisioning_support,
                  'pools': self.pools, 'updated': self.updated})
 
+    @staticmethod
+    def get_storage_protocol_variants(storage_protocol):
+        if storage_protocol in constants.ISCSI_VARIANTS:
+            return constants.ISCSI_VARIANTS
+        if storage_protocol in constants.FC_VARIANTS:
+            return constants.FC_VARIANTS
+        if storage_protocol in constants.NFS_VARIANTS:
+            return constants.NFS_VARIANTS
+        if storage_protocol in constants.NVMEOF_VARIANTS:
+            return constants.NVMEOF_VARIANTS
+        return storage_protocol
+
 
 class PoolState(BackendState):
     def __init__(self,

cinder/scheduler/weights/goodness.py

@@ -41,10 +41,14 @@ class GoodnessWeigher(weights.BaseHostWeigher):
     def _weigh_object(self, host_state, weight_properties):
         """Determine host's goodness rating based on a goodness_function."""
         stats = self._generate_stats(host_state, weight_properties)
-        LOG.debug("Checking host '%s'", stats['host_stats']['host'])
+        LOG.debug("Checking host '%s'", stats[0]['host_stats']['host'])
-        result = self._check_goodness_function(stats)
+        # Run the goodness function for all possible storage_protocol values
+        # (e.g. FC, fibre_channel) and use the maximum value, as the function
+        # may look for an exact match on a protocol and the backend may be
+        # returning a variant.
+        result = max(self._check_goodness_function(stat) for stat in stats)
         LOG.debug("Goodness weight for %(host)s: %(res)s",
-                  {'res': result, 'host': stats['host_stats']['host']})
+                  {'res': result, 'host': stats[0]['host_stats']['host']})
 
         return result
 
@@ -101,8 +105,12 @@ class GoodnessWeigher(weights.BaseHostWeigher):
         return result
 
     def _generate_stats(self, host_state, weight_properties):
-        """Generates statistics from host and volume data."""
+        """Generates statistics from host and volume data.
 
+        Returns a list where each entry corresponds to a different
+        storage_protocol value for those backends that use a storage protocol
+        that has variants, but only if the function actually uses the protocol.
+        """
         host_stats = {
             'host': host_state.host,
             'volume_backend_name': host_state.volume_backend_name,
@@ -120,10 +128,12 @@ class GoodnessWeigher(weights.BaseHostWeigher):
         host_caps = host_state.capabilities
 
         goodness_function = None
+        uses_protocol = False
 
         if ('goodness_function' in host_caps and
                 host_caps['goodness_function'] is not None):
             goodness_function = str(host_caps['goodness_function'])
+            uses_protocol = 'storage_protocol' in goodness_function
 
         qos_specs = weight_properties.get('qos_specs', {}) or {}
 
@@ -143,4 +153,19 @@ class GoodnessWeigher(weights.BaseHostWeigher):
             'goodness_function': goodness_function,
         }
 
-        return stats
+        # Only create individual entries for the different protocols variants
+        # if the function uses the protocol and there are variants.
+        if uses_protocol and isinstance(host_state.storage_protocol, list):
+            result = []
+            for protocol in host_state.storage_protocol:
+                new_stats = stats.copy()
+                new_stats['host_stats'] = dict(new_stats['host_stats'])
+                new_stats['host_stats']['storage_protocol'] = protocol
+                new_stats['host_caps'] = dict(new_stats['host_caps'])
+                new_stats['host_caps']['storage_protocol'] = protocol
+                result.append(new_stats)
+
+        else:
+            result = [stats]
+
+        return result

cinder/tests/unit/scheduler/test_host_manager.py

@@ -1346,7 +1346,8 @@ class BackendStateTestCase(test.TestCase):
         fake_backend.update_from_volume_capability(capability)
 
         self.assertEqual('Local iSCSI', fake_backend.volume_backend_name)
-        self.assertEqual('iSCSI', fake_backend.storage_protocol)
+        # Storage protocol is changed to include its variants
+        self.assertEqual(['iSCSI', 'iscsi'], fake_backend.storage_protocol)
         self.assertEqual('OpenStack', fake_backend.vendor_name)
         self.assertEqual('1.0.1', fake_backend.driver_version)
 

doc/source/contributor/drivers.rst

@@ -113,6 +113,24 @@ extending. If it doesn't, it should report 'online_extend_support=False'.
 Otherwise the scheduler will attempt to perform the operation, and may leave
 the volume in 'error_extending' state.
 
+Value of ``storage_protocol`` is a single string representing the transport
+protocol used by the storage.  Existing protocols are present in
+``cinder.common.constants`` and should be used by drivers instead of string
+literals.
+
+Variant values only exist for older drivers that were already reporting those
+values.  New drivers must use non variant versions.
+
+The ``storage_protocol`` can be used by operators using the
+``cinder get-pools --detail`` command, by volume types in their extra specs,
+and by the filter and goodness functions.
+
+We must not mistake the value of the ``storage_protocol`` with the identifier
+of the os-brick connector, which is returned by the ``initialize_connection``
+driver method in the ``driver_volume_type`` dictionary key.  In some cases they
+may have the same value, but they are different things.
+
+
 Feature Enforcement
 -------------------
 

releasenotes/notes/fix-storage_protocol-6baf55e13249463c.yaml

@@ -0,0 +1,19 @@
+---
+upgrade:
+  - |
+    The ``storage_protocol`` treats all variants of the protocol name as the
+    same regarding matches, so for example using FC, fc, or fibre_channel will
+    be treated equally in the scheduler, be it when filtering using the volume
+    type's extra specs or when using filter and goodness functions.
+
+    The storage protocol reporting via the REST API will be now the same for
+    them all, using the preferred naming, FC, NVMe-oF, iSCSI, NFS...
+
+    If your deployment uses ``storage_protocol`` to differentiate between
+    backends that use the same protocol but report it using different variants,
+    be aware that they will no longer be differentiated.
+fixes:
+  - |
+    `Bug #1966103 <https://bugs.launchpad.net/cinder/+bug/1966103>`_: Fixed
+    inconsistent behavior of ``storage_protocol`` among different backends that
+    report variants of the protocol name, such as FC, fc, fibre_channel.