Fix discover_version

discover_version needs to find the proper module for server_api_version
method, and it needs to properly parse a list of versions.
Do some refactor to clean things up.
Add unit tests.

Co-Authored-By: waj334 <justin.wilson@intel.com>
Change-Id: I742bc33074cc55fe5f9682b8b97a82573c51183f
Closes-Bug: #1632872

cinderclient/api_versions.py

@@ -19,7 +19,6 @@ import re
 
 from oslo_utils import strutils
 
-import cinderclient
 from cinderclient import exceptions
 from cinderclient import utils
 from cinderclient._i18n import _
@@ -29,7 +28,9 @@ LOG = logging.getLogger(__name__)
 
 # key is a deprecated version and value is an alternative version.
 DEPRECATED_VERSIONS = {"1": "2"}
+DEPRECATED_VERSION = "2.0"
 MAX_VERSION = "3.27"
+MIN_VERSION = "3.0"
 
 _SUBSTITUTIONS = {}
 
@@ -234,12 +235,13 @@ def get_api_version(version_string):
 
 
 def _get_server_version_range(client):
-    version = client.versions.get_current()
+    versions = client.services.server_api_version()
 
-    if not hasattr(version, 'version') or not version.version:
+    if not versions:
         return APIVersion(), APIVersion()
-
+    for version in versions:
-    return APIVersion(version.min_version), APIVersion(version.version)
+        if '3.' in version.version:
+            return APIVersion(version.min_version), APIVersion(version.version)
 
 
 def discover_version(client, requested_version):
@@ -254,52 +256,87 @@ def discover_version(client, requested_version):
     server_start_version, server_end_version = _get_server_version_range(
         client)
 
-    both_versions_null = not (server_start_version or server_end_version)
+    valid_version = requested_version
-    if (not requested_version.is_latest() and
+    if not server_start_version and not server_end_version:
-            requested_version != APIVersion('2.0')):
+        msg = ("Server does not support microversions. Changing server "
-        if both_versions_null:
+               "version to %(min_version)s.")
-            raise exceptions.UnsupportedVersion(
+        LOG.debug(msg, {"min_version": DEPRECATED_VERSION})
-                _("Server doesn't support microversions"))
+        valid_version = APIVersion(DEPRECATED_VERSION)
-        if not requested_version.matches(server_start_version,
+    else:
-                                         server_end_version):
+        valid_version = _validate_requested_version(
+            requested_version,
+            server_start_version,
+            server_end_version)
+
+        _validate_server_version(server_start_version, server_end_version)
+    return valid_version
+
+
+def _validate_requested_version(requested_version,
+                                server_start_version,
+                                server_end_version):
+    """Validates the requested version.
+
+    Checks 'requested_version' is within the min/max range supported by the
+    server. If 'requested_version' is not within range then attempts to
+    downgrade to 'server_end_version'. Otherwise an UnsupportedVersion
+    exception is thrown.
+
+    :param requested_version: requestedversion represented by APIVersion obj
+    :param server_start_version: APIVersion object representing server min
+    :param server_end_version: APIVersion object representing server max
+    """
+    valid_version = requested_version
+    if not requested_version.matches(server_start_version, server_end_version):
+        if server_end_version <= requested_version:
+            if (APIVersion(MIN_VERSION) <= server_end_version and
+                    server_end_version <= APIVersion(MAX_VERSION)):
+                msg = _("Requested version %(requested_version)s is "
+                        "not supported. Downgrading requested version "
+                        "to %(server_end_version)s.")
+                LOG.debug(msg, {
+                    "requested_version": requested_version,
+                    "server_end_version": server_end_version})
+            valid_version = server_end_version
+        else:
             raise exceptions.UnsupportedVersion(
                 _("The specified version isn't supported by server. The valid "
                   "version range is '%(min)s' to '%(max)s'") % {
                     "min": server_start_version.get_string(),
                     "max": server_end_version.get_string()})
-        return requested_version
 
-    if requested_version == APIVersion('2.0'):
+    return valid_version
-        if server_start_version == APIVersion('2.1') or both_versions_null:
-            return APIVersion('2.0')
-        raise exceptions.UnsupportedVersion(
-            _("The server isn't backward compatible with Cinder V2 REST "
-              "API"))
 
-    if both_versions_null:
+
-        return APIVersion('2.0')
+def _validate_server_version(server_start_version, server_end_version):
-    if cinderclient.API_MIN_VERSION > server_end_version:
+    """Validates the server version.
+
+    Checks that the 'server_end_version' is greater than the minimum version
+    supported by the client. Then checks that the 'server_start_version' is
+    less than the maximum version supported by the client.
+
+    :param server_start_version:
+    :param server_end_version:
+    :return:
+    """
+    if APIVersion(MIN_VERSION) > server_end_version:
         raise exceptions.UnsupportedVersion(
-            _("Server version is too old. The client valid version range is "
+            _("Server's version is too old. The client's valid version range "
-              "'%(client_min)s' to '%(client_max)s'. The server valid version "
+              "is '%(client_min)s' to '%(client_max)s'. The server valid "
-              "range is '%(server_min)s' to '%(server_max)s'.") % {
+              "version range is '%(server_min)s' to '%(server_max)s'.") % {
-                  'client_min': cinderclient.API_MIN_VERSION.get_string(),
+                  'client_min': MIN_VERSION,
-                  'client_max': cinderclient.API_MAX_VERSION.get_string(),
+                  'client_max': MAX_VERSION,
                   'server_min': server_start_version.get_string(),
                   'server_max': server_end_version.get_string()})
-    elif cinderclient.API_MAX_VERSION < server_start_version:
+    elif APIVersion(MAX_VERSION) < server_start_version:
         raise exceptions.UnsupportedVersion(
-            _("Server version is too new. The client valid version range is "
+            _("Server's version is too new. The client's valid version range "
-              "'%(client_min)s' to '%(client_max)s'. The server valid version "
+              "is '%(client_min)s' to '%(client_max)s'. The server valid "
-              "range is '%(server_min)s' to '%(server_max)s'.") % {
+              "version range is '%(server_min)s' to '%(server_max)s'.") % {
-                  'client_min': cinderclient.API_MIN_VERSION.get_string(),
+                  'client_min': MIN_VERSION,
-                  'client_max': cinderclient.API_MAX_VERSION.get_string(),
+                  'client_max': MAX_VERSION,
                   'server_min': server_start_version.get_string(),
                   'server_max': server_end_version.get_string()})
-    elif cinderclient.API_MAX_VERSION <= server_end_version:
-        return cinderclient.API_MAX_VERSION
-    elif server_end_version < cinderclient.API_MAX_VERSION:
-        return server_end_version
 
 
 def update_headers(headers, api_version):

cinderclient/client.py

@@ -93,10 +93,17 @@ def get_server_version(url):
                         api_versions.APIVersion(version['version']))
     except exceptions.ClientException as e:
         logger.warning(_LW("Error in server version query:%s\n"
-                 "Returning APIVersion 2.0") % six.text_type(e.message))
+                 "Returning APIVersion 2.0"), six.text_type(e.message))
         return api_versions.APIVersion("2.0"), api_versions.APIVersion("2.0")
 
 
+def get_highest_client_server_version(url):
+    min_server, max_server = get_server_version(url)
+    max_server_version = api_versions.APIVersion.get_string(max_server)
+
+    return min(float(max_server_version), float(api_versions.MAX_VERSION))
+
+
 def get_volume_api_from_url(url):
     scheme, netloc, path, query, frag = urlparse.urlsplit(url)
     components = path.split("/")
@@ -203,7 +210,7 @@ class SessionClient(adapter.LegacyJsonAdapter):
                              'auth plugin.')
 
     def _cs_request_base_url(self, url, method, **kwargs):
-        base_url = self._get_base_url(**kwargs)
+        base_url = self._get_base_url()
         return self._cs_request(
             base_url + url,
             method,

cinderclient/tests/unit/test_api_versions.py

@@ -184,3 +184,72 @@ class GetAPIVersionTestCase(utils.TestCase):
         self.assertEqual(mock_apiversion.return_value,
                          api_versions.get_api_version(version))
         mock_apiversion.assert_called_once_with(version)
+
+
+@ddt.ddt
+class DiscoverVersionTestCase(utils.TestCase):
+    def setUp(self):
+        super(DiscoverVersionTestCase, self).setUp()
+        self.orig_max = api_versions.MAX_VERSION
+        self.orig_min = api_versions.MIN_VERSION or None
+        self.addCleanup(self._clear_fake_version)
+        self.fake_client = mock.MagicMock()
+
+    def _clear_fake_version(self):
+        api_versions.MAX_VERSION = self.orig_max
+        api_versions.MIN_VERSION = self.orig_min
+
+    def _mock_returned_server_version(self, server_version,
+                                      server_min_version):
+        version_mock = mock.MagicMock(version=server_version,
+                                      min_version=server_min_version,
+                                      status='CURRENT')
+        val = [version_mock]
+        if not server_version and not server_min_version:
+            val = []
+        self.fake_client.services.server_api_version.return_value = val
+
+    @ddt.data(
+        ("3.1", "3.3", "3.4", "3.7", "3.3", True),   # Server too new
+        ("3.9", "3.10", "3.0", "3.3", "3.10", True),   # Server too old
+        ("3.3", "3.9", "3.7", "3.17", "3.9", False),  # Requested < server
+        ("3.5", "3.8", "3.0", "3.7", "3.8", False, "3.7"),  # downgraded
+        ("3.5", "3.5", "3.0", "3.5", "3.5", False),  # Server & client same
+        ("3.5", "3.5", "3.0", "3.5", "3.5", False, "2.0", []),  # Pre-micro
+        ("3.1", "3.11", "3.4", "3.7", "3.7", False),  # Requested in range
+        ("3.1", "3.11", None, None, "3.7", False),  # Server w/o support
+        ("3.5", "3.5", "3.0", "3.5", "1.0", True)    # Requested too old
+    )
+    @ddt.unpack
+    def test_microversion(self, client_min, client_max, server_min, server_max,
+                          requested_version, exp_range, end_version=None,
+                          ret_val=None):
+        if ret_val is not None:
+            self.fake_client.services.server_api_version.return_value = ret_val
+        else:
+            self._mock_returned_server_version(server_max, server_min)
+
+        api_versions.MAX_VERSION = client_max
+        api_versions.MIN_VERSION = client_min
+
+        if exp_range:
+            self.assertRaisesRegexp(exceptions.UnsupportedVersion,
+                                    ".*range is '%s' to '%s'.*" %
+                                    (server_min, server_max),
+                                    api_versions.discover_version,
+                                    self.fake_client,
+                                    api_versions.APIVersion(requested_version))
+        else:
+            discovered_version = api_versions.discover_version(
+                self.fake_client,
+                api_versions.APIVersion(requested_version))
+
+            version = requested_version
+            if server_min is None and server_max is None:
+                version = api_versions.DEPRECATED_VERSION
+            elif end_version is not None:
+                version = end_version
+            self.assertEqual(version,
+                             discovered_version.get_string())
+            self.assertTrue(
+                self.fake_client.services.server_api_version.called)

cinderclient/tests/unit/v3/test_services.py

@@ -36,3 +36,8 @@ class ServicesTest(utils.TestCase):
             # Make sure cluster fields from v3.7 is present and not None
             self.assertIsNotNone(getattr(service, 'cluster'))
         self._assert_request_id(services_list)
+
+    def test_api_version(self):
+        client = fakes.FakeClient(version_header='3.0')
+        svs = client.services.server_api_version()
+        [self.assertIsInstance(s, services.Service) for s in svs]

cinderclient/v3/services.py

@@ -25,14 +25,14 @@ Service = services.Service
 
 class ServiceManager(services.ServiceManager):
     @api_versions.wraps("3.0")
-    def server_api_version(self, url_append=""):
+    def server_api_version(self):
         """Returns the API Version supported by the server.
 
-        :param url_append: String to append to url to obtain specific version
         :return: Returns response obj for a server that supports microversions.
                  Returns an empty list for Liberty and prior Cinder servers.
         """
+
         try:
-            return self._get_with_base_url(url_append, response_key='versions')
+            return self._get_with_base_url("", response_key='versions')
         except LookupError:
             return []